[BugFix][MoE] Keep shared and final maybe_all_reduce decisions runtime-aware - #16550
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 addresses a critical issue in Mixture-of-Experts (MoE) model inference where graph tracing and replay could result in mismatched communication types, leading to incorrect output. By decoupling the reduction logic from static tracing decisions and introducing a runtime-aware custom operation, the system now correctly manages shared-expert and final tensor-parallel reductions based on the live communication context. 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
|
|
👋 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. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][Feature] Register maybe_all_reduce_tensor_model_parallel custom op for MoE reduction tracingSuggested PR Summary:
### What this PR does / why we need it?
This PR registers a new custom operator `maybe_all_reduce_tensor_model_parallel` to handle conditional tensor model parallel all-reductions dynamically during graph replay, avoiding static branching on tracing-specific variables. It updates the Ascend MoE runner and fused MoE module to utilize this operator, ensuring correct reduction behavior across different communication configurations.
Additionally, a review feedback points out that the `fake_impl` for the registered custom op should return `states.clone()` instead of `states` to prevent PyTorch's `torch.compile` tracing from making incorrect aliasing assumptions.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
The changes are tested with new and updated unit tests in `tests/ut/ops/test_fused_moe.py`, including a new test `test_maybe_all_reduce_graph_replay_preserves_shared_and_routed_outputs` that verifies graph replay behavior.| direct_register_custom_op( | ||
| op_name="maybe_all_reduce_tensor_model_parallel", | ||
| op_func=_maybe_all_reduce_tensor_model_parallel_impl, | ||
| fake_impl=lambda states, is_sequence_parallel=False, reduce_when_reduced=False: states, | ||
| mutates_args=[], | ||
| dispatch_key="PrivateUse1", | ||
| ) |
There was a problem hiding this comment.
Returning the input tensor states directly in the fake_impl of maybe_all_reduce_tensor_model_parallel causes PyTorch's functionalization and aliasing analysis during torch.compile tracing to assume that the output always aliases the input.
However, at runtime, _maybe_all_reduce_tensor_model_parallel_impl can perform an out-of-place tensor_model_parallel_all_reduce(states) which returns a new tensor with different storage. This mismatch between compile-time aliasing assumptions and runtime behavior can lead to subtle correctness bugs, compilation failures, or incorrect buffer reuse optimizations under torch.compile.
To prevent this, the fake_impl should return a new tensor that does not alias the input, such as states.clone().
| direct_register_custom_op( | |
| op_name="maybe_all_reduce_tensor_model_parallel", | |
| op_func=_maybe_all_reduce_tensor_model_parallel_impl, | |
| fake_impl=lambda states, is_sequence_parallel=False, reduce_when_reduced=False: states, | |
| mutates_args=[], | |
| dispatch_key="PrivateUse1", | |
| ) | |
| direct_register_custom_op( | |
| op_name="maybe_all_reduce_tensor_model_parallel", | |
| op_func=_maybe_all_reduce_tensor_model_parallel_impl, | |
| fake_impl=lambda states, is_sequence_parallel=False, reduce_when_reduced=False: states.clone(), | |
| mutates_args=[], | |
| dispatch_key="PrivateUse1", | |
| ) |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
8bf0e00 to
cfffd82
Compare
cfffd82 to
eaca0c2
Compare
This reverts commit bc46ecb. Co-authored-by: GPT-6 Codex <codex@openai.com> Signed-off-by: drslark <slarksblood@qq.com>
Use separate shared and routed reduction ops and resolve the layer configuration by name. Co-authored-by: GPT-6 Codex <codex@openai.com> Signed-off-by: drslark <slarksblood@qq.com>
Co-authored-by: GPT-6 Codex <codex@openai.com> Signed-off-by: drslark <slarksblood@qq.com>
9fc232b to
ed8709e
Compare
|
/rerun Rerun (failed jobs only):
|
…to main_fix_mrv2_eagle3_mamba * 'main' of https://github.com/vllm-project/vllm-ascend: [BugFix][MoE] Keep shared and final maybe_all_reduce decisions runtime-aware (vllm-project#16550) [BugFix][Kernel] Restore QLIV2 A5 build dependencies (vllm-project#16534) [BugFix]Fix MRV2 Mamba block-table capacity for dspark,eagle3,dflash RecurrentGatedDeltaRule MTE OOM, when running with vLLM 0.28. without enable prefix cache (vllm-project#16562)
…dMoERunner - AscendMoERunner is instantiated through the OOT PluggableLayer dispatch of upstream MoERunner; override forward directly instead of monkey-patching the upstream base class. - The forward mirrors upstream MoERunner.forward with a single delta: the shared+routed output combine uses torch._foreach_add, the NPU-validated fused multi-tensor add path whose kernel launch count is independent of the tensor-list length. All runtime-aware reduction hooks (vllm-project#16550) are inherited unchanged. - Can be dropped once aclnnAdd_AddAiCore_Add is available via the superkernel integration. Signed-off-by: caiqigang <cabbage_alex@hotmail.com>
…dMoERunner - AscendMoERunner is instantiated through the OOT PluggableLayer dispatch of upstream MoERunner; override forward directly instead of monkey-patching the upstream base class. - The forward mirrors upstream MoERunner.forward with a single delta: the shared+routed output combine uses torch._foreach_add, the NPU-validated fused multi-tensor add path whose kernel launch count is independent of the tensor-list length. All runtime-aware reduction hooks (vllm-project#16550) are inherited unchanged. - Can be dropped once aclnnAdd_AddAiCore_Add is available via the superkernel integration. Signed-off-by: caiqigang <cabbage_alex@hotmail.com>
…dMoERunner - AscendMoERunner is instantiated through the OOT PluggableLayer dispatch of upstream MoERunner; override forward directly instead of monkey-patching the upstream base class. - The forward mirrors upstream MoERunner.forward with a single delta: the shared+routed output combine uses torch._foreach_add, the NPU-validated fused multi-tensor add path whose kernel launch count is independent of the tensor-list length. All runtime-aware reduction hooks (vllm-project#16550) are inherited unchanged. - Can be dropped once aclnnAdd_AddAiCore_Add is available via the superkernel integration. Signed-off-by: caiqigang <cabbage_alex@hotmail.com>
…s runtime-aware (vllm-project#16550)" This reverts commit b710edb. Signed-off-by: chenzeyu <2978509328@qq.com>
…ns runtime-aware (vllm-project#16550)" This reverts commit dc4cee9. Signed-off-by: chenzeyu <2978509328@qq.com>
What this PR does / why we need it?
This PR reverts #16495 and replaces the complete MoE forward wrapper with runtime-aware reduction at three points: routed output before a transform or shared DP addition, shared TP output before addition, and final output after addition.
flowchart TD R["Routed output"] --> R1["1. Maybe reduce routed early<br/>Required by transform or shared DP?<br/>If yes, reduce unless already reduced"] R1 --> T["Routed transform, if any"] S["Shared output"] --> S1["2. Maybe reduce shared<br/>Shared TP + routed already reduced?<br/>If yes, reduce shared"] T --> A["Add shared and routed, if shared exists"] S1 --> A A --> F["3. Maybe reduce final output<br/>No SP + no early reduction<br/>+ routed not already reduced?<br/>If yes, reduce combined output"] F --> O["Return"]Reduction conditions
TENSOR_PARALLELRuntime check
Static vs. runtime decisions
_maybe_reduce_*runner methodsfake_implThe runner methods select reduction points using static conditions. Communication-dependent decisions stay inside the custom ops and are evaluated by their real implementations at runtime. Both ops receive
layer_nameand read the layer's MoE configuration internally.Does this PR introduce any user-facing change?
Fixes incorrect MoE output when tracing and replay use different communication types. No configuration changes are required.
How was this patch tested?
Validated the runtime reduction patch from
eaca0c24don the native 20260914 Ascend image. Only the reduction changes infused_moe.pyandregister_custom_ops.pywere applied; the rest of the image was retained. This does not validate the entire PR branch against current main.The two incorrect responses at concurrency 4 contain complete reasoning and final answers:
The concurrency-dependent accuracy difference has not been isolated to a root cause. The absence of looping or garbled output does not establish batch-invariant accuracy.
Graph mode was enabled:
FULL_DECODE_ONLY.Only AIS Bench concurrency changed between the two runs; the dataset, generation settings and running service were retained. The full PR test suite and full lint workflow have not been rerun on the latest revision.