Repository navigation
perf(mtp): localize CUDA-graph-reuse blocker; land dormant retention seam (no speedup — capture-unsafe) - #1647
Merged
Conversation
Root-cause the native MTP self-spec `replays=0` (no CUDA-graph reuse): the captured M=1 decode graph is invalidated twice per verify step — by the eager M>1 verify forward (run_cuda_eager_rows_owned) and by the commit rewind (backend.rs rewind_inner). GPU A/B proves naive retention is both insufficient (the un-retained site still tears the graph down) and, when both sites retain, capture-unsafe: the eager M=K verify reserves a larger StepScoped step_workspace freed after the run, so the next M=1 replay reads a stale workspace pointer -> non-finite logits (finite-guard caught). Land the two-site invalidation seam as a documented dormant flag (retain_decode_graph_across_spec, default OFF) + test-only setters, mirroring the existing dormant retain_graph_on_rewind/option-c scaffolding. Behaviorally inert vs origin (flag never enabled): full native-backend lib suite 575 pass; greedy replays=92 fallbacks=0; MTP acceptance 78.9% fallbacks=0, no NaN. A real speedup needs the M=K verify itself captured (option-c padded verify capture) — a multi-turn executor workstream — since eager verify can never beat graphed greedy. No speedup number is claimed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1647 +/- ##
==========================================
- Coverage 81.74% 81.03% -0.71%
==========================================
Files 384 384
Lines 180260 180256 -4
Branches 180260 180256 -4
==========================================
- Hits 147359 146079 -1280
- Misses 27957 29226 +1269
- Partials 4944 4951 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
…d_code #1640 cleared the four defects from #1637/#1641, but #1647 reintroduced the same two classes on `7ccdb920e`. Verified on a clean detached `origin/main` worktree, not inferred from CI: cargo fmt --all -- --check exit 1 native_decode/mod.rs:1115, native_decode/tests.rs:1412 clippy -p onnx-genai-engine --features native-backend ... exit 101 error: methods `set_retain_decode_graph_across_spec` and `retain_decode_graph_across_spec` are never used (cuda.rs:5516) The two accessors are already `#[cfg(test)]`, but the seam was landed ahead of the option-c tests that will drive it, so it has no caller in the `lib test` target either. Kept and marked `#[allow(dead_code)]` rather than deleted: the field docs state it is deliberately exposed for the option-c work. Verified: fmt exit 0; clippy exit 0 for default `-p onnx-genai-cli`, `--features native-backend`, and `--features native-cuda`; engine lib tests pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
This was referenced Aug 21, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
…abling primitive) (#1648) ## 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=0` blocker from #1647) requires a **second, independent captured-graph slot** on the shared CUDA EP. Today the EP owns a *single* `CudaGraphLifecycle` (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 route `Primary`→the existing single-slot methods and reject other slots, so **every existing EP compiles unchanged**. - `onnx-runtime-ep-cuda`: a second `CudaGraphLifecycle` (`verify_graph`) on `CudaRuntime`, sharing the compute stream; slot-aware runtime + provider methods (per-slot reset also resets the capture-error latch). - GPU test `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`) - EP graph API is single-slot: `CudaRuntime.graph` → EP `*_device_graph*` → session `device_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, #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`). - The NaN (#1647): the StepScoped GQA attention scratch scales with M and is freed each run; the larger M=K verify perturbs the arena so a retained M=1 graph replays against a stale workspace pointer → non-finite logits. ## Remaining gap to a replays-on-verify speedup (next-turn wiring, all anchored in the decision note) 1. Session per-slot `device_graph_signature` + slot-parameterized capture/replay/reset (delegating to the new EP `_in` methods). 2. Fixed padded verify shape (constant `M = k+1`, causal-masked padding; padded GDN state discarded by the existing snapshot→restore→re-advance commit) → shape-invariant replayable verify. 3. Pinned StepScoped verify workspace (reserve at M=K peak, stop freeing while the Verify graph is installed; self-stabilizing invalidate-on-grow). 4. Native verify state machine that captures the fixed-M verify into the Verify slot and replays it; M=1 stays in Primary. 5. Correctness gates unchanged: MTP token-identical to greedy, greedy inert, fallbacks=0. ## Validation (H200 `CUDA_VISIBLE_DEVICES=5`, all 8 idle; PATH/CUDA_HOME set; ORT 1.28 cuda13; int4 block-32; branch off origin/main `7ccdb920e`) - `ep-cuda graph::tests`: **8/8 pass** incl. the new two-slot test. - `onnx-genai-engine` native-backend lib suite: **575 pass, 0 fail** (engine untouched, greedy inert). - Full bench build `--features bench-native,native-cuda,cuda-13000`: clean. - GPU inertness on real Qwen3.8-27B int4 hybrid: MTP steady 14.59 tok/s, acceptance 78.9%, `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> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
`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>
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
…line sibling (#1652) ## Summary Lands the full option-(c) fixed-M (M=k+1) verify CUDA-graph capture machinery for native MTP self-speculative decode (steps 1-4 of the #1650 plan), **gated to arm only when a decode-inline sibling executor exists**. On the current q38-27b-int4-mtp hybrid artifact — which has no such sibling — the feature is **inert and correct** (no regression, no NaN, greedy byte-identical). ## What - **Executor/session** (`onnx-runtime-session`): `pin_step_workspace` flag + pin-aware `release_step_workspace` (persistent verify workspace — fixes the #1647 stale-device-ptr NaN); Session pin setters over the #1650 per-slot Verify routing. - **Engine** (`native_decode/cuda.rs`): 8 `DecodeCudaState` verify fields; `configure_verify_capture` (persistent padded M=k+1 bindings widened from each live binding's own physical shape — **no hardcoded dims** — pins workspace, routes main exec to the Verify slot; gated on `enable_decode_inline()`); `run_verify_captured` + `run_verify_graph_phase` (NeedsWarmup→Armed→Ready) with graceful self-disabling replay (`verify_phase_after_invalidation` re-warms once, then latches to permanent-eager `Unsupported` — a clobber never becomes per-step recapture churn); `swap_verify_bindings`; module-level `widen_query_last`/`widen_query_seq`. - **Observability**: Verify-slot counters via `CudaGraphDebugStats` + `cuda_graph_verify:` line in `profile_native`. - **Driver**: one-time `configure_verify_capture(draft_width)` in `native_speculative.rs::generate`, gated on `NativeProposer::Mtp`. ## Validation - Engine lib suite `--features native-backend`: **579 passed / 0 failed** (575 baseline + 4 new `verify_capture_helper_tests`; greedy inert). - ep-cuda `graph::tests`: **8/8** (incl. `primary_and_verify_graph_slots_are_independent`). - GPU (H200 ord 5, ORT 1.28 cuda13, `--release --features bench-native,native-cuda,cuda-13000`, `profile_native --steady --tokens 128 --warmups 3`, median-of-5): MTP **14.63 tok/s**, acceptance **83.3%**, no NaN. `cuda_graph_verify: captures=0 replays=0` (inert on this artifact, by design). `cuda_graph: captures=64 replays=0 invalidations=515` (pre-existing MTP behavior preserved). ## Precise remaining gap (the campaign's next lever) GPU-confirmed root cause of verify replays=0: **this artifact has no decode-inline sibling**, so M=1 base decode + commit re-advance run on the SAME main executor as the M=2 verify. The executor keeps a single host capture signature, so the M=1 decode clobbers the Verify slot's M=2 signature between capture and replay. The fix is **per-slot host capture state on `Executor`** (`device_graph_signature` + `capture_warm_*`/schedule/cf_shapes indexed by `graph_slot`; non-resetting `set_graph_slot`) so Primary(M=1) and Verify(M=2) coexist and both replay — a large, high-risk refactor deferred to a follow-up. Greedy only ever uses slot Primary, so the split stays greedy-byte-identical. Alternatively a sibling-capable hybrid MTP artifact would let this gated feature demonstrate replays>0 directly.⚠️ Validated by unit tests + GPU no-regression; end-to-end verify replays>0 pending the executor per-slot refactor (or a sibling-capable artifact). No speedup number is fabricated — MTP is correct but cannot yet beat greedy on this artifact because the verify graph never replays. 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
`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
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
Investigated the "make native MTP self-spec
replays>0" speedup lever (turn the ~78% acceptance into a real net win vs the 62.56 tok/s greedy baseline). No speedup number exists — naive graph retention is both insufficient and capture-unsafe (GPU-verified). This PR lands the root-cause analysis + a documented dormant seam (retain_decode_graph_across_spec, default OFF), byte-inert vs origin.Per the coordinator's fallback clause (if retention across differing shapes is infeasible in one turn, land the root-cause + partial capture-safety improvement and report precisely — do not fabricate a speedup).
Root cause of MTP
replays=0(GPU-localized)The captured M=1 decode graph is torn down twice per spec step:
cuda.rsrun_cuda_eager_rows_owned— the eager M>1 verify forward callsinvalidate_graph.backend.rs:145rewind_inner— the commit rewind.binding_signaturekeys only on physical_shape+device_ptr (not logical length/data), so greedy replays fine as KV grows but MTP never survives to a replay.Why naive retention fails (GPU A/B, temporary env toggles, since reverted)
replays=0(verify still invalidates).replays=0(rewind still invalidates).step_workspacefreed after the run (release_step_workspace); the captured M=1 graph baked the old pointer → next replay reads a stale/moved address → NaN. Greedy is immune (every step same M=1 shape → same arena address).Deeper structural blocker
Even solving the workspace issue, the M=K verify stays eager and pays full per-op launch overhead that graphed greedy avoids. A real MTP speedup requires capturing the verify itself (option-c padded verify capture: pad to maxK, capture once, replay for base+verify) + a pinned workspace + a shape-keyed graph slot (EP holds a single graph signature today). Multi-turn executor workstream — recommend scoping the next turn explicitly to option-c.
What landed
retain_decode_graph_across_spec: bool(default false) gating both invalidation sites, mirroring the existing dormantretain_graph_on_rewind/option-c convention, with the GPU evidence documented inline at each site + on the field.Validation (GPU H200,
CUDA_VISIBLE_DEVICES=5, all 8 idle;--features bench-native,native-cuda,cuda-13000; ORT 1.28 cuda13; int4 block-32; branch off origin/main843b0bf7d)cargo test -p onnx-genai-engine --no-default-features --features native-backend --lib: 575 passed, 0 failed, 1 ignored (greedy inert).--steady): 14.45 tok/s, acceptance 78.9%,cuda_graph enabled=true captures=16 replays=0 fallbacks=0 invalidations=99, no NaN.cuda_graph captures=2 replays=92 fallbacks=0 invalidations=1(healthy).Decision note:
.squad/decisions/inbox/gaff-mtp-graph-retain-capture-unsafe-blocker.md.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com