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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
add mt5 to ORTConfigManager conf list #341
add mt5 to ORTConfigManager conf list #341
Conversation
The documentation is not available anymore as the PR was closed or merged. |
Hi @chainyo, thank you for adding mt5 in transformers and in optimum! Sorting the models in alphabetical order makes sense to me. Can you also add a test of mt5 on |
Same as above, I re-arranged in alphabetical order all tested architectures. Because EDIT: tests are failing for MT5 because it's not the main branch fro transformers installed while testing. Should we wait until it's included in a future release? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactoring looks neat to me, pinging @regisss and @michaelbenayoun for some inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this @chainyo!!
The build PR doc test should pass now |
As for opening an issue for implementing all available ONNX models in the ORTConfigManager, why not. It could be a good way to track which models still have to be added. What do you think @JingyaHuang @echarlaix @mfuntowicz @fxmarty ? |
Besides, it seems that the CI of the optimization has one failed test:
I can't see details about where it failed. Can you run the test locally to get the complete log of the failure? Thank you! |
IMO, it makes sense to keep them all in the test as far as it won't slow down the CI. When it turns to be too slow, we can archive a part of them as slow tests. |
I was actually talking about the suggestion of opening an issue such as this one to keep track of the models to add to |
@regisss oh sorry, I misunderstood it. I think that is a good idea! Both users and contributors could have a better idea about what has been integrated, what needs to be integrated, and probably some references on how to do that. |
Cool, I will open it right now!
I will take a look at the failure message. |
When you scroll up, you can access the full traceback. It's still due to MT5 not being implemented in Transformers. Can you re-launch the CI tests, or should I commit to rerun them automatically? @JingyaHuang
|
Hi @chainyo, Thanks for explaining, that's clear. Indeed, as the CI leverages the latest transformers, it won't pass until your contribution of MT5 ONNX config makes its way to a release of transformers. |
Hi @chainyo,
it fails at
Error:
If I set |
HI @customer101, could you check your model's Are you using it in a pipeline or with vanilla PyTorch? Where do you add the |
I used transformers main branch, the model is vanilla PyTorch,
does the conversion, then when I save the model it dumps 2 onnx models(encoder and decoder).
I noticed that mt5 optimizations are not implemented yet in onnxruntime, so I think I won't gain much speed-up by converting to onnx. |
Transformers v4.22.0 has included the MT5 OnnxConfig and stuff. Can we relaunch tests and see if all is fine here? |
Hi @chainyo, Sure, before launching the tests again, can you do a rebase with the main branch of Optimum? Thank you! |
d6b8632
to
7fcf3e4
Compare
It's done @JingyaHuang Optimizer behavior has changed, so adding a feature argument is not needed anymore! |
I fixed a typo for tests. I don't know if |
Hi @chainyo, the current As @customer101 observes, for the moment, there is a problem with the dimension of past-key-value, so you need to set Here is the snippet:
|
Hi @customer101 , Thanks for reporting the issue with
|
Thanks, I fixed the code. I didn't see it was separated into two different test functions. It should be good for now. |
Hi @JingyaHuang, I see there are three problems according to the error tests in the CI.
I don't know if
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @chainyo,
Thanks for tracking the PR.
To pass the CIs, here are several modifications to apply:
- For the benchmark suite test, can you rebase your branch? The evaluate module has been added since Add evaluate to setup聽#384
- For the MT5 support, the name of hidden size shall be
d_model
. - ONNX Runtime optimization supports bert-like, gpt-like and bart-like models. As t5/mt5 are encoder-decoder models, I suggest setting the model type as "bart" to benefit from some extra fusions.
- And we recently reduced the size of some models to accelerate the CIs, can you use "hf-internal-testing/tiny-random-onnx-mt5" for the test?
With the modifications beyond, the PR should be good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating the PR, LGTM!
What does this PR do?
Add MT5 to
ORTConfigManager
.Fixes #321
I re-arranged in alphabetical order all available models. I can put it back like it was if needed. 馃
@JingyaHuang
Aside from this PR, I was wondering if opening an issue like huggingface/transformers#16308 for implementing all available onnx models in the ORTConfigManager could be nice?