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

Jump to bottom

ONNX export may fail for Hiera due to math.sqrt and python type int casts #33181

Closed
xenova opened this issue Aug 29, 2024 · 2 comments · Fixed by #33226
Closed

ONNX export may fail for Hiera due to math.sqrt and python type int casts #33181

xenova opened this issue Aug 29, 2024 · 2 comments · Fixed by #33226

Comments

@xenova
Copy link
Contributor

xenova commented Aug 29, 2024

Similar to a previous issue (#31311) we had with vision models. cc @merveenoyan @amyeroberts @NielsRogge

Offending lines:

transformers/src/transformers/models/hiera/modeling_hiera.py

Line 340 in 5c1027b

pos_embeds = pos_embeds.reshape(1, int(math.sqrt(num_positions)), int(math.sqrt(num_positions)), dim)

transformers/src/transformers/models/hiera/modeling_hiera.py

Line 344 in 5c1027b

scale_factor=(h0 / math.sqrt(num_positions), w0 / math.sqrt(num_positions)),

We should probably abstract interpolate_pos_encoding, since this is reused across many different vision architectures.
@xenova xenova changed the title ONNX export fails for Hiera due to math and python type casts ONNX export fails for Hiera due to math.sqrt and python type int casts Aug 29, 2024
@xenova xenova changed the title ONNX export fails for Hiera due to math.sqrt and python type int casts ONNX export may fail for Hiera due to math.sqrt and python type int casts Aug 29, 2024
@qubvel
Copy link
Member

qubvel commented Aug 29, 2024

Hi @xenova, there are more issues with the current implementation of interpolate_pos_encoding

Maybe it's better to rewrite interpolation using size argument instead of scale to avoid errors described in the issue above, will it be compatible with ONNX export? WDYT?

@xenova
Copy link
Contributor Author

xenova commented Aug 29, 2024

Maybe it's better to rewrite interpolation using size argument instead of scale to avoid errors described in the issue above, will it be compatible with ONNX export? WDYT?

Yes I agree! Some other issues/PRs where this is discussed:

I'll try draft a PR for this soon.

@xenova xenova mentioned this issue Aug 30, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

🚨 Fix torch.jit.trace for interpolate_pos_encoding in all vision models huggingface/transformers
2 participants
@xenova @qubvel