Skip to content

Support ONNX export for causal LM sequence classifiers#27450

Merged
fxmarty merged 1 commit intohuggingface:mainfrom
dwyatte:causal_classification_onnx
Nov 16, 2023
Merged

Support ONNX export for causal LM sequence classifiers#27450
fxmarty merged 1 commit intohuggingface:mainfrom
dwyatte:causal_classification_onnx

Conversation

@dwyatte
Copy link
Contributor

@dwyatte dwyatte commented Nov 11, 2023

What does this PR do?

Partial fix for huggingface/optimum#1527 in optimum when exporting causal LMs with sequence classification support to ONNX

ONNX's argmax operator does not support int64, but that should not be needed here since these are just boolean tensors

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@ArthurZucker and @younesbelkada (CC @fxmarty)

fxmarty
fxmarty approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@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.

Thank you for the fix @dwyatte, LGTM! Feel free to open a PR in optimum as well to re-enable the export of those models for text-classification.

ArthurZucker
ArthurZucker approved these changes Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Nice followup to #24979. Not really sure why I did not use int at the time, but I ran the slow tests and this seems to be alright! Thanks 🤗

@ArthurZucker
Copy link
Collaborator

ArthurZucker commented Nov 14, 2023

The failing test might just need a rebase to main otherwise I'll skip it on main and work on a fix

amyeroberts
amyeroberts approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@amyeroberts amyeroberts 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 fixing!

@VsonicV VsonicV mentioned this pull request Nov 14, 2023
Merged
@dwyatte dwyatte force-pushed the causal_classification_onnx branch from 14c5099 to 8076250 Compare November 14, 2023 14:57
@dwyatte
Copy link
Contributor Author

dwyatte commented Nov 14, 2023 •
edited
Loading

The failing test might just need a rebase to main otherwise I'll skip it on main and work on a fix

@ArthurZucker I rebased in 8076250 but looks like something is still up with CI. Perhaps different tests get selected based on files changed or between PRs/main

@amyeroberts
Copy link
Contributor

amyeroberts commented Nov 14, 2023

@dwyatte Yes, the test fetcher selects a subset of the tests to run based on the files that are touched. In this case, the failing tests (I believe) are unreleated to your PR. The tests involving safetensors have had a patch pushed on main. Could you rebase on main to include these in the test runners?

@dwyatte dwyatte force-pushed the causal_classification_onnx branch from 8076250 to d8ab2c9 Compare November 14, 2023 21:06
@dwyatte
Copy link
Contributor Author

dwyatte commented Nov 14, 2023 •
edited
Loading

@amyeroberts I think there are some other problems on main still. Here's what's failing in the tests for this PR after the rebases in 8076250 and d8ab2c9

FAILED tests/models/switch_transformers/test_modeling_switch_transformers.py::SwitchTransformersModelTest::test_assisted_decoding_sample - RuntimeError: The size of tensor a (3) must match the size of tensor b (4) at non-singleton dimension 3
FAILED tests/models/t5/test_modeling_t5.py::T5ModelTest::test_assisted_decoding_sample - RuntimeError: The size of tensor a (3) must match the size of tensor b (4) at non-singleton dimension 3
FAILED tests/models/speech_to_text/test_modeling_speech_to_text.py::Speech2TextModelTest::test_tf_from_pt_safetensors - AssertionError: False is not true

@VsonicV
Copy link
Contributor

VsonicV commented Nov 15, 2023

@dwyatte Exactly same unrelated CI failures in #27351 . In addition to the failures related to safetensors (you already mentioned above), we also need to resolve the other CI failures caused by test_assisted_decoding_sample in tests_torch.

@ArthurZucker
Copy link
Collaborator

ArthurZucker commented Nov 15, 2023

Sorry both for the delays, I'll skip these 3 tests as well. cc @gante I'll look into the test_assisted_decoding_sample.

@VsonicV
Copy link
Contributor

VsonicV commented Nov 15, 2023

Hi, @ArthurZucker , regarding the failures caused by test_assisted_decoding_sample, there have already been some discussions in #26892 , these failures did not only happen for switch_transformers and t5, but also happened for blenderbot , pegasus and umt5 in my previous CI checks. It seems to be a more general issue that may happen to more than these listed models. Thanks for the help in fixing them!

@ArthurZucker
Copy link
Collaborator

ArthurZucker commented Nov 15, 2023 •
edited
Loading

just merged #27508 which should skip it for all models

@dwyatte dwyatte force-pushed the causal_classification_onnx branch from d8ab2c9 to fef0c8b Compare November 15, 2023 14:34
@dwyatte
Copy link
Contributor Author

dwyatte commented Nov 15, 2023

Thanks @ArthurZucker that took care of the remaining failures. This is ready to merge

@fxmarty fxmarty merged commit 1394e08 into huggingface:main Nov 16, 2023
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
@dwyatte @EduardoPach
Support ONNX export for causal LM sequence classifiers (huggingface#2…
7463f37 
…7450)

support onnx for causal lm sequence classification
@dwyatte dwyatte mentioned this pull request Dec 19, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ArthurZucker ArthurZucker ArthurZucker approved these changes

+2 more reviewers

@fxmarty fxmarty fxmarty approved these changes

@amyeroberts amyeroberts amyeroberts approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dwyatte @ArthurZucker @amyeroberts @VsonicV @fxmarty