[AMD] Port MI355X DeepSeek V4 fixed-sequence disagg to srt-slurm - #2630
[AMD] Port MI355X DeepSeek V4 fixed-sequence disagg to srt-slurm#2630cquil11 wants to merge 23 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 关于重新运行失败任务的文档 |
8084da4 to
1494f62
Compare
ea2d817 to
41d0e1d
Compare
3dc4003 to
85269af
Compare
3ac7c06 to
ee8c921
Compare
8581966 to
1d4522c
Compare
ee8c921 to
f285e40
Compare
1d4522c to
6da0605
Compare
f285e40 to
389534c
Compare
|
Claude finished @cquil11's task in 7m 32s —— View job PR Review
SummaryFound 1 blocking issue and 1 warning (posted as inline comments):
Everything else checked out: changelog entry is correctly appended at the end and paired with the master-config change, the container image is a public Docker Hub reference, both MTP recipes include |
68ef3a8 to
58cfe39
Compare
|
Native full-sweep validation started on head 58cfe39: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33533164273 Expected scheduled production scope: six DSV4 8k1k throughput lanes (STP/MTP TP8 c1–128 and DEP8 c256/c512) plus four standard accuracy lanes. This follows restoration of upstream image pins, server context, router policy, decode chunk-size semantics and benchmark defaults. No other recipe port is being tested in parallel. Results will be staged only after the complete native run is green. |
|
Current native full sweep: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33533933546 Head 84b6055 additionally restores the legacy explicit --disaggregation-ib-device rdma0,...,rdma7 flag on both roles. The old run 33533164273 was superseded while every hardware lane was still queued; this is a source correction, not a flake rerun or fill-in run. All eight generated native worker commands were checked. The PR description now also explicitly documents the shared runtime's startup warmup bypass and allocated-port differences. |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33533164273 |
| local metrics_csv="${AIPERF_SERVER_METRICS_URLS:-}" | ||
| if [[ -z "$metrics_csv" ]]; then | ||
| echo "[clear_caches] WARN: AIPERF_SERVER_METRICS_URLS unset; skipping cache flush" >&2 | ||
| return 0 |
There was a problem hiding this comment.
Cache flush uses metrics URLs
Medium Severity
clear_agentic_worker_caches reads AIPERF_SERVER_METRICS_URLS and returns success when that variable is unset. The retired replay flushed workers via SERVER_FLUSH_URLS_CSV, which was always populated and distinct from optional metrics scrape URLs. Cache isolation between concurrency points is skipped whenever metrics URLs are absent.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 84b6055. Configure here.
Restore the proven STP and MTP search-space grouping, server flags, router policy, CUDA graph batches, and MI355X runtime environment while keeping the srt-slurm orchestration port.
Keep the proven 128-token per-rank decode dispatch setting for both DEP8 fixed-sequence recipes while the shared launcher derives only admission and the inter-kernel threshold.
84b6055 to
c38fb02
Compare
|
Fixed the workload parity error and rebased on latest main. Head: c38fb02; shared #2542: cdb9e5f. The original successful MTP run used RANDOM_RANGE_RATIO=0.8, but these ported recipes hardcoded 1.0. Restored workflow forwarding and the legacy 0.8 fallback in all four recipes. No context increase, image change, or MTP change; benchmark_serving.py is untouched. Original evidence: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/30189328462/job/89759633413 The new source requires a new full sweep; rerunning the failed jobs of 33533933546 would still execute the old incorrect workload. Local validation: 246 matrix tests, 126 eval/logging tests, four pinned srt dry-runs, and 12 actual launcher/benchmark-command cases passed. Hardware validation remains pending. |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33533933546 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c38fb02. Configure here.
| ep: 1 | ||
| dp-attn: false | ||
| additional-settings: | ||
| - "PREFILL_NODES=1" |
There was a problem hiding this comment.
AMD disagg runtime deleted for remaining recipes
High Severity
This commit removes the entire amd_utils Slurm/runtime tree, but active AMD entries such as dsv4-fp4-mi355x-atom-disagg and dsr1-fp4-mi355x-sglang-disagg-mtp still use the old PREFILL_NODES / DECODE_NODES contract that those scripts implemented. Those lanes no longer have a launcher, so they cannot allocate nodes, start workers, or produce results.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c38fb02. Configure here.
There was a problem hiding this comment.
This is a real integration/merge-order constraint, not a claim that the remaining legacy entries already work on this branch. The amd_utils deletion belongs to shared base #2542, as requested; this child only ports the DSV4 SGLang fixed-sequence entries. The remaining eligible AMD entries are covered by separate child ports (#2627, #2628, #2629, #2631, #2633, plus AgentX #2555). The shared deletion must not be released on its own while those active entries still depend on the retired launcher. We are validating the ports one at a time and are not merging the incomplete migration. This PR currently targets main solely for the native full-sweep trigger; its commits remain based on #2542.
| num-worker: 1 | ||
| tp: 8 | ||
| ep: 8 | ||
| dp-attn: true |
There was a problem hiding this comment.
Shared DEP8 recipe loses per-point pins
Medium Severity
Both DEP8 concurrency points (256 and 512) reuse one CONFIG_FILE for STP and one for MTP. The retired path set per-point max-running-requests to that point’s concurrency and retuned the MoRI inter-kernel switch from it. A single static recipe cannot keep those admission and dispatch pins different per allocation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c38fb02. Configure here.
There was a problem hiding this comment.
Checked against the current head: the recipe is transformed per allocation by the shared launcher before srtctl loads it. runners/launch_mi355x-amds-srt.sh lines 231–266 read CONC_LIST (or CONC), set both roles’ max-running-requests to the allocation concurrency, and set the decode inter-kernel switch threshold to 2 * (concurrency // DECODE_TP). The matrix keeps c256 and c512 as separate entries, so these resolve to admission 256 / threshold 64 and admission 512 / threshold 128 respectively. The explicit model-specific per-rank dispatch pin is preserved via setdefault. The static YAML value is not the realized launch value. No source change is needed for this comment; complete hardware validation of this head is still pending in run 33554199275.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33554199275 |
|
/stage-results 33554199275 |
|
@cquil11 staged run 33554199275: https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-09-01~r33554199275 This run remains available across future |
|
Validation complete: native label-triggered full sweep https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33554199275 is fully green, attempt 1, at current head c38fb02. Audited all 20 expected throughput points across six jobs, plus all four dedicated GSM8K evaluations (1,319 distinct questions each). Strict accuracies: STP TP8 96.36%, MTP TP8 96.66%, STP DEP8 95.83%, MTP DEP8 95.68%; all pass the existing 91% threshold. Result collection, comparison, changelog metadata, success-rate calculation, and evaluation collection also passed. Request-count caveat: 20,471/20,472 measured requests completed. STP DEP8 c256 had one incomplete request and one upstream router stream error; its existing failure threshold was not changed. Every other throughput point completed every request, with no recurrence in the later router logs. This is CI-green, not a claim of zero request errors. Optional power telemetry was not validated. The corrected sweep retains the original 0.8 random-length ratio, 9,217 context, images, MTP settings, and engine options documented above; benchmark_serving.py remains unchanged. No config edits or failed-only reruns were needed during this sweep. Staging was requested in #2630 (comment). |


Scope
Port only the non-deprecated MI355X DeepSeek V4 fixed-sequence STP/MTP 1P1D configurations to AMD-enabled srt-slurm. No performance tuning and no changes to benchmark_serving.py.
Shared implementation: #2542. Runtime: srt-slurm #7, stacked on #1, pinned by the shared launcher to
f1537b3810ebe4fc287dbc1ad572b370e15fc4d6.The branch is stacked on #2542. The PR base is temporarily main for native label-triggered run-sweep.yml validation, because that workflow only accepts PRs targeting main.
Preserved upstream contract
lmsysorg/sglang-rocm:v0.5.14-rocm720-mi35x-20260701.lmsysorg/sglang-rocm:v0.5.15-rocm720-mi35x-20260713.Corrections to the earlier WIP
Removed the unrequested common July 19 image bump, 9472-token context increase, DP-aware/rank-query recipe overrides, extra server metrics/cache-report flags, and serial random-data generation override. Decode no longer inherits prefill-only chunk sizing. Restored the original eight-hour Slurm limit.
The first fixed-sequence sweep exposed another porting error: the recipes hardcoded random-range-ratio 1.0 instead of consuming the workflow's 0.8. This forced every MTP request to the maximum length and triggered the pinned engine's context validation (0 successful requests). The original successful MTP sweep confirms ratio 0.8 and 2560/2560 successful requests with context 9217. The port now forwards RANDOM_RANGE_RATIO and uses the legacy 0.8 fallback in all four recipes. The previously suggested context increase was not applied.
Orchestration-only differences remain explicit: srt-slurm owns allocation, readiness, native router startup, AMD transport setup and teardown; model/cache paths and API model aliases use the shared integration; logs and artifacts use job-scoped shared staging. On-cluster SHA-256 checks confirm the cached model config, tokenizer and shard index match the legacy checkpoint files.
Startup behavior difference
The shared srt-slurm backend adds
--skip-server-warmupfor native SGLang P/D workers. This is a startup behavior change: SGLang's local synthetic warmup can use a placeholder peer and block readiness. The native router readiness check and the unchanged benchmark warmups exercise the real P/D path instead. Model inference options and measured-workload warmup counts are not changed by this startup bypass. srt-slurm also assigns explicit NCCL/bootstrap/API ports to avoid orchestration port collisions.Validation
At head
c38fb02378565de853fed5860c7459473f655a83, based on latest mainbdca939fe5ca04cf23bb06cbc83cbdfe95030203and shared #2542 headcdb9e5fed:Hardware validation is not complete. Current native full-sweep-enabled run: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33554199275
This replaces failed run 33533933546 because that run used the incorrect hardcoded 1.0 workload ratio. A failed-only rerun would still use that old commit, so it cannot validate the correction. No results from that run will be staged or reused as full validation. Stage only after the corrected full native sweep and every required eval are green and artifacts have been checked.
Note
High Risk
Deletes the entire amd_utils multinode benchmark orchestration stack; any workflow or recipe still pointing at those scripts will fail until fully on srt-slurm and updated agentic entrypoints.
Overview
Removes the legacy
benchmarks/multi_node/amd_utilsSLURM harness (submit/job flow, SGLang/vLLM/ATOM server launchers, model YAMLs, RDMA preflight, and fixed-seqbench.sh) and deletes the DSV4 agentic recipe that submitted through that tree. MI355X DeepSeek V4 fixed-sequence disagg is expected to run via srt-slurm recipes instead (per PR scope).Agentic path stays on
agentic_srt.sh: when prefill DP-attention is on, it sets the SGLang router correlation routing header; before each concurrency step (defaultCLEAR_CACHE_BETWEEN_CONC=1) it flushes worker GPU/HiCache and optionally L3 via metrics URLs—behavior aligned with the old amd_utils trace replay.Eval wiring:
benchmark_lib.shbuilds the OpenAI eval base URL fromEVAL_SERVER_HOST(default0.0.0.0) so clients on another node (e.g. srt-slurm) can reach the API.CI: benchmark workflow git-repair re-chowns leftover files under
benchmarks/multi_node/amd_utilswhen containers left root-owned artifacts on persistent runners.Reviewed by Cursor Bugbot for commit c38fb02. Bugbot is set up for automated code reviews on this repo. Configure here.