-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow saving / loading from Huggingface Hub preset #1510
Allow saving / loading from Huggingface Hub preset #1510
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Love how simple this is to add. Let's hold tight for a bit while we nail down the public version of save_to_preset
.
keras_nlp/utils/preset_utils.py
Outdated
@@ -109,6 +123,9 @@ def save_to_preset( | |||
weights_filename="model.weights.h5", | |||
): | |||
"""Save a KerasNLP layer to a preset directory.""" | |||
push_to_hf = preset.startswith(HF_PREFIX) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we might end up doing some tweaks. We want to support a split flow for saving preprocessing and model weights. So our final flow might need to allow something like this
tokenizer.save_to_preset(dir)
backbone.save_to_preset(dir)
upload_preset("hf://user/model", dir)
I don't think we need to solve that here though! @SamanehSaadat is working on a draft of our upload flow currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes perfect sense!
@Wauplin with #1512 I think we are all ready to go here. Note that we will keep landing features related to the whole upload flow (in particular uploading a classifier with extra head weights, uploading lora weights that are essentially a diff on another preset). But we will keep the |
Thanks for the ping @mattdangerw! I've just updated the PR accordingly and we should now be good to go :) And agree with you, we shouldn't have much HF-specific logic apart from that given how isolated Regarding documentation, do I have to update some markdown somewhere or are the different preset handlers not really documented for now? Please let me know if I can be of any assistance here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just some comments on error messages.
keras_nlp/utils/preset_utils.py
Outdated
"Please install with `pip install huggingface_hub`." | ||
) | ||
hf_handle = preset.removeprefix(HF_PREFIX) | ||
return huggingface_hub.hf_hub_download(repo_id=hf_handle, filename=path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do the error messages look like if a handle is unformed? Will it read well enough, or should we validate roughly here so we can have a message similar to the Kaggle error above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, hf_handle
must correspond to a repo_id so something in the form username/repo_name
(e.g. "google/gemma-7b") since the hf prefix has been removed. If it's not the case, an HFValidationError
(which is a custom ValueError
) is raised. Here are the validation rules we are checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed 24ef262 to raise a ValueError similar to the kaggle handle one. I tried to be as consistent as possible. Please let me know what you think :)
keras_nlp/utils/preset_utils.py
Outdated
) | ||
hf_handle = uri.removeprefix(HF_PREFIX) | ||
repo_url = huggingface_hub.create_repo(repo_id=hf_handle, exist_ok=True) | ||
huggingface_hub.upload_folder(repo_id=repo_url.repo_id, folder_path=preset) | ||
else: | ||
raise ValueError( | ||
f"Unexpected URI `'{uri}'`. Kaggle upload format should follow " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, might want to reword this error message.
Side note: we are kinda inconsistent in how we refer to these model handles. I'm not sure if we should call these URIs or something else, but we should be consistent in our wording. No need to fix on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've update the error message in 24ef262. I kept the uri
naming to be consistent with the rest of the logic but agree with you it would be good to harmonize naming. I think inconsistency comes from the fact that in get_file
the preset
refers to either a local directory or a URI while in upload_preset
, the preset
refers to a local directory and uri
to the URI. So maybe not so inconsistent after all?
Thanks both for the review and feedback! I have addressed all comments and completed the |
f"Unexpected URI `'{uri}'`. Kaggle upload format should follow " | ||
"`kaggle://<KAGGLE_USERNAME>/<MODEL>/<FRAMEWORK>/<VARIATION>`." | ||
"Unknown URI. An URI must be a one of:\n" | ||
"1) a Kaggle Model handle like `'kaggle://<KAGGLE_USERNAME>/<MODEL>/<FRAMEWORK>/<VARIATION>'`\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I followed the existing message but I find it inconsistent with the error in get_file
. In get_file
, we provide real examples (e.g. 'kaggle://keras/bert/keras/bert_base_en'
) while here we only provide the format ('kaggle://<KAGGLE_USERNAME>/<MODEL>/<FRAMEWORK>/<VARIATION>'
). Both are fine IMO but if you prefer one or the other, please let me know and I can update in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge this as is, and I'll chat with folk later to figuring out our broader naming a push a small fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much! Looking forward to being on the hub!
Great thanks for the approval! I just pushed a commit to fix linting. Hope it's fine now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Hi @Wauplin! Thanks again for the PR! Just wanted to make sure my understanding is correct and there isn't any way to upload a model folder if the model hasn't been created on the HF website. |
@SamanehSaadat yes that's true but you can use |
* first draft * update upload_preset * lint * consistent error messages * lint
Solves #1294. As mentioned in #1294 (comment), this PR adds support for the
hf://
prefix to load presets from the Huggingface Hub.The integration requires the
huggingface_hub
library. Authentication can be configured with theHF_TOKEN
environment variable (only for private models or for uploads, similarly to KaggleHub). Here is a Colab notebook showcasing it.Here is how it looks like once uploaded on the Hub: https://huggingface.co/Wauplin/bert_base_en_uncased_retrained/tree/main..
If we go this way, I think we should also upload a default model card with
keras-nlp
tag to make all KerasNLP models discoverable on the Hub. On the Hugging Face side, we could makeKerasNLP
an official library (e.g. searchable, with code snippets, download counts, etc.).In the current implementation, saving to
"hf://Wauplin/bert_base_en_uncased_retrained"
will save the model locally toWauplin/bert_base_en_uncased_retrained
subfolder + create the repository on the Hub + upload the local folder to this repo on the Hub. An alternative could be to save to a temporary folder before uploading to the Hub (to avoid the local copy). Both solutions are correct in my opinion, it's more a matter of how the KerasNLP envision thesave_to_preset
method.