[WIP] Add GB300 DeepSeek-V4 dynamo-sglang agentic recipes#2157
[WIP] Add GB300 DeepSeek-V4 dynamo-sglang agentic recipes#2157csahithi wants to merge 2 commits into
Conversation
Add aggregated (TP4) and disaggregated (DEP8 pareto) GB300 DeepSeek-V4 agentic-coding recipes with prefill-side MTP to match decode, along with the master-config sweep entries, GB300 launcher support, and the sgl-deep-gemm downgrade helper.
csahithi
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 关于重新运行失败任务的文档 |
2 similar comments
|
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 关于重新运行失败任务的文档 |
|
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 关于重新运行失败任务的文档 |
csahithi
added
NVIDIA
full-sweep-enabled
agentx
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29116228021 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
runners/launch_gb300-nv.sh:7-8— The launcher'sSLURM_PARTITION/SLURM_ACCOUNTexports at lines 7-8 are unconditional — they flip every gb300-nv job (glm5, dsr1, dsv4-trt/vllm, minimaxm3, kimik2.5, and ~11 others in configs/nvidia-master.yaml) from(batch_1, benchmark)to(batch_2, restricted), not just the new DSv4 sglang agentic recipes. If this cluster-wide migration is intentional it should be called out in the PR description; otherwise please gate it inside the same$IS_AGENTIC == 1 && $FRAMEWORK == dynamo-sglang && $MODEL_PREFIX == dsv4branch used later in the file.Extended reasoning...
What changes and where
At
runners/launch_gb300-nv.shlines 7-8, the launcher unconditionally exports:export SLURM_PARTITION="batch_2" # was batch_1 export SLURM_ACCOUNT="restricted" # was benchmark
These are at the top of the file, before any framework/model branching. Every job whose GHA runner name starts with
gb300-nv_invokes this launcher (viabash ./runners/launch_${RUNNER_NAME%%_*}.shin the benchmark templates), so these values are inherited by all gb300-nv jobs.Blast radius
Grepping
configs/nvidia-master.yamlforrunner: (cluster:)?gb300-nvreturns ~15 pre-existing config keys that will silently switch to the new partition/account after this PR merges:glm5-fp4-gb300-dynamo-trt(and MTP variants)glm5-fp4/fp8-gb300-dynamo-sglang(and MTP variants)glm5.1-fp4/fp8-gb300-dynamo-sglangdsr1-fp8-gb300-dynamo-trtdsv4-fp4-gb300-dynamo-vllm,-trt,-trt-mtp,-vllm-agenticminimaxm3-fp8-gb300-dynamo-vllmkimik2.5-fp4-gb300-dynamo-vllm
Sibling launchers (
launch_b300-nv.shstill usesbatch_1/benchmark,launch_gb200-nv.shusesbatch/benchmark) were not touched, so this isn't a repo-wide cluster migration.Inconsistency with the rest of the diff
The same PR gates its srt-slurm repo checkout with
if [[ "$IS_AGENTIC" == "1" && $FRAMEWORK == "dynamo-sglang" && $MODEL_PREFIX == "dsv4" ]](line ~136), acknowledging the new stack is DSv4-agentic-specific. The partition/account exports ignore that pattern.Failure modes
- Submission fails. If the
restrictedaccount has no scheduling permission onbatch_2for the pre-existing non-DSv4 recipes, they all fail submission with a SLURMACCOUNT_LIMIT/QOS_JobLimiterror on their next sweep run. - Silent baseline shift. Even if submission succeeds, if
batch_2differs frombatch_1in QoS/time-limit/node-count/interconnect topology, the ingested benchmark numbers for those 12 pre-existing configs will diverge from the pre-PR baseline already in the DB — attributed to this PR but caused by an unrelated infra change.
Step-by-step proof of scope leak
- Merge this PR to main.
- A push-to-main sweep triggers because
perf-changelog.yamlchanged. - The unchanged config
glm5-fp4-gb300-dynamo-trt(routes tocluster:gb300-nv) is in the matrix. - GHA schedules it on a
gb300-nv_*runner; the runner invokesbash ./runners/launch_gb300-nv.sh. - Line 7 exports
SLURM_PARTITION=batch_2; line 8 exportsSLURM_ACCOUNT=restricted. srtctl applyreads these viadefault_partition/default_accountin the generatedsrtslurm.yamland submits to (batch_2,restricted) — even though nothing about the glm5 recipe changed and the PR description never mentions this partition move.
How to fix
If the change is intentionally cluster-wide — please call it out in the PR description (why the migration, and confirmation that all pre-existing recipes have permission on
batch_2/restricted).If it is DSv4-agentic-only, guard it with the same predicate the srt-slurm checkout already uses:
if [[ "$IS_AGENTIC" == "1" && $FRAMEWORK == "dynamo-sglang" && $MODEL_PREFIX == "dsv4" ]]; then export SLURM_PARTITION="batch_2" export SLURM_ACCOUNT="restricted" else export SLURM_PARTITION="batch_1" export SLURM_ACCOUNT="benchmark" fi
The full-sweep-enabled label on this PR will catch scenario (1) if it fires, but (2) is silent — the CI can't distinguish "the recipe got slower" from "a different partition served it".
| # Runs via `setup_script: pin-sgl-deep-gemm.sh` (before dynamo install + worker | ||
| # startup, inside each worker's container). Prints before/after so the run log | ||
| # shows what the container shipped vs what we pinned. | ||
| set -euo pipefail | ||
| export PIP_BREAK_SYSTEM_PACKAGES=1 | ||
|
|
||
| TARGET="${SGL_DEEP_GEMM_VERSION:-0.1.3}" | ||
|
|
||
| echo "[pin-sgl-deep-gemm] before:" | ||
| pip show sgl-deep-gemm 2>/dev/null | grep -iE '^(Name|Version):' || echo " (not installed)" | ||
|
|
||
| echo "[pin-sgl-deep-gemm] installing sgl-deep-gemm==${TARGET} ..." | ||
| pip install --force-reinstall --no-deps \ | ||
| --extra-index-url https://www.piwheels.org/simple \ | ||
| "sgl-deep-gemm==${TARGET}" | ||
|
|
||
| echo "[pin-sgl-deep-gemm] after:" |
There was a problem hiding this comment.
🟡 The new downgrade-sgl-deep-gemm.sh script has stale pin-sgl-deep-gemm references left over from a rename that missed the file's internals: the header comment on line 11 still says Runs via setup_script: pin-sgl-deep-gemm.sh`` and the log prefixes on lines 19, 22, and 27 print [pin-sgl-deep-gemm]. No functional impact — the script runs correctly — but anyone grepping sweep logs for `downgrade-sgl-deep-gemm` will miss the actual before/after markers. Renaming those four occurrences to `downgrade-sgl-deep-gemm` restores log discoverability.
Extended reasoning...
What the bug is
The new script at benchmarks/multi_node/srt-slurm-recipes/configs/downgrade-sgl-deep-gemm.sh contains stale references to a previous name (pin-sgl-deep-gemm.sh) inside its own body. Specifically:
- Line 11 header comment:
# Runs viasetup_script: pin-sgl-deep-gemm.sh(before dynamo install + worker - Line 19:
echo "[pin-sgl-deep-gemm] before:" - Line 22:
echo "[pin-sgl-deep-gemm] installing sgl-deep-gemm==${TARGET} ..." - Line 27:
echo "[pin-sgl-deep-gemm] after:"
Everywhere else in the PR, the script is referenced by its actual filename downgrade-sgl-deep-gemm.sh:
- All 6 new recipe YAMLs (agg-gb300-tp4-…, disagg-gb300-2p1d-…, 2p4d-…, 4p4d-…, 6p4d-…, 12p4d-…) set
setup_script: downgrade-sgl-deep-gemm.sh runners/launch_gb300-nv.sh:147setsSRTCTL_SETUP_SCRIPT="downgrade-sgl-deep-gemm.sh"and copies the file under that name
Grepping the repo for pin-sgl-deep-gemm returns matches only inside this script itself — no file named pin-sgl-deep-gemm.sh exists anywhere. This is a clear stale-rename artifact: the file was renamed from pin-… to downgrade-… but the internal comment and log prefixes were not updated.
Impact
Purely cosmetic — the script still executes correctly because setup_script: wiring in srtctl and SRTCTL_SETUP_SCRIPT in the launcher both go by filename, and echo prefixes are only read by humans. The one real cost is log-grep discoverability: someone tailing a sweep log and searching for the string downgrade-sgl-deep-gemm (the recipe-declared script name) will miss the before/after installation markers, which are the whole reason the echos exist.
Step-by-step proof
- In the PR diff, the new file appears as
new file mode 100644atbenchmarks/multi_node/srt-slurm-recipes/configs/downgrade-sgl-deep-gemm.sh— that is its final name on disk. - Line 11 of that same file, still in the diff:
# Runs viasetup_script: pin-sgl-deep-gemm.sh(before dynamo install + worker— refers to a filename that does not exist in the tree. - Lines 19, 22, 27 emit log lines prefixed
[pin-sgl-deep-gemm]. - Every recipe YAML in this PR sets
setup_script: downgrade-sgl-deep-gemm.sh— a user who reads the recipe, then greps the sweep log for that string, matches nothing except a possible copy step in the launcher output.
Fix
Mechanical rename inside the script only — no callers change:
- Line 11:
pin-sgl-deep-gemm.sh→downgrade-sgl-deep-gemm.sh - Lines 19, 22, 27:
[pin-sgl-deep-gemm]→[downgrade-sgl-deep-gemm]
Severity
Nit — a doc/logging inconsistency with no runtime effect. Does not warrant blocking the PR.
| @@ -0,0 +1,28 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
is there an lmsysorg container where we don't need to patch downgrade deepgemm?
There was a problem hiding this comment.
When this SGL PR sgl-project/sglang#30697 is merged, we can then switch to the nightly container with the fix.
3 participants
Add aggregated (TP4) and disaggregated (DEP8 pareto) GB300 DeepSeek-V4 agentic-coding recipes with prefill-side MTP to match decode, along with the master-config sweep entries, GB300 launcher support.