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.

Sign up for GitHub

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

Remove height and width from default shapes pre-configuration #1104

Merged
merged 1 commit into from
Jan 9, 2025
Merged

Remove height and width from default shapes pre-configuration #1104

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions optimum/exporters/openvino/convert.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,9 @@ def export_from_model(
# Get the shapes to be used to generate dummy inputs
input_shapes = {}
for input_name in DEFAULT_DUMMY_SHAPES.keys():
if input_name in ["height", "width"]:
# use H and W from generator defaults
continue
input_shapes[input_name] = (
kwargs_shapes[input_name] if input_name in kwargs_shapes else DEFAULT_DUMMY_SHAPES[input_name]
)
Comment on lines 665 to 672
Copy link
Member

Choose a reason for hiding this comment

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

I think this entire part is unnecessary and could/should be removed, as we already have the default shapes in each input generator signature. This part enforces the values in DEFAULT_DUMMY_SHAPES on all model inputs generators, restricting model-specific export optimizations (like reducing input dimensions for a dynamic axis to lower export memory requirements). It also populates input shapes with irrelevant shapes.
@echarlaix wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I feel that it may be dangerous to remove it now as it was there for a long time and some models may rely on it. As for height and weight the impact is much less, because not many models receive image in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

I will merge for now and follow up with a PR investigating the possibility of its removal.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @mvafin

Expand Down