-
Notifications
You must be signed in to change notification settings - Fork 27.7k
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
Fix mixtral ONNX Exporter Issue. #29858
Fix mixtral ONNX Exporter Issue. #29858
Conversation
@thiagocrepaldi FYI. |
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.
to expedite the code review, use git blame and find previous users who actually approved prs for changes in this file and add them to the code review.
Otherwise, the PR can be waiting for triage a long time
@ArthurZucker FYI
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 a lot!
Would love to merge this, we had to go with this because indexing is faster with list.
would you mind just running some benchmarks in generation?
Really sorry we don't have them yet in transformers
but coming soon! (FYI @ydshieh )
Hey @ArthurZucker any updates on this? |
I think what @ArthurZucker 's comment is about
on your own side to see if the PR and |
On point @ydshieh . This is very core, and we added this change for a reason, we need a good reason to update, and make sure we are not affecting performances |
@ArthurZucker, I actually found it faster when we use the changes in the PR. Environment specs: With the PR changes: Without PR changes: Thank you |
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.
Alright. Let's also update the qwen
models as well then! (ctrl + f for this operation)
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
I could not find anything like that on qwen models, are you sure it uses the same thing? |
This here! :
qwen2 sorry |
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
This PR addresses issue #29857
by removing the list conversion of top_x and idx in the Mixtral model code, which currently obstructs the ONNX exporter's functionality.
By removing this conversion, the Mixtral model can be exported successfully using the ONNX exporter, resolving the compatibility issue.