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

Support ONNX export for causal LM sequence classifiers #27450

Merged
merged 1 commit into from
Nov 16, 2023

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)

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.

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

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

Copy link
Collaborator

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

@dwyatte
Copy link
Contributor Author

dwyatte 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

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

@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

@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

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

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
3 checks passed
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
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.

5 participants