-
Notifications
You must be signed in to change notification settings - Fork 27.6k
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
Dynamically load model code from the Hub #13467
Conversation
c195c2a
to
de17667
Compare
Forgive me for interjecting and asking, but does this allow custom modules to be uploaded to modelhub and loaded and run with the official Transformers library ? just to be sure. If that's the case, do you plan to add some code scanning ? I think on some cases where this would be an security vulnerability for remote code execution |
The PR requires an extra argument in the call to |
Great, thanks for clearing this |
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'm having a few failures so I'll wait until you have updated the description of the PR before finishing my review
init_path.touch() | ||
|
||
|
||
def check_imports(filename): |
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.
That's extremely useful!
|
||
if type(config) in cls._model_mapping.keys(): | ||
if hasattr(config, "auto_map") and cls.__name__ in config.auto_map: | ||
if not trust_remote_code: |
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 wonder if it wouldn't be good practice to force a revision here too. Either through raising an error (best) or through logging a warning (ok): if a public repository becomes trendy, it wouldn't be surprising to see the user update the code and for it to, at best break systems running the code, at worst run arbitrary code through torch.load
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.
That would make the API harder to use, no?
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.
Then let's go with a warning?
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.
After discussing with @LysandreJik, we will need to remember to parse this auto_map
attribute in the hub so we know which models are custom code models
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 is in very good shape! I tried to break it as hard as I could but couldn't manage to do so!
Following our IRL discussions, the following point stood out: if defining a model that has a significantly different architecture than the original model (for example using a BertConfig
as the model configuration, and then defining a totally different model to BERT), then the model can still be loaded in traditional BERT architectures; this is quite an edge case here, but we should keep that in mind as new configurations are allowed -> we should then make sure that models error out when loaded in architectures that don't support them.
Fantastic that it works out of the box for private models too. This should enable a myriad of use-cases.
Looks good to me, eagerly awaiting the next update enabling configuration/tokenizer creation!
|
||
if type(config) in cls._model_mapping.keys(): | ||
if hasattr(config, "auto_map") and cls.__name__ in config.auto_map: | ||
if not trust_remote_code: |
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.
Then let's go with a warning?
753fca4
to
ffa125f
Compare
Co-authored-by: Lysandre Debut <[email protected]>
Co-authored-by: Lysandre Debut <[email protected]>
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, feel free to merge!
Co-authored-by: Lysandre Debut <[email protected]>
hi, quick question about this PR: It lets people upload a custom model to the hub great, but it seems that in order to call that model using the API it still requires the model to be using one of the provided Pipelines? Is there any way of also allowing a custom pipeline when calling the custom models? |
Custom pipelines is something we will add support for, it's on the roadmap, but for now you can't use the custom model in the API indeed. |
What does this PR do?
This PR allows a user to upload a modeling file along their model weights, and then lets the
Auto
classes load that model. The API is still a bit manual for now, it will be smoothed in follow-up PRs.In terms of user of the model, the only thing to add is the flag
trust_remote_code=True
when using an auto class:This will load a
FakeModel
as defined in this file.For the person uploading the model there is a tiny bit more work. First save the code of the model (with all necessary imports) in a single python file, for instance
modeling.py
. In the rest of this PR description, I'll use MODELING_FILE to represent the name of that file without the suffix (so in my example ofmodeling.py
, MODELING_FILE is "modeling"). That file needs to be in the repository you upload.Second, add a filed
auto_map
to the configuration of the model you want to upload. This needs to be a dictionary with the name of auto classes as keys, and the full name of the corresponding modeling class as values. By full name, I mean MODELING_FILE.CLASS_NAME. For instancemodeling.FakeModel
for a classFakeModel
defined in themodeling.py
module. Here is an example:This needs to be done before the model is saved, so that when doing the
model.save_pretrained
call, the config is saved with that field. Once this is done, push everything to the Hub, and you should be good!