-
-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[Core] Rename PassConfig flags as per RFC #27995 #29646
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
[Core] Rename PassConfig flags as per RFC #27995 #29646
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
|
This pull request has merge conflicts that must be resolved before it can be |
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 refactors PassConfig flags to be more descriptive, which is a great improvement for clarity. The changes are mostly mechanical renames across the test suite and configuration files. However, I've found a critical issue in the backward compatibility logic for the deprecated flags in vllm/config/compilation.py. The current implementation incorrectly handles cases where old flags are set to False, which would unintentionally enable features. I've provided a suggestion to fix this to ensure correct behavior.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: arpitkh101 <[email protected]>
15411fa to
0c4eb98
Compare
Signed-off-by: arpitkh101 <[email protected]>
ece81a1 to
289336c
Compare
ProExpertProg
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.
A few minor notes, looks ready otherwise! Could you also add a test that checks that specifying the old fields results in new fields set correctly and a warning (see #29557 for example)
Signed-off-by: arpitkh101 <[email protected]>
|
@arpitkh101 will you be able to finish this today so we can include it in the 0.12.0 release? |
If not, we should bump the removal version. Deprecation messges must be present for a whole minor version. |
|
I think I'll be able to push these changes today. |
@arpitkh101 please keep us posted! Someone else can pick up and help if you're not able to finish today, would really like to get this in for this release! |
Signed-off-by: arpitkh101 <[email protected]>
…flags with None Signed-off-by: arpitkh101 <[email protected]>
Head branch was pushed to by a user without write access
Signed-off-by: arpitkh101 <[email protected]> Co-authored-by: Luka Govedič <[email protected]> (cherry picked from commit d7284a2)
hmellor
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.
Couple of nits, not necessary for the release but would be nice to clean up asap.
…oject#29646) Signed-off-by: arpitkh101 <[email protected]> Co-authored-by: Luka Govedič <[email protected]> Signed-off-by: Xingyu Liu <[email protected]>
Purpose
Implements the PassConfig flag renaming from issue #27995.
Changes: