perf(amd): port MI355X Qwen3.5 FP8 disagg sweep to srt-slurm - #2628
perf(amd): port MI355X Qwen3.5 FP8 disagg sweep to srt-slurm#2628cquil11 wants to merge 11 commits into
Conversation
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
72ee996 to
2182835
Compare
dda2f3a to
32d3ee2
Compare
|
Claude finished @cquil11's task in 6m 38s —— View job PR Review
SummaryFound 1 blocking issue and 2 warnings (posted inline):
Checks that passed: master config change is paired with a |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| decode: | ||
| <<: *common_config | ||
| mem-fraction-static: 0.85 | ||
| prefill-round-robin-balance: true |
There was a problem hiding this comment.
Decode inherits prefill TP4
High Severity
The decode block merges common_config and never overrides tensor-parallel-size, so decode launches with TP4 while gpus_per_decode and the master config still describe TP8. The 1P1D TP4P+TP8D topology this recipe is named for does not actually start.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 32d3ee2. Configure here.
| --dataset-name random \ | ||
| --random-input-len 8192 \ | ||
| --random-output-len 1024 \ | ||
| --random-range-ratio 1.0 \ |
There was a problem hiding this comment.
Benchmark uses exact sequence lengths
Medium Severity
The custom client hardcodes --random-range-ratio 1.0, while the production 8k1k path this replaces always used 0.8 from the multi-node workflow. That changes sampled prompt and output lengths, so the new points are not the same 8k1k workload.
Reviewed by Cursor Bugbot for commit 32d3ee2. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| decode: | ||
| <<: *common_config | ||
| mem-fraction-static: 0.85 | ||
| prefill-round-robin-balance: true |
There was a problem hiding this comment.
🔴 BLOCKING: Decode inherits tensor-parallel-size: 4 from the &common_config anchor and never overrides it, so the decode engine is TP4, not the TP8 this PR claims everywhere else (gpus_per_decode: 8 at line 39, the tp8d recipe name, decode.tp: 8 in configs/amd-master.yaml, and the perf-changelog's "one-decode TP8").
Why it matters: Existing srt-slurm sglang recipes (e.g. disagg-b300-1p1d-dep4-dep8.yaml) set tensor-parallel-size explicitly per role, so srt-slurm will not derive it from gpus_per_decode. Best case the run fails on the 4-vs-8 mismatch; worst case decode silently runs TP4 while the launcher stamps result filenames with gen_8/gpus_12 (it computes DECODE_GPUS from the master config's DECODE_TP=8), publishing incorrect perf/GPU numbers.
Fix:
| decode: | |
| <<: *common_config | |
| mem-fraction-static: 0.85 | |
| prefill-round-robin-balance: true | |
| decode: | |
| <<: *common_config | |
| tensor-parallel-size: 8 | |
| mem-fraction-static: 0.85 | |
| prefill-round-robin-balance: true |
Also double-check the other inherited prefill-oriented values: decode inherits cuda-graph-bs: [1..8], so at the c64/c128 points decode batches above 8 run in eager mode — if that's not intentional, the published high-concurrency points will understate decode throughput.
| # in explicitly with CONFIG_FILE; all existing MI355X launch behavior remains | ||
| # unchanged for every other row. | ||
| SRT_SLURM_REPOSITORY="https://github.com/SemiAnalysisAI/srt-slurm.git" | ||
| SRT_SLURM_COMMIT="2f18c3ba4a59061b41bd1d26c8d04fb530692105" |
There was a problem hiding this comment.
🟡 WARNING: The pinned commit doesn't match the validation provenance stated elsewhere. The PR description says the dry-run passed against SemiAnalysisAI/srt-slurm#1 at c609754b5622f96d5c12a93149e245308d4f1e9b, the perf-changelog entry says "Pin the launcher to the exact SemiAnalysisAI/srt-slurm PR #7 revision used for validation", and this script pins ad63a66c72404691bdda98c656d6e211156fb582. At most one of these can be the revision that was actually validated.
Why it matters: If ad63a66c was never exercised by the dry-run/full sweep, the first production run executes an unvalidated launcher revision; if it was, the changelog and PR description are documenting the wrong provenance for a pinned production dependency.
Fix: Reconcile the three references — update the PR description/changelog to the commit that actually ran the full MI355X sweep, or re-pin this to the validated commit.
| RESULT_DIR="${SHARED_RESULTS}/${JOB_ID}" | ||
| mkdir -p "$GITHUB_WORKSPACE/LOGS" | ||
| if [[ -f "$RESULT_DIR/runtime-logs.tar.gz" ]]; then | ||
| cp "$RESULT_DIR/runtime-logs.tar.gz" "$GITHUB_WORKSPACE/multinode_server_logs.tar.gz" | ||
| fi | ||
| cp -R "$RESULT_DIR/." "$GITHUB_WORKSPACE/LOGS/" |
There was a problem hiding this comment.
🟡 WARNING: The failure path dies before reaching its own diagnostics. If the srt-slurm job fails before the benchmark step runs (e.g. a server never passes health check), ${SHARED_RESULTS}/${JOB_ID} is never created — the recipe's benchmark.command is what mkdirs it. Under set -euo pipefail, cp -R "$RESULT_DIR/." then aborts the script with a raw "No such file or directory", so the intended srt-slurm validation failed: ${JOB_STATE} message at the bottom never prints and no logs are collected for debugging. Relatedly, the read -r JOB_STATE ... from sacct at line 144 exits nonzero if sacct hasn't recorded the job yet (a common lag right after it leaves squeue), which also kills the script under set -e after an up-to-8-hour run.
Fix: Guard the copies on the directory existing (and consider a short retry around sacct):
| RESULT_DIR="${SHARED_RESULTS}/${JOB_ID}" | |
| mkdir -p "$GITHUB_WORKSPACE/LOGS" | |
| if [[ -f "$RESULT_DIR/runtime-logs.tar.gz" ]]; then | |
| cp "$RESULT_DIR/runtime-logs.tar.gz" "$GITHUB_WORKSPACE/multinode_server_logs.tar.gz" | |
| fi | |
| cp -R "$RESULT_DIR/." "$GITHUB_WORKSPACE/LOGS/" | |
| RESULT_DIR="${SHARED_RESULTS}/${JOB_ID}" | |
| mkdir -p "$GITHUB_WORKSPACE/LOGS" | |
| if [[ -f "$RESULT_DIR/runtime-logs.tar.gz" ]]; then | |
| cp "$RESULT_DIR/runtime-logs.tar.gz" "$GITHUB_WORKSPACE/multinode_server_logs.tar.gz" | |
| fi | |
| if [[ -d "$RESULT_DIR" ]]; then | |
| cp -R "$RESULT_DIR/." "$GITHUB_WORKSPACE/LOGS/" | |
| fi |
With this guard, a failed job falls through to the existing JOB_STATE != COMPLETED check and exits with the meaningful message (the RESULTS array check already handles the no-results case).
| prefill: &common_config | ||
| served-model-name: "Qwen/Qwen3.5-397B-A17B-FP8" | ||
| tensor-parallel-size: 4 | ||
| disaggregation-transfer-backend: mori | ||
| disaggregation-ib-device: "rdma0,rdma1,rdma2,rdma3,rdma4,rdma5,rdma6,rdma7" | ||
| kv-cache-dtype: "fp8_e4m3" | ||
| attention-backend: aiter | ||
| moe-dense-tp-size: 1 | ||
| load-balance-method: round_robin | ||
| watchdog-timeout: 3600 | ||
| decode-log-interval: 1000 | ||
| log-level: warning | ||
| mem-fraction-static: 0.80 | ||
| max-running-requests: 128 | ||
| chunked-prefill-size: 262144 | ||
| context-length: 16384 | ||
| disable-radix-cache: true | ||
| cuda-graph-bs: [1, 2, 3, 4, 5, 6, 7, 8] | ||
| enable-metrics: true | ||
| decode: | ||
| <<: *common_config | ||
| mem-fraction-static: 0.85 | ||
| prefill-round-robin-balance: true |
There was a problem hiding this comment.
🔴 decode config merges *common_config (prefill's tensor-parallel-size: 4) via YAML anchor and never overrides it, so decode runs TP4 instead of TP8 despite the recipe name/gpus_per_decode: 8 and configs/amd-master.yaml decode.tp: 8 specifying TP8 decode.
Extended reasoning...
The benchmark topology silently regresses from the documented '1P TP4 + 1D TP8' to '1P TP4 + 1D TP4' on an 8-GPU decode node: decode either fails to start (TP size mismatch with the 8-GPU allocation/MoRI KV transfer expecting 8 decode ranks) or runs with half the GPUs actually used, producing throughput/latency numbers that do not reflect the intended TP8 decode topology and invalidating the perf-changelog claim of preserving 'the current one-prefill TP4 plus one-decode TP8 topology.'
Verification: normal. The new recipe's decode block (benchmarks/multi_node/srt-slurm-recipes/sglang/qwen3.5/mi355x/disagg-1p1d-tp4p-tp8d-fixed-seq.yaml:100-103) is decode:\n <<: *common_config\n mem-fraction-static: 0.85\n prefill-round-robin-balance: true. It merges &common_config which sets tensor-parallel-size: 4, and never overrides it, so decode inherits TP4. Meanwhile the same file sets…
| max-running-requests: 128 | ||
| chunked-prefill-size: 262144 | ||
| context-length: 16384 | ||
| disable-radix-cache: true | ||
| cuda-graph-bs: [1, 2, 3, 4, 5, 6, 7, 8] |
There was a problem hiding this comment.
🔴 The recipe's cuda-graph-bs is hard-coded to [1,2,3,4,5,6,7,8] for prefill (and inherited unchanged by decode via the *common_config anchor), even though max-running-requests is 128 and the benchmark sweeps concurrency up to 128. The legacy amd_utils config this replaces (benchmarks/multi_node/amd_utils/models.yaml, Qwen3.5-397B-A17B-FP8, no_dp branch used since dp-attn is false here) used cuda_graph_bs_range: "1-128", expanded via seq 1 128 in server_sglang.sh, i.e. a CUDA graph for every batch size 1-128.
Extended reasoning...
During the c16/c32/c64/c128 points of the sweep (the majority of the 5-point search space), SGLang's running batch size exceeds 8, so no captured CUDA graph exists and the engine silently falls back to eager-mode execution for prefill and decode alike. This produces throughput/latency numbers for most of the sweep that no longer reflect the previously-measured, CUDA-graph-covered production performance -- a silent, undocumented perf regression relative to the pre-migration amd_utils path, despite the PR claiming to 'preserve the production 1P1D TP4P+TP8D topology' and complete search space with parity. Fix: restore cuda-graph-bs to cover 1-128 (or the actual running-request ceiling) for both prefill and decode.
Verification: normal — benchmark-fidelity regression vs base introduced by this change. The new recipe caps CUDA-graph capture at batch size 8 while admitting batches up to 128 and sweeping concurrency to 128: - disagg-1p1d-tp4p-tp8d-fixed-seq.yaml:97 cuda-graph-bs: [1, 2, 3, 4, 5, 6, 7, 8] in the prefill &common_config anchor. - :93 max-running-requests: 128. - :100-102 decode does <<: *common_config…
6513cc6 to
70228fa
Compare
| chunked-prefill-size: 262144 | ||
| context-length: 16384 | ||
| disable-radix-cache: true | ||
| cuda-graph-bs: [1, 2, 3, 4, 5, 6, 7, 8] |
There was a problem hiding this comment.
CUDA graph batch list truncated
High Severity
cuda-graph-bs is [1, 2, 3, 4, 5, 6, 7, 8], but the production Qwen3.5 FP8 no-DP recipe captures graphs for batch sizes 1 through 128. The sweep still runs conc 16/32/64/128 with max-running-requests: 128, so those points miss CUDA graphs and will understate decode throughput versus the current MI355X baseline.
Reviewed by Cursor Bugbot for commit 70228fa. Configure here.
| python3 /infmax-workspace/utils/bench_serving/benchmark_serving.py \ | ||
| --backend openai-chat \ | ||
| --base-url "http://${SRT_FRONTEND_HOST}:${SRT_FRONTEND_PORT}" \ | ||
| --endpoint /v1/chat/completions \ |
There was a problem hiding this comment.
Benchmark backend switched to chat
Medium Severity
The custom client uses --backend openai-chat and /v1/chat/completions. The production AMD sweep called benchmark_serving.py with --backend openai (completions) and no chat template. Chat wrapping changes tokenized ISL/OSL and the router path, so results will not match the prior 8k1k disagg numbers.
Reviewed by Cursor Bugbot for commit 70228fa. Configure here.
2ffe415 to
e0d0626
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 5 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e0d0626. Configure here.
e0d0626 to
3f4f3fa
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33075858940 |
1 participant
Summary
benchmark_serving.pyamd_utils/submit.sh-srt-disaggvalidation key back into the original production config keyValidation
c609754b5622f96d5c12a93149e245308d4f1e9bgit diff --checkpassesStack
Based on #2542. This remains draft until the exact-head MI355X full recipe sweep completes.
Note
Medium Risk
Changes how production MI355X disagg benchmarks are submitted and how results are collected; behavior is preserved but depends on a pinned external srt-slurm revision and new Slurm staging logic.
Overview
Replaces the MI355X Qwen3.5 FP8 disaggregated path that went through
amd_utils/submit.shwith srt-slurm (srtctl apply), SGLang Router, and MoRI KV transfer. The legacy wrapperqwen3.5_fp8_mi355x_sglang-disagg.shis removed.Adds an MI355X AMD Slurm cluster profile, a production recipe (
disagg-1p1d-tp4p-tp8d-fixed-seq.yaml) that keeps 1P1D TP4 prefill + TP8 decode and runs the 8k/1k fixed-seq sweep at c8–c128 inside one eight-hour job viabenchmark_serving.py.amd-master.yamlnow expands to a single matrix row (conc-list: [1]) and passesCONFIG_FILEinto prefill settings instead of per-concurrency Slurm submissions.Adds
launch_mi355x-amds-srt.sh(staging squashfs/HF cache, pinned srt-slurm commit, result collection) and dispatches fromlaunch_mi355x-amds.shwhenCONFIG_FILEis set. perf-changelog documents the migration.Reviewed by Cursor Bugbot for commit d2d9db8. Bugbot is set up for automated code reviews on this repo. Configure here.