-
Notifications
You must be signed in to change notification settings - Fork 26.3k
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
Make T5 compatible with ONNX #5518
Make T5 compatible with ONNX #5518
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5518 +/- ##
==========================================
- Coverage 77.83% 76.81% -1.03%
==========================================
Files 141 141
Lines 24634 24637 +3
==========================================
- Hits 19175 18925 -250
- Misses 5459 5712 +253
Continue to review full report at Codecov.
|
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 investigating, your PR looks very cool.
Just one comment so far, which is more related to the coding style we use in the repo.
Happy to merge right after 👍.
Thanks a lot for the review @mfuntowicz! I adjusted it to the coding style you outlined. Feel free to merge if you're happy with it. |
LGTM! Thanks @abelriboulot, great addition 👍 |
Hey, did you happen to make a colab which shows this off? I was trying to figure out exporting T5 as ONNX a week ago, but got stuck. It seems you've fixed it though? |
@ConProgramming sure thing, I’ll share something this weekend! |
@abelriboulot Did you ever get around to making that colab? It'd help a lot. 😅 |
Hey @ConProgramming, I had a very ad-hoc solution for this, therefore I worked on a PR to make the huggingface conversion compatible with all models with a compatible graph. You can take a look at it there: #5687 I checked and it seems to work well! Let me know if it works for you. |
Thanks @abelriboulot, but I'm still having some issues with it... it works with t5-base, but depending on how I provide the path to my own model I get two different errors:
Is it designed to work with finetuned models? |
Hey @ConProgramming, it should work on fine tuned models, you can have a look at the test_onnx file as an example of this. The model path should be to the directory that contains the model (and the tokenizer in case you do not specify it). It looks like the second error relates to not being able to find a tokenizer, is it present in your directory? If you are using another directory / pretrained model you can specify it with --tokenizer |
@abelriboulot Adding |
Oh awesome! Great to hear it! I might add a message to make it more obvious to the user. |
I tried this (for cpu): but getting error: ONNX opset version set to: 12 Am I doing something wrong here? |
Hey @oliversms, are you using the specific fork or master? I can confirm the command you submitted works on my side. |
Apologies for the delayed reply; Im actually using the fork. I beleive it may have been an env related issue. However after getting past that issue Im now running into a new issue: Attempting set padding & truncation to True doesnt fix the issue. |
Hey @oliversms! It looks like you are not using the right branch. You need this specific branch for it to work. Hope it works for you! Abel |
If anyone needs, I created a small package (onnxt5) which lets you easily and efficiently export T5 and serve it! Feel free to raise issues, it's an alpha at the moment. |
@@ -953,6 +958,12 @@ def forward( | |||
|
|||
hidden_states = encoder_outputs[0] | |||
|
|||
# If the model is only provided with either input_ids or inputs_embeds, |
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.
This actually does not make much sense - if no decoder_input_ids
are provided they should not just be set to the encoder_input_ids
IMO. @mfuntowicz - can we revert this change? or will it break onnx support?
@abelriboulot Hi, I pulled your branch and tried to convert a t5-base with and still got the "Error while converting the model: You have to specify either decoder_input_ids or decoder_inputs_embeds" Any ideas? |
This is a small PR to make T5 exportable to ONNX with any op>9. It addresses an issue outlined in #5075 where T5 would not export to ONNX. In order to make it exportable, 2 changes are made:
model(input_ids=input_ids, decoder_input_ids=input_ids)
. It also allows t5 to be executed with the more common paradigm of calls like model(inputs)