-
Notifications
You must be signed in to change notification settings - Fork 613
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
ModelHubMixin
: more metadata + arbitrary config types + proper guide
#2230
Conversation
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
can we move the logic outside the inheritance? class A (nn.Module , PyTorchModelHubMixin, meta):
def __init_(self,...) :
(...)
@classmethod
def encode(*args,**kwargs):
"""logic for configuring _hub_mixin_config"""
for key,value in kwargs.items() :
if value is_jasonable :
self._hub_mixin_config[key] = value
else :
warnings.warn(f"""the parameter {key} has a value of type {type(value)} and could not be added to _hub_mixin_config
we advise you create your own "configure_hub_config" method, current jasonable types are {jasonable_types}
if you chose to make any changes to the encode method we encourage you to update the decode method too""")
@classmethod
def decode(config) :
"""logic for decoding"""
try :
return model(**config)
except :
raise error (f""""model could not be initialized, this is either due to model expecting a parameter that is not a {jsonable_types}
or because you have a logic for reading a file from a local directory in the init method and that file does not exist currently
to fix this error we advise you to create your own "decode" method """") and sice there's way more metadata now can we do it like this now : class A (nn.Module, PyTorchModelHubMixin):
meta1 = v1
meta2 = v2
def __init__(self, ... ) :
(...) accessing meta later is as easy as |
Is there a benefit in doing so? Attributes can conflicts with existing methods or properties of class we are inheriting from so I would prefer to avoid them. |
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.
Nice additions to the ModelHubMixin
! 🔥
Co-authored-by: Steven Liu <[email protected]>
As always, thanks for your very welcomed comments @stevhliu! ❤️ I addressed them all. |
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 ! Thanks for your work @Wauplin
Thanks for the review! Will merge it :) |
This PR adds some improvement to the
ModelHubMixin
class and in particular:license
,license_link
,license_name
,pipeline_tag
,languages
.VoiceCraft
(expects aargparse.Namespace
) or MichelAngelo (expects aOmegaConf
see #2212). It is currently possible to mitigate this (see VoiceCraft#90) but solution is quite hacky/unintuitive. Instead of manually adding support for more types inhuggingface_hub
, this PR adds a way to define a custom encoder/decoder for the type. For example, the voicecraft integration would become:In addition to this, I have update the
Integration
guide to explain in details how to use the advanced features of the mixin (metadata, model card template, config, custom encoders, etc.)cc @NielsRogge @not-lain
EDIT: link to updated guide + package reference.