Add DeepSeek-V4-Pro NVFP4 GB300 1k1k STP dynamo-sglang scenarios#2099
Add DeepSeek-V4-Pro NVFP4 GB300 1k1k STP dynamo-sglang scenarios#2099hshrivastava-droid wants to merge 3 commits into
Conversation
Appends 7 new fixed-seq-len entries (isl=1024, osl=1024) to the existing dsv4-fp4-gb300-dynamo-sglang key: 1P1D DEP8/DEP16, 1P4D and 1P6D TP4, 2P1D DEP12/DEP16/DEP48. Recipes under benchmarks/multi_node/srt-slurm-recipes/sglang/deepseek-v4/1k1k/, container aligned with the existing GB300 8k1k image (lmsysorg/sglang:nightly-dev-cu13-20260520-425dffbd). launch_gb300-nv.sh gains a dynamo-sglang + dsv4 overlay branch (srt-slurm sa-submission-q2-2026 base, in-tree recipes overlaid on top).
hshrivastava-droid
requested review from
Ankur-singh,
jgangani and
kedarpotdar-nv
as code owners
|
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 关于重新运行失败任务的文档 |
1 similar comment
|
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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -0,0 +1,201 @@ | |||
| # 1k1k 1P1D / DEP8 / total 3 nodes | |||
| # conc sweep: 2048, 4096, 8192 (c/4, c/2, c where c = X * 8192) | |||
| # stack: lmsysorg/sglang nightly-dev-cu13-20260520-425dffbd (megamoe) + dynamo dd37acac04 | |||
There was a problem hiding this comment.
🟡 Header comment on line 3 in 5 new 1k1k recipes says dynamo dd37acac04 but the actual pinned dynamo.hash in the same file is 81d0555ee23519cea80a42b4fe824e30368b7300 — two different commits. Pinned hash is what gets built, so this is documentation-only staleness, but a reader debugging a dynamo issue could grep for the wrong short hash. Affects disagg-1p1d-dep8-conc8192.yaml, disagg-1p1d-dep16-conc8192.yaml, disagg-2p1d-dep12-conc18432.yaml, disagg-2p1d-dep16-conc16384.yaml, disagg-2p1d-dep48-conc16384.yaml. Trivial fix: update the comment to 81d0555e or drop the hash from it.
Extended reasoning...
What the bug is
Each of the five new fixed-seq-len 1k1k recipes carries a header comment on line 3 of the form:
# stack: lmsysorg/sglang nightly-dev-cu13-20260520-425dffbd (megamoe) + dynamo dd37acac04
But the same file, a few lines below, has:
dynamo:
hash: "81d0555ee23519cea80a42b4fe824e30368b7300"
install: trueThe short hash dd37acac is unambiguously not a prefix of 81d0555e... — these are two different dynamo commits.
Why the pinned hash is what actually matters
Only the dynamo.hash field is consumed by srtctl to install dynamo (install: true triggers the source build against that ref). The header comment is purely human-facing documentation. So the benchmark itself runs against 81d0555e...; the comment is stale from an earlier iteration of the recipe template.
Impact
Zero runtime impact — the benchmark builds and runs against the pinned hash regardless of what the comment says. The concrete failure mode is human: someone debugging a dynamo-related issue reads the header, greps upstream dynamo history for dd37acac04, and either finds the wrong commit or nothing at all, wasting a small amount of triage time.
Step-by-step proof
- Open
benchmarks/multi_node/srt-slurm-recipes/sglang/deepseek-v4/1k1k/disagg-1p1d-dep8-conc8192.yaml. - Line 3:
# stack: lmsysorg/sglang nightly-dev-cu13-20260520-425dffbd (megamoe) + dynamo dd37acac04 - Around line 29:
dynamo:/hash: "81d0555ee23519cea80a42b4fe824e30368b7300". - Compare short hash
dd37acacto the first 8 chars of the pinned full hash:81d0555e. They are not equal, and one is not a prefix of the other, so they refer to different commits. - Repeat for the other four affected files — same mismatch verbatim.
Cross-check against the existing 8k1k recipes on the same master-config key (dsv4-fp4-gb300-dynamo-sglang): they consistently pin 81d0555ee23519cea80a42b4fe824e30368b7300, so the pinned hash is the intended one and the header comment is what's stale.
How to fix
Two equally acceptable fixes; either takes one line per file:
- Update the comment:
# stack: lmsysorg/sglang nightly-dev-cu13-20260520-425dffbd (megamoe) + dynamo 81d0555e - Or drop the short hash from the header entirely, letting the pinned
dynamo.hashfield be the single source of truth.
Severity
Nit. This is stale docs with no runtime effect, so it does not block merge. Worth fixing in a follow-up (or squashed into this PR before merge if convenient) to keep the header honest for future readers.
| set -e | ||
| ulimit -n 1048576 | ||
| REPO=/configs/upstream-sa-bench/InferenceX | ||
| [ -d "$REPO" ] || git clone https://github.com/fzyzcjy/InferenceX.git "$REPO" |
There was a problem hiding this comment.
🟡 The 5 new custom-command 1k1k recipes (disagg-1p1d-dep8, -dep16, disagg-2p1d-dep12, -dep16, -dep48) clone bench_serving from https://github.com/fzyzcjy/InferenceX.git — a personal user fork rather than SemiAnalysisAI/InferenceX. Since no commit is pinned and the clone is only performed on first use per-worker, a fork rename/deletion/force-push would break these sweeps on any fresh runner node where /configs/upstream-sa-bench/InferenceX doesn't yet exist. Suggest cloning from SemiAnalysisAI/InferenceX (or better, using the checked-out $GITHUB_WORKSPACE) — the flags used (--num-warmups, --random-num-workers) all exist in the official repo's utils/bench_serving.
Extended reasoning...
What the bug is. Each of the 5 new custom-command recipes contains this block in benchmark.command:
REPO=/configs/upstream-sa-bench/InferenceX
[ -d "$REPO" ] || git clone https://github.com/fzyzcjy/InferenceX.git "$REPO"
cd "$REPO/utils/bench_serving"The URL points at a personal user fork (fzyzcjy/InferenceX), not the official SemiAnalysisAI/InferenceX org repo. No branch or commit SHA is pinned — the clone takes whatever HEAD the fork owner has today.
Why this is a real concern. A grep of the repo confirms these 5 files are the only occurrences of fzyzcjy/InferenceX anywhere in the tree. The rest of the codebase (e.g. utils/runner_setup/RUNNER_SETUP.md:81) consistently uses SemiAnalysisAI/InferenceX, and the sibling 8k1k sglang/deepseek-v4 recipes don't clone bench_serving at all — they use benchmark.type: sa-bench. So this is a novel one-off pattern introduced by this PR, not an existing convention.
Failure mode — step-by-step. Consider a fresh GB300 compute node that has never run one of these recipes before:
benchmark.commandruns;$REPO=/configs/upstream-sa-bench/InferenceXdoesn't exist yet.- The
[ -d "$REPO" ] ||guard triggersgit clone https://github.com/fzyzcjy/InferenceX.git. - If the fork has been deleted, renamed, made private, or the org has revoked the user's access,
git cloneexits non-zero. - Because the block starts with
set -e, the whole custom command aborts and every concurrency shard fails to produce a result JSON. - All 5 recipes hit the same failure — the DEP8/DEP16/DEP12/DEP16/DEP48 sweeps in this family all break together.
Additionally, since no SHA is pinned, if the fork owner force-pushes different benchmark_serving.py code (intentionally or by accident) between two sweep runs on different runners, the two sweeps could produce inconsistent numbers with no obvious cause.
Why existing code doesn't prevent it. The [ -d "$REPO" ] check only shields cached workers; any first-run worker (new node, cleared /configs, new SLURM allocation cache) is fully exposed. There is no fallback, no retry, no alternate URL. The clone's stderr also isn't logged separately — it will surface in the benchmark output but only as the trailing set -e abort, not as a labeled cause.
Impact. This is a fragility/reproducibility issue, not a correctness one — a broken clone produces a loud failure, not silent bad numbers (unless the fork force-pushes bad code). Today, as long as the fork exists and matches upstream, the sweeps run fine, so this doesn't block correct behavior on the current merge. But it introduces long-term breakage risk on a hot path.
How to fix. All flags used (--num-warmups, --random-num-workers, etc.) already exist in utils/bench_serving/benchmark_serving.py in this repo — the fork is not offering a unique payload. So the fix is one of:
- Change the URL to
https://github.com/SemiAnalysisAI/InferenceX.git(and optionally pin a SHA viagit -C "$REPO" checkout <sha>). - Or better, copy from the already-checked-out workspace mounted into the container (e.g.
cp -r $GITHUB_WORKSPACE/utils/bench_serving …) — no clone required.
The change is a one-line replacement in each of the 5 files.
| - "Topologies: 1P1D DEP8 and DEP16 (custom conc sweep), 1P4D and 1P6D TP4 (sa-bench conc sweep), 2P1D DEP12/DEP16/DEP48 (custom conc sweep)." | ||
| - "Recipes live under benchmarks/multi_node/srt-slurm-recipes/sglang/deepseek-v4/1k1k/, container aligned with the existing 8k1k image (lmsysorg/sglang:nightly-dev-cu13-20260520-425dffbd)." | ||
| - "launch_gb300-nv.sh gains a dynamo-sglang + dsv4 overlay branch (srt-slurm sa-submission-q2-2026 base, in-tree recipes overlaid on top)." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2099 |
There was a problem hiding this comment.
🟡 The new perf-changelog.yaml entry has pr-link: XXX — a placeholder that wasn't replaced before opening the PR. All other entries in this file use https://github.com/SemiAnalysisAI/InferenceX/pull/<number>; this one should be updated to https://github.com/SemiAnalysisAI/InferenceX/pull/2099 before merge.
Extended reasoning...
What the bug is. The last entry appended to perf-changelog.yaml (line 4585) has pr-link: XXX. Every other pr-link in this file — including the immediately preceding entries at lines 4556, 4563, 4570, and 4576 — points to a real GitHub PR URL of the form https://github.com/SemiAnalysisAI/InferenceX/pull/<number>. A grep across the file shows 575 total pr-link occurrences, and this is the only one that is a bare XXX string. The template documented in AGENTS.md → Updating Docker images also shows the expected shape: a real PR URL.\n\nHow it manifests. The changelog is data — it is not consumed by the benchmark sweep or the recipe loaders, so the sweep will run fine and CI will not flag this. The consequence is purely at the traceability layer: anyone looking back through the changelog to find which PR triggered the dsv4-fp4-gb300-dynamo-sglang 1k1k STP additions will land on the literal string XXX instead of a clickable link back to this PR.\n\nWhy existing checks don't catch it. There is no linter for the pr-link field format — the schema (implicit) accepts any string. Reviewers rely on the convention.\n\nStep-by-step proof.\n1. Open perf-changelog.yaml at line 4574 (the diff hunk).\n2. The new entry (lines 4578–4585) has description bullets and ends with pr-link: XXX.\n3. Compare to the immediately preceding entry (lines 4568–4576, unchanged): pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/1994.\n4. Same for every other entry in the file — grep -c '^ pr-link: https' perf-changelog.yaml returns 574, and grep -c '^ pr-link: XXX' perf-changelog.yaml returns 1.\n\nHow to fix. Replace pr-link: XXX on the last line of perf-changelog.yaml with pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2099 before merging.\n\nSeverity rationale. Marking as nit rather than normal: this is data-hygiene in an append-only metadata file; the sweep and recipes are unaffected by the string value, and there is no runtime failure. It is, however, trivially fixed before merge and worth catching as part of the author checklist.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=28844113707 |
1 participant
Summary
Appends 7 new 1k1k STP disagg scenarios to the existing
dsv4-fp4-gb300-dynamo-sglangmaster-config key. No changes to the existing 8k1k scenarios.lmsysorg/sglang:nightly-dev-cu13-20260520-425dffbd(aligned with the existing 8k1k image on this key)benchmarks/multi_node/srt-slurm-recipes/sglang/deepseek-v4/1k1k/launch_gb300-nv.shgains adynamo-sglang + dsv4branch that clones srt-slurm atsa-submission-q2-2026and overlays the in-tree recipes on top.