Port GB200 DeepSeek V4 fixed-sequence disaggregation to srt-slurm - #2757
Port GB200 DeepSeek V4 fixed-sequence disaggregation to srt-slurm#2757cquil11 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 关于重新运行失败任务的文档 |
|
Claude finished @cquil11's task in 11m 35s —— View job Review of PR #2757 — GB200 DSV4 llm-d → srt-slurm/vLLM Router port
No blocking issues found. Two 🟡 warnings posted inline:
Everything else checked out: perf-changelog entry present, correctly appended at EOF, and documents the topology change; master-config topology matches all three recipes (workers/TP/EP/dp-attn), and node demand resolves from the recipes to 4/4/8 nodes as claimed; all Caveats: I could not execute the matrix test suite or |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| --dataset-name random \ | ||
| --random-input-len 8192 \ | ||
| --random-output-len 1024 \ | ||
| --random-range-ratio 1.0 \ |
There was a problem hiding this comment.
Benchmark ratio silently changes workload
Medium Severity
The custom benchmark_serving.py invocation hardcodes --random-range-ratio 1.0, so every request is exactly 8192/1024 tokens. The retired llm-d path passed the workflow RANDOM_RANGE_RATIO of 0.8, and other checked-in srt-slurm recipes still use 0.8. That makes sampled lengths vary in [0.8 * seq_len, seq_len], so TTFT, throughput, and token counts for the retained mid-curve and max-tpt topologies will not match the historical series.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 7801df4. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # The recipe deliberately runs all three points after one model load. | ||
| conc-list: [1] |
There was a problem hiding this comment.
🟡 WARNING: Changing this entry from conc-list: [256, 512, 1024] to [1] silently drops the eval that used to run for this topology, and leaves eval handling inconsistent across the series.
Why it matters: mark_eval_entries (utils/matrix_logic/generate_sweep_configs.py, MIN_EVAL_CONC = 16) marks the highest eligible concurrency per multinode parallelism group. Previously this DEP8/DEP8 1p1d entry was eval-marked at conc 1024; with [1] every conc is below the threshold, so no eval is marked. Meanwhile the c4096 entry (line 4481) is still eval-marked (RUN_EVAL=true, EVAL_CONC=4096), but the new recipes contain no eval stage and it's not stated whether srt-slurm PR #7's native vLLM path implements post-benchmark evals. In a full sweep a missing eval fails silently (copy_eval_artifacts warns and returns 0; the upload step uses if-no-files-found: ignore), so the historical llm-d series' eval coverage would just disappear — and an evals-only changelog lane on this key would hard-fail (if-no-files-found: error).
Fix: Either keep the real concurrency list here — the recipe ignores it for benchmarking (result concurrency comes from the result JSON's max_concurrency), so [256, 512, 1024] only restores truthful labeling and the eval marking at 1024:
| # The recipe deliberately runs all three points after one model load. | |
| conc-list: [1] | |
| # The recipe deliberately runs all three points after one model load. | |
| conc-list: [256, 512, 1024] |
…and confirm srt-slurm PR #7 runs evals for the vLLM frontend/backend path; or, if dropping evals for this series is intentional, document that in the perf-changelog.yaml entry and also make the c4096 entry consistent (it will still request an eval today).
| SRT_SLURM_PIN="76e7d76961b2dcb27cb05c1e9e0910ceb75104ec" | ||
| git clone https://github.com/SemiAnalysisAI/srt-slurm.git "$SRT_REPO_DIR" || exit 1 | ||
| cd "$SRT_REPO_DIR" || exit 1 | ||
| git checkout "$SRT_SLURM_PIN" || exit 1 |
There was a problem hiding this comment.
🟡 WARNING: SRT_SLURM_PIN points at the head of the unmerged SemiAnalysisAI/srt-slurm#7.
Why it matters: git clone only fetches branch heads (refs/heads/*), not refs/pull/*. This works only while commit 76e7d76… is reachable from a branch in SemiAnalysisAI/srt-slurm. The moment PR #7 is squash-merged (SHA disappears from branch history) or its branch is deleted, git checkout "$SRT_SLURM_PIN" fails and every sweep run of dsv4-fp4-gb200-llmd-vllm dies at setup. It also fails outright if PR #7's head lives in a fork rather than a same-repo branch, and the CI clone is unauthenticated, so the repo must be public.
Fix: After SemiAnalysisAI/srt-slurm#7 merges, re-pin to a SHA reachable from its main branch before enabling the runtime sweep (and prefer a merge commit / mainline SHA over a PR-branch head so the pin can't be garbage-collected later). Worth a # TODO: re-pin after srt-slurm#7 merges marker so this doesn't get forgotten.
There was a problem hiding this comment.
Beyond the inline findings, I also checked two other spots the diff touches for related risk: runners/launch_gb200-nv.sh's new vllm framework branch reusing /mnt/numa1 for dsv4/fp4 (matches the historical llm-d path and is commented as intentional, not the stale /mnt/numa1 issue it might look like), and install-vllm-router.sh's pip install vllm-router==0.1.15 (it's a plain pip install with default dependency resolution, but since the image already pins vllm and orjson, this shouldn't touch the serving runtime in practice — worth a second look if router installs ever start pulling unexpected vllm/torch versions).
Extended reasoning...
This PR retires the llm-d benchmark framework and repoints several GB200 runner/config paths to a renamed vllm framework path, plus adds a vllm-router install script and search-space/recipe config updates. Two confirmed findings are already queued as inline comments: stale llm-d documentation (dead TOC anchor and leftover llm-d instructions after the section was deleted) and a conc-list: [1] mismatch in configs/nvidia-master.yaml for a recipe that actually iterates concurrencies 256/512/1024, which will mislabel result artifacts. Beyond those, I independently examined two other candidates flagged during investigation: the /mnt/numa1 vs /mnt/lustre01 path selection logic in runners/launch_gb200-nv.sh for the new vllm framework case, which is intentionally preserving the historical llm-d node-local path via an explicit comment rather than being a leftover bug, and the install-vllm-router.sh comment claim that installing vllm-router "does not replace the serving runtime" despite running a normal pip install with full dependency resolution — this is plausible given the base image already pins vllm/orjson but is not guaranteed by the install command itself. Both were ruled out as not rising to confirmed-bug status but are adjacent enough to the confirmed findings that flagging them briefly is useful without duplicating the inline comments. Given two confirmed, non-trivial findings already exist (one causing silently mislabeled benchmark artifacts, another leaving broken documentation), this is not a case for approval, and per the review policy I am not restating those findings in the body — instead using the narrow allowance to report additional items examined and ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
docs/configuration-procedures.md— Dangling TOC link and stale llm-d prose after the diff deletes the 'Register an llm-d recipe' section: sweep:llm-d— line 31 links to #register-an-llm-d-recipe (now removed), and lines 88, 202, 253 still instruct readers to 'update the llm-d recipe/orchestration' / validate llm-d endpoint discovery, describing a procedure that no longer exists in this file. [also at: docs/configuration-procedures_zh.md:31 - Same stale llm-d TOC anchor and prose as the English doc (sweep:llm-dapplies here too) — lines 31, 88, 202, 253…]Extended reasoning...
An engineer following this doc after merge clicks the TOC entry and hits a dead anchor, or follows step 88/202/253's llm-d guidance and finds no corresponding procedure section, since llm-d was fully retired by this PR but these references were not cleaned up.
Verification: nit (docs-only regression; no code path affected). The diff deletes the entire
## Register an llm-d recipeheading and section (old lines 126-147 in configuration-procedures.md), which was the only source of the#register-an-llm-d-recipeanchor, AND deletes all 10 llm-d implementation files (git diff --statshows 1501 deletions across benchmarks/llm-d/* and benchmarks/multi_node/llm-d/*).…
|
|
||
| # Mid curve: 1 prefill DEP8 + 1 decode DEP8. | ||
| - spec-decoding: "none" | ||
| conc-list: [256, 512, 1024] | ||
| # The recipe deliberately runs all three points after one model load. | ||
| conc-list: [1] |
There was a problem hiding this comment.
🟡 Mid-curve dsv4-fp4-gb200-llmd-vllm search-space entry sets conc-list: [1] even though its recipe (disagg-gb200-1p1d-dep8-dep8-c256-c512-c1024.yaml) loops over concurrencies 256, 512, and 1024 in a single job. Before this diff conc-list correctly held [256, 512, 1024] (matching the codebase convention elsewhere, e.g. b200 dsv4 entries name conc-list to match the recipe's actual points and file name suffix). generate_sweep_configs.py passes the whole multinode conc-list as inputs.conc-list to one job (comment: 'Pass the entire list for multinode') -- collapsing to one job was already achieved by that mechanism, so the new placeholder is not needed and only desyncs metadata from reality.
Extended reasoning...
Every run of this mid-curve entry now produces a RESULT_FILENAME embedding 'conc1' (from benchmark-multinode-tmpl.yml's conc${{ join(fromJson(inputs.conc-list), 'x') }}) even though the job actually benchmarks concurrencies 256/512/1024, mislabeling artifacts/experiment names for every execution. It also breaks sweep-CLI concurrency filtering (utils/matrix_logic/generate_sweep_configs.py --min-conc/--max-conc): e.g. --min-conc 500 would drop this entire search-space entry (since its only declared point is 1) even though it legitimately contains 512 and 1024, whereas before the change the accurate [256,512,1024] list let those filters work correctly.
Verification: nit. configs/nvidia-master.yaml:4464 sets the mid-curve entry conc-list:[1] while its recipe benchmarks/multi_node/srt-slurm-recipes/vllm/deepseek-v4-pro/GB200/8k1k/disagg-gb200-1p1d-dep8-dep8-c256-c512-c1024.yaml hardcodes for concurrency in 256 512 1024. Sibling entries confirm the convention (low-latency conc-list:[1] -> recipe concurrency=1; max-throughput conc-list:[4096] -> recipe…
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33040971662 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33042839302 |
2d2c664 to
69330f3
Compare
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 69330f3. Configure here.
| if [[ "$FRAMEWORK" == "vllm" ]]; then | ||
| # Preserve the checkpoint used by the historical llm-d series. | ||
| # It is pre-staged at the same node-local path on every GB200 node. | ||
| export MODEL_PATH="/mnt/numa1/models/DeepSeek-V4-Pro" |
There was a problem hiding this comment.
Watchtower DSV4 path uses missing NVMe
High Severity
The new vllm path now runs through watchtower srt-slurm, but still points MODEL_PATH at /mnt/numa1/models/DeepSeek-V4-Pro. The same branch documents that this NVMe path is gone on watchtower compute nodes and the working dynamo-vllm sibling already loads the CamelCase FP4 checkpoint from Lustre. Because srtctl is invoked with --no-preflight, a missing mount is not caught at submit time and every GB200 job for this key fails when vLLM cannot open the weights.
Reviewed by Cursor Bugbot for commit 69330f3. Configure here.
| enable-sleep-mode: true | ||
| tokenizer-mode: deepseek_v4 | ||
| prefill_environment: {} | ||
| decode_environment: {} |
There was a problem hiding this comment.
C1 prefill drops required DSV4 env
Medium Severity
The new TP8/TP8 c1 recipe leaves prefill_environment empty, so it never sets VLLM_SPARSE_INDEXER_MAX_LOGITS_MB or VLLM_MAX_TOKENS_PER_EXPERT_FP4_MOE. Every other DSV4 GB200 prefill in this PR and the working dynamo-vllm siblings sets both, because 8k prefill with max-num-batched-tokens 32768 is what stresses the sparse indexer and FP4 MoE tiling. Without them the latency job can OOM or mis-size kernels even though the topology change to TP8 is intentional.
Reviewed by Cursor Bugbot for commit 69330f3. Configure here.
|
Closing because the project scope is now limited to porting active, nondeprecated AMD multi-node configurations to srt-slurm. This PR is outside that scope. |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=33077802985 |
1 participant
Summary
benchmark_serving.pydirectly through the custom benchmark interfaceThe historical config key is retained for performance-series continuity. The c1 latency topology changes from DEP8-prefill/TP8-decode to TP8/TP8 because official vLLM Router has one global intra-node DP expansion factor and cannot correctly address mixed DP expansion across P/D pools.
Validation
srtctl dry-runpasses for all three recipes (4, 4, and 8 nodes)git diff --checkpassRuntime sweep will be enabled after the PR-linked changelog entry and static checks are green.
Depends on SemiAnalysisAI/srt-slurm#7 (stacked on #1).
Note
Medium Risk
Large orchestration swap on production GB200 benchmark infrastructure (multinode P/D, pinned forked srt-slurm, and matrix/eval behavior); mitigated by recipe parity and tests, but hardware regressions or series discontinuity (c1 topology) are plausible until sweeps land.
Overview
Replaces the bespoke GB200 DeepSeek-V4 disaggregation path (llm-d EPP/Envoy,
pd-sidecar, combined Docker image,submit.sh/job.slurm/server.sh, andllm-d-recipes) with srt-slurm using native vLLM, vLLM Router 0.1.15, and NIXL KV transfer. The historical master keydsv4-fp4-gb200-llmd-vllmis kept but now declaresframework: vllmand points at new checked-in recipes undersrt-slurm-recipes/vllm/deepseek-v4-pro/GB200/8k1k/.Throughput is still exercised at c1, c256, c512, c1024, and c4096, but c256/c512/c1024 are split into separate Slurm allocations (per-recipe wall times) instead of one multi-concurrency job. Recipes run
benchmark_serving.pyvia srt-slurm’s custom benchmark contract againstSRT_FRONTEND_HOST/SRT_FRONTEND_PORT, with a 6h router request timeout andinstall-vllm-router.shon the pinned v0.26 image.c1 latency topology changes from DEP8 prefill + TP8 decode to TP8 + TP8 because vLLM Router applies one intra-node DP expansion to both pools. GSM8K eval is capped at
eval-conc: 256; matrix generation now groups split recipes by serving topology (ignoringCONFIG_FILEwheneval-concis set).GB200 launcher drops the dedicated
llmd-vllmbranch, clones pinned srt-slurm (10a58d2e), stages recipes, and bind-mountsINFMAX_WORKSPACEfor DSV4vllmruns. Docs drop the llm-d registration procedure; perf-changelog records the migration and external srt-slurm dependency.Reviewed by Cursor Bugbot for commit 69330f3. Bugbot is set up for automated code reviews on this repo. Configure here.