[NV] Add dynamo trtllm gb300 qwen3.5 nvfp4 recipes for 8k1k and 1k1k#2206
[NV] Add dynamo trtllm gb300 qwen3.5 nvfp4 recipes for 8k1k and 1k1k#2206richardhuo-nv wants to merge 4 commits into
Conversation
cdf00ce to
7bb8fc0
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
perf-changelog.yaml:4355— The new entry'spr-linkishttps://github.com/SemiAnalysisAI/InferenceX/pull/{TBD}, but the changelog merge tooling only accepts a realpull/<PR>link or the literal placeholderXXX;{TBD}matches neither. This won't fail the PR-time CI gate, but it will raise aChangelogValidationErrorwhenutils/merge_with_reuse.shrunsprepare_perf_changelog_merge.py canonicalizeat merge time, blocking the standard merge path until fixed. Fix: replace{TBD}withXXX(or the literalpull/2206).Extended reasoning...
The newly appended
perf-changelog.yamlentry (line 4355) sets:pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/{TBD}The accepted values for an appended entry's
pr-linkare the canonicalpull/<PR-number>URL, or one of the literal placeholders inPR_LINK_PLACEHOLDERS('XXX'or'https://github.com/SemiAnalysisAI/InferenceX/pull/XXX', defined inutils/validate_perf_changelog.pylines 24-27). The string{TBD}is neither of these, so it will be rejected byvalidate_added_pr_link().Where it actually fires: it's worth correcting where this gets caught, since it's not where it would naively be assumed. The PR-time CI gate (
run-sweep.yml's "Validate perf-changelog matrix" step) invokesvalidate_perf_changelog.py'smain(), which only callsvalidate_matrix_compatible_change→validate_generated_config(process_changelog.py). That code path never inspectspr-linkat all —ChangelogEntry.pr_link(utils/matrix_logic/validation.py:497) is a plainstrfield with no format validator — so{TBD}sails through the PR CI check without complaint.The rejection instead happens at merge time.
utils/merge_with_reuse.sh(the documented, required merge entrypoint perREADME.mdand themerge-prsskill) unconditionally runsprepare_perf_changelog_merge.py canonicalize --pr-number <PR>as part of the merge flow. That script callscanonicalize_appended_links()→compare_entries()→validate_added_pr_link(link, pr_number). Withpr_numbersupplied, the expected value is the realpull/2206link, and the only alternative accepted is theXXXplaceholder (meant to be auto-substituted with the real PR number during canonicalization).{TBD}matches neither, sovalidate_added_pr_linkraisesChangelogValidationError('new PR entry must use ... or an XXX placeholder; found ...{TBD}'), and the merge script (running underset -euo pipefail) hard-fails.Step-by-step proof:
- PR #2206 is opened with
perf-changelog.yaml's new entry containingpr-link: .../pull/{TBD}. - The PR-time CI check (
validate_perf_changelog.py main()) runs and passes, since it never looks atpr-link. - A maintainer runs the standard merge flow:
utils/merge_with_reuse.sh <PR>. - That script invokes
prepare_perf_changelog_merge.py canonicalize --pr-number 2206. canonicalize_appended_linksiterates the appended changelog entries, callingcompare_entries(base, head, 2206)for each.- For this entry,
link = 'https://github.com/SemiAnalysisAI/InferenceX/pull/{TBD}'. It is compared against the expected canonical link (pull/2206) and againstPR_LINK_PLACEHOLDERS = {'XXX', '.../pull/XXX'}. It matches neither. validate_added_pr_linkraisesChangelogValidationError, aborting the merge script before the rest of the PRs in the batch (if any) are processed.
Impact: the PR looks green in CI but cannot be merged via the standard/enforced merge tooling until the placeholder is fixed — someone has to notice the failure, edit
perf-changelog.yaml, and re-push before merge can proceed.Fix: replace
{TBD}with theXXXplaceholder (matching the convention used by essentially every other entry in this file, which lets canonicalization substitute the real PR number automatically) or with the literalpull/2206link. - PR #2206 is opened with
-
🔴
runners/launch_gb300-nv.sh:205-211— In the new dynamo-trt + qwen3.5 + fp4 branch, the runner checks outv1.0.29from NVIDIA/srt-slurm, but every other sibling branch in this same if/elif chain (including the closest analog, dsv4/dynamo-trt directly above) checks outsa-submission-q2-2026— and this PR's own perf-changelog entry states the runner clones@sa-submission-q2-2026. If unintentional, the recipes overlaid into this checkout (authored against the submission branch's srtctl schema/layout) will likely fail to apply against the public v1.0.29 tag, breaking the sweep at launch.Extended reasoning...
The bug:
runners/launch_gb300-nv.sh:208(the newelifforFRAMEWORK==dynamo-trt && MODEL_PREFIX==qwen3.5 && PRECISION==fp4) doesgit checkout v1.0.29after cloningNVIDIA/srt-slurm. Every other branch of this sameif/elifchain that touchesdynamo-trtordynamo-sglang— thedsv4/dynamo-trtbranch directly above it (line 204), theglm5/qwen3.5dynamo-sglangbranches, and the finalelsefallback — all check outsa-submission-q2-2026. This PR's ownperf-changelog.yamlentry, added in the very same diff, spells this out explicitly: "Runner: launch_gb300-nv.sh updated to clone NVIDIA/srt-slurm@sa-submission-q2-2026 ... for the dynamo-trt+qwen3.5+fp4 path." So the code, the changelog, and every sibling code path in the file all point tosa-submission-q2-2026, and only this one new branch diverges to a different ref.Why this looks like a real bug, not a stale description: normally a description/code mismatch is more likely to be an imprecise PR write-up than a code defect (per the "nit by default" guidance). That reasoning doesn't hold as cleanly here, because the "description" in question isn't a vague high-level PR summary — it's a changelog entry added in the same commit that describes this exact new branch's behavior, and it agrees with five other code paths written in the same file, in the same PR history, using the same clone-and-checkout idiom. The odds that all of those independently converged on the wrong branch name, while only the new
elifhappens to be correct, are low. The much more parsimonious explanation is a copy/paste or typo slip when this branch was authored (e.g., pinning a tag while drafting/testing locally, then forgetting to switch back to the shared submission branch before opening the PR).Code path / mechanism:
launch_gb300-nv.shclonesNVIDIA/srt-slurmfresh into$SRT_REPO_DIR, checks out a ref, and then this new branch doesmkdir -p recipes/trtllm/qwen3.5/gb300-fp4/disaggfollowed bycp -rT .../benchmarks/multi_node/srt-slurm-recipes/trtllm/qwen3.5/gb300-fp4/disagg recipes/trtllm/qwen3.5/gb300-fp4/disagg, overlaying the 18 new recipe YAMLs into the checked-out repo.srtctl applyis then run against that recipe tree. The recipes carry provenance headers (# Source: /lustre/.../configs-variant1/...yaml) showing they were authored/validated against a specific run ofsrtctl/TensorRT-LLM — but that provenance says nothing about which srt-slurm git ref was used, so it doesn't establish thatv1.0.29(a public upstream tag) is compatible, and it cuts equally in favor ofsa-submission-q2-2026since that's the ref the PR author says was used for the whole PR.Why nothing in the existing code prevents this: the
if/elifstructure has no shared "default srt-slurm ref" variable — each branch independently hardcodes its owngit checkout <ref>, so there is no single point of truth that would catch a rogue ref, and nothing insrtctl applyvalidates ahead of time that the recipe's on-disk schema matches the checked-out srt-slurm version until the actualapply/parse step runs.Impact if this is indeed a mistake:
srtctl applywould attempt to parse/apply the 18 new recipe YAMLs (each with atrtllm_config,cache_transceiver_config,moe_config,dynamo:block, etc.) against whatever schema/CLIv1.0.29ships, which may differ from the submission branch's schema the recipes were actually written for. In the best case this fails loudly atsrtctl applytime (unknown field / validation error) and the whole GB300 qwen3.5 sweep — the entire point of this PR — never launches. In a worse case, ifv1.0.29's schema happens to overlap partially, it could silently apply a subset of settings and produce misleading benchmark numbers instead of a clean failure.Step-by-step proof of the inconsistency (this part is fully verifiable from the diff alone, no external repo access needed):
- Diff line ~204 (
dsv4/dynamo-trt, the closest analog — sameFRAMEWORK):git checkout sa-submission-q2-2026. - Diff line ~208 (new
qwen3.5/dynamo-trt/fp4branch):git checkout v1.0.29. - Diff to
perf-changelog.yaml, added in this same PR, describing this same new branch: "Runner: launch_gb300-nv.sh updated to clone NVIDIA/srt-slurm@sa-submission-q2-2026 and overlay local recipes ... for the dynamo-trt+qwen3.5+fp4 path." - The
elsefallback (line ~215, catches every framework/model combo not otherwise matched) also usessa-submission-q2-2026. - Four independent sources within this one file/PR (three sibling code branches + the fallback + the changelog prose) agree on
sa-submission-q2-2026; only the new branch's actual executedgit checkoutdisagrees.
Suggested fix: change line 208 from
git checkout v1.0.29togit checkout sa-submission-q2-2026, matching the siblingdynamo-trtbranch and the PR's own changelog — unless the author confirmsv1.0.29was a deliberate, tested pin, in which case the changelog line should be corrected instead so the two aren't contradictory.One counterpoint raised during review deserves acknowledgment: it's possible
v1.0.29is an intentional, deliberate pin to a stable release tag (arguably more reproducible than tracking a moving branch), and the changelog prose is simply reused boilerplate that wasn't updated for this specific branch. I can't rule this out without access to the upstreamNVIDIA/srt-slurmrepo to check whetherv1.0.29actually supports the TensorRT-LLM disagg recipe schema these 18 YAMLs use. That uncertainty is exactly why this should be flagged for the author to confirm before merge, rather than silently waved through — but the concrete failure mode (sweep launch fails) if it's wrong, combined with the accumulated weight of evidence (5 independent agreements vs. 1 outlier), pushes this from a nice-to-have nit to something the author should explicitly resolve before merging. - Diff line ~204 (
|
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 关于重新运行失败任务的文档 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29358360443 |
Summary
Add Qwen3.5-397B-A17B-NVFP4 GB300 disaggregated TensorRT-LLM + Dynamo multinode benchmarks (STP, no MTP) for both
1k1kand8k1kscenarios.benchmarks/multi_node/srt-slurm-recipes/trtllm/qwen3.5/gb300-fp4/disagg/(9 per scenario), sourced from validated experiment runsqwen3.5-fp4-gb300-dynamo-trtcovering the full topology sweep across both ISL/OSL scenarioslaunch_gb300-nv.shextended to cloneNVIDIA/srt-slurm@sa-submission-q2-2026and overlay local TRT-LLM recipes for thedynamo-trt + qwen3.5 + fp4pathConfig details
Image:
nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc18Dynamo: installed at runtime (
version: 1.3.0.dev20260708, TCP request plane)Model path:
/scratch/models/Qwen3.5-397B-A17B-NVFP4(node-local NVMe)Model alias:
qwen3.5-fp4(matchesmodel.pathin all recipes)All prefill workers use TP2/EP2 with attention DP (DEP2, 2 GPUs/worker). Decode topology varies per concurrency point:
1k1k (ISL=1024, OSL=1024) — 9 configs
ctx1dep2_gen17dep4_mtp0_c68ctx1dep2_gen17dep4_mtp0_c136ctx1dep2_gen17dep4_mtp0_c272ctx1dep2_gen4dep16_mtp0_c16ctx1dep2_gen5dep8_mtp0_c640ctx1dep2_gen1dep16_mtp0_c1280ctx2dep2_gen3dep16_mtp0_c1536ctx5dep2_gen3dep16_mtp0_c7680ctx5dep2_gen7dep4_mtp0_c179208k1k (ISL=8192, OSL=1024) — 9 configs
ctx1dep2_gen13tp4_mtp0_c78ctx1dep2_gen4tep16_mtp0_c24ctx1dep2_gen8tep8_mtp0_c48ctx2dep2_gen15tp4_mtp0_c180ctx2dep2_gen9tp4_mtp0_c216ctx3dep2_gen1dep4_mtp0_c1536ctx7dep2_gen12tep4_mtp0_c1152ctx8dep2_gen9tep4_mtp0_c1728ctx10dep2_gen3dep8_mtp0_c4608Files changed
benchmarks/multi_node/srt-slurm-recipes/trtllm/qwen3.5/gb300-fp4/disagg/1k1k/stp/— 9 new recipe YAMLsbenchmarks/multi_node/srt-slurm-recipes/trtllm/qwen3.5/gb300-fp4/disagg/8k1k/stp/— 9 new recipe YAMLs.github/configs/nvidia-master.yaml— newqwen3.5-fp4-gb300-dynamo-trtentry (18 search-space points)runners/launch_gb300-nv.sh— newelifbranch fordynamo-trt + qwen3.5 + fp4to overlay local recipesperf-changelog.yaml— new entry to trigger the GB300 sweep