Skip to content
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

Enable export of model with fixed shape #1643

Merged
merged 7 commits into from Jan 17, 2024

Conversation

mht-sharma
Copy link
Contributor

@mht-sharma mht-sharma commented Jan 12, 2024

What does this PR do?

Enable export of the models with fixed shapes.

This is required to support export of model like "vovnet" which cannot be exported with dynamic shapes.

 optimum-cli export onnx --model timm/ese_vovnet39b.ra_in1k  out_vov --no-dynamic-axes

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@mht-sharma mht-sharma requested review from fxmarty, echarlaix and michaelbenayoun and removed request for fxmarty and echarlaix January 12, 2024 07:39
Comment on lines 450 to 453
if library_name == "timm" and no_dynamic_axes is False and "vovnet" in model_type:
raise ValueError(
f"The export of {model_type} is not supported with dynamic axes. Please pass --no-dynamic-axes to export the model with fixed shapes"
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder rather avoid adding more model-specific code in here. Is it possible to move it elsewhere (like model_configs.py)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could not be possible, as the variable (no_dynamic_axes) is not under the scope of model_configs.py.

For now I have removed this from here. In documentation, it is mentioned that the following architecture support is limited.

Copy link
Member

@michaelbenayoun michaelbenayoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you write a test to test that when specifying shapes for the dummy inputs with no dynamic axes requested, the proper shapes are used in the exported model?

@@ -444,6 +447,11 @@ def main_export(
f" `--task {task_non_past} --monolith`, or `--task {task}` without the monolith argument."
)

if library_name == "timm" and no_dynamic_axes is False and "vovnet" in model_type:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if library_name == "timm" and no_dynamic_axes is False and "vovnet" in model_type:
if library_name == "timm" and not no_dynamic_axes and "vovnet" in model_type:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a small test, testing only a few models from transformers and vovnet from timm. This should be sufficient to see if it functionally works.

Should we extend testing to cover all supported models?

Copy link
Collaborator

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@michaelbenayoun michaelbenayoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mht-sharma mht-sharma merged commit 130197f into huggingface:main Jan 17, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants