-
Notifications
You must be signed in to change notification settings - Fork 292
[AMD] kimik2.5-fp4-mi355x-vllm-disagg: split P/D by concurrency (1D low, 2D high) / 优化 kimik2.5-fp4-mi355x-vllm-disagg:按并发拆分 P/D 拓扑(低并发 1D、高并发 2D) #2301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The disagg recipe now runs Kimi-K2.5-MXFP4 at TP4 (down from TP8) via configs/amd-master.yaml, but benchmarks/multi_node/amd_utils/models_vllm.yaml:28-30 still hardcodes VLLM_ROCM_USE_AITER_RMSNORM=1 unconditionally with no per-TP branching. The sibling single-node recipe (benchmarks/single_node/fixed_seq_len/kimik2.5_fp4_mi355x.sh:51-54) explicitly disables AITER RMSNorm for TP<8 due to known accuracy issues, so this multi-node recipe now silently ships a numerically-degraded config since the fixed-seq-len sweep only measures throughput. Fix: disable VLLM_ROCM_USE_AITER_RMSNORM when TP<8, matching the single-node guard.
Extended reasoning...
What the bug is
configs/amd-master.yaml'skimik2.5-fp4-mi355x-vllm-disaggrecipe is changed by this PR from all-TP8 prefill/decode to all-TP4 for both new topologies (1P/1D and 1P/2D). However,benchmarks/multi_node/amd_utils/models_vllm.yaml:28-30(theKimi-K2.5-MXFP4entry) still statically setsenv: "... VLLM_ROCM_USE_AITER_RMSNORM=1 ..."with no conditional logic based on tensor-parallel size. That env string is exported verbatim for every worker regardless of TP.Why this matters — the sibling recipe already flags this exact hazard
benchmarks/single_node/fixed_seq_len/kimik2.5_fp4_mi355x.sh:51-54serves the identical model (amd/Kimi-K2.5-MXFP4) and explicitly guards against this:This is direct, authoritative evidence from AMD/the repo's own single-node recipe that the AITER RMSNorm kernel has a known numerical-accuracy problem at TP<8 for this specific model/hardware combination. The multi-node
models_vllm.yamlconfig has no equivalent branching — it is a flat, static env string.The code path that triggers it
server_vllm.shloads theenvfield frommodels_vllm.yamlintoMODEL_ENVSand itssetup_vllm_env()does a simplefor env_pair in ${MODEL_ENVS}; do export "$env_pair"; done— there is no TP-conditional logic anywhere in the multi-node vLLM path. The same script sed-rewrites--tensor-parallel-sizetoPREFILL_TP_SIZE/DECODE_TP_SIZE, whichsubmit.shcomputes asPREFILL_NODES*PREFILL_TP/PREFILL_WORKERS. With the new master config (tp: 4for both prefill and decode,num-worker: 1for prefill,num-worker: 1or2for decode), every worker in both new topologies genuinely runs at TP4 — e.g.1*4/1 = 4for prefill, and1*4/1 = 4(1D) or2*4/2 = 4(2D) for decode. So this recipe now lands squarely in the TP<8 zone the single-node script deliberately avoids, but with AITER RMSNorm turned on.Why nothing catches it
The
fixed-seq-lenscenario is a throughput-only sweep (RUN_EVALdefaults tofalse, and this PR makes no eval-harness changes). Accuracy is never measured, so a numerically-degraded serving config would still produce green throughput numbers and merge/publish silently.Step-by-step proof
kimik2.5-fp4-mi355x-vllm-disaggran prefill and decode both at TP8 (old master config:tp: 8for both worker rows). At TP8, AITER RMSNorm is fine per the sibling script's own guard (if TP<8 disable; TP8 is not<8, so it stays enabled — the known-good case).tp: 4, decodetp: 4in both search-space rows (1P/1D and 1P/2D).submit.shcomputesPREFILL_TP_SIZE = PREFILL_NODES*PREFILL_TP/PREFILL_WORKERS = 1*4/1 = 4,DECODE_TP_SIZE = 1*4/1 = 4or2*4/2 = 4. Every worker really runs at TP4.models_vllm.yaml'sKimi-K2.5-MXFP4.envstill unconditionally setsVLLM_ROCM_USE_AITER_RMSNORM=1;setup_vllm_envexports it as-is, no TP check.How to fix
Add the same per-TP guard used in the single-node script — either branch in
server_vllm.sh'ssetup_vllm_env(or equivalent) to unsetVLLM_ROCM_USE_AITER_RMSNORMwhen the resolved TP is<8, or simply dropVLLM_ROCM_USE_AITER_RMSNORM=1from theKimi-K2.5-MXFP4yaml env now that this recipe runs exclusively at TP4. This is a one-line fix and directly mirrors precedent already in the codebase.Addressing the "is this just a PR-description mismatch" concern
This is not a stale-description nit — the code itself demonstrably runs a config (TP4 + RMSNorm=1) that the repo's own sibling script treats as a documented accuracy hazard, independent of anything the PR description claims. The PR description even states intent to "sync per-worker vLLM serve flags/env with the single-node recipe" — this is the one flag that was supposed to be synced but was missed.