Repository navigation
[Bugfix] Fix SP pass for multimodal models and PP+SP residual handling - #33322
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces two key fixes to enable Sequence Parallelism (SP) for multimodal models and models using prompt embeddings. First, it adds a new InputMutationEliminationPass to remove erroneous aten.copy_ operations inserted by AOT Autograd, which cause shape mismatches in SP mode. This is a clean solution to a tricky compiler-induced problem. Second, it corrects the residual tensor slicing logic in MiddleAllReduceRMSNormPattern and MiddleAllReduceRMSNormStaticFP8Pattern to correctly handle sharded computation across different tensor parallel ranks. The changes are well-explained, logical, and supported by performance and accuracy tests. The implementation is solid and effectively resolves the described bugs. Overall, this is an excellent contribution that improves vLLM's support for advanced parallelism features.
|
Sorry why do prompt embeds and MM models trigger different behavior here? Can you share the resulting FX graph that has this problem? Also please share the full commands that trigger this issue. Maybe we should just only use SP+AsyncTP with fullgraph compilation (inductor partition or |
The distinct behavior stems from how embeddings are computed for these cases in gpu_model_runner: vllm/vllm/v1/worker/gpu_model_runner.py Lines 2801 to 2853 in 07bdabe For prompt embeddings and multimodal models, the embedding lookup happens outside the model's
For prompt embeds: vllm serve \
Qwen/Qwen2.5-72B-Instruct \
--enable-prompt-embeds \
--tensor-parallel-size 4 \
--compilation-config '{"pass_config": {"enable_sp": true, "fuse_gemm_comms": true, "fuse_allreduce_rms": false}, "splitting_ops":[], "cudagraph_mode": "FULL"}' \
--load-format dummyFor MM models: vllm serve \
Qwen/Qwen2.5-VL-72B-Instruct \
--tensor-parallel-size 4 \
--compilation-config '{"pass_config": {"enable_sp": true, "fuse_gemm_comms": true, "fuse_allreduce_rms": false}, "splitting_ops":[], "cudagraph_mode": "FULL"}' \
--load-format dummy
Sure. Below are the FX graphs for Qwen2.5-VL-72B with TP=4 on vLLM main branch, with serve command: vllm serve \
Qwen/Qwen2.5-VL-72B-Instruct \
--tensor-parallel-size 4 \
--compilation-config '{"pass_config": {"enable_sp": true, "fuse_gemm_comms": true, "fuse_allreduce_rms": false}, "splitting_ops":[], "cudagraph_mode": "FULL"}' \
--load-format dummyThe first file shows the graph before the SP Pass applied. The second file shows the graph after the SP Pass applied, where the shape mismatch and slicing issues described above occur. |
There was a problem hiding this comment.
Thank you so much for the detailed response! This makes sense to me. Proper slicing seems right in this case. Perhaps include some of the details above in a comment somewhere? And can you add a relevant case (prompt embeds enabled) to the E2E correctness tests?
|
|
||
| for node in graph.nodes: | ||
| # 1. Find aten.copy_ operations | ||
| if is_func(node, torch.ops.aten.copy_.default): |
There was a problem hiding this comment.
can you rewrite this to if not ... continue to avoid deep if nesting?
There was a problem hiding this comment.
can you rewrite this to
if not ... continueto avoid deep if nesting?
Sure, fixed.
There was a problem hiding this comment.
@wangxingran222 can you also rewrite this comment because your approach now guarantees semantic correctness during the pass as well!
There was a problem hiding this comment.
(if I understand correctly)
There was a problem hiding this comment.
@wangxingran222 can you also rewrite this comment because your approach now guarantees semantic correctness during the pass as well!
Yes, fixed.
Signed-off-by: Xingran Wang <wangxingran123456@outlook.com> Signed-off-by: Hongjian Zhang <hirokenovo@gmail.com> Co-authored-by: Hongjian Zhang <hirokenovo@gmail.com>
Signed-off-by: Xingran Wang <wangxingran123456@outlook.com>
1502b31 to
35140f2
Compare
Signed-off-by: Xingran Wang <wangxingran123456@outlook.com>
Signed-off-by: Xingran Wang <wangxingran123456@outlook.com>
Signed-off-by: Xingran Wang <wangxingran123456@outlook.com>
Thanks for the review! I have integrated the requested E2E test case. It is currently failing, and I am investigating the root cause. I will push the fix once resolved. |
|
@wangxingran222 I think I might know why it's failing. When compiling piecewise (no inductor partition and non-empty splitting ops), the residual passed between graphs gets sliced, which I'm now remembering was the original reason my slicing was not tp-aware.i think we should just remove the piecewise support for SP, let me put up an RFC. EDIT: RFC here: #35771 |
Yes. I observed failures in all |
|
Do you mind chiming in on the RFC? |
Signed-off-by: Xingran Wang <wangxingran123456@outlook.com>
Signed-off-by: Xingran Wang <wangxingran123456@outlook.com>
|
There are 4 CI failures, but they all look unrelated to SP or PP. |
|
Do all the tests fail on main as well? If yes, can you post in #pr-merge-requests in the slack? |
Did a quick scan on recent PRs: the 3 AMD failures (amd-v1-spec-decode-mi325-1, amd-multi-modal-models-standard-3, amd-multi-modal-models-standard-4) also fail on #41486. elastic-ep-scaling-test seems to be consistently failing since ~05-05, e.g. #40731 |
vllm-project#33322) Signed-off-by: Xingran Wang <wangxingran123456@outlook.com> Signed-off-by: Hongjian Zhang <hirokenovo@gmail.com> Co-authored-by: Hongjian Zhang <hirokenovo@gmail.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com> Signed-off-by: 李少鹏 <lishaopeng21@huawei.com>
vllm-project#33322) Signed-off-by: Xingran Wang <wangxingran123456@outlook.com> Signed-off-by: Hongjian Zhang <hirokenovo@gmail.com> Co-authored-by: Hongjian Zhang <hirokenovo@gmail.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com> Signed-off-by: xqchen7 <chenxueqing7@huawei.com>
### What this PR does / why we need it? 1. fix vllm-project/vllm#33322 overwrite `gpu_modelrunner.sync_and_gather_intermediate_tensors`, for the sceniro `pp+sp+tp`, skip scatter the residual for ascend 2. vllm-project/vllm#35520 Adapted to the modifications of `ModelRunner v2` for hybrid attn in interface level, . Todo: Added support for Mamba in ModelRunner in Ascend. any pull_request is welcome 3. vllm-project/vllm#40711 4. vllm-project/vllm#42121 5. vllm-project/vllm#41706 6. vllm-project/vllm#39917 Disable `async_schedule` when `enable_return_routed_experts=True` 7. vllm-project/vllm#41046 8. vllm-project/vllm#41055 9. vllm-project/vllm#41035 10. vllm-project/vllm#42434 ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.20.1 - vLLM main: vllm-project/vllm@c7aa186 --------- Signed-off-by: wangli <wangli858794774@gmail.com>
Purpose
When deploying multimodal models on the current vLLM main branch, or deploying text models with the
--enable-prompt-embedsoption enabled, Sequence Parallelism (SP) cannot be enabled due to torch compile errors. Specifically, two issues occur:aten.copy_operations: When torch compile detects that the forward function modifies input parameters (e.g.,inputs_embeds), the compiler insertsaten.copy_nodes to write modified values back to input tensors (functionalization and copy_ epilogue). In SP mode, the computation is sharded while input parameters remain full-length, causing shape mismatch errors in these copy operations.residual[0:local_len, ...], which always takes the first shard regardless of TP rank. This causes incorrect results whentp_rank > 0.This PR fixes both issues by:
InputMutationEliminationPassto remove shape-mismatchedcopy_operations on graph inputsresidual[tp_rank * local_len : (tp_rank + 1) * local_len, ...]Supplementary Material
Details
Issue 1: Computation Graph Error Details
The tail end of the original computation graph shows the shape mismatch issue:
What's happening here:
arg1_1is theinput_embedswith shapebf16[s59, 5120](full length)getitem_1923has shapebf16[(s59//4), 5120](sharded length after Sequence Parallelism)copy_operation tries to copy the sharded tensor back into the full-length input tensor, causing a shape mismatchexpand: attempting to expand a dimension of length (s59//4) -> s59!Issue 2: Incorrect Residual Slicing Details
For multimodal models or text models with
--enable-prompt-embeds, the vocabulary embedding computation doesn't happen inside the model's forward function—it's done ingpu_model_runnerinstead. This means theFirstAllReduceRMSNormPatternnever gets matched, so its output residual never gets sharded by SP. As a result, the firstMiddleAllReduceRMSNormPatternreceives the full-length residual instead of a sharded one.The problem is in the
MiddleAllReduceRMSNormPatternreplacement, which usesresidual = residual[0 : reduce_scatter.size(0), ...]. This causes all TP ranks to slice the same first chunk of the residual[0, sym_size_int_129], leading to incorrect computation results.All TP ranks end up with the same computation graph:
Notice how
slice_tensor_127always slices from index0tosym_size_int_129, regardless of TP rank. This is wrong—each rank should be slicing its own chunk of the residual.Test Plan
We ran both performance and accuracy tests to validate the fix.
Performance Testing:
--enable-prompt-embedsenabledAccuracy Testing:
--enable-prompt-embeds: tested on gsm8kTest Result
Performance Testing
Text model(Qwen3-32B) with
--enable-prompt-embeds:Details
This PR, Qwen3-32B, enable-prompt-embeds, SP + Async TP on
Main, Qwen3-32B, enable-prompt-embeds, SP + Async TP off
Main, Qwen3-32B, Token IDs, SP + Async TP on
Multimodal model(Qwen2.5-VL-32B):
Details
This PR, Qwen2.5-VL-32B, SP + Async TP on
Main, Qwen2.5-VL-32B, SP + Async TP off
Accuracy Testing
Text model(Qwen3-32B) with
--enable-prompt-embeds:Details
This PR, Qwen3-32B, enable-prompt-embeds, SP + Async TP on
Main, Qwen3-32B, enable-prompt-embeds, SP + Async TP off
Multimodal model(Qwen2.5-VL-32B):
Details
This PR, Qwen2.5-VL-32B, SP + Async TP on
Main, Qwen2.5-VL-32B, SP + Async TP off
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.