Add GB200 MiniMax-M3 FP4 AgentX sweep - #2609
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 关于重新运行失败任务的文档 |
…-fp4-agentx # Conflicts: # perf-changelog.yaml # runners/launch_gb200-nv.sh
…-fp4-agentx # Conflicts: # perf-changelog.yaml
…-fp4-agentx # Conflicts: # configs/nvidia-master.yaml
cquil11
added
agentx-fast
|
Claude finished @cquil11's task in 7m 19s —— View job PR Review: GB200 MiniMax-M3 FP4 AgentX sweep
SummaryNo blocking issues; two 🟡 warnings posted inline. (1) All 14 recipes set Everything else checked out: perf-changelog entry is correctly appended at the end and paired with the master-config change; |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| args: | ||
| router-mode: "kv" | ||
| router-kv-events: true | ||
| router-temperature: "0" | ||
| kv-cache-block-size: 128 |
There was a problem hiding this comment.
🟡 WARNING: Session-affinity TTL is not configured, and AIPERF_DYNAMO_SESSION_TIMEOUT_SECONDS: "14400" (line 84) is dead on this routing path — this applies to all 14 new recipes.
Why it matters: These recipes opt into header-based session routing (AIPERF_HTTP_X_DYNAMO_SESSION_ID_FROM_CORRELATION_ID: "true" + AIPERF_USE_DYNAMO_CONV_AWARE_ROUTING: "0"). With that opt-in, benchmark_lib.sh skips the conv-aware-routing branch (benchmarks/benchmark_lib.sh:2055-2064) — the only consumer of AIPERF_DYNAMO_SESSION_TIMEOUT_SECONDS — so the 14400s value is never applied anywhere. Per the comment in that same block, the upstream router default affinity TTL is 300s, which is shorter than an overloaded high-concurrency agentic request. Every existing dsv4 recipe that uses this exact header-routing pattern sets the TTL router-side instead (e.g. disagg-gb200-1p1d-dep8-dep8-mtp-agentic.yaml:46 uses router-session-affinity-ttl-secs: 14400). Without it, sessions at c128–c256 can lose worker affinity mid-conversation, silently degrading the KV-aware routing this sweep is meant to measure.
Fix: Set the TTL on the frontend like the dsv4 recipes (and drop the dead AIPERF_DYNAMO_SESSION_TIMEOUT_SECONDS env entries):
| args: | |
| router-mode: "kv" | |
| router-kv-events: true | |
| router-temperature: "0" | |
| kv-cache-block-size: 128 | |
| args: | |
| router-mode: "kv" | |
| router-kv-events: true | |
| router-temperature: "0" | |
| router-session-affinity-ttl-secs: 14400 | |
| kv-cache-block-size: 128 |
If srt-slurm v1.0.50 / Dynamo v1.3.1 applies a long affinity TTL by default for X-Dynamo-Session-ID sessions, please note that in the PR description instead. Fix this →
There was a problem hiding this comment.
The functional half of this is fixed: commit d707e11 ("perf(agentx): prune GB200 MiniMax tuning grid") added router-session-affinity-ttl-secs: 14400 to the frontend args block in this recipe (and the other 13), so session affinity is now actually enforced router-side instead of relying on the dead env var. However, the second part of the suggested fix — dropping the now-confirmed-dead AIPERF_DYNAMO_SESSION_TIMEOUT_SECONDS: "14400" entry from benchmark.env — was not done; it is still present at line 93 in this file (and in all the other 13 recipes). It remains inert cleanup at this point, not a functional issue, since benchmark_lib.sh skips the conv-aware-routing branch that would have consumed it.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31845487635 |
| description: | ||
| - "Inject the committed synthetic MTP acceptance length only for GB300 AgentX throughput; keep eval-only jobs on real target verification." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2571 | ||
|
|
||
|
|
||
| - config-keys: | ||
| - minimaxm3-fp4-gb200-dynamo-vllm-agentic-agg-mtp | ||
| - minimaxm3-fp4-gb200-dynamo-vllm-agentic-disagg-mtp | ||
| scenario-type: | ||
| - agentic-coding | ||
| description: | ||
| - "Add GB200 MiniMax-M3 NVFP4 AgentX with EAGLE3-GQA, the B200 TP4 baseline, DEP4/DEP8, KV-routed P/D, and high-concurrency SimpleCPU offload." | ||
| - "Use vLLM v0.27.1, NVIDIA/srt-slurm v1.0.50, and Dynamo v1.3.1 with full 1M context, MiniMax parsing, KV-cache token metrics, and every logical vLLM metrics endpoint." | ||
| pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2609 |
There was a problem hiding this comment.
🔴 This diff mutates a pre-existing byte in perf-changelog.yaml: the prior last line (two trailing spaces after the #2571 pr-link) is deleted and replaced with a bare empty line before the new #2609 entry is appended, instead of leaving it untouched. AGENTS.md explicitly requires the file to be append-only and byte-sensitive ('Preserve all existing bytes and separator whitespace, and append only at the tail'). Fix: restore the original (two-space) separator line and append the new entry after it.
Extended reasoning...
What the bug is
AGENTS.md states, verbatim, in its "Non-negotiable benchmark invariants" section (line 21): "Every change ... requires a new perf-changelog.yaml entry. The file is append-only and byte-sensitive. Preserve all existing bytes and separator whitespace, and append only at the tail."
The diff for this PR does not honor that. Looking at the hunk around line 5980-5992:
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2571
-
+
+- config-keys:
+ - minimaxm3-fp4-gb200-dynamo-vllm-agentic-agg-mtp:
+ ...The pre-existing separator line after the #2571 entry — which was two trailing spaces (" ") followed by a newline — is deleted and replaced with a bare empty line, and only then is the new #2609 block appended. I confirmed this directly against the working tree with cat -A: the current file shows $ (empty line) where the base (pre-PR) content had $ (two spaces before EOL). That is a mutation of a historical byte, not a pure append.
Why it is not caught by the main CI gate
The PR description claims "Performance changelog validation passes against current main," which is true but incomplete. The primary check-changelog job runs validate_perf_changelog.py::main(), which calls validate_matrix_compatible_change -> validate_generated_config -> process_changelog.py. process_changelog.py::get_added_lines (lines ~41-42) explicitly treats whitespace-only/empty-line deletions as ignorable, so this diff sails through that check. That is exactly why a human reviewer skimming a unified diff, or the automated CI gate, would not flag it — and is precisely the failure mode the repo docs (docs/ci-procedures.md:158, docs/testing.md:100) warn about: whitespace-only historical deletions can slip past the diff reader, requiring a manual byte-diff gate instead.
Where it actually breaks something
utils/validate_perf_changelog.py::validate_raw_change() is a separate, stricter byte-level gate used by the changelog merge/ingest path (utils/prepare_perf_changelog_merge.py, invoked by merge_with_reuse.sh at lines 97/130/155, and also used by recover_failed_ingest.py). Its logic:
def validate_raw_change(base_raw, head_raw, additions, corrections):
if additions:
if not head_raw.startswith(base_raw):
raise ChangelogValidationError(
"appended entries changed historical perf-changelog.yaml bytes; "
"restore the base file byte-for-byte and append at the end"
)Because the base file (merge-base) ends with ...pull/2571\n \n and this PR's head ends with ...pull/2571\n\n- config-keys:..., head_raw.startswith(base_raw) is False ("\n \n" != "\n\n"). validate_raw_change raises ChangelogValidationError("appended entries changed historical perf-changelog.yaml bytes; restore the base file byte-for-byte and append at the end") — so the standard reuse-merge path used to land this PR onto main would reject it.
Step-by-step proof
- Base (main, commit
d136fa6, the#2571entry's tail) ends:...pull/2571\n+\n. - This PR's diff shows
-(removing that two-space line) and+(adding a bare empty line), then appends the new#2609block. - Head file (this PR) now ends:
...pull/2571\n+ `` (nothing) +\n- config-keys:\n - minimaxm3-fp4-gb200-dynamo-vllm-agentic-agg-mtp\n... base_raw = ...pull/2571\n \n,head_raw = ...pull/2571\n\n- config-keys:....head_raw.startswith(base_raw)compares byte-for-byte: at the position right afterpull/2571\n,base_rawexpects\n(space, space, newline) buthead_rawhas\n(newline) directly — mismatch, sostartswithreturnsFalse.validate_raw_changeraisesChangelogValidationError, blocking the merge-prep/ingest step even though the primary CIcheck-changelogjob passed.
The fix
Restore the original two-space (" ") line exactly as it existed on main, and append the new #2609 config-keys block immediately after it (rather than replacing that line with an empty one first).
cquil11
removed
full-sweep-enabled
agentx-fast
|
Claude finished @cquil11's task in 5m 42s —— View job Re-review: GB200 MiniMax-M3 FP4 AgentX sweep (pruned 60 → 17 points)
Still open from the previous review — 🔴 BLOCKING
Fixed since the previous review
New/changed code since the previous review — checked, no new issues
Verdict: one blocking issue remains — the |
There was a problem hiding this comment.
Beyond the inline finding, I also checked the disaggregated recipe's bare kv_events_config: true (vs. the sibling aggregate recipes' { aggregated: true }) for its DP-attention prefill/decode workers — this is a valid, distinct schema shape for disaggregated KV-events config, not a mistake, so it's not flagged as a bug.
Extended reasoning...
This run's bug-hunting pass found one new minor issue (posted inline: an unused dram-utilization field on the disagg-mtp scenario, since its only search-space arm uses kv-offloading: none). A finder also raised the disaggregated recipe's bare kv_events_config: true as a possible inconsistency versus the aggregate recipes' { aggregated: true } form; on inspection this is a legitimate alternate shape for the disaggregated P/D topology rather than an error, so it was ruled out and is not included as a bug.
| disagg: true | ||
| scenarios: | ||
| agentic-coding: | ||
| - dram-utilization: 0.61 |
There was a problem hiding this comment.
🟡 The new minimaxm3-fp4-gb200-dynamo-vllm-agentic-disagg-mtp scenario sets dram-utilization: 0.61 (configs/nvidia-master.yaml:7346), but its only search-space entry uses kv-offloading: none — there's no dram arm to consume it. This is a pre-existing pattern issue in this PR's new config, not a runtime bug: it's dead and misleads a reader into thinking DRAM offload is exercised here, unlike the sibling agg-mtp scenario which legitimately uses two dram arms. Either drop the field or add the intended dram arm.
Extended reasoning...
dram-utilization is read by exactly two consumers in this repo, and both gate on the presence of a kv-offloading: dram search-space entry:
validate_dram_offload_capacityinutils/matrix_logic/validation.pyonly requires/validatesdram_utilizationwhen a search-space entry haskv_offloading == 'dram'. It never reads or enforces the field for entries whosekv_offloadingisnone.agentic_dram_offload_gbinutils/matrix_logic/generate_sweep_configs.py(lines ~153-161) returns0immediately whenkv_offloading != 'dram', before it ever readsFields.DRAM_UTILIZATION.
The new minimaxm3-fp4-gb200-dynamo-vllm-agentic-disagg-mtp scenario (configs/nvidia-master.yaml:7346-7360) has exactly one search-space entry, and it is spec-decoding: mtp, kv-offloading: none, conc-list: [48]. There is no kv-offloading: dram arm anywhere in this scenario. That means dram-utilization: 0.61 set at the scenario level is never consumed by either code path — it's inert configuration.
This is easy to miss because the sibling scenario added in the same PR, minimaxm3-fp4-gb200-dynamo-vllm-agentic-agg-mtp (line ~7263), legitimately needs dram-utilization: 0.61 for its two kv-offloading: dram / kv-offload-backend: vllm-simple arms. It looks like the value was copied from the agg-mtp block into the disagg-mtp block without adjusting for the fact that the disagg scenario's search space doesn't (yet) include a dram-offload arm.
Impact is limited to code clarity, not runtime correctness: no validation error is raised, no job fails, and no sweep behavior changes, since the field is simply never read for a kv-offloading: none entry. But it's misleading for anyone reading the config later — they'd reasonably assume the disagg-mtp scenario measures DRAM KV offload the way the agg-mtp one does, when in fact it doesn't.
Step-by-step proof:
- Read
configs/nvidia-master.yaml:7333-7360— theminimaxm3-fp4-gb200-dynamo-vllm-agentic-disagg-mtpscenario has oneagentic-codingentry:{ dram-utilization: 0.61, search-space: [ { spec-decoding: mtp, kv-offloading: none, conc-list: [48], prefill: {...}, decode: {...} } ] }. - Grep the search-space entry list for
kv-offloading: dram— no match; onlykv-offloading: noneappears. - Read
utils/matrix_logic/generate_sweep_configs.pyagentic_dram_offload_gb:if kv_offloading != 'dram': return 0runs beforeagentic_config[Fields.DRAM_UTILIZATION]is ever accessed, so for this scenario the function always short-circuits to0without touching the config value. - Read
utils/matrix_logic/validation.pyvalidate_dram_offload_capacity: the check that requires/usesdram_utilizationis inside the branch keyed onentry.kv_offloading == 'dram'; since no entry in this scenario satisfies that, the validator never looks atdram_utilizationhere either. - Therefore
dram-utilization: 0.61at line 7346 has zero effect on sweep generation, validation, or job configuration for the disagg-mtp scenario — confirmed dead by both of its would-be consumers.
Fix: remove dram-utilization: 0.61 from this scenario (cleanest, since no dram arm currently exists), or, if a DRAM-offloaded disagg arm was actually intended (the PR body's "SimpleCPU offload" language suggests DRAM offload was a goal), add a kv-offloading: dram search-space entry analogous to the ones in agg-mtp, wired to a disagg recipe that sets kv-transfer-config with SimpleCPUOffloadConnector.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31860385838 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31867547214 |
b7f1d38 to
1cf03b2
Compare
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31867893435 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31871181628 |
1 participant
What changed
Correctness
Tuning result
Direct GB200 tests covered aggregate DEP4/DEP8, one to three prefill workers, DEP4/DEP8 P/D combinations, and resident/SimpleCPU variants. The final matrix retains the non-dominated points and removes multi-prefill, symmetric P8/D8, and high-concurrency offload points that were worse in both normalized throughput and P90 E2E interactivity.
Validation