[UX] Fail if an invalid attention backend is specified #22217
Conversation
Signed-off-by: mgoin <michael@neuralmagic.com>
|
👋 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 🚀 |
mgoin
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly changes the behavior to fail fast when an invalid attention backend is specified via an environment variable, rather than silently falling back to a default. The implementation is straightforward and the tests are updated accordingly to verify the new exception-raising behavior. I've identified one potential issue related to caching that could cause stale environment variable values to be used, which could lead to unexpected behavior.
| if selected_backend is None: | ||
| raise ValueError( | ||
| f"Invalid attention backend: '{backend_by_env_var}'. " | ||
| f"Valid backends are: {list(_Backend.__members__.keys())}") |
There was a problem hiding this comment.
The _cached_get_attn_backend function is decorated with @cache, which memoizes its results. However, it reads envs.VLLM_ATTENTION_BACKEND directly. This can lead to incorrect behavior if the environment variable is changed while the process is running, as a cached result based on a stale value of the environment variable might be returned.
A similar issue is already addressed for VLLM_USE_V1 in the get_attn_backend wrapper function, where the environment variable is read and passed as an argument to the cached function. A similar approach should be taken for VLLM_ATTENTION_BACKEND to ensure that changes to the backend configuration are always respected.
DarkLight1337
left a comment
DarkLight1337
left a comment
There was a problem hiding this comment.
Makes sense, thanks for improving the UX!
github-actions
bot
added
the
ready
|
Plugin Tests are failing because of this PR: https://buildkite.com/vllm/ci/builds/26033/steps/canvas?sid=01987a40-050a-40d2-b36c-92e23ebf4d0c |
…22217) Signed-off-by: mgoin <michael@neuralmagic.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com> Signed-off-by: Jinzhen Lin <linjinzhen@hotmail.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com> Signed-off-by: Noam Gat <noamgat@gmail.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com> Signed-off-by: Paul Pak <paulpak58@gmail.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com> Signed-off-by: Diego-Castan <diego.castan@ibm.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com> Signed-off-by: Xiao Yu <xiao.yu@amd.com>
…22217) Signed-off-by: mgoin <michael@neuralmagic.com>
3 participants
Purpose
As the title states, I think if an invalid attention backend is manually specified like VLLM_ATTENTION_BACKEND=INVALID it should fail rather than fallback to some default
Rob found that this PR #21966 causes fallback to V0 if that env variable is set incorrectly
Test Plan
CI
Test Result