[AgentX] vLLM DeepSeek-V4 GB300 disagg#2269
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 6m 39s —— View job PR Review — [AgentX] vLLM DeepSeek-V4 GB300 disagg
SummaryNo blocking issues; 3 warnings posted inline:
Everything else checked out: the changelog entry is appended at the end of the file; master config and changelog were updated together; the image is a public Docker Hub reference; |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| - "Add GB300 Dynamo-vLLM AgentX aggregate TP4/TP8 low-latency sweeps at conc [1,4,8,16]." | ||
| - "Add NVIDIA/srt-slurm#229-derived GB300 P/D topologies: 1P/1D DEP8/DEP8 at c512, 2P/1D DEP8/DEP8 at c1280, and 3P/1D DEP8/DEP16 at c1536." | ||
| - "Use vllm/vllm-openai:nightly-dev-arm64-cu13.0.1-c188b96 for all new recipes." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2260 |
There was a problem hiding this comment.
🟡 WARNING: Wrong PR link — this points to #2260, which is the GB200 disagg PR. This entry is for the GB300 recipes added in #2269.
Why it matters: The changelog is the audit trail tying config changes to PRs; a wrong link sends anyone tracing a perf change to the wrong diff.
Fix:
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2260 | |
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2269 |
| SRT_SLURM_AGENTIC_SHA="f6eb42aee4664207dcf2ec601e3bd57bd527efd6" | ||
| git clone --branch v1.0.27 --depth 1 https://github.com/NVIDIA/srt-slurm.git "$SRT_REPO_DIR" |
There was a problem hiding this comment.
🟡 WARNING: This repin doesn't only affect the new recipes — this same elif branch launches the existing GB300 vLLM agentic recipes disagg-gb300-1p6d-dep4-tp4-agentic.yaml and disagg-gb300-4p1d-dep4-dep8-24-c4096-agentic.yaml (still active in configs/nvidia-master.yaml:8263-8296), which pin dynamo.wheel: "1.2.0.dev20260426" rather than dynamo.hash.
Why it matters: The old pin (cquil11 fork @ 6e34b8b) carried DynamoConfig.wheel plus the benchmark-stage srun_options propagation fix (needed for container-remap-root to reach the agentic_srt.sh srun). If NVIDIA/srt-slurm v1.0.27 + the two cherry-picks dropped either — e.g. wheel was replaced by hash instead of coexisting — the existing 1p6d/4p1d sweeps will fail at recipe parse or benchmark launch. I couldn't verify the v1.0.27 schema from this environment.
Fix: Please confirm v1.0.27 still supports DynamoConfig.wheel and propagates srun_options into the benchmark stage (or migrate the two older recipes to dynamo.hash in this PR and note it in the changelog).
| MAX_MULTINODE_AGENTIC_CONCURRENCIES_PER_ALLOCATION = 4 | ||
| # One task/Slurm allocation per concurrency gives each benchmark point a | ||
| # fresh server deployment, matching single-node agentic sweep isolation. | ||
| MAX_MULTINODE_AGENTIC_CONCURRENCIES_PER_ALLOCATION = 1 |
There was a problem hiding this comment.
🟡 WARNING: Changing MAX_MULTINODE_AGENTIC_CONCURRENCIES_PER_ALLOCATION from 4 to 1 is global — it applies to every multinode agentic config, not just the new GB300 vLLM ones. For example, dsv4-fp4-gb300-dynamo-sglang-agentic-agg (conc-list [1, 4, 8, 16]) now generates 4 Slurm allocations with 4 full server deployments instead of 1.
Why it matters: Each extra allocation repeats queueing, container start, model load, and warmup for very large multi-node deployments, which meaningfully increases full-sweep wall-clock and cluster load for configs owned by other teams. The perf-changelog entry doesn't mention this behavioral change either.
Fix: If per-concurrency isolation for all multinode agentic sweeps is intended, add a line to the perf-changelog.yaml entry documenting it. Otherwise, keep the constant at 4 and drive isolation per-config (or scope the value to the configs that need it).
| MIN_EVAL_CONC = 16 | ||
| # Bound how many multinode agentic conc points share one server allocation. | ||
| MAX_MULTINODE_AGENTIC_CONCURRENCIES_PER_ALLOCATION = 4 | ||
| # One task/Slurm allocation per concurrency gives each benchmark point a |
There was a problem hiding this comment.
📊 Line Count Report
- Total Lines: 1336
- Base Lines: 1334
- Change: +2 lines 📈
cb438b7 to
2d5de20
Compare
|
|
||
| - config-keys: | ||
| - dsv4-fp4-gb300-dynamo-vllm-agentic-agg | ||
| - dsv4-fp4-gb300-dynamo-vllm-agentic-1p1d-dep8-dep8 | ||
| - dsv4-fp4-gb300-dynamo-vllm-agentic-2p1d-dep8-dep8 | ||
| - dsv4-fp4-gb300-dynamo-vllm-agentic-3p1d-dep8-dep16 | ||
| description: | ||
| - "Add GB300 Dynamo-vLLM AgentX aggregate TP4/TP8 low-latency sweeps at conc [1,4,8,16]." | ||
| - "Add NVIDIA/srt-slurm#229-derived GB300 P/D topologies: 1P/1D DEP8/DEP8 at c512, 2P/1D DEP8/DEP8 at c1280, and 3P/1D DEP8/DEP16 at c1536." | ||
| - "Use vllm/vllm-openai:nightly-dev-arm64-cu13.0.1-c188b96 for all new recipes." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2260 |
There was a problem hiding this comment.
🟡 The new perf-changelog.yaml entry added by this PR sets pr-link to #2260, but this PR is #2269 — likely a copy-paste leftover from drafting before the PR number was assigned. Every other entry in the file self-references the PR that introduced it, so this one line should be corrected to point at #2269.
Extended reasoning...
The final entry appended to perf-changelog.yaml by this PR (lines 4936-4946) documents the new dsv4-fp4-gb300-dynamo-vllm-agentic-* config keys this PR adds, but its pr-link field reads:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2260
The PR metadata confirms this change is landing as PR #2269, not #2260. Scrolling up the same file shows the established convention: every existing entry's pr-link points at the PR that introduced that entry (e.g. the MiniMax M3 entry a few lines above links to pull/2182, matching commit d85fa13 which is PR #2182; the Qwen3.5 entry links to pull/2094, matching PR #2094). There is no other reference to #2260 anywhere in this diff, and the entry's description text ("Add GB300 Dynamo-vLLM AgentX aggregate TP4/TP8 low-latency sweeps...") matches exactly the recipes and nvidia-master.yaml keys this PR introduces — so this isn't a deliberate cross-reference to a different PR, it's a stale/guessed PR number left over from before #2269 was assigned.
Step-by-step proof:
- PR metadata states this PR's number is 2269.
- The diff adds a new
perf-changelog.yamlentry withconfig-keysexactly matching the fourdsv4-fp4-gb300-dynamo-vllm-agentic-*keys added toconfigs/nvidia-master.yamlin this same diff. - That new entry's
pr-linkis.../pull/2260, i.e. 9 less than 2269. - Every other entry in the file (visible in the surrounding context, e.g.
#1800,#2241,#2094,#2182) self-references its own introducing PR, confirming the file's convention. - Therefore
pull/2260is inconsistent with both the convention and the actual PR number, and should readpull/2269.
Impact: This is purely a documentation/traceability field in a changelog — it has no effect on config generation, sweep execution, or any runtime behavior. Anyone consulting the changelog to trace where the GB300 AgentX recipes originated would be sent to the wrong (and unrelated) PR #2260. A one-line fix (2260 → 2269) resolves it; not blocking for merge.
| tensor-parallel-size: 4 | ||
| pipeline-parallel-size: 1 | ||
| enable-cumem-allocator: true | ||
| numa-bind: true |
There was a problem hiding this comment.
need numa-bind-nodes: [0, 0, 1, 1] as well
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29639451578 |
d08a708 to
b0c8987
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29657205671 |
b0c8987 to
fc9b778
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29657914718 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29662247247 |
fc9b778 to
351db64
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29664107401 |
Backport per-node Dynamo health counting and provide explicit GB300 NUMA mappings for aggregate workers. 中文：回移按节点启动模式下的 Dynamo 健康检查计数修复，并为 GB300 聚合式工作进程显式配置 NUMA 映射。
中文：将性能变更日志中的 PR 链接更新为 #2269。
89c52b8 to
870df10
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29673080337 |
2 participants
No description provided.