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 |
0d7ca6f to
6d439d6
Compare
d2d9db8 to
6cc49f1
Compare
1 participant
Scope
Port MI355X Qwen3.5 FP8 to the shared AMD-enabled srt-slurm integration.
engineandroles) and preserve their engine, image, topology, and workload settings.benchmark_serving.pycustom-benchmark interface; no engine patches.CONC_LIST, replacing placeholder[1]rows and private recipe sweep lists where present. This preserves the existing measured points while matching current result collection.Stack
Based directly on shared AMD integration #2542, which is rebased onto InferenceX
mainat96bccfe12b637dad23d32f0bf7ceef0ea86de15a. This child contains only its workload configs, schema-2 recipes, and append-only performance changelog entry. Shared launcher/profile/adapter changes live in #2542.The base selects SemiAnalysisAI/srt-slurm
03863ab23804f8a31811d9aaccc54abc8961a41c: #1 → #11 → #7 → #14, synchronized with NVIDIA srt-slurm85086d3c1fe7b319a1d0d7db8781047dd1940772. NVIDIA runners retain their existing runtime selection.Refresh status
Offline matrix validation: 1 topology/workload rows covering 5 benchmark concurrency points; all 2 prepared throughput/eval configurations pass the pinned runtime's schema/topology checks.
No fresh E2E tests or sweeps were started or rerun. Sweep-trigger labels are removed. Historical runtime evidence is listed in the base PR and does not validate these new heads. Full GPU validation remains pending authorization; this refresh makes no new performance or merge-readiness claim.
AI model disclosure
The exact AI model/version for this refresh is not exposed by the runtime and could not be verified. No subagents contributed to this refresh; earlier model identities are not reliably recorded.