[Refactor][Ops] Move expert routing into router classes - #13417
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 refactors the Ascend fused MoE execution flow to align with the upstream router abstraction. By decoupling expert selection from quantization methods and moving it into dedicated router classes, the codebase achieves better modularity and maintainability. The changes include the introduction of new router implementations, updates to the FusedMoE patching logic, and a revised API contract for the apply method, which now receives precomputed routing information. 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
|
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][Feature] Refactor Fused MoE routing and expert selection to use dedicated router classes
Suggested PR Summary:
### What this PR does / why we need it?
This PR refactors the Fused MoE routing and expert selection logic by introducing dedicated router classes (`AscendFusedTopKRouter` and `AscendGroupedTopKRouter`) and a factory (`create_ascend_fused_moe_router`). This aligns the codebase with upstream vLLM's router-based design, decoupling expert selection from quantization methods and the `RoutedExperts` class. Quantization methods now accept pre-computed `topk_weights` and `topk_ids` directly.
Feedback:
- In `vllm_ascend/ops/fused_moe/routed_experts.py`, calling `router._select_experts` with `input_ids` will raise a `TypeError` as the upstream method does not accept this argument.
- In `vllm_ascend/ops/fused_moe/router/fused_topk_router.py`, calling `.to(torch.int64)` on `forward_context.input_ids` when it is `None` will raise an `AttributeError`. A safety check should be added.
- Test assertions in `tests/ut/ops/test_fused_moe.py` should be updated accordingly.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Unit tests were updated to reflect the new router-based interface, and mock assertions were adjusted.| topk_weights, topk_ids = router._select_experts( | ||
| hidden_states=hidden_states, | ||
| router_logits=router_logits, | ||
| input_ids=getattr(forward_context, "input_ids", None), | ||
| ) |
There was a problem hiding this comment.
The BaseRouter._select_experts method in upstream vLLM does not accept an input_ids keyword argument. Passing input_ids here will raise a TypeError: _select_experts() got an unexpected keyword argument 'input_ids' at runtime. Since input_ids is already retrieved from the forward context inside the router's _compute_routing method when needed, it should be removed from this call.
topk_weights, topk_ids = router._select_experts(
hidden_states=hidden_states,
router_logits=router_logits,
)| routed_experts.router._select_experts.assert_called_once_with( | ||
| hidden_states=prepared_hidden_states, | ||
| router_logits=prepared_router_logits, | ||
| input_ids=None, | ||
| ) |
| if self.tid2eid is not None: | ||
| forward_context = get_forward_context() | ||
| input_ids = forward_context.input_ids.to(torch.int64) |
There was a problem hiding this comment.
If forward_context.input_ids is None (which can happen during profiling, dummy runs, or when not set in the context), calling .to(torch.int64) on it will raise an AttributeError. We should check that input_ids is not None before attempting to cast it.
| if self.tid2eid is not None: | |
| forward_context = get_forward_context() | |
| input_ids = forward_context.input_ids.to(torch.int64) | |
| forward_context = get_forward_context() | |
| if self.tid2eid is not None and getattr(forward_context, "input_ids", None) is not None: | |
| input_ids = forward_context.input_ids.to(torch.int64) |
1e3c85f to
b7fc423
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Introduce Ascend router implementations and create them through the FusedMoE platform patch. Move expert selection out of quantization methods so routed expert execution consumes precomputed top-k weights and ids, aligning the flow with the upstream router abstraction. Signed-off-by: weijinqian0 <1184188277@qq.com>
Introduce Ascend router implementations and create them through the FusedMoE platform patch. Move expert selection out of quantization methods so routed expert execution consumes precomputed top-k weights and ids, aligning the flow with the upstream router abstraction. Signed-off-by: weijinqian0 <1184188277@qq.com>
Pass grouped-routing configuration through the fused router parent constructor. Update 310P MoE methods to consume preselected top-k weights and ids so their interfaces match the routed-experts execution flow, with regression coverage for quantized and unquantized paths. Signed-off-by: weijinqian0 <1184188277@qq.com>
Pass grouped-routing configuration through the fused router parent constructor. Update 310P MoE methods to consume preselected top-k weights and ids so their interfaces match the routed-experts execution flow, with regression coverage for quantized and unquantized paths. Signed-off-by: weijinqian0 <1184188277@qq.com>
Pass grouped-routing configuration through the fused router parent constructor. Update 310P MoE methods to consume preselected top-k weights and ids so their interfaces match the routed-experts execution flow, with regression coverage for quantized and unquantized paths. Signed-off-by: weijinqian0 <1184188277@qq.com>
Drop the legacy select_experts patch from the W4A8 MXFP4 MoE unit test and call the quantization method through its current top-k based apply signature after the router refactor. Signed-off-by: weijinqian0 <1184188277@qq.com>
Introduce Ascend router implementations and create them through the FusedMoE platform patch. Move expert selection out of quantization methods so routed expert execution consumes precomputed top-k weights and ids, aligning the flow with the upstream router abstraction. Signed-off-by: weijinqian0 <1184188277@qq.com>
Pass grouped-routing configuration through the fused router parent constructor. Update 310P MoE methods to consume preselected top-k weights and ids so their interfaces match the routed-experts execution flow, with regression coverage for quantized and unquantized paths. Signed-off-by: weijinqian0 <1184188277@qq.com>
Signed-off-by: weijinqian0 <1184188277@qq.com>
Signed-off-by: weijinqian0 <1184188277@qq.com>
0093ceb to
a0735e9
Compare
Signed-off-by: weijinqian0 <1184188277@qq.com>
|
/rerun |
…into vllm-new # By shenhui-cli (8) and others # Via GitHub (1) and shenhui-cli (1) * 'vllm-new' of https://github.com/shenhui-cli/vllm-ascend: (34 commits) When the PR modifies any files in the CSRC folder, skip the test case filtering logic and execute all test cases by default. [MRV2][Bugfix] Fix two triton ops errors in model_runner_v2 num_nans_kernel and apply_panalties (vllm-project#13159) [Cherry-pick][main][Doc][BugFix] Update proxy script name in DeepSeek-V3.2 tutorial (from vllm-project#13537) (vllm-project#13575) [Refactor][quantization]remove mxfp_compat compatibility shim and inline dtype references (vllm-project#13447) [MTP][BugFix] Preserve MoE weight loaders during online updates (vllm-project#13337) [Feature][xlite] Support MLA and DSA (`DeepseekV3ForCausalLM`, `DeepseekV32ForCausalLM` and `GlmMoeDsaForCausalLM`) in the xlite adapter (vllm-project#13378) [BugFix][xlite] Fix DP metadata handling in XliteWrapper and pass down the max number of tokens across all DP ranks for collective communications. (vllm-project#11213) [Refactor][Ops] Move expert routing into router classes (vllm-project#13417) [Platform][Refactor] Refactor NPUPlatform for better organization and clarity (vllm-project#13484) [BugFix] fix fiaV2 contiguous err in GQA (vllm-project#13456) [feature][KV Offload] Support Sparse KV Cache Offload (vllm-project#13026) [Bugfix][MRV2]Skip D2H copy and synchronize when spec decode is not active (vllm-project#13382) [Bugfix] Change AscendSFAIndexerCacheSpec to inherit MLAAttentionSpec (vllm-project#12849) [BugFix][FusedMoE] Restore BF16 quant method initialization (vllm-project#13412) [Doc] Fix link errors and add section anchors (vllm-project#13485) [TEST]Revise the A3 case (vllm-project#13495) [CI] modify default cann_version and add build_type support in nightly (vllm-project#13480) [CI]Improve logging, optimize function-level recommendation algorithm, and add early exit for no product code changes (vllm-project#13472) This PR restricts the test case discovery scope to the following directories: tests/e2e/pull_request/ tests/ut/ This PR restricts the test case discovery scope to the following directories: tests/e2e/pull_request/ tests/ut/ ... # Conflicts: # .github/workflows/scripts/test_selector.py
…#13417) ### What this PR does / why we need it? Come from vllm-project#13220 This PR aligns the Ascend fused MoE execution flow with the upstream router abstraction: - introduces Ascend grouped top-k and fused top-k router implementations; - creates the Ascend router through the platform-level `FusedMoE` patch; - moves expert selection out of quantization methods; - passes precomputed top-k weights and expert IDs into routed expert execution; and - updates unquantized and quantized MoE tests for the new execution contract. Keeping routing separate from quantization reduces duplicated selection logic across quantization schemes and makes the Ascend implementation follow the upstream `FusedMoERouter` lifecycle. ### Does this PR introduce _any_ user-facing change? No. This is an internal fused MoE refactor with no intended API or behavior change. ### How was this patch tested? - `python -m py_compile` on all modified Python implementation files. - `pytest -q tests/ut/ops/test_fused_moe.py tests/ut/quantization/methods/a2/test_w4a16.py tests/ut/quantization/methods/a2/test_w4a8.py tests/ut/quantization/methods/a2/test_w8a8_dynamic.py tests/ut/quantization/methods/test_w4a16_mxfp4.py tests/ut/quantization/methods/test_w4a4_mxfp4.py tests/ut/quantization/methods/test_w8a8_mxfp8.py tests/ut/quantization/methods/test_w8a8fp8_dynamic.py tests/ut/quantization/test_method_adapters.py` - 101 tests passed locally. - 7 NPU-dependent tests could not run in the local macOS environment because the installed CPU-side `torch_npu` stub does not provide `Tensor.npu`, `npu_weight_quant_batchmatmul`, or `npu_dynamic_quant`. - vLLM version: v0.26.0 - vLLM main: vllm-project/vllm@0351e9a --------- Signed-off-by: weijinqian0 <1184188277@qq.com>
…#13417) ### What this PR does / why we need it? Come from vllm-project#13220 This PR aligns the Ascend fused MoE execution flow with the upstream router abstraction: - introduces Ascend grouped top-k and fused top-k router implementations; - creates the Ascend router through the platform-level `FusedMoE` patch; - moves expert selection out of quantization methods; - passes precomputed top-k weights and expert IDs into routed expert execution; and - updates unquantized and quantized MoE tests for the new execution contract. Keeping routing separate from quantization reduces duplicated selection logic across quantization schemes and makes the Ascend implementation follow the upstream `FusedMoERouter` lifecycle. ### Does this PR introduce _any_ user-facing change? No. This is an internal fused MoE refactor with no intended API or behavior change. ### How was this patch tested? - `python -m py_compile` on all modified Python implementation files. - `pytest -q tests/ut/ops/test_fused_moe.py tests/ut/quantization/methods/a2/test_w4a16.py tests/ut/quantization/methods/a2/test_w4a8.py tests/ut/quantization/methods/a2/test_w8a8_dynamic.py tests/ut/quantization/methods/test_w4a16_mxfp4.py tests/ut/quantization/methods/test_w4a4_mxfp4.py tests/ut/quantization/methods/test_w8a8_mxfp8.py tests/ut/quantization/methods/test_w8a8fp8_dynamic.py tests/ut/quantization/test_method_adapters.py` - 101 tests passed locally. - 7 NPU-dependent tests could not run in the local macOS environment because the installed CPU-side `torch_npu` stub does not provide `Tensor.npu`, `npu_weight_quant_batchmatmul`, or `npu_dynamic_quant`. - vLLM version: v0.26.0 - vLLM main: vllm-project/vllm@0351e9a --------- Signed-off-by: weijinqian0 <1184188277@qq.com>
…#13417) ### What this PR does / why we need it? Come from vllm-project#13220 This PR aligns the Ascend fused MoE execution flow with the upstream router abstraction: - introduces Ascend grouped top-k and fused top-k router implementations; - creates the Ascend router through the platform-level `FusedMoE` patch; - moves expert selection out of quantization methods; - passes precomputed top-k weights and expert IDs into routed expert execution; and - updates unquantized and quantized MoE tests for the new execution contract. Keeping routing separate from quantization reduces duplicated selection logic across quantization schemes and makes the Ascend implementation follow the upstream `FusedMoERouter` lifecycle. ### Does this PR introduce _any_ user-facing change? No. This is an internal fused MoE refactor with no intended API or behavior change. ### How was this patch tested? - `python -m py_compile` on all modified Python implementation files. - `pytest -q tests/ut/ops/test_fused_moe.py tests/ut/quantization/methods/a2/test_w4a16.py tests/ut/quantization/methods/a2/test_w4a8.py tests/ut/quantization/methods/a2/test_w8a8_dynamic.py tests/ut/quantization/methods/test_w4a16_mxfp4.py tests/ut/quantization/methods/test_w4a4_mxfp4.py tests/ut/quantization/methods/test_w8a8_mxfp8.py tests/ut/quantization/methods/test_w8a8fp8_dynamic.py tests/ut/quantization/test_method_adapters.py` - 101 tests passed locally. - 7 NPU-dependent tests could not run in the local macOS environment because the installed CPU-side `torch_npu` stub does not provide `Tensor.npu`, `npu_weight_quant_batchmatmul`, or `npu_dynamic_quant`. - vLLM version: v0.26.0 - vLLM main: vllm-project/vllm@0351e9a --------- Signed-off-by: weijinqian0 <1184188277@qq.com>
What this PR does / why we need it?
Come from #13220
This PR aligns the Ascend fused MoE execution flow with the upstream router abstraction:
FusedMoEpatch;Keeping routing separate from quantization reduces duplicated selection logic across quantization schemes and makes the Ascend implementation follow the upstream
FusedMoERouterlifecycle.Does this PR introduce any user-facing change?
No. This is an internal fused MoE refactor with no intended API or behavior change.
How was this patch tested?
python -m py_compileon all modified Python implementation files.pytest -q tests/ut/ops/test_fused_moe.py tests/ut/quantization/methods/a2/test_w4a16.py tests/ut/quantization/methods/a2/test_w4a8.py tests/ut/quantization/methods/a2/test_w8a8_dynamic.py tests/ut/quantization/methods/test_w4a16_mxfp4.py tests/ut/quantization/methods/test_w4a4_mxfp4.py tests/ut/quantization/methods/test_w8a8_mxfp8.py tests/ut/quantization/methods/test_w8a8fp8_dynamic.py tests/ut/quantization/test_method_adapters.pytorch_npustub does not provideTensor.npu,npu_weight_quant_batchmatmul, ornpu_dynamic_quant.vLLM version: v0.26.0
vLLM main: vllm-project/vllm@0351e9a