-
-
Notifications
You must be signed in to change notification settings - Fork 12k
[Misc] update dict parse to EPLBConfig from json dumps to dict unpacking #23305
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
[Misc] update dict parse to EPLBConfig from json dumps to dict unpacking #23305
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 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 🚀 |
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 comments out the logic for handling eplb_config, which is critical for expert parallelism load balancing. The title "[Test] test remove this code after ci" strongly suggests this is a temporary change for testing and should not be merged in its current state. If this functionality is intended to be removed, the code should be deleted rather than commented out. Otherwise, it must be restored before merging.
vllm/engine/arg_utils.py
Outdated
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.
This change comments out the logic for processing eplb_config. Based on the pull request title, this appears to be a temporary change for testing purposes. Merging this as-is would break the expert parallelism load balancing configuration. This code should be restored before merging.
| # if isinstance(self.eplb_config, dict): | |
| # self.eplb_config = EPLBConfig.from_cli(json.dumps( | |
| # self.eplb_config)) | |
| if isinstance(self.eplb_config, dict): | |
| self.eplb_config = EPLBConfig.from_cli(json.dumps( | |
| self.eplb_config)) |
55e41c7 to
0782444
Compare
0782444 to
afd50f6
Compare
Signed-off-by: rongfu.leng <[email protected]>
afd50f6 to
919346a
Compare
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.
Thanks!
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]> Signed-off-by: johnnynunez <[email protected]>
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]>
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]>
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]>
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]>
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]> Signed-off-by: Ekagra Ranjan <[email protected]>
…ing (vllm-project#23305) Signed-off-by: rongfu.leng <[email protected]>
Purpose
Test Plan
Test Result
(Optional) Documentation Update
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.