-
Notifications
You must be signed in to change notification settings - Fork 590
[Feat] Add worker interface "update_config" #4277
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
Signed-off-by: ivyilike <[email protected]>
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
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 introduces an update_config interface to the worker, allowing for dynamic configuration updates. The implementation is straightforward, adding the method to NPUWorker and NPUModelRunner, along with a corresponding unit test. My main feedback is to replace the use of assert for input validation with a ValueError to ensure robustness, as assertions can be disabled. This change also requires a small update to the new test case.
| assert config_name in allowed_config_names, \ | ||
| f"Config `{config_name}` not supported. " \ | ||
| f"Allowed configs: {allowed_config_names}" |
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.
Using assert for input validation is not recommended as assertions can be disabled with the -O Python flag, which would lead to this check being skipped. It's more robust to raise a ValueError for invalid configuration keys.
Additionally, sorting the allowed_config_names in the error message will ensure deterministic output, which is good practice.
Note that this change will require updating test_update_config to expect a ValueError.
| assert config_name in allowed_config_names, \ | |
| f"Config `{config_name}` not supported. " \ | |
| f"Allowed configs: {allowed_config_names}" | |
| if config_name not in allowed_config_names: | |
| allowed = sorted(list(allowed_config_names)) | |
| raise ValueError(f"Config `{config_name}` not supported. Allowed: {allowed}") |
| model_runner.update_config({"load_config": {"load_format": "dummy"}}) | ||
| assert model_runner.load_config.load_format == "dummy" | ||
| # Raise error on non-existing config | ||
| with pytest.raises(AssertionError): |
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 job ! |
Signed-off-by: ivyilike <[email protected]>
Signed-off-by: ivyilike <[email protected]>
Signed-off-by: ivyilike <[email protected]>
Signed-off-by: ivyilike <[email protected]>
What this PR does / why we need it?
Some worker interfaces called by vLLM are missing in vLLM Ascend. It's good to add the support.
this PR add support to "update_config" interface. Update config in model_runner_v1.
For example, collective_rpc("update_config", overrides={"load_config": {"load_format": "auto"}})