Repository navigation
perf(mtp): add second CUDA graph slot for option-c verify capture (enabling primitive) - #1648
Merged
Merged
Conversation
The native MTP self-spec `replays=0` blocker is architectural: the CUDA EP
owns a single CudaGraphLifecycle, shared across the main + decode-inline
execs, so the M=1 decode graph and the M=K verify graph cannot coexist —
each invalidates the other every step. Capturing the verify (the real MTP
speedup lever) needs a second, independent captured-graph slot.
Land that enabling primitive: a DeviceGraphSlot{Primary,Verify} abstraction
threaded through the EP graph API (additive default methods route Primary to
the existing single-slot methods and reject other slots, so every existing
EP compiles unchanged), and a second CudaGraphLifecycle (verify_graph) on the
CUDA runtime sharing the compute stream. GPU test proves the two slots
capture/replay/reset independently and one reset leaves the other intact.
Dormant: the engine never captures into the Verify slot yet, so greedy and
MTP are byte-identical to origin (GPU: MTP 14.59 tok/s, 78.9% accept,
replays=0, fallbacks=0, no NaN — unchanged). No speedup number is claimed;
the remaining replay wiring (session per-slot signatures, fixed-padded verify
shape, pinned StepScoped workspace, native verify state machine) is scoped in
the decision note.
Validation: ep-cuda graph::tests 8/8 on H200 (incl. new two-slot test);
onnx-genai-engine native-backend lib suite 575 pass (greedy inert); full
bench build clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
==========================================
- Coverage 81.04% 81.03% -0.01%
==========================================
Files 384 384
Lines 180256 180299 +43
Branches 180256 180299 +43
==========================================
+ Hits 146082 146111 +29
- Misses 29224 29243 +19
+ Partials 4950 4945 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This was referenced Aug 21, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
…p 1) (#1650) ## Summary Option-c's replays-on-verify speedup needs the main executor to capture the fixed **M=k+1 verify** into a **second, independent** CUDA graph slot so it stops invalidating the M=1 decode graph every step (the `replays=0` root cause). #1648 added that second slot raw at the EP (`DeviceGraphSlot::Verify`). This PR lands **step 1 of my 5-step plan**: lifting it into the **executor/session** layer so the main executor can actually *drive* the Verify slot independently of the Primary M=1 decode graph. **Dormant + byte-inert.** The native verify state machine is intentionally NOT wired here — this is the plumbing brick. Default `Primary` routing makes greedy and MTP behave exactly as before. **No MTP speedup number is claimed** (the Verify slot is dormant in decode), and — see the blocker below — none is measurable in this environment right now regardless of wiring. Per the coordinator's standing fallback clause: land incremental capture-safe progress + the precise remaining gap + GPU evidence; never fabricate a speedup or half-wire a risky, unvalidated path. ## What landed - `Executor.graph_slot: DeviceGraphSlot` (default `Primary`), threaded through **every** EP graph call the executor makes: capture begin/end/abort + segment replay (dispatch.rs), single-graph replay + reset (bindings.rs), `SegmentCaptureGuard` abort (capture.rs), defensive resets (run.rs, mod.rs). Kernel-variant eviction resets **both** slots (an evicted kernel can retire a graph in either; resetting an empty slot is a no-op). - `Executor::set_graph_slot`/`graph_slot` + `Session::set_main_exec_graph_slot`/`main_exec_graph_slot` (re-exports `DeviceGraphSlot`). Retargeting resets the old slot first, so a later capture records cleanly into the new slot. Because the main-exec `try_capture`/`replay`/`reset` now route through `self.graph_slot`, the native verify path (step 4) captures into Verify simply by setting the slot once — no new capture/replay Session methods needed. Default `Primary` ⇒ `*_in(Primary)` delegates to the historical single-slot EP methods ⇒ provably identical behavior until a caller retargets the slot. ## Validation (H200 ord 5 idle; ORT 1.28 cuda13; int4 block-32; off origin/main `73e6fe15a`) - `onnx-runtime-session --features cuda` lib suite: **190/190**, incl. new GPU test `main_exec_drives_verify_graph_slot_end_to_end` (main exec captures→replays→resets on the **Verify** slot with persistent I/O + zero replay-time allocs, then reverts to Primary) and all existing Primary-path graph tests still green. - `onnx-genai-engine --features native-backend` lib suite: **575/575** (greedy inert; engine untouched). - `onnx-runtime-ep-cuda --features cuda,cuda-13000 graph::tests`: **8/8** (#1648 two-slot invariant unchanged). - Full bench build `--features bench-native,native-cuda,cuda-13000`: clean. - **GPU greedy-inertness on the real Qwen3.8-27B int4 hybrid** (`--ep cuda --steady --tokens 128 --warmups 3`): **56.56 tok/s, `cuda_graph replays=504 fallbacks=0` (captures=4, invalidations=3), no non-finite logits** — the Primary/inline slot captures & replays exactly as before (≈ the ~55.9 tok/s greedy baseline). The routing change is byte-inert in production greedy. ##⚠️ New E2E blocker (independent of this change) The MTP head **fails to load** on this ORT build, from the pristine artifact dir: ``` Failed to load MTP head: ORT error: Type Error: Type parameter (T) of Optype (Add) bound to different types (tensor(bfloat16) and tensor(float)) in node (). ``` A graph-level type mismatch **inside `mtp/model.onnx`**, rejected at ORT session creation — this Rust change cannot affect ORT's type-checking of the head graph. Native MTP self-spec therefore **cannot run or be token-identity-validated end-to-end in this environment right now**, regardless of engine wiring. Needs an artifact fix (re-export the head with consistent `Add` operand dtypes / cast the bf16↔f32 operands) before any real MTP number is measurable again. ## Remaining gap (steps 2-4; plan unchanged, now also gated on the head fix) 2. Fixed padded verify shape (constant M=k+1, causal-masked trailing padding; padded GDN advance discarded by the existing snapshot→restore→re-advance commit). The `leverb_increment0` throwaway probe already demonstrates the mechanism (persistent padded `[1,M,vocab]` logits binding + pre-capture warm at M=K + KV-symbol pin → capturable, replayable, captured-vs-eager token parity). 3. Pinned StepScoped verify workspace (the #1647 NaN fix). 4. Native `verify_graph_phase` capturing/replaying the fixed-M verify into the Verify slot (set the main exec's slot to Verify once); Primary M=1 stays on the decode-inline sibling. 5. GPU-validate MTP token-identical to greedy, both slots replays>0, fallbacks=0, median-of-5 A/B — **once the MTP head loads again.** Decision note: `.squad/decisions/inbox/gaff-mtp-verify-slot-executor-routing.md`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
#1649) `origin/main` (`73e6fe15a`) fails a required check on an unmodified checkout: ``` error: methods `set_retain_decode_graph_across_spec` and `retain_decode_graph_across_spec` are never used --> crates/onnx-genai-engine/src/native_decode/cuda.rs:5516 ``` That is the **Check the native backend compiles** step of `Rust quality`, so it blocks every open PR regardless of contents. It surfaced on an unrelated CPU-kernel PR (#1628). Both methods are `#[cfg(test)]` accessors for the option-c graph-retention seam that #1648 landed as an *enabling primitive* — deliberately ahead of the WP4 tests that will drive them. Their `#[cfg(test)]` siblings either side (`set_retain_graph_on_rewind`, `padded_query_capacity`) are already called, which is why only these two trip. Fix is `#[allow(dead_code)]` on the pair with the reason recorded at the site — rather than deleting a seam that is about to be used, or widening the allow to the whole `impl` block. ### Local verification | gate | result | |---|---| | `cargo fmt --all -- --check` | clean | | `clippy -p onnx-genai-engine --features native-backend --all-targets -D warnings` | clean | | `clippy -p onnx-genai-engine --features native-cuda --all-targets -D warnings` | clean | | `clippy -p onnx-genai-cli --all-targets -D warnings` | clean | | `cargo test -p onnx-genai-engine --features native-backend` | 0 failed | > **Process note — this is the third main-is-red repair today**, and I am only finding them because they land on an unrelated PR: > - this morning: fmt + clippy debt from #1637/#1641 (four required gates red at once) → fixed by #1640 > - midday: #1644 reintroduced fmt violations → fixed by #1642 > - now: #1648 introduces this dead-code lint > > Each costs a full CI round-trip to discover, because the gates are sequential — the clippy steps only run once formatting passes. Running the `Rust quality` job on `main` post-merge, or a merge queue, would catch these at the source instead of on whoever's PR is open next. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 21, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
`cargo fmt --all -- --check` fails on an **unmodified `origin/main`** (`90ddd284e`): ``` crates/onnx-runtime-session/src/lib.rs:38 ``` `pub use onnx_runtime_ep_api::DeviceGraphSlot;` was added above the existing `WorkspaceRequirement` re-export rather than in sorted order. One-line swap, pure `cargo fmt --all` output. Formatting is a required check, so this blocks every open PR regardless of contents. It surfaced on an unrelated CPU-kernel PR (#1628). ### This is the fourth main-is-red repair today | # | PR | what was red on main | source | |---|---|---|---| | 1 | #1640 (not mine) | fmt + 3 clippy lints, four required gates at once | #1637 / #1641 | | 2 | #1642 | fmt, two sites | #1644 | | 3 | #1649 | clippy `dead_code`, `native_decode/cuda.rs` | #1648 | | 4 | **this** | fmt, one re-export | #1647 / #1648 | The pattern is consistent and worth fixing at the source: quality gates run on PR branches *before* merge but not on the merge result, so any merge can land violations that then fail whoever opens the next PR. Because the gates are sequential — clippy steps only run once formatting passes — each breakage costs a full CI round-trip to even *discover*, and they arrive one at a time. Two concrete options: enable a merge queue (gates run on the merge result), or run the `Rust quality` job on `main` post-merge so the break is attributed to the PR that caused it instead of the next unrelated one. Also still red on main and **not** fixed here, because I cannot reproduce it locally and it is not mine: `Rust (Windows ARM64)` → *Test cross-platform offline crates* has been failing on main since at least `73e6fe15a` (it is non-required, so it does not block merges). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 21, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
`Rust quality → Check formatting` is failing on `main` at `6923a016b` (#1652). That job runs its steps sequentially, so while formatting is red **every other check in it is skipped** — clippy, publish order, the dispatch-manifest lints, feature-gate coverage, all of it. Every open PR is blocked and none of them are getting linted. Three sites, all in `crates/onnx-genai-engine/src/native_decode/cuda.rs`: two `verify_graph_phase` assignments (`:1815`, `:1823`) and one `assert_eq!` in `verify_capture_helper_tests` (`:7042`). Straight `cargo fmt --all` output, no hand edits. **Verified inert.** The before/after texts are identical after stripping whitespace *and* trailing commas — the only non-whitespace delta is commas rustfmt adds before a closing delimiter when it breaks a call across lines, which are semantically meaningless in Rust. Reproduced on a pristine `origin/main` worktree first, so this is main's breakage and not an artifact of my branch. ### This is the fifth time today `main` has been red on formatting or clippy five separate times in one day: #1637/#1641 (fixed by #1640), #1644 (#1642), #1648 (#1649), #1647/#1648 (#1651), and now #1652. The cause is structural, not carelessness. Required checks run on a PR's **merge ref**, but nothing re-runs them on `main` **after** the merge, so two PRs that are each green against an older base can land in sequence and leave the result red. Because the quality job is sequential, the breakage also masks every later step in it. The cost lands on whoever opens the next PR, who then has to distinguish "my change broke this" from "main was already broken" — a full CI round-trip each time. Two things would fix it, either one sufficient: - a **merge queue**, which tests the actual post-merge result; or - running **`Rust quality` on `main` post-merge**, which at least detects it immediately and attributes it correctly. I have now spent four PRs on this. I would rather not spend a fifth. cc @justinchuby Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
…d again) (#1655) Sixth formatting break on `main` today, from #1654 — a few hours after #1653 fixed the fifth. Four sites in `onnx-runtime-session/src/executor/{capture.rs,run.rs,tests.rs}`, all the same shape: a `self.cap_mut().field.insert(..)` chain rustfmt wants split across lines. Straight `cargo fmt --all`, no hand edits. **Verified inert** per file: each before/after pair is identical after stripping whitespace and trailing commas. Reproduced on a pristine `origin/main` worktree first, so it is main's breakage, not my branch's. Because `Rust quality` runs sequentially, this red `Check formatting` step is again **skipping every other lint in that job** — clippy, publish order, dispatch-manifest lints, feature-gate coverage. Those have effectively not run on main since #1654 landed. This is the sixth today (#1637/#1641, #1644, #1648, #1647/#1648, #1652, #1654) and my fifth repair PR. The structural cause and the two candidate fixes — a merge queue, or running `Rust quality` on `main` post-merge — are written up in #1653. Nothing about the individual changes is careless; required checks simply run on each PR's merge ref and never on the actual post-merge `main`, so sequentially-green PRs can still leave the tip red. cc @justinchuby Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Option-c (capture the M=K MTP verify forward so it replays instead of tearing down the M=1 decode graph every step — the
replays=0blocker from #1647) requires a second, independent captured-graph slot on the shared CUDA EP. Today the EP owns a singleCudaGraphLifecycle(shared across the main + decode-inline execs), so the M=1 decode graph and the M=K verify graph — which bake different query geometries — cannot coexist without invalidating each other every step.This PR lands that enabling primitive (dormant, zero hot-path/token risk). The full replay wiring is a multi-file executor change scoped for a follow-up (see the decision note). No speedup number is claimed — none exists yet (the Verify slot is not wired into decode).
Per the coordinator's fallback clause: land incremental capture-safe progress + the exact remaining gap + GPU evidence; do not fabricate a speedup.
What landed
onnx-runtime-ep-api:DeviceGraphSlot { Primary, Verify }enum +*_device_graph_*_in(slot, ..)trait methods. Default impls routePrimary→the existing single-slot methods and reject other slots, so every existing EP compiles unchanged.onnx-runtime-ep-cuda: a secondCudaGraphLifecycle(verify_graph) onCudaRuntime, sharing the compute stream; slot-aware runtime + provider methods (per-slot reset also resets the capture-error latch).primary_and_verify_graph_slots_are_independent: the two slots capture/replay/reset independently, interleaved, and resetting one leaves the other's executable intact.Why the second slot is the blocker (code-anchored, origin/main
7ccdb920e)CudaRuntime.graph→ EP*_device_graph*→ sessiondevice_graph_signature: Option<..>. The decode-inline sibling shares that one slot (lib.rs doc: "one EP graph slot + one capture-error latch").replays=0(empirical, perf(mtp): localize CUDA-graph-reuse blocker; land dormant retention seam (no speedup — capture-unsafe) #1647): every verify step invalidates the M=1 graph at two sites — the eager M=K verify (run_cuda_eager_rows_owned) and the commit rewind (rewind_inner).Remaining gap to a replays-on-verify speedup (next-turn wiring, all anchored in the decision note)
device_graph_signature+ slot-parameterized capture/replay/reset (delegating to the new EP_inmethods).M = k+1, causal-masked padding; padded GDN state discarded by the existing snapshot→restore→re-advance commit) → shape-invariant replayable verify.Validation (H200
CUDA_VISIBLE_DEVICES=5, all 8 idle; PATH/CUDA_HOME set; ORT 1.28 cuda13; int4 block-32; branch off origin/main7ccdb920e)ep-cuda graph::tests: 8/8 pass incl. the new two-slot test.onnx-genai-enginenative-backend lib suite: 575 pass, 0 fail (engine untouched, greedy inert).--features bench-native,native-cuda,cuda-13000: clean.cuda_graph captures=16 replays=0 fallbacks=0 invalidations=99, no NaN — identical to origin (Verify slot dormant).Decision note:
.squad/decisions/inbox/gaff-mtp-verify-capture-second-graph-slot.md.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com