[Misc] Fix Current vLLM config is not set. warnings, assert to avoid issues in the future#31747
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the vLLM configuration handling by replacing a warning with an assertion when the configuration is not set. This helps to catch potential bugs early. An environment variable VLLM_ALLOW_DEFAULT_CONFIG is introduced to maintain the old behavior for testing purposes, which is a thoughtful addition.
The other changes in the pull request are logical consequences of this stricter configuration check. The use of a lazy dictionary in vllm/model_executor/layers/fused_moe/cpu_fused_moe.py correctly defers the instantiation of CustomOp subclasses until they are needed, avoiding errors during module import. Similarly, caching the GroupedTopk instance in vllm/model_executor/layers/fused_moe/layer.py is a good optimization that also resolves the configuration access issue.
Overall, the changes are well-implemented, improve code quality, and I have no concerns.
|
Hi @LucasWilkinson, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
ProExpertProg
left a comment
There was a problem hiding this comment.
Thanks for addressing this!
|
Hi @LucasWilkinson, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
1 similar comment
|
Hi @LucasWilkinson, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
ProExpertProg
left a comment
There was a problem hiding this comment.
Just a nit for the error message, thanks for this cleanup!
|
Hi @LucasWilkinson, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com>
ff497f1 to
54b1339
Compare
…d issues in the future (vllm-project#31747) Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com> Signed-off-by: Lucas Wilkinson <LucasWilkinson@users.noreply.github.com> Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
|
Thanks for fixing this! Really appreciate it |
…d issues in the future (vllm-project#31747) Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com> Signed-off-by: Lucas Wilkinson <LucasWilkinson@users.noreply.github.com> Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
…d issues in the future (vllm-project#31747) Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com> Signed-off-by: Lucas Wilkinson <LucasWilkinson@users.noreply.github.com> Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
…d issues in the future (vllm-project#31747) Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com> Signed-off-by: Lucas Wilkinson <LucasWilkinson@users.noreply.github.com> Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
#30531 and #29575 introduced accesses to
get_current_vllm_configon boot that are outside ofset_current_vllm_configcontexts leading to repeated logswhen
get_current_vllm_configfalls back to creating a default config. This also lead to slight config propagation bugs as some custom ops would see the default config instead of the real one.This PR fixes those accesses and makes it an assert to try to avoid this in the future.
Longer term we should consider something like: #30859
TODO: get CI green with targeted additions of
VLLM_ALLOW_DEFAULT_CONFIG