Skip to content

[BugFix] Fix aclgraph accu problem in A2.#3163

Merged
wangxiyuan merged 4 commits intovllm-project:mainfrom
whx-sjtu:fix_a2_accu
Sep 28, 2025
Merged

[BugFix] Fix aclgraph accu problem in A2.#3163
wangxiyuan merged 4 commits intovllm-project:mainfrom
whx-sjtu:fix_a2_accu

Conversation

@whx-sjtu
Copy link
Copy Markdown
Collaborator

@whx-sjtu whx-sjtu commented Sep 24, 2025 •
edited by github-actions Bot
Loading

This PR fixes accuracy problem of aclgraph on A2. The problem is introduced by PR #2980, which makes the all_reduce of shared_experts exposed to torch dynamo. This PR moves all the codes into forward_impl to shiled from torch dynamo.

@whx-sjtu
fix a2 accu problem
58c14f1 
Signed-off-by: whx-sjtu <2952154980@qq.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 24, 2025

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

@whx-sjtu whx-sjtu requested a review from ApsarasX September 24, 2025 13:35
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Sep 24, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request aims to fix an accuracy issue with aclgraph by refactoring the AscendSharedFusedMoE layer to shield some logic from torch.dynamo. The approach is to move the implementation from the forward method to forward_impl. However, the new forward method contains a critical bug that will cause a ValueError at runtime due to incorrect tuple unpacking. I've provided a comment with a suggested fix.

Comment thread vllm_ascend/ops/common_fused_moe.py
Comment on lines +336 to +341
shared_out, fused_out = AscendFusedMoE.forward(
self,
hidden_states=hidden_states,
router_logits=router_logits,
)
return shared_out, fused_out
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The call to AscendFusedMoE.forward is incorrect. The MRO for AscendSharedFusedMoE will cause this to resolve to vllm.model_executor.layers.fused_moe.layer.FusedMoE.forward, which returns a single tensor. Attempting to unpack this single tensor into two variables, shared_out and fused_out, will raise a ValueError at runtime.

Given that the implementation logic has been moved into forward_impl, which correctly returns a tuple of two tensors, the forward method should likely just call self.forward_impl.

        return self.forward_impl(
            hidden_states=hidden_states,
            router_logits=router_logits,
        )

@shen-shanshan
Copy link
Copy Markdown
Collaborator

shen-shanshan commented Sep 25, 2025 •
edited
Loading

LGTM.

@whx-sjtu whx-sjtu added ready read for review ready-for-test start test by label for PR labels Sep 25, 2025
@yiz-liu
Copy link
Copy Markdown
Collaborator

yiz-liu commented Sep 25, 2025

LGTM

@whx-sjtu
make maybe_all_reduce_tensor_model_parallel custom op
b256ff4 
Signed-off-by: whx-sjtu <2952154980@qq.com>
@whx-sjtu whx-sjtu force-pushed the fix_a2_accu branch 2 times, most recently from 5fdaa50 to f8aa50b Compare September 25, 2025 12:15
@whx-sjtu
fix lint
02da01a 
Signed-off-by: whx-sjtu <2952154980@qq.com>
realliujiaxu
realliujiaxu reviewed Sep 26, 2025
View reviewed changes
Comment thread vllm_ascend/ops/common_fused_moe.py Outdated
Comment thread vllm_ascend/ops/common_fused_moe.py Outdated
@whx-sjtu
change register position
64d351d 
Signed-off-by: whx-sjtu <2952154980@qq.com>
@wangxiyuan wangxiyuan mentioned this pull request Sep 28, 2025
Closed
42 tasks
@Yikun
Copy link
Copy Markdown
Member

Yikun commented Sep 28, 2025 •
edited
Loading

  1. This PR doesn't include any e2e tests, please describe how you tested it.
  2. What are your plans for adding e2e test to prevent this issue from breaking again? Please add the test plan as issue.

@wangxiyuan wangxiyuan merged commit 14d4ed5 into vllm-project:main Sep 28, 2025
19 checks passed
@wangxiyuan
Copy link
Copy Markdown
Collaborator

wangxiyuan commented Sep 28, 2025

@Yikun talked with @whx-sjtu the case is that A2 + aclgraph + EP + max_num_tokens>512 our CI doesn't cover this case. Let's add one later.

Yikun added a commit to Yikun/vllm-ascend that referenced this pull request Sep 28, 2025
@Yikun
Revert "[BugFix] Fix aclgraph accu problem in A2. (vllm-project#3163)"
2c54880 
This reverts commit 14d4ed5.

