Repository navigation
[ROCm] Enable Sequence Parallelism for AMD GPUs (MI300X/MI325X/MI355X) - #36851
ChuanLi1101 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request successfully enables Sequence Parallelism for ROCm platforms by generalizing the platform check and adding ROCm-specific configurations and pattern matchers for AITER operations. The changes are well-structured and accompanied by a comprehensive set of unit and distributed tests.
My main feedback is to improve the robustness of the new distributed test by removing its dependency on an external Hugging Face model configuration. Mocking this configuration would make the test more reliable and self-contained. Overall, this is a great contribution that extends an important optimization to a new hardware platform.
| model_name = "RedHatAI/Llama-3.2-1B-Instruct-FP8" | ||
| model_config = ModelConfig( | ||
| model=model_name, trust_remote_code=True, dtype=dtype, seed=42 | ||
| ) |
There was a problem hiding this comment.
This test relies on fetching a model configuration from the Hugging Face Hub ("RedHatAI/Llama-3.2-1B-Instruct-FP8"), which introduces an external dependency. This can make the test flaky if the model is unavailable or due to network issues. The test's behavior depends on the hidden_size from this external config to determine if sequence parallelism should be applied.
To make the test more robust and self-contained, I recommend mocking the configuration fetching to remove this external dependency. This also makes the test's preconditions (like hidden_size being large enough to trigger sequence parallelism) explicit.
You can use unittest.mock.patch to replace vllm.config.model.get_config and return a mock object with a fixed hidden_size. For example:
from unittest.mock import MagicMock, patch
# ... inside _rocm_aiter_sp_worker_fn
mock_hf_config = MagicMock()
# Ensure hidden_size is large enough to trigger sequence parallelism
mock_hf_config.hidden_size = 8192
with patch("vllm.config.model.get_config", return_value=mock_hf_config):
model_config = ModelConfig(
model="mock_model", trust_remote_code=True, dtype=dtype, seed=42
)
vllm_config = VllmConfig(
model_config=model_config,
device_config=device_config,
compilation_config=compilation_config,
)
with set_current_vllm_config(vllm_config):
# ... rest of the testThis change would require adding the necessary imports at the top of the file.
There was a problem hiding this comment.
This is the same model used for the CUDA test. Other than that, using HF model is the easiest way to perform E2E test.
|
@zejun-chen @AndreasKaratzas Could you please review this PR? It enables Sequence Parallelism for ROCm platforms (MI300X/MI325X/MI355X). Thanks! |
|
Correction: @zejun-chen @AndreasKaratzas @wuhuikx Could you please review this PR? It enables Sequence Parallelism for ROCm platforms (MI300X/MI325X/MI355X). Thanks! |
AndreasKaratzas
left a comment
There was a problem hiding this comment.
I think it's on the right track, apart from few points.
| @@ -322,3 +322,167 @@ def sequence_parallelism_pass_on_test_model( | |||
|
|
|||
| for op in model.ops_in_model(): | |||
| find_auto_fn(backend.graph_post_pass.nodes, op) | |||
|
|
|||
|
|
|||
| # --- ROCm AITER tests --- | |||
There was a problem hiding this comment.
Minor one: you can drop that comment from there, the class name has all the information.
| @pytest.mark.parametrize("seq_len", [16]) | ||
| @pytest.mark.parametrize("hidden_size", [16]) | ||
| @pytest.mark.parametrize("dtype", [torch.bfloat16]) | ||
| @pytest.mark.skipif(envs.VLLM_TARGET_DEVICE not in ["rocm"], reason="Only test on ROCm") |
There was a problem hiding this comment.
The proper way here is to use current_platform.is_rocm(). However, in this case we are not only trying to run this on ROCm, but also mi3xx, because AITER is not supported on MI250. So you will need to import from vllm.platforms.rocm import on_mi3xx and then use on_mi3xx().
|
Thank you for this work! Can we get e2e perf & accuracy eval, and can you check e2e_correctness tests pass, and add them to AMD CI? The new patterns will soon be redundant due to vLLM IR. To reduce churn, I would prefer to first merge vLLM IR PRs and reap immediate benefits. Instead, could you start working on the gemm+comms fusion (AsyncTP), get it to work, and see how much performance benefit we can achieve? By the time that's done, vLLM IR RMSNorm will be in so we can merge this PR and then the AsyncTP one right after. How does that sound? |
|
Documentation preview: https://vllm--36851.org.readthedocs.build/en/36851/ |
|
@ProExpertProg @AndreasKaratzas @ChuanLi1101 iirc, the unit test passes, but it could not mean it is functionally usable. Fusion Passes unit tests only attempted to fuse/split the nodes. @ChuanLi1101 please follow @ProExpertProg feedback. Provide vllm serve accuracy results, in addition to unit tests results. |
|
Thanks for the feedback @ProExpertProg! vLLM IR & pattern churn: Agreed — I've dropped the AITER-specific patterns (
Once vLLM IR RMSNorm merges, the existing patterns will work on ROCm without needing AITER-specific ones. AsyncTP (GEMM+comms fusion): I'd love to work on this. I did some investigation — the current E2E eval: I'll run e2e perf & accuracy benchmarks on MI325X and share numbers. I'll also add the Proposed plan:
Does this look good? |
|
Yep, keep us posted, thank you! |
|
Oh also, could you create a tracking issue with the work and I can assign you and add it to the central tracker? |
|
This pull request has merge conflicts that must be resolved before it can be |
Enable the SequenceParallelismPass on ROCm by adding an is_rocm() branch to get_sequence_parallelism_threshold, alongside the existing CUDA and XPU branches. Threshold configs are added for gfx942 (MI300X, capability 94) and gfx950 (MI325X/MI355X, capability 95), autoconfigured for models with hidden_size >= 7168. - vllm/compilation/passes/fusion/sequence_parallelism.py: ROCm branch + SP_MIN_HIDDEN_SIZE / SP_MIN_PER_GPU_SIZE_MB entries for 94/95 - tests/compile/conftest.py: add mock_rocm_platform fixture; pin is_rocm on the CUDA/XPU mocks - tests/compile/test_sequence_parallelism_threshold.py: ROCm threshold tests - docs/design/fusions.md: mark enable_sp supported on ROCm MI3xx Signed-off-by: Chuan Li <chuali@amd.com> Made-with: Cursor
d6a655f to
cbd458d
Compare
Explicitly restrict the ROCm sequence-parallelism threshold path to MI3xx capabilities (gfx942 -> 94, gfx950 -> 95) instead of looking up the shared threshold dicts directly. MI250 (gfx90a) reports capability 90, which aliases the CUDA H100 entry (also 90) in the shared dicts and would have enabled sequence parallelism unintentionally. AITER is not supported on MI250, so this addresses @AndreasKaratzas's MI3xx-gating review comment. Signed-off-by: Chuan Li <chuali@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@AndreasKaratzas — thanks for the review! Addressed your points. The test suite was also reworked since your review (the HF-model distributed E2E test was replaced with mockable capability-based unit tests), so responding against the current state: 1. MI3xx gating (was: use _ROCM_SP_SUPPORTED_CAPABILITIES = (94, 95) # gfx942 (MI300X), gfx950 (MI325X/MI355X)
...
elif current_platform.is_rocm():
...
if device_capability not in _ROCM_SP_SUPPORTED_CAPABILITIES:
return NoneThis is functionally equivalent to 2. Redundant comment (class name says it all) — Resolved by the test rewrite; the old comment is gone. The ROCm test class now just carries a one-line docstring. 3. HF model in the E2E test — The HF-model distributed test was dropped entirely in favor of self-contained mocked unit tests ( PR is (AI-assisted change + comment; on-hardware validation by the submitter.) |
|
Closing due to #60517 |
Summary
This PR enables the Sequence Parallelism compilation pass on ROCm platforms (AMD MI300X, MI325X, MI355X), which was previously restricted to CUDA only.
Changes
Core:
vllm/compilation/passes/fusion/sequence_parallelism.pyget_sequence_parallelism_threshold()fromis_cuda()tois_cuda_alike(), enabling ROCm support94(gfx942 / MI300X):min_hidden_size=7168,8MB/GPU95(gfx950 / MI325X/MI355X):min_hidden_size=7168,8MB/GPUDocs:
docs/design/fusions.mdenable_spTests:
tests/compile/conftest.py: Extendedmock_cuda_platformfixture to supportis_cuda_alikeparameter for ROCm platform mockingtests/compile/test_sequence_parallelism_threshold.py: AddedTestGetSequenceParallelismThresholdROCmclass with tests for MI300X (gfx942) and MI355X (gfx950) threshold calculation, boundary conditions, and TP scalingMotivation
The Sequence Parallelism pass converts
AllReduce → RMSNormpatterns intoReduceScatter → RMSNorm → AllGather, reducing memory pressure on the residual tensor and laying groundwork for GEMM+comms fusion (AsyncTP). This optimization was previously CUDA-only. ROCm users with multi-GPU setups (DeepSeek-V3, Kimi-K2, etc. on MI300X/MI355X) can now benefit from this pass.Note: AITER-specific RMSNorm patterns were intentionally excluded from this PR since they will be superseded by upcoming vLLM IR changes. The existing C++ RMSNorm patterns work on ROCm when
rms_normcustom op is enabled. Once vLLM IR merges, the patterns will be universal.Related: Central fusion work tracker #36066 (Sequence Parallelism was marked ❓ for ROCm)
Test plan
ruff checkandruff formatpass on all changed filespytest tests/compile/test_sequence_parallelism_threshold.py(includes new ROCm tests)test-amd.yamlalready runstest_sequence_parallelism.py)