[Refactor][Config] Remove FlashComm1 environment variable - #14707
[Refactor][Config] Remove FlashComm1 environment variable#14707kk-ss1999 wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the migration of the legacy Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Use additional_config.enable_flashcomm1 exclusively and migrate runtime examples, deployment configs, tests, and documentation away from the legacy environment variable. Signed-off-by: kk_1999 <97623903+kk-ss1999@users.noreply.github.com>
2b642e1 to
7b25a2b
Compare
|
👋 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.
Code Review
Suggested PR Title:
[Ops][Misc] Remove legacy VLLM_ASCEND_ENABLE_FLASHCOMM1 env var in favor of additional-configSuggested PR Summary:
### What this PR does / why we need it?
This PR removes the legacy `VLLM_ASCEND_ENABLE_FLASHCOMM1` environment variable, fully transitioning the enablement of FlashComm1 optimization to the `--additional-config '{"enable_flashcomm1": true}'` option. This cleanup spans across documentation, test configurations, and core codebase. Additionally, two potential crashes were identified in the test feature extraction scripts where missing or non-dictionary JSON configurations could trigger `AttributeError`s.
### Does this PR introduce _any_ user-facing change?
Yes, the legacy `VLLM_ASCEND_ENABLE_FLASHCOMM1` environment variable is no longer supported. Users must now use `--additional-config '{"enable_flashcomm1": true}'` to enable FlashComm1 optimization.
### How was this patch tested?
Tested via updated E2E nightly single-node and multi-node test configurations, along with unit tests in `tests/ut/test_ascend_config.py`.| for command in commands: | ||
| for flag in ("--additional-config", "--additional_config"): | ||
| try: | ||
| config = json.loads(command.cmd[command.cmd.index(flag) + 1]) | ||
| except (ValueError, IndexError, json.JSONDecodeError): | ||
| continue | ||
| if config.get("enable_flashcomm1"): | ||
| features.append("flashcomm1") | ||
| return features |
There was a problem hiding this comment.
There are two issues with this implementation:
- If
json.loadsparses a non-dictionary JSON value (such as a list, string, or number),configwill not have a.getmethod, resulting in anAttributeErrorthat crashes the script. We should verify thatconfigis a dictionary before calling.get(). - Returning
featuresearly from inside the loop prevents any subsequent feature extraction logic from executing if more checks are added to this function in the future. It is safer to use a flag and break the loops instead of returning early.
has_flashcomm1 = False
for command in commands:
for flag in ("--additional-config", "--additional_config"):
try:
config = json.loads(command.cmd[command.cmd.index(flag) + 1])
if isinstance(config, dict) and config.get("enable_flashcomm1"):
has_flashcomm1 = True
break
except (ValueError, IndexError, json.JSONDecodeError):
continue
if has_flashcomm1:
features.append("flashcomm1")
break| additional = _parse_json_flag(cmd_list, "--additional-config") or _parse_json_flag(cmd_list, "--additional_config") | ||
| for config_key, feature_name in _FEATURE_CONFIGS.items(): | ||
| if additional.get(config_key): | ||
| features.append(feature_name) |
There was a problem hiding this comment.
If _parse_json_flag returns None (e.g., when the flag is not found or parsing fails), additional will be None. This will cause an AttributeError when calling additional.get(config_key) or additional.get("enable_weight_nz_layout") later in the function. Appending or {} ensures additional is always a dictionary and prevents potential crashes.
| additional = _parse_json_flag(cmd_list, "--additional-config") or _parse_json_flag(cmd_list, "--additional_config") | |
| for config_key, feature_name in _FEATURE_CONFIGS.items(): | |
| if additional.get(config_key): | |
| features.append(feature_name) | |
| additional = _parse_json_flag(cmd_list, "--additional-config") or _parse_json_flag(cmd_list, "--additional_config") or {} | |
| for config_key, feature_name in _FEATURE_CONFIGS.items(): | |
| if additional.get(config_key): | |
| features.append(feature_name) |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
What this PR does / why we need it?
Removes the legacy
VLLM_ASCEND_ENABLE_FLASHCOMM1environment variable after its behavior was migrated toadditional_config.enable_flashcomm1. Runtime code, deployment examples, test configurations, and documentation are migrated to the config field where applicable.This is one independently reviewable part of the seven-variable split of #14637.
Does this PR introduce any user-facing change?
Yes.
VLLM_ASCEND_ENABLE_FLASHCOMM1is no longer read. Users must configure the behavior with--additional-config '{"enable_flashcomm1":true}'.How was this patch tested?
ruff checkandruff format --checkon changed Python filesPython byte-compilation of changed Python files
YAML parsing of changed YAML files
JSON parsing of concrete
additional-configpayloadsScan confirming no active references to
VLLM_ASCEND_ENABLE_FLASHCOMM1remain outside migration documentation and the ignored-variable unit testNPU end-to-end tests were not run in this Windows environment
vLLM version: v0.27.1
vLLM main: vllm-project/vllm@58d3918