Skip to content
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

Add simple class to ease saving loading from HF hub #9

Closed
thevasudevgupta opened this issue Jan 18, 2021 · 6 comments · Fixed by #11
Closed

Add simple class to ease saving loading from HF hub #9

thevasudevgupta opened this issue Jan 18, 2021 · 6 comments · Fixed by #11

Comments

@thevasudevgupta
Copy link
Contributor

Would like to have a class from which model class can be inherited to add support of Huggingface hub. It will look something like this :

from huggingface_hub import SavingUtils

class MyModel(nn.Module, SavingUtils):

   def __init__(self):
      self.layer1 = ....

   def forward(self, ...):
      return ...

model = MyModel()
model.save_pretrained("model_id")

# loading model
model = MyModel.from_pretrained("model_id")

I can make a PR for adding this simple feature if you approve it.

@julien-c
Copy link
Member

julien-c commented Jan 18, 2021

Sounds like a great feature!

Not sure about calling .save_pretrained() to git push though – might be confusing vs. saving to a local directory.

Maybe .upload_to_hub() or .git_push(). or .upload_pretrained() if we wanted to maintain the symmetry with other method names?

Finally, not sure about implementing as a mixin vs. subclassing nn.Module. I think I like the mixin but I'd like to get more feedback, maybe from @sgugger @thomwolf @patrickvonplaten and others.

@patrickvonplaten
Copy link
Contributor

I'm fine with adding a .upload_to_hub() method in PreTrainedModel(....) next to save_pretrained(...) and from_pretrained(...) - I don't think we really need a new Mixin class for that

@julien-c
Copy link
Member

@patrickvonplaten this is huggingface_hub not transformers so we do not have a PreTrainedModel here. (It is meant to be used from any library)

@patrickvonplaten
Copy link
Contributor

My bad! Then I think it's a good idea to have a ModelHubMixin class

@thevasudevgupta
Copy link
Contributor Author

Okay. I will start PR then.

@sgugger
Copy link
Contributor

sgugger commented Jan 19, 2021

I agree with @julien-c and @patrickvonplaten on the fact it should be a mixin. Seems like a useful feature, so thanks for tackling this @vasudevgupta7 !

julien-c added a commit that referenced this issue Mar 18, 2021
…sed in issue #9). (#11)

* work initiated

* start upload_to_hub

* add changes

* final-push

* i feel this is better.

* updated for Repositary class

* small updates

* fix mutiple calling

* small fix

* make style

* add everything

* minor fix

* minor fix

* done evrything

* small fix

* [doc] remove mention of TF support

* Fix typings (i think)

* We do NOT want to have a hard requirement on torch

* Fix flake8

* Fix CI

Co-authored-by: Julien Chaumond <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants