Skip to content

[Klaud Cold] minimaxm3-fp4-b200-vllm-agentic-mtp: long-prefill-token-threshold 512 / 设置 long-prefill-token-threshold 512 #2538

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xinli-sw wants to merge 2 commits into from
Closed

[Klaud Cold] minimaxm3-fp4-b200-vllm-agentic-mtp: long-prefill-token-threshold 512 / 设置 long-prefill-token-threshold 512 #2538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37d861d
minimaxm3 fp4 b200 agentic mtp: set --long-prefill-token-threshold 512
xinli-sw Aug 9, 2026
8d81fc7
fix perf-changelog
xinli-sw Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions benchmarks/single_node/agentic/minimaxm3_fp4_b200_mtp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ VLLM_CMD=(
--max-cudagraph-capture-size 512
--max-num-batched-tokens 16384
--stream-interval 20
--long-prefill-token-threshold 512 \

@claude claude Bot Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

@claude claude Bot Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

--trust-remote-code
--speculative-config "$SPEC_CONFIG"
"${OFFLOAD_ARGS[@]}"
Comment on lines 194 to 200

@claude claude Bot Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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:

  1. 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.
  2. The bot's recipe-reminder comment tells the author: add the full-sweep-fail-fast label and 'the benchmark sweep only runs on labeled PRs.'
  3. The author (or automation) applies full-sweep-fail-fast — this is confirmed present in the PR's labels.
  4. GitHub fires the pull_request / labeled event for run-sweep.yml. Before scheduling any job, GitHub Actions checks the paths filter against the PR's full diff. The diff is — it does not include perf-changelog.yaml — so the filter fails and the entire workflow (including check-changelog and reuse-sweep-gate, which gate the actual sweep dispatch) does not run at all.
  5. The same holds for any later synchronize push and for ready_for_review if this were opened as a draft.
  6. 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 512 value 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.

Expand Down
6 changes: 6 additions & 0 deletions perf-changelog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5688,3 +5688,9 @@
- "Use the official lmsysorg/sglang:v0.5.16-rocm720-mi30x image, enable SGLang prompt-cache reporting, and pass the backend Prometheus metrics endpoint explicitly to AIPerf"
- "Allow up to 30 minutes for healthy AgentX responses admitted near the end of the measurement window to drain before AIPerf finalizes profile metric coverage"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2528

- config-keys:
- minimaxm3-fp4-b200-vllm-agentic-mtp
description:
- "Set --long-prefill-token-threshold 512 for MiniMax M3 NVFP4 b200 AgentX MTP"
pr-link: https://github.com/SemiAnalysisAI/InferenceX/pull/2538
Loading