Signed-off-by: Yikun Jiang <yikunkero@gmail.com>
Angazenn pushed a commit to Angazenn/vllm-ascend that referenced this pull request Oct 21, 2025
@whx-sjtu @Angazenn
[BugFix] Fix aclgraph accu problem in A2. (vllm-project#3163)
b702978 
This PR fixes accuracy problem of aclgraph on A2. The problem is
introduced by PR vllm-project#2980, which makes the `all_reduce` of shared_experts
exposed to torch dynamo. This PR moves all the codes into forward_impl
to shiled from torch dynamo.

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@17b4c66

---------

Signed-off-by: whx-sjtu <2952154980@qq.com>
luolun pushed a commit to luolun/vllm-ascend that referenced this pull request Nov 19, 2025
@whx-sjtu
[BugFix] Fix aclgraph accu problem in A2. (vllm-project#3163)
9902e5b 
This PR fixes accuracy problem of aclgraph on A2. The problem is
introduced by PR vllm-project#2980, which makes the `all_reduce` of shared_experts
exposed to torch dynamo. This PR moves all the codes into forward_impl
to shiled from torch dynamo.

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@17b4c66

---------

Signed-off-by: whx-sjtu <2952154980@qq.com>
Signed-off-by: luolun <luolun1995@cmbchina.com>
luolun pushed a commit to luolun/vllm-ascend that referenced this pull request Nov 19, 2025
@whx-sjtu @IuoIun
[BugFix] Fix aclgraph accu problem in A2. (vllm-project#3163)
3e9faea 
This PR fixes accuracy problem of aclgraph on A2. The problem is
introduced by PR vllm-project#2980, which makes the `all_reduce` of shared_experts
exposed to torch dynamo. This PR moves all the codes into forward_impl
to shiled from torch dynamo.

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@17b4c66

---------

Signed-off-by: whx-sjtu <2952154980@qq.com>
Signed-off-by: luolun <luolun1995@cmbchina.com>
hwhaokun pushed a commit to hwhaokun/vllm-ascend that referenced this pull request Nov 19, 2025
@whx-sjtu @hwhaokun
[BugFix] Fix aclgraph accu problem in A2. (vllm-project#3163)
ba9bd2b 
This PR fixes accuracy problem of aclgraph on A2. The problem is
introduced by PR vllm-project#2980, which makes the `all_reduce` of shared_experts
exposed to torch dynamo. This PR moves all the codes into forward_impl
to shiled from torch dynamo.

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@17b4c66

---------

Signed-off-by: whx-sjtu <2952154980@qq.com>
Signed-off-by: hwhaokun <haokun0405@163.com>
NSDie pushed a commit to NSDie/vllm-ascend that referenced this pull request Nov 24, 2025
@whx-sjtu @NSDie
[BugFix] Fix aclgraph accu problem in A2. (vllm-project#3163)
9db0018 
This PR fixes accuracy problem of aclgraph on A2. The problem is
introduced by PR vllm-project#2980, which makes the `all_reduce` of shared_experts
exposed to torch dynamo. This PR moves all the codes into forward_impl
to shiled from torch dynamo.

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@17b4c66

---------

Signed-off-by: whx-sjtu <2952154980@qq.com>
Signed-off-by: nsdie <yeyifan@huawei.com>
Clorist33 pushed a commit to Clorist33/vllm-ascend that referenced this pull request Dec 9, 2025
@whx-sjtu
[BugFix] Fix aclgraph accu problem in A2. (vllm-project#3163)
162ce5e 
This PR fixes accuracy problem of aclgraph on A2. The problem is
introduced by PR vllm-project#2980, which makes the `all_reduce` of shared_experts
exposed to torch dynamo. This PR moves all the codes into forward_impl
to shiled from torch dynamo.

- vLLM version: v0.10.2
- vLLM main:
vllm-project/vllm@17b4c66

---------

Signed-off-by: whx-sjtu <2952154980@qq.com>
@wangxiyuan wangxiyuan mentioned this pull request Jan 26, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wangxiyuan wangxiyuan wangxiyuan approved these changes

@zzzzwwjj zzzzwwjj zzzzwwjj approved these changes

@realliujiaxu realliujiaxu realliujiaxu approved these changes

@yiz-liu yiz-liu yiz-liu approved these changes

@shen-shanshan shen-shanshan Awaiting requested review from shen-shanshan

@ApsarasX ApsarasX Awaiting requested review from ApsarasX

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module:ops ready read for review ready-for-test start test by label for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@whx-sjtu @shen-shanshan @yiz-liu @Yikun @wangxiyuan @zzzzwwjj @realliujiaxu