[Feat][SP] Suport SP for VL MoE models - #7044
Conversation
|
👋 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. |
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 significantly refactors the sequence parallelism (SP) compilation pass architecture to enhance modularity and maintainability. It introduces a two-stage pass system, separating AllReduce and AllGather pattern applications, and integrates a new pass for eliminating redundant graph operations. Concurrently, the documentation for sequence parallelism has been comprehensively updated to reflect these architectural changes, providing clearer explanations and pattern details for users and developers. These changes streamline the SP implementation without altering user-facing APIs or behavior, focusing on internal code quality and clarity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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. Changelog
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
92682a8 to
eb6464e
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the sequence parallelism (SP) pass architecture by splitting it into two passes, SequenceParallelismPass and SequenceParallelismAllgatherEpPass, and reorganizes the associated patterns. The documentation for SP is also significantly improved. The changes are mostly well-structured, but there are a few critical issues. I've found some incomplete logic marked with TODO comments in custom ops that could lead to bugs, an unused parameter in a method, and an obsolete file that should be removed. Please address these points to ensure the correctness and quality of the code.
I am having trouble creating individual review comments. Click here to see my feedback.
vllm_ascend/ops/register_custom_ops.py (56-57)
The TODO: do unpad comment indicates that the unpadding logic is missing when enable_sp_by_pass() is true. This will cause the function to return a padded tensor, which can lead to shape mismatches and incorrect results in downstream operations. This should be implemented to ensure correctness.
vllm_ascend/ops/register_custom_ops.py (93-94)
The TODO: do pad comment indicates that the padding logic is missing when enable_sp_by_pass() is true. The function performs reduce_scatter without padding the input tensor x. This can lead to incorrect behavior or errors if the downstream operations expect a padded tensor. The padding logic should be implemented here.
vllm_ascend/compilation/passes/allgather_chunk_noop_pass.py (1-40)
This file seems to be obsolete. The PR description states that AllGatherChunkNoOpCleanupPass is merged into SequenceParallelismAllgatherEpPass as AllGatherChunkNoOpPattern. The new file vllm_ascend/compilation/passes/sequence_parallelism_moe.py already contains AllGatherChunkNoOpPattern. This file allgather_chunk_noop_pass.py defining AllGatherChunkNoOpCleanupPass appears to be a leftover and should be removed to avoid confusion and dead code.
vllm_ascend/worker/worker.py (514)
The newly added parameter profile_prefix is not used within the profile method. It should either be used or removed to avoid confusion and maintain clean code.
def profile(self, is_start: bool = True):
eb6464e to
a4e426a
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
e3a3eb2 to
07aa792
Compare
a140561 to
0d39195
Compare
f5a67e8 to
299fb0a
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
299fb0a to
22267e4
Compare
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com> Made-with: Cursor
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com> Made-with: Cursor
Signed-off-by: realliujiaxu <realliujiaxu@163.com> Made-with: Cursor
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Keep the three sequence parallelism MoE cases as separate pytest items while reusing a single distributed worker setup. This cuts repeated initialization cost and updates CI timing to reflect the longer multicard runtime. Signed-off-by: realliujiaxu <realliujiaxu@163.com> Made-with: Cursor
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
55e7adb to
4f03ab1
Compare
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
wxsIcey
left a comment
There was a problem hiding this comment.
Not familiar with the details of the SP, but from pass perspective, approve.
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
Signed-off-by: realliujiaxu <realliujiaxu@163.com>
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com>
…to qwen3next_graph * 'main' of https://github.com/vllm-project/vllm-ascend: (94 commits) [bugfix] Fixed the error issue when overlaying MTP and full decode on DSV3.1 C8. (vllm-project#7571) [eagle3][pcp] fix acceptance rate for eagle3 and pcp enabled (vllm-project#7549) [bugfix][CI] fix '_OpNamespace' 'vllm' object has no attribute 'qkv_rmsnorm_rope' (vllm-project#7620) [Nightly] Nightly pre-build image (vllm-project#7388) [Bugfix]Fix deepseek 3.2 C8 precision by rotary tensor (vllm-project#7537) adapt to main2main for model runner v2 (vllm-project#7578) [Patch] Fix balance scheduling (vllm-project#7611) [310P]fused recurrent gated delta rule pytorch core and ut (vllm-project#7398) [CI] refine issue triage rules, wan regex and update stale setting (vllm-project#7531) [Lint]Add lint hooks for clang-format, shellcheck, forbidden imports, and boolean context manager checks (vllm-project#7511) [doc] add enable_sparse_c8 option in configuration options (vllm-project#7600) lower log level in PD Disaggregation (vllm-project#7589) [model_runner_v2]:optimize the performance of the _compute_slot_mappings_kernel (vllm-project#7575) [Feat][SP] Suport SP for VL MoE models (vllm-project#7044) Fix Qwen3Next CI Config (vllm-project#7561) [Feat] Add npugraph_ex enablement logging (vllm-project#7574) [UT] Align input arguments with Ascend(Yarn)RotaryEmbedding with vLLM and add ut (vllm-project#7358) [P/D] Check wildcard address for layerwise connector (vllm-project#7389) [P/D] [Bugfix] fix mooncake layerconnector dead when update_decoder_info fail (vllm-project#7514) [BugFix][P/D] fix padding error on FullGraph mode && fix layerwise connector mamba accuracy (vllm-project#7506) ...
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com>
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com>
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com> Signed-off-by: zouyida2052 <zouyida2002@gmail.com>
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com>
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com> Signed-off-by: nanxing <1014662416@qq.com>
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com>
### What this PR does / why we need it? 2nd PR for vllm-project#5712, extend SP to VL MoE models. ### Does this PR introduce _any_ user-facing change? remove `sp_threshold` in additional config and reuse `sp_min_token_num` from vLLM. ### How was this patch tested? - Model: Qwen3-VL-30B-A3B, - TP4 DP2 - 100 reqs - max concurrency 1 | Seq length | Mean TTFT (ms) main | Mean TTFT (ms) this PR | |------------|---------------------|------------------------| | 4k | 429.40 | 323.3 | | 16k | 1297.01 | 911.74 | - vLLM version: v0.16.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: realliujiaxu <realliujiaxu@163.com>
What this PR does / why we need it?
2nd PR for #5712, extend SP to VL MoE models.
Does this PR introduce any user-facing change?
remove
sp_thresholdin additional config and reusesp_min_token_numfrom vLLM.How was this patch tested?