[Deprecation] Remove deprecated environment variables#32812
[Deprecation] Remove deprecated environment variables#32812ProExpertProg merged 2 commits intomainfrom
Conversation
Signed-off-by: yewentao256 <zhyanwentao@126.com>
yewentao256
requested review from
ProExpertProg,
WoosukKwon,
hmellor,
houseroad,
mgoin,
robertgshaw2-redhat,
tjtanaa,
tlrmchlsmth and
youkaichao
as code owners
![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 removes several deprecated environment variables related to attention configuration. The changes are consistent across the codebase, with updates to configuration classes, environment variable definitions, and direct usages in tests and platform-specific code. The migration from environment variables to fields within the AttentionConfig dataclass is well-executed and improves configuration management. I found no high or critical issues in these changes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
| if envs.VLLM_V1_USE_PREFILL_DECODE_ATTENTION: | ||
| from vllm.config import get_current_vllm_config | ||
|
|
||
| vllm_config = get_current_vllm_config() |
There was a problem hiding this comment.
Can you make sure that this doesn't cause Current vLLM config is not set. log spam?
There was a problem hiding this comment.
There is no log spam now, raise AssertionError()
def get_current_vllm_config() -> VllmConfig:
if _current_vllm_config is None:
raise AssertionError(
"Current vLLM config is not set. This typically means "
MatthewBonanni
left a comment
MatthewBonanni
left a comment
There was a problem hiding this comment.
Thanks for doing this! Can you make sure to double check that you got everything? Looks like there are some uses in test-amd.yaml, e.g.
Line 1476 in e675dda
Signed-off-by: yewentao256 <zhyanwentao@126.com>
OK everything checked, thanks! |
github-actions
bot
added
the
ready
| commands: | ||
| - uv pip install --system -r /vllm-workspace/requirements/kv_connectors_rocm.txt | ||
| - VLLM_ATTENTION_BACKEND=ROCM_ATTN bash v1/kv_connector/nixl_integration/config_sweep_accuracy_test.sh | ||
| - bash v1/kv_connector/nixl_integration/config_sweep_accuracy_test.sh --attention-backend ROCM_ATTN |
There was a problem hiding this comment.
Does this actually work? It doesn't seem that the extra arguments are forwarded through the bash script
There was a problem hiding this comment.
It doesn't. This PR #32837 fixes it, along with another error caused by the deprecation of VLLM_V1_USE_PREFILL_DECODE_ATTENTION
There was a problem hiding this comment.
Does this actually work? It doesn't seem that the extra arguments are forwarded through the bash script
They indeed are forwarded. Thanks for the cc btw :)
…2812) Signed-off-by: yewentao256 <zhyanwentao@126.com> Signed-off-by: mohammad najafi <mohammad.najafi@amd.com>
…2812) Signed-off-by: yewentao256 <zhyanwentao@126.com> Signed-off-by: 陈建华 <1647430658@qq.com>
|
how's one supposed to set FA version manually now that |
You can use |
…2812) Signed-off-by: yewentao256 <zhyanwentao@126.com>
…2812) Signed-off-by: yewentao256 <zhyanwentao@126.com>
7 participants
Purpose
As v0.14.0 has been out, we can remove these deprecated envs.
CC: @MatthewBonanni