-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Make mistral-common dependency optional #16738
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 mistral-common dependency optional #16738
Conversation
|
Moving the imports won't fix the typings btw. :) You need to use try/except ImportError instead of importlib. Edit: Actually, I see it's now complaining about |
cee35a4 to
1671309
Compare
|
Yep I tried moving things a bit and it didn't work so I just added the flags to ignore the relevant lines if that's ok. |
Yep, though I think try/except ImportError is still preferable. Also, since we are no longer installing |
|
Thank you for taking our concerns into account. |
CISC
left a comment
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, thank you! However I'm currently away travelling so great if @ngxson can take a final look and potentially merge.
ngxson
left a comment
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.
Nice! Thanks for addressing the issue
* Make mistral-common dependency optional * Fix typing
* Make mistral-common dependency optional * Fix typing
In PRs #14737 #15420, we added our format support for conversion, and therefore
mistral-commondependency for conversion, and made chat templates optional. This is because we noticed that chat templates and custom tokenization are generally not matchingmistral-commonwhich can affect model performance.The goal of these PRs was to provide to the community the freedom to experience the best use of our models with the llama.cpp engine.
However part of this was not well welcomed by the community that particularly disliked having
mistral-commonas a hard dependency as discussed in #16146. This PR aims to remove this hard dependency and instead raise an error if it is not installed. This occurs for converting Mistral models for the following cases:Fix #16146
cc @CISC @ngxson hope this matches what was dicussed in #16146 and that it passes the CI in particular for typing.