Skip to content

MTP graph profiles carry no topology class, so one executable serves two attention routes and any draft window past five fails at startup - #221

Open
MichaelDementii wants to merge 1 commit into
Neroued:masterfrom
MichaelDementii:fix/mtp-graph-profile-topology-class
Open

MichaelDementii wants to merge 1 commit into
Neroued:masterfrom
MichaelDementii:fix/mtp-graph-profile-topology-class

Conversation

@MichaelDementii

Copy link
Copy Markdown
Contributor

instantiate_graph_family builds one cudaGraphExec_t per topology class and installs every other
profile of that class through cudaGraphExecUpdate, which cannot cross a change of node count.

dflash_graph_profiles honours that twice: it breaks the frontier where the attention route flips
(variant.cpp:49-51) and derives topology_class from the same predicate (:114-124).
mtp_graph_profiles, three functions above it in the same file, does neither — every profile it
returns carries class 0.

It is a hard failure, not a style point

Raising the MTP draft cap — four constants, no other change — on this build:

draft tokens master with this change
1–5 ok ok
6, 7, 8, 11, 12, 15 error: startup failed | preparing CUDA graphs
cudaErrorGraphExecUpdateFailure (update result 2)
ok

Change

mtp_graph_profiles now breaks the frontier at the route flip and sets topology_class from the
same predicate dflash_graph_profiles uses.

  • the predicate is renamed verify_uses_chunked_small_t because both planners read it now — a
    second copy is what would drift;
  • it gains the width bound the real route has: causal_attention_resolve_route returns Prompt for
    every envelope past kMaximumVerifyTokens, so claiming a target dependence beyond width 16 costs
    a topology class no verify path can request;
  • verify_route_flip_target locates the boundary by bisecting that predicate — monotone in
    max_visible_keys — rather than restating the route table, which is how the planner and the route
    could drift apart in the first place.

1 file, +46 / −4.

Evidence

New contract test over capacities 2048 / 16384 / 65536 / 262144, draft windows 1..16, all five
KvCacheStorage values: no profile may span a route flip inside its own frontier range, and no
topology class may carry two routes.

b88c0f6 with this change
contract violations 1150, over 40 (capacity, k) pairs, k = 6..15 0
ctest 114/114 116/116

At the shipped cap it is a no-op

At k ≤ 5 every width resolves to SmallT, so no class changes and the added boundary sits behind
if (flip_target != 0) — the profile list does not move by one element. Measured too: stock caps,
greedy, k = 1..5, two binaries of different md5 print byte-identical token ids, 5 of 5, while the
same comparison separates k=1 from k=5.

dflash_graph_profiles is unchanged: the renamed predicate takes the same arguments, and the new
width bound cannot fire there because DFlash caps at 15.

This does not raise the MTP draft window and proposes no wider window — that needs its own case on
a workload. It removes the crash that meets anyone who raises it, and pins the contract with a test.

RTX 5090 sm_120a, CUDA 13.1, Release, qwen3_6_35b_a3b.ninfer.

🤖 Generated with Claude Code

instantiate_graph_family builds one cudaGraphExec_t per topology class and
installs every other profile of that class through cudaGraphExecUpdate, which
cannot cross a change of node count. dflash_graph_profiles honours that twice:
it breaks the frontier where the attention route flips, and derives
topology_class from the same predicate. mtp_graph_profiles did neither, so at a
draft window of six or more one executable served both the prompt and the
chunked small-T route.

Today the MTP draft cap of five hides it. With the cap raised - four constants,
no other change - draft windows 6 through 15 fail at startup with
cudaErrorGraphExecUpdateFailure; with this change all of them run.

The shared predicate is renamed verify_uses_chunked_small_t because both
planners now read it, and gains the width bound the real route has:
causal_attention_resolve_route returns Prompt for every envelope past
kMaximumVerifyTokens. verify_route_flip_target locates the boundary by bisecting
that predicate rather than restating the route table, which is how the planner
and the route could drift apart.

At the shipped cap nothing moves: every width up to six resolves to SmallT, so
no class changes and the added boundary sits behind a zero check. Adds
tests/targets/qwen3_6_35b_a3b/test_mtp_graph_profiles.cpp, which asserts the
contract over four capacities, draft windows one to sixteen and all five KV
storages.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T15:44:41.849560Z f4bad56 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4bad56c5a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +140 to +141
const std::uint32_t flip_target = verify_route_flip_target(draft_window);
if (flip_target != 0U) { add_shifted(flip_target, draft_window + 1); }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Remove planning for unsupported MTP windows

validate_spec rejects every MTP draft window above kMtpDecodeMaximumDrafts (currently 5), while for every accepted window draft_window + 1 <= 6, so verify_route_flip_target always returns zero and the new topology classification always remains class 0. Consequently, this branch only implements behavior for configurations that cannot reach graph planning; the accompanying test obtains its failures solely by calling the private planner with those invalid windows. Remove this speculative path, or introduce it as part of an explicit product change that raises the MTP limit and updates the complete affected contract.

AGENTS.md reference: AGENTS.md:L59-L61

Useful? React with 👍 / 👎.

@MichaelDementii

Copy link
Copy Markdown
Contributor Author

On the Codex P1: it is right that nothing reaches this today. kMtpDecodeMaximumDrafts is 5, so
verify_route_flip_target returns zero for every accepted window and every class stays 0 — which is
also why the PR shows byte-identical output at k = 1..5 rather than claiming a behaviour change.

What I would put on the other side, for you to weigh:

  • dflash_graph_profiles carries exactly this guard, three functions below, for a window that is
    reachable. The two planners consume one route table and only one of them mirrors it.
  • The failure mode is not a wrong answer but cudaErrorGraphExecUpdateFailure at startup, and the
    test is the only thing that would catch a reintroduction.
  • It is 46 lines and it is not on any hot path.

If you would rather the MTP limit stay where it is and the planner not carry a guard for a window it
cannot be asked for, say so and I will close this — the report itself is in the description and
loses nothing by the PR going away.

ashalliants added a commit to ashalliants/ninfer-3090 that referenced this pull request Sep 14, 2026
Port upstream Neroued#197 (ignore_eos) and Neroued#221 (MTP graph topology class)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants