Skip to content

Add position ids in ONNX export and ORT#1381

Merged
fxmarty merged 10 commits intohuggingface:mainfrom
fxmarty:add-position-ids
Sep 18, 2023
Merged

Add position ids in ONNX export and ORT#1381
fxmarty merged 10 commits intohuggingface:mainfrom
fxmarty:add-position-ids

Conversation

@fxmarty
Copy link
Contributor

@fxmarty fxmarty commented Sep 13, 2023

Supersede #1289 & #984

Fix a critical issue where the exported ONNX models could not produce meaningful results for batch size > 1 due to a very strong assumption in transformers: that all sequences are of the same length if position_ids is not passed. See: https://github.com/huggingface/transformers/blob/a796f7eea6c86b54671a6f522cebbe41f630bb62/src/transformers/models/gpt2/modeling_gpt2.py#L802

Left to do:

  • Add an option to export without position_ids for backward compatibility (disabled by default)
  • Fix models on the Hub [will only do for those under Optimum namespace - but not until we have a release]
  • Add ORT tests that would have catched the issue [the added batched generation test indeed fails on main)

cc @xenova

fxmarty and others added 4 commits September 13, 2023 17:38
Co-authored-by: Kunal Vaishnavi <kvaishnavi@microsoft.com>
Co-authored-by: "Feng, Jiqing" <jiqing.feng@intel.com>
@fxmarty fxmarty requested a review from echarlaix September 13, 2023 16:33
@xenova
Copy link
Contributor

xenova commented Sep 16, 2023

Thanks for this fix @fxmarty! I'll be testing this with transformers.js hopefully soon.

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.

Thanks for handling this @fxmarty

Comment on lines +89 to +90
# TODO: remove no_position_ids once optimum is sufficiently above 1.13
self.no_position_ids = no_position_ids
Copy link
Member

Choose a reason for hiding this comment

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

Should we make helper function like deprecated(version_number) that does nothing when the version is lower than that and fails after?
This way we dont forget to remove deprecated code.

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.

3 participants