Skip to content

[AMD] Port MI355X DeepSeek V4 fixed-sequence disagg to srt-slurm - #2630

Open
cquil11 wants to merge 23 commits into
mainfrom
agent/port-dsv4-mi355x-fixedseq-srt-slurm
Open

[AMD] Port MI355X DeepSeek V4 fixed-sequence disagg to srt-slurm#2630
cquil11 wants to merge 23 commits into
mainfrom
agent/port-dsv4-mi355x-fixedseq-srt-slurm

Conversation

@cquil11

@cquil11 cquil11 commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

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

  • STP image: lmsysorg/sglang-rocm:v0.5.14-rocm720-mi35x-20260701.
  • MTP image: lmsysorg/sglang-rocm:v0.5.15-rocm720-mi35x-20260713.
  • Six throughput allocations: TP8 c1/2/4/8/16/32/64/128, DEP8 c256, DEP8 c512; each STP and MTP.
  • Four standard accuracy allocations: TP8 c128 and DEP8 c512, each STP and MTP.
  • EAGLE two-step on TP8 and one-step on DEP8, on both prefill and decode.
  • 9217-token server context, prefill/decode memory fractions 0.80/0.85, original graph range, per-point DEP admission and MoRI dispatch pin.
  • Original random router policy and retries. No new DP-aware routing. Both roles receive the original explicit rdma0-rdma7 disaggregation device list.
  • Original nominal 8192/1024 workload with workflow RANDOM_RANGE_RATIO=0.8, chat encoding for MTP, warmup/prompt counts, default random-data workers, and TTFT/TPOT/ITL/E2EL percentile reporting.

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-warmup for 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 main bdca939fe5ca04cf23bb06cbc83cbdfe95030203 and shared #2542 head cdb9e5fed:

  • 246 current matrix tests passed.
  • All four recipes pass the pinned srt-slurm dry-run and embedded Bash syntax checks.
  • 12 command-level checks executed the real shared launcher transformation and each recipe's shell command with unset, standard 0.8, and explicit alternate ratio inputs; verified propagation, original context, warmups, prompt counts, and MTP chat flags.
  • Changelog validation against latest main and git diff --check passed.
  • 126 focused eval dispatch, artifact, and Slurm logging tests passed after syncing main's test cleanup.
  • benchmark_serving.py, serving images, context limits, and MTP settings remain unchanged.

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_utils SLURM harness (submit/job flow, SGLang/vLLM/ATOM server launchers, model YAMLs, RDMA preflight, and fixed-seq bench.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 (default CLEAR_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.sh builds the OpenAI eval base URL from EVAL_SERVER_HOST (default 0.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_utils when 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.

@github-actions

Copy link
Copy Markdown
Contributor

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 As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

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 模板,包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have

如需进行 PR 验证,请为此 PR 添加 full-sweep-fail-fast 标签(强烈推荐)— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动(flake),重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-agentx-srt-slurm branch 2 times, most recently from 8084da4 to 1494f62 Compare August 26, 2026 23:42
@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-fixedseq-srt-slurm branch from ea2d817 to 41d0e1d Compare August 27, 2026 00:13
@cquil11
cquil11 changed the base branch from agent/port-dsv4-mi355x-agentx-srt-slurm to main August 27, 2026 00:13
@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-fixedseq-srt-slurm branch 4 times, most recently from 3dc4003 to 85269af Compare August 27, 2026 06:42
@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-fixedseq-srt-slurm branch from 3ac7c06 to ee8c921 Compare August 27, 2026 16:24
@cquil11
cquil11 changed the base branch from main to agent/srt-slurm-amd-integration August 27, 2026 16:25
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from 8581966 to 1d4522c Compare August 27, 2026 16:28
@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-fixedseq-srt-slurm branch from ee8c921 to f285e40 Compare August 27, 2026 16:28
@cquil11
cquil11 force-pushed the agent/srt-slurm-amd-integration branch from 1d4522c to 6da0605 Compare August 27, 2026 17:27
@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-fixedseq-srt-slurm branch from f285e40 to 389534c Compare August 27, 2026 17:27
@cquil11
cquil11 marked this pull request as ready for review August 27, 2026 17:27
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @cquil11's task in 7m 32s —— View job


PR Review

  • Gather context and diff vs base branch
  • Review 4 new srt-slurm recipe YAMLs
  • Review configs/amd-master.yaml changes (image accessibility, model prefixes)
  • Validate perf-changelog.yaml entry (appended at end, paired with master config change)
  • Check MTP benchmark requirements (--use-chat-template) and server launch formatting
  • Post review findings

Summary

Found 1 blocking issue and 1 warning (posted as inline comments):

  1. 🔴 BLOCKING (configs/amd-master.yaml): Both DEP8 lanes (dp-attn: true, ep: 8) trigger the DP+EP resize block in runners/launch_mi355x-amds-srt.sh:213, which does int(os.environ["CONC"]) — but fixed-seq multinode jobs only pass CONC_LIST, never CONC, so the launcher crashes with ValueError before submission. Additionally, any fix that supplies a single CONC value conflicts with the recipe's internal c256/c512 loop (the c512 point would run against a server admission-capped at 256).
  2. 🟡 WARNING (perf-changelog.yaml): Three different srt-slurm commits are referenced — the changelog says PR [NVIDIA] Add DSR1 TensorRT Support and Enhanced Plotting #7 @ 10a58d2e, the PR body says [NVIDIA] Add TRT-LLM 70B FP8 via slurm #1 @ c609754b, and the launcher (unchanged in this PR) pins c87d7b34. If the DP-aware routing features the DEP8 recipes rely on aren't in the pinned commit, those lanes fail at runtime.

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 --use-chat-template (with --dsv4, which requires it), all benchmark flags exist in utils/bench_serving/benchmark_serving.py, result filenames match the launcher's -c<N>.json parsing, and the TP8 lanes follow the established qwen3-0.6b srt-disagg pattern cleanly.

Comment thread configs/amd-master.yaml
@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-fixedseq-srt-slurm branch from 68ef3a8 to 58cfe39 Compare September 1, 2026 16:39
@cquil11
cquil11 changed the base branch from agent/srt-slurm-amd-integration to main September 1, 2026 16:40
@cquil11
cquil11 requested a review from a team September 1, 2026 16:40
@cquil11

cquil11 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@cquil11

cquil11 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

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.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

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.
@cquil11
cquil11 force-pushed the agent/port-dsv4-mi355x-fixedseq-srt-slurm branch from 84b6055 to c38fb02 Compare September 1, 2026 20:14
@cquil11

cquil11 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

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
Corrected native full sweep: https://github.com/SemiAnalysisAI/InferenceX/actions/runs/33554199275

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.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Fix All in Cursor

❌ 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.

Comment thread configs/amd-master.yaml
ep: 1
dp-attn: false
additional-settings:
- "PREFILL_NODES=1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c38fb02. Configure here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread configs/amd-master.yaml
num-worker: 1
tp: 8
ep: 8
dp-attn: true

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c38fb02. Configure here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@cquil11

cquil11 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

/stage-results 33554199275

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@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 /stage-results requests. Staging the same run ID again updates its staged data. Staging workflow

@cquil11

cquil11 commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant