[Refactor][Config] Remove msMonitor daemon environment variable - #14708
[Refactor][Config] Remove msMonitor daemon environment variable#14708kk-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 msmonitor daemon configuration from a legacy environment variable to a structured configuration field. By removing the dependency on MSMONITOR_USE_DAEMON, the codebase achieves better configuration consistency and aligns with the project's broader refactoring goals for Ascend-specific settings. 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.msmonitor_use_daemon as the only configuration source and update profiler behavior, tests, and documentation. Signed-off-by: kk_1999 <97623903+kk-ss1999@users.noreply.github.com>
282b5a7 to
2bd4fb2
Compare
![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 removes the legacy MSMONITOR_USE_DAEMON environment variable, migrating its functionality entirely to the additional_config.msmonitor_use_daemon configuration option. The documentation, profiler code, and associated unit tests have been updated accordingly. The reviewer provided constructive feedback, including a suggested PR title and summary formatted according to the repository style guide, and pointed out that some existing unit tests still reference the removed environment variable and need to be cleaned up to prevent test failures.
| | `mega_moe_max_tokens` | int | `65536` | Per-rank token capacity after dispatch in the mega moe (dispatch_ffn_combine) fused operator. When load imbalance causes a rank to receive more tokens than this limit, the excess tokens are dropped and skipped from computation, degrading accuracy. Do not set this too large: workspace memory scales linearly with this value. | | ||
| | `enable_flashcomm1` | bool | `False` | Whether to enable FlashComm1 optimization. Can also be configured via the `VLLM_ASCEND_ENABLE_FLASHCOMM1` environment variable during the migration period. | | ||
| | `msmonitor_use_daemon` | bool | `False` | Whether to use daemon mode for msmonitor. Can also be configured via the `MSMONITOR_USE_DAEMON` environment variable during the migration period. | | ||
| | `msmonitor_use_daemon` | bool | `False` | Whether to use daemon mode for msmonitor. The legacy `MSMONITOR_USE_DAEMON` environment variable is no longer supported. | |
There was a problem hiding this comment.
Suggested PR Title:
[Config][Misc] Remove msMonitor daemon environment variableSuggested PR Summary:
### What this PR does / why we need it?
Removes the legacy `MSMONITOR_USE_DAEMON` environment variable after its behavior was migrated to `additional_config.msmonitor_use_daemon`. Runtime code, deployment examples, test configurations, and documentation are migrated to the config field where applicable.
### Does this PR introduce _any_ user-facing change?
Yes. `MSMONITOR_USE_DAEMON` is no longer read. Users must configure the behavior with `--additional-config '{"msmonitor_use_daemon":true}'`.
### How was this patch tested?
- Unit tests in `tests/ut/test_ascend_config.py` and `tests/ut/profiler/test_torch_npu_profiler.py`
- Ruff check and formattingReferences
- The repository style guide requires providing a suggested PR Title and PR Summary in markdown code blocks. (link)
| def test_msmonitor_daemon_ignores_removed_env(self, mock_fix_incompatible_config): | ||
| test_vllm_config = VllmConfig() | ||
| test_vllm_config.additional_config = {"msmonitor_use_daemon": False} | ||
| with patch.dict(os.environ, {"MSMONITOR_USE_DAEMON": "1"}): | ||
| ascend_config = init_ascend_config(test_vllm_config) | ||
|
|
||
| self.assertFalse(ascend_config.msmonitor_use_daemon) |
There was a problem hiding this comment.
The existing unit tests test_migrated_config_falls_back_to_envs (around line 409) and test_migrated_config_overrides_envs (around line 461) still reference MSMONITOR_USE_DAEMON and assert fallback/override behaviors for it.
Since MSMONITOR_USE_DAEMON has been completely removed from vllm_ascend/envs.py and vllm_ascend/ascend_config.py, test_migrated_config_falls_back_to_envs will now fail because msmonitor_use_daemon will evaluate to False instead of falling back to True.
Please clean up the legacy MSMONITOR_USE_DAEMON references and assertions from those two existing tests to prevent test failures.
github-actions
Bot
added
documentation
|
👋 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. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 participant
What this PR does / why we need it?
Removes the legacy
MSMONITOR_USE_DAEMONenvironment variable after its behavior was migrated toadditional_config.msmonitor_use_daemon. 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.
MSMONITOR_USE_DAEMONis no longer read. Users must configure the behavior with--additional-config '{"msmonitor_use_daemon":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
MSMONITOR_USE_DAEMONremain 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