-
Notifications
You must be signed in to change notification settings - Fork 31k
Improve AutoTokenizer error message for Voxtral models missing mistral-common #41592
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
base: main
Are you sure you want to change the base?
Improve AutoTokenizer error message for Voxtral models missing mistral-common #41592
Conversation
…Khansa435/transformers into fix-bad-error-voxtral-tokenizer
|
Hi @ArthurZucker @Rocketknight1 My PR passes 21 out of 22 checks and 1 is pending. |
…Khansa435/transformers into fix-bad-error-voxtral-tokenizer
|
@ArthurZucker @Rocketknight1 It passed all checks after the changes I made but then when I merged both branches to stay up-to-date now it gave an error in tests but now all checks are being successful. Plz review! |
|
@vasqu Can you please review this PR.It passess 21 out of 22 checks and none failed.One is pending. |
|
Hey @Khansa435, I directly responded in the issue #41553 (comment). Thx for the PR, but I will leave the decision to the respective maintainers who know more than me on this :D |
|
Also dont worry about the CI, we had some issue which should be good now |
|
@vasqu may you please help me to contribute on some issue as I'm new to opensource I just want to learn. |
|
Hey @AvinashDwivedi, sorry about this one. It got a bit messy, usually we should only open 1 PR / coordinate with other people that already provided something. No worries, we will guide you as best as we can if the PR makes sense! We have contributions guide here for example https://huggingface.co/docs/transformers/contributing and those labeled with good first issue are good to start with in general; just make sure that not others already worked on it or that your solution is not the same |
9bdf8a1 to
7685851
Compare
|
Hi, Can you please review this and let me know if it satisfies the requirements :) |
|
Hi, Just checking in to see if there’s any update on this PR. Hacktoberfest is wrapping up soon, and I’d love to have this issue resolved before the event ends. |
…Khansa435/transformers into fix-bad-error-voxtral-tokenizer
…Khansa435/transformers into fix-bad-error-voxtral-tokenizer
…Khansa435/transformers into fix-bad-error-voxtral-tokenizer
…Khansa435/transformers into fix-bad-error-voxtral-tokenizer
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto |
|
Reverted to the original error message and adjusted indentation as suggested. Thanks for clarifying! |
|
Thanks for bearing with me and iterating! I started to look into this myself as this fix didn't work. Turns out that this problem goes a bit deeper so please wait a bit. |
|
Thanks a lot for the update and for taking the time to look into this deeper! I really enjoyed working on this and learned a lot through your feedback. This was my first deeper dive into the AutoTokenizer internals. |
What does this PR do?
Adds a clearer ImportError message when users try to load a Voxtral tokenizer without having
mistral-commoninstalled.Issue
Fixes #41553
Fixes misleading
TypeError: not a stringwhen loading Voxtral models.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@ArthurZucker @Rocketknight1
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.