-
Notifications
You must be signed in to change notification settings - Fork 597
Propagate library_name parameter in from_pretrained to export #2328
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
Propagate library_name parameter in from_pretrained to export #2328
Conversation
Required to avoid automatic inferring of the library_name
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. |
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 @tomaarsen ! Also would be nice to add a test for this (to ensure SparseEncoder
export is not broken in the future)
optimum/onnxruntime/modeling_ort.py
Outdated
use_io_binding: Optional[bool] = None, | ||
# other arguments | ||
model_save_dir: Optional[Union[str, Path, TemporaryDirectory]] = 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.
would prefer to have it as a class attribute instead which can be set to None by default (will be inferred in this case) and for ORTModelForMaskedLM
can be set to transformers directly as it should never be set to anything else no ?
like done in optimum-intel https://github.com/huggingface/optimum-intel/blob/d93fd59aebd24ac887deb1eaab7ff6af41b09946/optimum/intel/openvino/modeling_base.py#L647
Will address your comments on Monday (most likely), thank you!
|
Under modeling_diffusion it looks like ORTModel isn't used
I've used the class attribute approach that you proposed, and also added a test using https://huggingface.co/sparse-encoder-testing/splade-bert-tiny-nq. I'm also using this model in my own tests, so it'll stay. It's a good example of a model that would be automatically detected as Sentence Transformers, but we might want to load with
|
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, thanks @tomaarsen
Also the optimum onnx / ort integration is moved in https://github.com/huggingface/optimum-onnx @tomaarsen. I'll take care of opening a PR to add these PR changes there as well |
Oh, good to know! Thank you. |
from huggingface/optimum#2328 --------- Co-authored-by: Ilyas Moutawwakil <[email protected]>
Resolves #2327
Hello!
Pull Request overview
library_name
inORTModel....from_pretrained("...", library_name=...)
Details
As described in #2327, this will allow exporting and loading models using a specific library name rather than relying on Optimum's automatic inferring of the library. For Sentence Transformers, this allows me to add ONNX exporting to SparseEncoder models with:
As Sentence Transformers only exports models in the "transformers" way, I can add this internally in Sentence Transformers so the eventual usage by the end user is simply
model = SparseEncoder("naver/splade-cocondenser-ensembledistil", backend="onnx")
. Note also that I can export with OpenVINO withoutlibrary_name
just fine, only ONNX has this issue.Please let me know if you'd rather go in a different direction here.