Revert "[feat] enable hierarchical mc2 ops on A2 by default (#5300)"#5434
Revert "[feat] enable hierarchical mc2 ops on A2 by default (#5300)"#5434realliujiaxu merged 2 commits intovllm-project:mainfrom
Conversation
…ject#5300)" This reverts commit 12da9f9. Signed-off-by: realliujiaxu <realliujiaxu@163.com>
|
👋 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
This pull request appears to be a revert of a previous feature, likely to stabilize the main branch for an upcoming release. The changes correctly undo the addition of hierarchical communication for MoE operations. However, if my understanding is incorrect and this is actually adding the feature, there is a potential issue in vllm_ascend/utils.py where a function's implementation doesn't match its accompanying comment, which I've detailed in a specific comment. Assuming this is a revert as titled, the changes look correct and complete.
| def is_hierarchical_communication_enabled(): | ||
| return (os.getenv("HCCL_INTRA_ROCE_ENABLE", "") == "0" | ||
| and os.getenv("HCCL_INTRA_PCIE_ENABLE", "") == "1") |
There was a problem hiding this comment.
The comment for this function specifies that this hierarchical communication optimization is for A2 devices. However, the function implementation does not include a check for the device type. This could lead to this communication path being enabled on other device types if the environment variables are set, which might cause unexpected behavior or performance issues if not supported. To align the implementation with the documentation and prevent potential issues, it's recommended to add a device type check.
| def is_hierarchical_communication_enabled(): | |
| return (os.getenv("HCCL_INTRA_ROCE_ENABLE", "") == "0" | |
| and os.getenv("HCCL_INTRA_PCIE_ENABLE", "") == "1") | |
| def is_hierarchical_communication_enabled(): | |
| return (get_ascend_device_type() == AscendDeviceType.A2 and | |
| os.getenv("HCCL_INTRA_ROCE_ENABLE", "") == "0" and | |
| os.getenv("HCCL_INTRA_PCIE_ENABLE", "") == "1") |
…to eplb_refactor * 'main' of https://github.com/vllm-project/vllm-ascend: (46 commits) [Feature] Support to use fullgraph with eagle (vllm-project#5118) [EPLB][refactor] Modification of the initialization logic for expert_map and log2phy(depend on pr5285) (vllm-project#5311) [Refactor]6/N Extract common code of class AscendMLAImpl (vllm-project#5314) [Refactor] cache cos/sin in mla & remove parameter model in builder. (vllm-project#5277) update vllm pin to 12.27 (vllm-project#5412) [ReleaseNote] Add release note for v0.13.0rc1 (vllm-project#5334) [Bugfix] Correctly handle the output shape in multimodal attention (vllm-project#5443) Fix nightly (vllm-project#5413) [bugfix] fix typo of _skip_all_reduce_across_dp_group (vllm-project#5435) [Doc]modify pcp tutorial doc (vllm-project#5440) [Misc] fast fail for exiting if tools/install_flash_infer_attention_score_ops_a2.sh (vllm-project#5422) [Doc] Update DeepSeek V3.1/R1 2P1D doc (vllm-project#5387) [DOC]Fix model weight download links (vllm-project#5436) [Doc] Modify DeepSeek-R1/V3.1 documentation (vllm-project#5426) Revert "[feat] enable hierarchical mc2 ops on A2 by default (vllm-project#5300)" (vllm-project#5434) [Bugfix] fix greedy temperature detection (vllm-project#5417) [doc] Update Qwen3-235B doc for reproducing latest performance (vllm-project#5323) [feat] enable hierarchical mc2 ops on A2 by default (vllm-project#5300) [Doc] delete environment variable HCCL_OP_EXPANSION_MODE in DeepSeekV3.1/R1 (vllm-project#5419) [Doc] add long_sequence feature user guide (vllm-project#5343) ...
…ject#5300)" (vllm-project#5434) We'll release 0.13.0 soon. The main branch is freeze. Let's revert the newest change and redo it once 0.13.0 is released. - vLLM version: release/v0.13.0 - vLLM main: vllm-project/vllm@81786c8 Signed-off-by: realliujiaxu <realliujiaxu@163.com> Signed-off-by: zrj026 <zhangrunjiang026@gmail.com>
…ject#5300)" (vllm-project#5434) We'll release 0.13.0 soon. The main branch is freeze. Let's revert the newest change and redo it once 0.13.0 is released. - vLLM version: release/v0.13.0 - vLLM main: vllm-project/vllm@81786c8 Signed-off-by: realliujiaxu <realliujiaxu@163.com>
…ject#5300)" (vllm-project#5434) We'll release 0.13.0 soon. The main branch is freeze. Let's revert the newest change and redo it once 0.13.0 is released. - vLLM version: release/v0.13.0 - vLLM main: vllm-project/vllm@81786c8 Signed-off-by: realliujiaxu <realliujiaxu@163.com> Signed-off-by: zrj026 <zhangrunjiang026@gmail.com>
We'll release 0.13.0 soon. The main branch is freeze. Let's revert the newest change and redo it once 0.13.0 is released.