-
Notifications
You must be signed in to change notification settings - Fork 487
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
fix infer task from model_name if model from sentence transformer #2151
fix infer task from model_name if model from sentence transformer #2151
Conversation
@IlyasMoutawwakil @echarlaix could you please take a look? |
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. |
@@ -1770,6 +1770,7 @@ def _infer_task_from_model_name_or_path( | |||
revision: Optional[str] = None, | |||
cache_dir: str = HUGGINGFACE_HUB_CACHE, | |||
token: Optional[Union[bool, str]] = None, | |||
library_name: Optional[str] = None, |
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 see you're adding library_name
to the signature but shouldn't this argument be specified in the method's calls as well ?
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 want to get confirmation that it is reliable approach before doing that.
In case of optimum itself it also need to provide it to infer_task_from_model too https://github.com/huggingface/optimum/blob/main/optimum/exporters/onnx/__main__.py#L259
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.
yeah you can go ahead and add it where needed, it seems reliable.
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.
ok, added.
Only not sure about tflite exporter, as I can see for getting export config, it uses library_name="transformers" explicitly, so it means that other library are not supported, right? I also added library_name="transformers" for avoid model type mismatch (e.g. in case of sentence transformers described above, if it will not be provided, model will be loaded as sentence transformer and raises error in some steps later
failed test is not related to my changes, modernbert is available only on transformers master branch, there is no transformers package that contains this model code yet |
What does this PR do?
issue found during attempt to export https://huggingface.co/BAAI/bge-small-zh-v1.5 via optimum-intel without --task specification
The root cause is infer_library_from_model detects task as sentence-transformers even if --library transformers selected in cli and there is no additional condition for specification task for sentence transformers, as the result method raises error
provided default task for sentence-transformers and added mechanism to explicitly set library_name (in case if user force --library_name in cli, need additional changes for enabling this behaviour in optimum-intel and other integrations that allow to select library_name) for avoiding mismatch between auto-detected and selected library (sentence transformers model can be exported with both transformers and sentence transformers library name and export configuration and exported model may be different for this case)