[Klaud Cold] minimaxm3-fp4-b200-vllm-agentic-mtp: long-prefill-token-threshold 512 / 设置 long-prefill-token-threshold 512 - #2538
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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| --max-cudagraph-capture-size 512 | ||
| --max-num-batched-tokens 16384 | ||
| --stream-interval 20 | ||
| --long-prefill-token-threshold 512 \ | ||
| --trust-remote-code | ||
| --speculative-config "$SPEC_CONFIG" | ||
| "${OFFLOAD_ARGS[@]}" |
There was a problem hiding this comment.
🔴 This PR adds --long-prefill-token-threshold 512 but never touches perf-changelog.yaml. AGENTS.md marks a changelog entry as required - triggers benchmarks for exactly this kind of engine-flag change, and run-sweep.yml's pull_request trigger is gated with paths: perf-changelog.yaml — evaluated against the whole PR diff, not the single event — so check-changelog/reuse-sweep-gate/the sweep dispatch never fire (not on open, not on the full-sweep-fail-fast label, not on synchronize). The flag can merge completely unvalidated by CI. Fix: append a perf-changelog.yaml entry for this config so the sweep actually runs.
Extended reasoning...
The bug: run-sweep.yml's pull_request trigger block declares:
pull_request:
branches: [main]
types: [ready_for_review, synchronize, labeled, unlabeled]
paths:
- "perf-changelog.yaml"GitHub Actions evaluates a paths: filter against the full set of files changed in the PR (base...head), for every event type in types: — including labeled. It is not scoped to 'did this specific event touch this path'; it's 'does this PR's diff touch this path at all'. This PR's diff contains exactly one file, benchmarks/single_node/agentic/minimaxm3_fp4_b200_mtp.sh, and never touches perf-changelog.yaml.
Why this matters here specifically: AGENTS.md's 'Updating Docker images'/engine-param section (line 156) is explicit that changing a script's engine params requires appending a perf-changelog.yaml entry, and calls that entry out as required - triggers benchmarks. Line 82 restates it: 'Changes to perf-changelog.yaml trigger benchmark runs.' This PR is a pure engine-flag change (--long-prefill-token-threshold 512 added to the vllm serve invocation) with no accompanying changelog entry — so it falls squarely into the case the docs warn about.
The concrete failure mode — step by step:
- PR [Klaud Cold] minimaxm3-fp4-b200-vllm-agentic-mtp: long-prefill-token-threshold 512 / 设置 long-prefill-token-threshold 512 #2538 opens, changing only
minimaxm3_fp4_b200_mtp.sh. - The bot's recipe-reminder comment tells the author: add the
full-sweep-fail-fastlabel and 'the benchmark sweep only runs on labeled PRs.' - The author (or automation) applies
full-sweep-fail-fast— this is confirmed present in the PR's labels. - GitHub fires the
pull_request/labeledevent forrun-sweep.yml. Before scheduling any job, GitHub Actions checks thepathsfilter against the PR's full diff. The diff is — it does not includeperf-changelog.yaml— so the filter fails and the entire workflow (includingcheck-changelogandreuse-sweep-gate, which gate the actual sweep dispatch) does not run at all. - The same holds for any later
synchronizepush and forready_for_reviewif this were opened as a draft. - Net result: no CI signal is ever produced for this change. The PR can be approved and merged with the label attached, all checks 'passing' only because they never ran, and the new
--long-prefill-token-threshold 512value is never exercised by the sweep it was supposedly added to validate.
Why nothing else catches this: there's no separate required-status-check enforcing 'sweep ran'; the whole mechanism relies on the paths filter itself gating admission, so a diff that never mentions perf-changelog.yaml silently short-circuits the entire validation pipeline rather than failing loudly.
The fix: append a perf-changelog.yaml entry for the affected config at the end of the file (per AGENTS.md, changelog entries are append-only, added at the tail, never inserted/prepended), matching the model/precision/SKU/topology this script benchmarks. That satisfies the paths filter, lets check-changelog/reuse-sweep-gate run, and actually dispatches the sweep so the new flag gets validated before merge.
| --max-cudagraph-capture-size 512 | ||
| --max-num-batched-tokens 16384 | ||
| --stream-interval 20 | ||
| --long-prefill-token-threshold 512 \ |
There was a problem hiding this comment.
🟡 Line 197 in the new VLLM_CMD=(...) entry has a stray trailing backslash (--long-prefill-token-threshold 512 \) — every other flag in the array has no line-continuation. It's harmless (the whitespace still splits --trust-remote-code into its own array element), but it's a copy-paste artifact; remove the backslash for consistency.
Extended reasoning...
The newly added line --long-prefill-token-threshold 512 \ inside the VLLM_CMD=(...) array carries a trailing line-continuation backslash. Every other flag in this array (lines ~178-200, e.g. --stream-interval 20, --trust-remote-code, --speculative-config "$SPEC_CONFIG") is a plain array element with no trailing backslash, so this new line stands out stylistically.
Why it's harmless: In bash, inside an array literal, a trailing \ followed by a newline is a line continuation — it removes the backslash and the newline character, but any actual whitespace on either side of the join point is untouched. Here the line ends with 512 \ (a space before the backslash) and the next line begins with leading indentation before --trust-remote-code. After the continuation is processed, the effective text is 512 --trust-remote-code — word-splitting on that leftover whitespace still produces separate array elements 512 and --trust-remote-code. I confirmed this behavior directly:
$ arr=(a b 512 \
c)
$ declare -p arr
declare -a arr=([0]="a" [1]="b" [2]="512" [3]="c")
Four elements, not three — so the flag and value are not accidentally merged with --trust-remote-code, and the server will still receive --long-prefill-token-threshold 512 --trust-remote-code as intended. No functional regression.
Why it should still be fixed: The backslash reads as if it were meant to merge this line with the next one (e.g., a leftover from an edit where the two flags were intended to be joined, or copy-pasted from a context that used \ for continuation), which is misleading to future readers and inconsistent with the rest of the array's formatting. Since it's cosmetic rather than a correctness issue, this doesn't block merging.
Fix: simply drop the trailing backslash so the line reads --long-prefill-token-threshold 512 like its neighbors.
| --max-cudagraph-capture-size 512 | ||
| --max-num-batched-tokens 16384 | ||
| --stream-interval 20 | ||
| --long-prefill-token-threshold 512 \ |
There was a problem hiding this comment.
🟡 This PR adds --long-prefill-token-threshold 512 to the B200 VLLM_CMD (line 197), but the header (lines 12-15) explicitly states every engine flag is unchanged from the B300 sibling so the two SKU curves stay directly comparable, with only two exceptions marked B200:. The B300 script (minimaxm3_fp4_b300_mtp.sh) has no such flag, so this new flag is an undocumented delta that breaks the stated parity invariant. Please either mirror the flag into minimaxm3_fp4_b300_mtp.sh or update the header comment to document this new B200-only asymmetry.
Extended reasoning...
The minimaxm3_fp4_b200_mtp.sh header is unusually explicit about a design invariant: lines 12-15 state "The only B200 deltas are the two blocks marked "B200:" below -- the checkpoint-resolution guard and the draft staging path. Every engine flag is the B300 script unchanged so the two SKU curves stay directly comparable." This is a strong, testable claim about the relationship between the two sibling scripts, and it's the kind of comment that exists specifically so a reader diffing the two scripts' VLLM_CMD blocks can trust they're identical without needing to actually diff them.
This PR adds --long-prefill-token-threshold 512 to the B200 VLLM_CMD array (new line 197), inserted between --stream-interval 20 and --trust-remote-code. I checked the B300 sibling (benchmarks/single_node/agentic/minimaxm3_fp4_b300_mtp.sh, lines 103-105): it goes directly from --max-num-batched-tokens 16384 / --stream-interval 20 to --trust-remote-code, with no --long-prefill-token-threshold flag anywhere in the file. This new flag is also not inside either of the two blocks the header calls out as legitimate B200-specific deltas (the checkpoint-resolution guard function and the draft-model staging path) — it's a bare addition to the shared engine-flag list.
Concretely, walking through it: (1) read the B200 header's claim that engine flags are identical to B300; (2) grep both scripts' VLLM_CMD arrays for long-prefill-token-threshold — present only in B200 after this PR; (3) confirm the new flag sits outside both named B200: exception blocks; (4) conclude the header's invariant is now false, and the "directly comparable" property it was meant to guarantee for the two SKU curves no longer holds for prefill-chunking behavior specifically.
The existing code doesn't prevent this because there's no automated check tying the two scripts' engine-flag lists together — the guarantee is purely documentary, enforced by convention and by whoever reviews future diffs against the stated header contract. Nothing stops a flag from being added to one script and not the other; that's exactly what happened here.
Impact is a documentation/consistency issue rather than a runtime failure: the benchmark will still run correctly on B200, but the two SKU curves are no longer running exactly comparable engine configurations for long-prefill chunking, and the header comment now asserts something untrue, which could mislead a future contributor who reads it and assumes parity holds. Recommended fix: either mirror --long-prefill-token-threshold 512 into the B300 VLLM_CMD (if it's meant to be a genuine engine improvement) or, if it's intentionally B200-only, mark it B200: in a comment and update the header's list of B200 deltas to include it.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=31297580611 |
|
/stage-results 31297580611 |
|
@xinli-sw staged run 31297580611: https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-08-09~r31297580611 This run remains available across future @xinli-sw 已将运行 31297580611 发布到预发布环境：https://inferencemax-app-git-staging-semianalysisai.vercel.app/inference?i_dates=2026-08-09~r31297580611 后续的 |
1 participant
Add
--long-prefill-token-threshold 512tominimaxm3_fp4_b200_mtp.sh.中文说明
在
minimaxm3_fp4_b200_mtp.sh中添加--long-prefill-token-threshold 512。