[AgentX] vLLM DeepSeek-V4 GB200 disagg#2260
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 @ivanium's task in 4m 37s —— View job Review complete
Findings (2 inline comments)
Checked and clean
Not verifiable from CII couldn't reach github.com from this environment to confirm that commit |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - "Add GB200 Dynamo-vLLM AgentX aggregate TP8 at conc [1,4,8,16] and disaggregated topologies: 1P/1D DEP8/DEP8 at [64,128,256,320], 1P/1D DEP8/DEP12 at [128,256,320], 2P/1D DEP8/DEP12 at [480,640,768], and 3P/1D DEP8/DEP16 at [640,800,960,1280]." | ||
| - "Aggregate TP8 uses sparse DSV4 attention, NUMA binding, max-num-seqs/CUDA graph size 32, max-num-batched-tokens 32768, and 0.95 GPU memory utilization." | ||
| - "Image: vllm/vllm-openai:nightly-dev-arm64-cu13.0.1-c188b96" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/YYYY |
There was a problem hiding this comment.
🔴 BLOCKING: Placeholder PR link left in the changelog entry
Why it matters: The changelog is the audit trail tying config changes to PRs; pull/YYYY is a dead link and breaks that traceability.
Fix:
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/YYYY | |
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2260 |
|
|
||
| backend: | ||
| type: vllm | ||
| connector: "null" |
There was a problem hiding this comment.
🟡 WARNING: connector: "null" is the YAML string "null", not a null value
Why it matters: Every other recipe in this repo (117 of them, including the new agg-gb200-tp8-agentic.yaml and the existing disagg-gb200-2p1d-dep8-dep8-agentic.yaml) uses an unquoted connector: null. If srt-slurm checks truthiness or matches connector names (rather than string-comparing against "null"), the string will take the "connector configured" code path and could conflict with the explicit kv-transfer-config MultiConnector setup in vllm_config. This same quoting appears in all four new disagg recipes (1p1d-dep8-dep8, 1p1d-dep8-dep12, 2p1d-dep8-dep12, 3p1d-dep8-dep16), which suggests a copy-paste artifact rather than intent.
Fix (apply to all four new disagg recipes):
| connector: "null" | |
| connector: null |
If the pinned srt-slurm commit specifically requires the string "null", please note that in a comment so the inconsistency with the other 117 recipes doesn't get "fixed" later.
| git clone https://github.com/NVIDIA/srt-slurm.git "$SRT_REPO_DIR" | ||
| cd "$SRT_REPO_DIR" | ||
| git checkout de59739b172e507e15ebf145bfe305f606e82fbf | ||
| git checkout 975e1e60d1442f42384b439b80dbaa686498b4f3 |
There was a problem hiding this comment.
🔴 The IS_AGENTIC branch now clones NVIDIA/srt-slurm.git and does git checkout 975e1e60d1442f42384b439b80dbaa686498b4f3, but that SHA exists only as refs/pull/90/head (an open, unmerged PR per the code's own comment), not on any branch or tag. A plain git clone never fetches PR-head refs, so the checkout fails with fatal: reference is not a tree, breaking srt-slurm setup for all 5 new AgentX recipes in this PR before srtctl is even installed. Fix by explicitly fetching the PR ref (git fetch origin pull/90/head \&\& git checkout FETCH_HEAD) or by pinning a branch-reachable commit once NVIDIA/srt-slurm#90 merges.
Extended reasoning...
In runners/launch_gb200-nv.sh (lines 307-309), the IS_AGENTIC branch was repinned from cquil11/srt-slurm-nv.git@de59739b... to:
git clone https://github.com/NVIDIA/srt-slurm.git "$SRT_REPO_DIR"
cd "$SRT_REPO_DIR"
git checkout 975e1e60d1442f42384b439b80dbaa686498b4f3The new commit 975e1e60d1442f42384b439b80dbaa686498b4f3 is not reachable from any branch or tag in NVIDIA/srt-slurm. git ls-remote https://github.com/NVIDIA/srt-slurm.git shows it exists solely as refs/pull/90/head — i.e. it is the tip of an open, unmerged pull request (matching the PR's own new comment about needing "NVIDIA/srt-slurm#90's per-node DP launch mode"). A default git clone (no --filter, no explicit refspec) only fetches refs/heads/* and tags; it never fetches refs/pull/*/head. That means the object for 975e1e60... is never transferred into the local repository, and the subsequent git checkout cannot resolve it.
Proof (reproduced independently by three verifiers):
- Run
git ls-remote https://github.com/NVIDIA/srt-slurm.git— the SHA appears only underrefs/pull/90/head(and GitHub's syntheticrefs/pull/90/merge), never underrefs/heads/*orrefs/tags/*. - Run the script's exact sequence:
git clone https://github.com/NVIDIA/srt-slurm.git srt-slurm && cd srt-slurm && git checkout 975e1e60d1442f42384b439b80dbaa686498b4f3. - This fails with
fatal: reference is not a tree: 975e1e60d1442f42384b439b80dbaa686498b4f3, exit code 128.
One verifier also checked the escape hatch: a --filter=blob:none partial clone configures a promisor remote that can lazily fetch arbitrary SHAs via GitHub's uploadpack.allowReachableSHA1InWant, which would make the checkout succeed — but the script uses a plain, unfiltered clone, so that path does not apply here.
Because the script has no set -e on this line, execution wouldn't necessarily abort at the failed checkout, but the repository is left on NVIDIA/srt-slurm's default branch — which, per the PR's own comment, lacks the features these recipes depend on (BenchmarkType.CUSTOM/benchmark.command hook, DynamoConfig.wheel, mooncake_kv_store, and PR #90's per-node DP launch mode). So even without a hard stop, the recipe copy/setup and subsequent srtctl apply would fail downstream anyway.
This IS_AGENTIC code path is the sole setup path for all 5 new recipes added in this PR (agg-gb200-tp8-agentic, disagg-gb200-1p1d-dep8-dep8, 1p1d-dep8-dep12, 2p1d-dep8-dep12, 3p1d-dep8-dep16), so the entire new AgentX benchmark matrix this PR adds cannot run as written.
Fix: explicitly fetch the PR ref before checking it out — git fetch origin pull/90/head && git checkout FETCH_HEAD — or wait for NVIDIA/srt-slurm#90 to merge and then pin to the resulting branch-reachable commit SHA.
|
|
||
| backend: | ||
| type: vllm | ||
| connector: "null" |
There was a problem hiding this comment.
🔴 The four new disaggregated AgentX recipes (disagg-gb200-1p1d-dep8-dep8, 1p1d-dep8-dep12, 2p1d-dep8-dep12, 3p1d-dep8-dep16) set backend.connector: "null" - a quoted YAML string - instead of a real YAML null. Every other recipe in the repo, including the sibling agg-gb200-tp8-agentic.yaml added in this very PR, uses the bare connector: null; a truthiness/identity check on this field in srt-slurm would treat the string as a real (non-None) connector value and could route these four configs down a different code path than intended, conflicting with the MultiConnector kv-transfer-config they already set explicitly. Should be connector: null in all four files.
Extended reasoning...
The bug: all four new disaggregated recipes in this PR set backend.connector: "null" (quotes included, so YAML parses it as the 3-character string "null") instead of backend.connector: null (YAML's real null / Python None). This is visible at line 34 (recipe source) / line 53 (as reported) in each of disagg-gb200-1p1d-dep8-dep8-agentic.yaml, disagg-gb200-1p1d-dep8-dep12-agentic.yaml, disagg-gb200-2p1d-dep8-dep12-agentic.yaml, and disagg-gb200-3p1d-dep8-dep16-agentic.yaml.
Why it's clearly unintended: the sibling aggregate recipe added in this exact same PR, agg-gb200-tp8-agentic.yaml:49, sets connector: null correctly. A repo-wide grep of backend.connector across roughly 100 existing recipes (all vllm/deepseek-v4/8k1k/*, the pre-existing agentic disagg siblings like disagg-gb200-2p1d-dep8-dep8-agentic.yaml, disagg-gb300-1p6d-dep4-tp4-agentic.yaml, etc.) shows every single one uses the bare null. There is no disagg-vs-agg distinction driving this difference - pre-existing disagg recipes that also configure a manual kv-transfer-config still use bare null. So this is a quoting mistake isolated to exactly the four new files, not a deliberate semantic choice, and it is inconsistent even within this PR's own diff.
Code path this affects: these recipes deliberately set backend.connector to indicate "no srt-slurm auto-injected connector - I'm supplying my own KV transfer wiring", which is exactly what they do via the explicit kv-transfer-config: '{"kv_connector":"MultiConnector", ...}' for both prefill and decode roles (Nixl + MooncakeStore chained). The srt-slurm consumer is an external repo (NVIDIA/srt-slurm, pinned via the commit hash changed in runners/launch_gb200-nv.sh in this same PR) and isn't inspectable here, but the field's own name and the consistent repo-wide convention make its intended semantics unambiguous: null means "omit/auto-detect", not the literal string "null".
Why existing code doesn't catch this: YAML parses a quoted scalar as a string unconditionally - there is no automatic coercion of "null" back to None just because the text matches the null keyword. Nothing in this repo validates recipe YAML against a schema before handing it to srt-slurm, so the typo silently produces a well-formed but semantically different config that would only be caught once the sweep actually runs.
Step-by-step proof of the divergence:
- In
agg-gb200-tp8-agentic.yaml:49:connector: null-> YAML parses this to Python None.bool(None)is False, andNone is Noneis True. - In
disagg-gb200-1p1d-dep8-dep8-agentic.yaml(and the other 3 files):connector: "null"-> YAML parses this to the Python string "null".bool("null")is True (non-empty string), and"null" is Noneis False. - Any srt-slurm code of the common shape "if config.backend.connector:" or "if config.backend.connector is not None:" (a standard pattern for an Optional[str] field selecting/overriding a connector) evaluates to False for the aggregate recipe's correct null, but True for these four disagg recipes' string value.
- That means these four recipes could take the "connector override supplied" branch - attempting to look up or inject a connector literally named "null", or otherwise treating the field as explicitly set - a different path than every other recipe (roughly 99 existing plus the 1 new aggregate sibling) exercises, and one that plausibly conflicts with the manual kv-transfer-config MultiConnector wiring these recipes already provide.
Impact: at minimum this is a real, PR-introduced inconsistency deviating from the author's own convention within the same PR; at worst it causes benchmark launch failures or silently wrong connector wiring for 4 of the 5 new recipes this PR adds, which is exactly the kind of thing that would only surface once the sweep actually runs against the (externally pinned) srt-slurm parsing logic.
Fix: change connector: "null" to connector: null (remove the quotes) in all four disagg recipe files, matching agg-gb200-tp8-agentic.yaml and every pre-existing recipe.
| - "Add GB200 Dynamo-vLLM AgentX aggregate TP8 at conc [1,4,8,16] and disaggregated topologies: 1P/1D DEP8/DEP8 at [64,128,256,320], 1P/1D DEP8/DEP12 at [128,256,320], 2P/1D DEP8/DEP12 at [480,640,768], and 3P/1D DEP8/DEP16 at [640,800,960,1280]." | ||
| - "Aggregate TP8 uses sparse DSV4 attention, NUMA binding, max-num-seqs/CUDA graph size 32, max-num-batched-tokens 32768, and 0.95 GPU memory utilization." | ||
| - "Image: vllm/vllm-openai:nightly-dev-arm64-cu13.0.1-c188b96" | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/YYYY |
There was a problem hiding this comment.
🔴 The new perf-changelog.yaml entry ends with pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/YYYY, an unfilled placeholder, instead of this PR's actual number (2260). Since YYYY isn't in utils/validate_perf_changelog.py's recognized PR_LINK_PLACEHOLDERS set (only XXX is), this will fail the changelog-link validation CI check on this PR, and the auto-canonicalization step in prepare_perf_changelog_merge.py will also raise on merge. Fix by using pull/2260 or the canonical XXX placeholder.
Extended reasoning...
What the bug is: The newly appended perf-changelog.yaml entry for this PR ends with:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/YYYYEvery other entry in the file (e.g. the three preceding entries referencing pull/2241, pull/2228, pull/1800) uses a real PR number. This PR is #2260, so the link should read pull/2260. YYYY is a literal, unfilled placeholder that was never replaced with the real PR number before the diff was committed.
Why the existing tooling doesn't silently tolerate this: utils/validate_perf_changelog.py defines the set of placeholders CI is willing to accept on a PR run:
PR_LINK_PLACEHOLDERS = {
"XXX",
"https://github.com/SemiAnalysisAI/InferenceX/pull/XXX",
}validate_added_pr_link() is called with the PR's pr_number and enforces:
expected = f"https://github.com/SemiAnalysisAI/InferenceX/pull/{pr_number}"
if link not in PR_LINK_PLACEHOLDERS and link != expected:
raise ChangelogValidationError(...)YYYY is neither "XXX" nor .../pull/XXX, and it isn't equal to the expected .../pull/2260. So this check raises ChangelogValidationError and the changelog-validation CI job fails for this PR as written.
Step-by-step proof:
- PR [AgentX] vLLM DeepSeek-V4 GB200 disagg #2260 appends the new changelog entry with
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/YYYY. - CI runs
validate_perf_changelog.pywithpr_number=2260. expectedis computed ashttps://github.com/SemiAnalysisAI/InferenceX/pull/2260.- The check
link not in PR_LINK_PLACEHOLDERS and link != expectedevaluates toTrue(link is.../pull/YYYY, which matches neither branch of the OR). ChangelogValidationErroris raised:"new PR entry must use 'https://github.com/SemiAnalysisAI/InferenceX/pull/2260' or an XXX placeholder; found 'https://github.com/SemiAnalysisAI/InferenceX/pull/YYYY'".- The CI job fails.
Merge-time impact is the same: utils/prepare_perf_changelog_merge.py's canonicalize_pr_links step only auto-substitutes links that are already in PR_LINK_PLACEHOLDERS:
if link not in PR_LINK_PLACEHOLDERS:
raise ChangelogValidationError(
f"appended entry {index + 1} has unexpected pr-link {link!r}"
)Since YYYY isn't a recognized placeholder, this step also raises, so even the automated merge-prep canonicalization path (which would otherwise fix up XXX → the real PR number) cannot recover from this — it breaks the merge-prep tooling too.
Fix: Change pull/YYYY to either the canonical placeholder pull/XXX (letting the existing merge automation fill in the real number) or directly to pull/2260.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29570553875 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29571851688 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29573199869 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29575501131 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29584428160 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29584782979 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29588715396 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29589939601 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29590495431 |
ab0c086 to
caa5603
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29594390480 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29596617703 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29597538338 |
4ce9365 to
da77f5a
Compare
da77f5a to
762e4a1
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29632307243 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29666762764 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29667287021 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29668075836 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29670908163 |
9d20486 to
5115510
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29671100337 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29671550943 |
…to dynamo version mismatch vs aiperf
Backport srt-slurm health expectations for per-node vLLM data-parallel launches and apply the patch from the GB200 agentic launcher.
9417241 to
f0c052a
Compare
2 participants
No description provided.