-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Add exllamav2 better #27111
Add exllamav2 better #27111
Conversation
The documentation is not available anymore as the PR was closed or merged. |
Since @ArthurZucker is out next week, it would be great if you could review this PR @amyeroberts. I'm trying to have this PR in the next release. In this modified version, I make sure to deprecate |
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 adding this!
Two general comments:
- Having two additional config arguments isn't ideal. There should be ideally a single parameter which configures this behaviour and is "off" when not set
- There needs to be argument verification for safe deprecation of the old flag
Co-authored-by: amyeroberts <[email protected]>
…nsformers into add_exllamav2_better
Thanks for the review @amyeroberts . I've addressed all the points. LMK if something is missing ! |
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 on this!
There's at least one more iteration of the input argument logic - but we're close! Otherwise code and PR looks v. good
Co-authored-by: amyeroberts <[email protected]>
Thanks for the deep review @amyeroberts ! I've added the input logic and simplified the link with optimum config. |
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.
Great - thanks for the work iterating on this!
Just a few nits were the docs need to be updated. Otherwise LGTM!
Co-authored-by: amyeroberts <[email protected]>
Thanks again @amyeroberts for iterating on this PR in such a short time ! |
* add_ xllamav2 arg * add test * style * add check * add doc * replace by use_exllama_v2 * fix tests * fix doc * style * better condition * fix logic * add deprecate msg * deprecate exllama * remove disable_exllama from the linter * remove * fix warning * Revert the commits deprecating exllama * deprecate disable_exllama for use_exllama * fix * fix loading attribute * better handling of args * remove disable_exllama from init and linter * Apply suggestions from code review Co-authored-by: amyeroberts <[email protected]> * better arg * fix warning * Apply suggestions from code review Co-authored-by: amyeroberts <[email protected]> * switch to dict * Apply suggestions from code review Co-authored-by: amyeroberts <[email protected]> * style * nits * style * better tests * style --------- Co-authored-by: amyeroberts <[email protected]>
What does this PR do ?
This PR is a modified version of this PR that make
disable_exllama
go through a deprecation cycle.I also fixed the following test
test_device_and_dtype_assignment
that broke other tests in the CI introduced by this PR.I confirm that all the tests are green