-
Notifications
You must be signed in to change notification settings - Fork 571
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
Adding mixin class for ease saving, uploading, downloading (as discussed in issue #9). #11
Conversation
Shall I integrate model uploading directly in Please put your views on this. |
Hi @vasudevgupta7 thanks for the PR! I have no more bandwidth this week but plan on actively looking at this early next week! |
403b3e4
to
03dd73a
Compare
Ooops @vasudevgupta7 I wanted to reach out to let you know that we had merged the Will take a look at your updated PR with @philschmid and others, soon. Thanks! |
Replying late to this but yeah I feel like it'd be nice to expose both. |
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 for the PR! I'm not entirely sure adding a complex logic in the default save_pretrained
and from_pretrained
is desirable as users that need such a logic will probable need to overload those methods anyway (for instance transformers will definitely need to, or the API assumes the model is initialized with a config when it exists but some library might use a class method instead). So I would limit those two methods to the base save of the model weights and document they can be overloaded to add more logic.
The push_to_hub on the other hand is absolutely awesome to have!
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.
This looks like a great addition!
@vasudevgupta7 let us know when you feel like this is ready to squash/merge and I'll make a final review. Thanks again |
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.
Cool stuff! 🚀
First time reviewing on this repo, mostly shallow review; the rest looks sound to me, but I'm not confident approving on my own.
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.
Great PR!
Hoping everything works now. Sorry for the delay. |
Looks good! Does anyone tagged here have any last comments? @LysandreJik and @philschmid maybe? Otherwise I'll merge and push a release soon. |
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.
LGTM!
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.
LGTM too! Thank you for creating a test. Looking forward to using this!🚀
I've removed torch as a hard dependency, and please note that I don't think the tests can actually pass given that they are pushing to your userspace @vasudevgupta7 (not in a staging env) and the CI env of course doesn't have your git credentials. I'll merge like this but would be nice to set up CI for this at some point @LysandreJik By the way do you know how to activate GitHub Actions for pull requests from forks? |
Thanks for the contribution @vasudevgupta7 ❤️ |
Yes, setting it up can be done like it was done here: https://github.com/huggingface/transformers/pull/10383/files. I'm opening a PR with that proposal now! |
Glad it's merged finally 🤩. Thank you @julien-c, @philschmid, @LysandreJik, @patrickvonplaten, @theo-m, @sgugger for reviewing and approving. |
@julien-c there is small error in the example you shared in release. It should be |
Oops, no big deal, but I will update the release notes: https://github.com/huggingface/huggingface_hub/releases/tag/v0.0.7 |
* addition of new language: Mongolia I am adding the new language templates for the hosted API. Please review this for me. Thank you so much! * update mn summaraization I just fixed the wrong initialization for `mn` summarization it was fault that i copied `english` version of what I am doing commit imported from https://github.com/huggingface/widgets/commit/0c8b3e0ea843145623a865e8e7fe2f4653f168d9 * addition of new language: Mongolia I am adding the new language templates for the hosted API. Please review this for me. Thank you so much! * update mn summaraization I just fixed the wrong initialization for `mn` summarization it was fault that i copied `english` version of what I am doing
* addition of new language: Mongolia I am adding the new language templates for the hosted API. Please review this for me. Thank you so much! * update mn summaraization I just fixed the wrong initialization for `mn` summarization it was fault that i copied `english` version of what I am doing commit imported from https://github.com/huggingface/widgets/commit/0c8b3e0ea843145623a865e8e7fe2f4653f168d9 * addition of new language: Mongolia I am adding the new language templates for the hosted API. Please review this for me. Thank you so much! * update mn summaraization I just fixed the wrong initialization for `mn` summarization it was fault that i copied `english` version of what I am doing
This PR will add feature as discussed in issue #9.
Example
Note
._save_pretrained()
method.PyTorch
model.tf2
not supported currently