-
Notifications
You must be signed in to change notification settings - Fork 113
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
Allow for OpenVINO models to be stored in a model subdirectory #923
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. |
P.s. the original problematic snippet: from optimum.intel import OVModelForFeatureExtraction
model_id = "sentence-transformers-testing/stsb-bert-tiny-openvino"
model = OVModelForFeatureExtraction.from_pretrained(model_id, file_name="openvino/openvino_model.xml")
print(model) does work with optimum-intel 1.18.3, it's related to the new "setting export=True when needed".
|
Thanks a lot for the PR @tomaarsen ! We are using the from optimum.intel import OVModelForFeatureExtraction
model_id = "sentence-transformers-testing/stsb-bert-tiny-openvino"
model = OVModelForFeatureExtraction.from_pretrained(model_id, file_name="openvino_model.xml", subfolder="openvino") The issue here is that the model's configuration is missing from the subfolder (which is something currently expected on the optimum side https://github.com/huggingface/optimum/blob/049b00f61c9bb17bd2b20a3b77d04cc4c0f20d86/optimum/modeling_base.py#L383) if too constraining we could remove this need and check in the parent folder for the configuration when not present in the specified subfolder. In the meantime adding the model's config directly in the subfolder should fix this |
I'm hesitant to duplicate configuration, so for now my workaround is to restrict optimum-intel to from optimum.intel import OVModelForFeatureExtraction
model_id = "sentence-transformers-testing/stsb-bert-tiny-openvino"
model = OVModelForFeatureExtraction.from_pretrained(model_id, file_name="openvino/openvino_model.xml")
print(model) works again in the future. Are you planning on that? Then it'll have feature parity with ONNX - I'd rather keep the same structure for both ONNX and OV in Sentence Transformers.
|
…nto openvino_in_subdirectory
@tomaarsen I'll open a PR in optimum to remove the need for the configuration to be in the subfolder, in the meantime for you to be able to use
|
@echarlaix I don't think I'm following - how could I modify that line? Do you mean in this PR? Or will you expose this |
files = [Path(p) for p in repo_files if re.match(pattern, str(p)) and str(p.parent) == subfolder] | ||
files = [Path(p) for p in repo_files if re.match(pattern, str(p))] |
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.
in this case subfolder
is not used at all which is an issue (will result in issue when subfolder is not defined correctly)
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 modification shouldn't be needed with huggingface/optimum#2044
It was in the case where you were using |
@@ -439,7 +439,7 @@ def from_pretrained( | |||
|
|||
ov_files = _find_files_matching_pattern( | |||
model_dir, | |||
pattern=r"(.*)?openvino(.*)?\_model.xml", | |||
pattern=r"(.*)?openvino(.*)?\_model.xml$", |
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.
good catch !
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.
As a bit of extra context: The HfApi().snapshot_download
with local_files creates a .cache
folder with every file in the remote repository appended with .metadata
. The resulting openvino_model.xml.metadata
was also matched by the pattern.
@echarlaix
But only if I use exactly I'll await the
|
Great to hear!
A PR for the regex pattern + the test you added would be great !
One last thing needs to be fixed for the onnx export to be compatible with transformers v4.45 and we can do the release. Will let you know when ready (hopefully today) |
Sounds good! |
See #931. Apologies for the delay.
|
What does this PR do?
This PR allows users to load a model from a subdirectory via just
file_name
.My goal:
Where only the modeling files are in the
openvino
subfolder.Result:
I.e. it starts exporting because it doesn't find any openvino file anywhere (because it only looks in the root directory).
For reference, this is exactly how I do it for ONNX via Optimum, and it works well there.
An alternative
So I tried using
subfolder
:But then the rest of the files outside of the subfolder can't be read, and I get:
I'm also pretty sure that I can't specify the library anywhere, so this is a dead end. Ideally, I want it to work equivalently to ONNX anyways.
The fix
The simple fix that I've applied for now is also look for openvino files in subdirectories. After all, if there's an exported openvino file anywhere, you probably want to inform the user rather than re-export a model.
After the fix, I get this:
Before submitting
Note
This issue will likely prevent me from including UKPLab/sentence-transformers#2712 into the next Sentence Transformers release, sadly. That PR adds OpenVINO and ONNX backends to Sentence Transformers, but currently it's not possible to load saved OpenVINO files due to this issue.
P.s. I have no plans to further update
sentence-transformers-testing/stsb-bert-tiny-openvino
, I'll be using it in my own tests as well.cc @echarlaix @helena-intel