[bugfix]change log2phy map to npu #3282
Conversation
[BugFiModify eplb feature guide.
Signed-off-by: offline0806 <[email protected]>
![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 addresses a bug causing EPLB failures by ensuring the log2phy map is on the correct NPU device. The fix is applied consistently across three files where this logic is present. While the fix is correct, I've identified a critical potential bug in the called function determine_default_log2phy_map and a high-severity maintainability issue due to code duplication. Please see my detailed comments.
| self.log2phy = determine_default_log2phy_map( | ||
| self.global_num_experts, self.ep_size, self.ep_rank, | ||
| self.global_redundant_expert_num) | ||
| self.global_redundant_expert_num).npu() |
There was a problem hiding this comment.
This change correctly moves the log2phy tensor to the NPU device, fixing a device mismatch bug.
However, there are two related points to consider:
-
Potential Bug in
determine_default_log2phy_map: The called functiondetermine_default_log2phy_mapinvllm_ascend/eplb/core/eplb_utils.pyappears to have a bug. On line 122, it usesrank_idinside a loop that iterates over ranks with variabler(for r in range(world_size):). The condition should likely ber < global_redundant_expert_numinstead ofrank_id < global_redundant_expert_num. Becauseexpert_map_allis constructed for all ranks within this loop, usingrank_idwill result in an incorrect map for all ranksr != rank_id. This will causegenerate_log2phy_mapto compute an incorrectlog2phy_map_all, ultimately providing a faulty map for the current rank. This is a critical issue that should be investigated and fixed. -
Code Duplication: This same fix is required in three files (
vllm_ascend/ops/common_fused_moe.py,vllm_ascend/ops/fused_moe.py, andvllm_ascend/torchair/ops/torchair_fused_moe.py) due to duplicated initialization logic. This code duplication is a maintainability risk, as demonstrated by this bug appearing in multiple places. I recommend refactoring this logic into a shared helper function or a base class method in a follow-up PR to improve maintainability.
Given the critical nature of the potential bug, I recommend addressing it.
| self.log2phy = determine_default_log2phy_map( | ||
| self.global_num_experts, self.ep_size, self.ep_rank, | ||
| self.global_redundant_expert_num) | ||
| self.global_redundant_expert_num).npu() |
There was a problem hiding this comment.
This is the same change as in vllm_ascend/ops/common_fused_moe.py. Please see my comment there regarding a potential critical bug in determine_default_log2phy_map and the code duplication issue.
| self.log2phy = determine_default_log2phy_map( | ||
| self.global_num_experts, self.ep_size, self.ep_rank, | ||
| self.global_redundant_expert_num) | ||
| self.global_redundant_expert_num).npu() |
There was a problem hiding this comment.
This is the same change as in vllm_ascend/ops/common_fused_moe.py. Please see my comment there regarding a potential critical bug in determine_default_log2phy_map and the code duplication issue.
|
👋 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. |
What this PR does / why we need it?
Resolved the issue of EPLB failure caused by changes in the log2phy map due to device type modifications when using MTP rotation position encoding.
Does this PR introduce any user-facing change?
How was this patch tested?
vllm-project/vllm@releases/v0.11.0