[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=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
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29680343224 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29708231420 |
Dynamo's primary etcd lease defaults to a 10s TTL. During the ~380s cold start (weight load + cudagraph capture), a brief etcd-server-side stall (etcd is co-located with a worker; peak node memory/IO pressure) revokes every worker's lease at once. Registration then fails with "requested lease not found", workers never go ready, and the health check waits the full 2h before timing out. Observed on the 3p1d-dep8-dep16 run: all 10 workers across 10 nodes, leases granted at staggered times, expired simultaneously within 30ms. A byte-identical rerun passed because no qualifying etcd stall landed in the window -- confirming it's a TTL-gated race, not a fixed config bug. 600s rides through transient etcd stalls; the only cost, slower cleanup of a genuinely dead worker, is handled independently by the health check and the process fail-fast monitor. Applies to the 5 configs added in PR #2260 (agg + 1p1d-dep8-dep8 + 1p1d-dep8-dep12 + 2p1d-dep8-dep12 + 3p1d-dep8-dep16). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolved conflicts in utils/matrix_logic/{generate_sweep_configs,
test_generate_sweep_configs}.py: main's AMD MI355X change refactored the
same agentic sweep logic our branch touched. Both sides independently set
MAX_MULTINODE_AGENTIC_CONCURRENCIES_PER_ALLOCATION = 1, so the code
resolves to main's version verbatim. Our branch's extra test assertions
(router default, len==2) were superseded by main's refactor (router is now
optional pass-through; the code now yields one allocation per concurrency),
so the test file is taken from main. Full matrix_logic suite: 224 passed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Remove glm5-fp4-gb300-dynamo-trt-mtp from configs/nvidia-master.yaml: it was accidentally re-added by 6b06ade to the active master, but it belongs to PR #1799 (GLM-5 GB300 TRT-LLM) and lives in configs/deprecated on main -- unrelated to this DSV4 GB200 AgentX PR (#2260). Restores main's state; the historical #1799 changelog entry is left intact. - Revert disagg-gb200-2p1d-dep8-dep8 and disagg-gb200-3p2d-tep8-tp8 to main (they are not among the 5 PR #2260 config-keys; the max_attempts and router-reset-states edits were out of scope). nvidia-master.yaml now adds only the 5 dsv4-fp4-gb200-dynamo-vllm-agentic-* config-keys this PR owns. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29708259258 |
2 participants
No description provided.