-
Notifications
You must be signed in to change notification settings - Fork 301
Update dependency bounds for llmcompressor 0.9 release #2086
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?
Conversation
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
Summary of ChangesHello @dhuangnm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on updating the dependency version constraints within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Signed-off-by: Dan Huang <[email protected]>
Signed-off-by: Dan Huang <[email protected]>
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.
Code Review
This pull request updates the dependency bounds for several packages. The changes are mostly straightforward version bumps. However, I've identified a potential dependency issue with accelerate in non-release builds, which could lead to conflicts with auto-round. I've also pointed out a redundant conditional for the auto-round dependency that can be simplified. My suggestions aim to ensure dependency consistency across different build types and improve code clarity.
| "accelerate>=1.6.0,<=1.10.1" | ||
| "accelerate>=1.10.0,<=1.12.0" | ||
| if BUILD_TYPE == "release" | ||
| else "accelerate>=1.6.0" |
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.
The accelerate dependency for non-release builds is set to >=1.6.0. However, the comment on line 131 states that auto-round==0.9.1 requires accelerate >= 1.10.0. Since auto-round is a dependency for all build types, the lower bound for accelerate should be 1.10.0 for non-release builds as well to avoid potential dependency conflicts.
| else "accelerate>=1.6.0" | |
| else "accelerate>=1.10.0" |
setup.py
Outdated
| else "compressed-tensors>=0.12.3a2" | ||
| ), | ||
| ("auto-round==0.9.1"), | ||
| ("auto-round==0.9.1" if BUILD_TYPE == "release" else "auto-round==0.9.1"), |
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.
The conditional logic for the auto-round dependency is redundant as it specifies the same version 0.9.1 for both release and non-release builds. This can be simplified to a static dependency for better readability and maintainability.
| ("auto-round==0.9.1" if BUILD_TYPE == "release" else "auto-round==0.9.1"), | |
| ("auto-round==0.9.1"), |
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.
I think we may want to update this to be its own install tag, similar to dev
e. g uv pip install -e .[dev,autoround]
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.
Updated, I'll file another PR for the workflows to use the new [autoround] install tag.
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.
https://github.com/neuralmagic/llm-compressor-testing/pull/183 to update the workflows accordingly.
dsikka
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.
Thanks!
Just an FYI - I think we want to pin the transformers version, regardless of release. This is for the time being while we ensure we can support the v5 release that will be out soon.
Signed-off-by: Dan Huang <[email protected]>
Got it, updated the upper bound for transformers to 5.0.0 although currently its latest is 5.0.0.rc0 on the PyPi. |
Signed-off-by: Dan Huang <[email protected]>
Oh sorry, I meant the upper bound should be the current stable release |
| ("auto-round==0.9.1"), | ||
| ], | ||
| extras_require={ | ||
| "autoround": [ |
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! We will also need to change the transformers test workflow to run autoround tests as they require this install:
| - name: Running AutoRound Tests |
If we think this will be a bigger change, we can make the autoround optional install change in a followup
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.
Good catch, updated the workflow to install "autoround" dependency as well.
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.
Should be a separate job, no?
Sorry, I think my request is becoming larger than we expected. We can revert to install autoround how you were previously (i.e not using the optional install) and I'll add a follow-up to fix this
Signed-off-by: Dan Huang <[email protected]>
Signed-off-by: Dan Huang <[email protected]>
Gotcha, updated. |
SUMMARY:
Update dependency bounds based on their latest releases.
TEST PLAN:
All nightly tests.
Run with upper bounds: https://github.com/neuralmagic/llm-compressor-testing/actions/runs/19864870462
Run with lower bounds: https://github.com/neuralmagic/llm-compressor-testing/actions/runs/19867046299