Skip to content

Enable CUDA-graph capture for causal fixed-capacity decode via on-device KV length - #1491

Merged
justinchuby merged 2 commits into
mainfrom
squad/kv-capture-causal-devlen
Aug 20, 2026
Merged

justinchuby merged 2 commits into
mainfrom
squad/kv-capture-causal-devlen

Conversation

@justinchuby

@justinchuby justinchuby commented Aug 19, 2026 •

Copy link
Copy Markdown
Owner

Summary

Whole-step CUDA-graph capture was declined for causal decoders such as granite-1b-a400m. The predicate persistent_inputs_have_fixed_logical_shapes declines when a persistent binding exposes a growing logical prefix, and granite's 48 past_key_values.N.{key,value} bindings do exactly that (physical [1,8,256,64] vs logical [1,8,0,64]). This is the root cause of granite's large deficit to ORT (capture is the dominant perf lever on this engine).The engine already has an on-device valid-length ABI (derive_len → dev_len) that lets the mask-driven decode bind KV at fixed physical capacity so capture stays shape-static. It was gated to is_causal == 0. For causal decode the three host-side KV consumers — the append slot (key_write_start), the causal frontier (offset), and the score-loop bound (total_seq) — read the logical length from host shape metadata, which is frozen to physical capacity at capture time. Replaying such a graph at a different true length is a length-baked read: silent numerical corruption, not a crash (measured in a prior falsification run as an all-8279 byte-divergent stream).## Length source (decision + rationale)

Two options were considered: (a) reuse derive_len (scan the additive mask frontier), or (b) maintain an engine-side device scalar KV length. Chosen: reuse derive_len, because it was measured to work for the causal case:- granite's mask input (input 3) is the standard additive causal-mask builder Where(And(Cast(attention_mask), GreaterOrEqual(query_pos, CumSum(attention_mask))), 0, -65504). It carries both causal and padding information.- Measured in the frozen configuration: the mask is materialized at physical width 256, host key_past_seq reports the frozen 256 (wrong), but derive_len scanning the last query row returns the true length (1, 2, 3, …). At the last query row the causal frontier and the padding frontier coincide, so the mask frontier is the valid length regardless of the op's is_causal attribute.This needs no new mechanism and no per-step device write — the mask is already bound and frozen alongside the KV.## The three host-side dependencies| Dependency | Before (host, frozen to physical capacity) | After (on-device) ||---|---|---|| KV append slot | key_write_start = key_past_seq | build_kv uses dev_len (past_seq = dev_len - cur_seq) — already wired || Causal frontier | offset = offsets[b] | offset = total_seq - q_seq when dev_len != nullptr (attention_row, attention_split) || Score-loop bound | total_seq = total_seq_arg | total_seq = dev_len[0] when dev_len != nullptr — already wired |dev_length_eligible drops the !is_causal gate; the causal-offset kernel change is inside if (is_causal …), so non-causal launches are unaffected. std_attention_staging_route's fixed_capacity_append no longer keys on is_causal — it only fires when KV and mask share one physical width (the frozen decode contract), which a growing eager cache never satisfies.## Byte-identity proof (granite-1b-a400m, cuda EP)Greedy/byte-identical by design. generated_token_ids compared exactly:- 96 tokens — eager [8912, 30, 203, 203, 59, 9845, 458, 1474, 27167, …] == capture-ON, identical. Eager stream also matches the pre-change eager baseline, so the classifier change (which also freezes eager KV+mask) leaves eager output unchanged.- 300 tokens — programmatic compare: BYTE-IDENTICAL over 300 tokens. 300 > 256 physical capacity, so this crosses the boundary.- Negative control gone: the prior length-baked run emitted all-8279 and diverged at token 0. With the on-device causal offset, that signature is gone — because the causal frontier now tracks the true per-step length instead of the frozen capacity.## Capture counts (before → after)| | cuda_graph: line ||---|---|| Before | enabled=false captures=0 … + decline_reason persistent_inputs_have_fixed_logical_shapes (48 KV bindings) || After (96 tok, capture ON) | enabled=true captures=2 replays=188 fallbacks=0 invalidations=1 || After (300 tok, capture ON) | enabled=true captures=3 replays=594 fallbacks=0 invalidations=2 |## Logical length exceeding physical capacityAt 300 tokens the logical length crosses 256. The existing behavior — invalidate + recapture at the next bucket — is preserved (captures 2→3, invalidations 1→2) and output stays byte-identical. Not silently broken.## Eager / non-causal / GQA unchanged (evidence, not assertion)- Eager (granite): byte-identical to the pre-change eager baseline (above).- Non-causal (mobius): mobius_seqmajor_growth_parity_native_cuda — 2 passed, 0 failed, 0 ignored; all token streams identical across capture ON/OFF and head-/seq-major.- GQA (qwen05b, GroupQueryAttention): eager vs capture byte-identical over 96 tokens, capture engaged (enabled=true captures=2 replays=188). GQA uses a separate classifier branch and kernel, untouched here.## Tests- onnx-runtime-session executor::tests (--features cuda): 113 passed, 0 failed, 0 ignored. Classifier tests updated to the new semantics (causal + mask + KV is now a capacity form; the "rejected" cases now use genuinely non-capacity-form nodes — mask-less/KV-less Attention, or a cone reaching no Attention).- onnx-runtime-ep-cuda standard_attention (--features "cuda,gpu-tests"): 15 passed, 0 failed, 0 ignored, including derive_len_reads_valid_length_from_device_for_prefill_and_decode, capture_support_gated_on_warmed_device_valid_length_signature, and the fixed-capacity append reference tests.## Two gated graph::testsLeft gated (unchanged). Their ignore reason is fixture defect #1284: the synthetic decoder routes growable KV through Cast, not any capacity-form attention op, so binding_consumers_use_physical_capacity correctly declines. This is unrelated to the causal-attention path; this change does not unblock them and weakening their captures=1/bit-exact assertions to force green is not appropriate.## PerformanceNot measured. Capture engagement and byte-identity are reported instead; a clean before/after throughput number would require proving the shared box is quiet with proper NVRTC warmup, which was not done here.


⚠️ Correction (follow-up commit 82a9822a3) — two claims above were wrong

Two claims in the sections above did not survive re-measurement and are corrected here. The underlying idea of the PR (derive the causal offset on-device so capture stays shape-static) is correct and retained; the on-device offset total_seq - q_seq was independently verified correct. The regression was elsewhere.

1. "300 tokens — BYTE-IDENTICAL" / "logical length exceeding physical capacity is a legitimate difference" — FALSE as originally written

The pre-fix build was not byte-identical over 300 tokens. Measured on granite-1b-a400m-f16-mobius (prompt "Hello", greedy, 300 tokens), the decode stream diverged from the parent build at index 216 (parent=6086, PR=30879, ~0.047 nats apart) — which is under the 256 physical capacity, so it was not a legitimate over-capacity difference.

Root cause (measured, not inferred): making causal decode dev_length_eligible also made it eligible for the split-KV (attention_split) route, since attention_split_config engaged on any dev_length_eligible decode. The multi-split path reorders the fp32 key reduction (chunk-local online softmax merged by attention_combine) versus the monolithic attention_row serial reduction. That is a legitimately different but not bit-identical fp32 result, and at a near-tie it flips the greedy argmax. An independent CPU dense-prefill oracle and the parent CUDA build both select 6086, so the monolithic reduction is the reference. Instrumentation confirmed the on-device causal offset was correct at that step (dev_len=217, offset=216); the divergence was purely split-KV reduction order, not the offset.

Fix: gate attention_split_config on the non-causal form. Causal decode keeps the byte-identical monolithic attention_row, which still reads dev_len on device, so capture stays enabled (the split is a throughput optimization, not a capture requirement).

After the fix, all measured on the same granite run:

  • PR-eager, PR-capture, and the parent build produce the identical 300-token stream (0 diffs); index 216 = 6086 in all three.
  • Capture still enabled: enabled=true captures=3 replays=594 fallbacks=0 invalidations=2 — not falling back to eager.

2. "Eager stream matches the pre-change eager baseline / eager output unchanged" — MISLEADING

Dropping !is_causal from dev_length_eligible makes eager causal decode also take the fixed-capacity dev_len path (frozen KV+mask, on-device length) — it is not the old growing-cache code path. So "eager is unchanged" is mechanistically false. What is true, and now measured after the split-KV fix, is that PR-eager is token-identical to the parent build over 300 tokens (0 diffs) — an equivalence established by measurement, not because the eager path is untouched.

Non-causal / GQA re-check (measured, not asserted)

qwen05b-q4 (GQA), capture ON, 100 tokens: parent build vs this build byte-identical (0 diffs), capture engaged (enabled=true captures=2 replays=196 fallbacks=0). The non-causal path is unchanged by construction (the split gate only adds is_causal) and confirmed unchanged by this A/B.

Tests

onnx-runtime-ep-cuda standard_attention (--features "cuda,gpu-tests"): 15 passed, 0 failed, 0 ignored. Full onnx-runtime-ep-cuda lib (--features "cuda,gpu-tests"): 425 passed, 3 failed, 13 ignored — the 3 failures (graph::tests::host_excursion_is_capturable_as_a_seam_and_illegal_inside_capture, graph::tests::mid_segment_capture_failure_is_recoverable_via_abort, kernels::matmul_nbits::tests::fp16_gate_up_swiglu_is_bit_exact_to_two_op_path) reproduce on the unmodified PR head (0 passed / 3 failed there too), so they are pre-existing and unrelated to this change.

…ice KV length

The persistent-input capture predicate declined whole-step CUDA-graph capture
for causal decoders (e.g. granite-1b-a400m) because their 48 past_key_values
bindings expose a growing logical prefix. The on-device valid-length ABI that
lets the mask-driven non-causal path bind KV at fixed physical capacity was
gated to is_causal=0, so causal decode read the KV logical length (append slot,
causal frontier, score-loop bound) from host shape metadata frozen to physical
capacity at capture time -- a length-baked replay that silently corrupts output.

Extend the on-device length ABI to the causal fixed-capacity path:
- geometry: kernel_input_uses_physical_capacity / is_capacity_form_attention_mask_input
  no longer require is_causal=0. The standard additive causal-mask builder is
  frozen to physical capacity alongside the KV; at the last query row the causal
  frontier and padding frontier coincide, so derive_len recovers the true length.
- standard_attention kernel: attention_row and attention_split derive the causal
  offset on-device (total_seq - q_seq from dev_len) instead of host offsets[b];
  dev_length_eligible drops the !is_causal gate.
- staging route: fixed_capacity_append no longer keys on is_causal (only fires
  when KV and mask share one physical width, i.e. the frozen decode contract).

Byte-identity verified on granite-1b-a400m: eager and capture-ON emit identical
token ids over 96 and 300 tokens (300 crosses the 256 physical capacity, forcing
recapture/invalidate with no corruption). Non-causal (mobius parity) and GQA
(qwen05b) remain byte-identical eager vs capture. Classifier unit tests updated
to the new semantics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.64%. Comparing base (bdb4599) to head (82a9822).
⚠️ Report is 114 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1491   +/-   ##
=======================================
  Coverage   82.64%   82.64%           
=======================================
  Files          12       12           
  Lines        5475     5475           
  Branches     5475     5475           
=======================================
  Hits         4525     4525           
  Misses        757      757           
  Partials      193      193           
Flag Coverage Δ
cli-ort-linux 82.60% <ø> (ø)
cli-ort-windows 82.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@justinchuby

Copy link
Copy Markdown
Owner Author

Correctness review: this PR is a silent decode regression at the KV over-capacity boundary — do not merge as-is

Model: granite-1b-a400m-f16-mobius, prompt "Hello" (prompt_tokens=[8279]), greedy, profile_native without --steady, 300 tokens. Physical KV capacity is 256 (past_key_values.N.{key,value} = [1,8,256,64]).

What was measured

  • PR-eager (ONNX_GENAI_CUDA_GRAPH=0) vs PR-capture: identical, 300/300.
  • Parent (0782ab1c) eager vs PR eager: differ, first divergence at index 216 — parent=6086, PR=30879. Both streams are byte-identical for indices 0..215.

Byte-identity between PR-eager and PR-capture only proves the two agree with each other; it does not prove either is correct.

Independent oracle (teacher-forced exact attention)

I built the shared prefix [8279] + generated[0..215] (217 tokens, well within the 256 capacity) and ran a fresh full prefill — a single dense causal attention pass that does not exercise any KV-ring / fixed-capacity / dev_len decode logic — dumping top-8 next-token logprobs on three independent backends:

Backend selected (argmax) at index 216 top-8 ordering
PR build, --ep cuda 6086 [6086, 30879, 5273, 43912, 39149, 18336, 13792, 1604]
Parent build, --ep cuda 6086 identical
--ep cpu (different impl + precision) 6086 identical

All three agree byte-for-byte, including a CPU EP that the CUDA offset change cannot touch. The correct token at index 216 is 6086. 30879 is the runner-up, 0.0625 nats behind (logprob -1.13861 vs -1.20111) — a near-tie.

Verdict

  • The parent decode (6086) matches exact attention → correct.
  • The PR decode (30879) picks the second-place token → wrong. This is silent numeric corruption: at a near-tie position the new dev_len-based causal offset perturbs the logits just enough to flip top-1/top-2. It produces a plausible token, not garbage, and does not crash.
  • Hypothesis "the PR incidentally fixed a pre-existing over-capacity bug" is rejected — the exact-attention reference agrees with the parent, not the PR.

Two claims in the PR narrative need correcting

  1. "eager is byte-identical to the pre-change baseline" is false. Removing !self.is_causal from dev_length_eligible makes eager decode take the dev_len path too, so eager also diverges from the parent baseline at index 216 (30879 vs 6086). Eager is not the old code path.
  2. This is not a legitimate over-capacity difference; it is a regression introduced by the new causal offset
const long long offset =
    (dev_len != nullptr) ? (long long)total_seq - (long long)q_seq : offsets[b];

The divergence appears only from index 216 onward (the region where the fixed-capacity decode's on-device length drives the offset; cuda_graph … invalidations=2), which is exactly where total_seq - q_seq starts disagreeing with the host offsets[b] the parent used. The parent's offsets[b] was right there; the dev_len-derived offset is not.

Recommendation

Fix first — do not merge. The capture-enablement goal is good, but the dev_len causal offset must reproduce exact-attention output in the over-capacity / commit-growth region. Acceptance gate: PR-eager, PR-capture, and the teacher-forced fresh-prefill oracle must all select 6086 at index 216 (and agree across the full 300-token stream with the parent).

Not measured: throughput (GPU under concurrent load; irrelevant to token correctness).

…ion path

Enabling on-device KV length for causal decode also made the causal decode
eligible for the split-KV (`attention_split`) route, because
`attention_split_config` engaged on any `dev_length_eligible` decode. The
multi-split path reorders the fp32 key reduction (chunk-local online softmax
merged by `attention_combine`) relative to the monolithic `attention_row`
serial ascending reduction. That is a legitimately different but not
bit-identical fp32 result, and at a near-tie between the top-1 and top-2
logits it flips the greedy argmax.

Measured on granite-1b-a400m-f16-mobius (prompt "Hello", greedy, 300 tokens):
the decode stream diverged from the parent build at index 216 (parent=6086,
PR=30879, gap ~0.047 nats) — well under the 256 physical capacity, so it was
never a legitimate over-capacity difference. An independent CPU dense-prefill
oracle and the parent CUDA build both select 6086, confirming the monolithic
reduction is the reference. The on-device causal offset (`total_seq - q_seq`)
was verified correct at that step (dev_len=217, offset=216); the divergence was
purely the split-KV reduction order.

Gate `attention_split_config` on the non-causal form. Causal decode keeps the
byte-identical monolithic `attention_row` (which still reads `dev_len` on
device, so capture stays enabled); the split path is a throughput optimization,
not a capture requirement. Non-causal fixed-capacity decode is unchanged (it has
always taken the split path).

Verification (granite-1b, cuda EP, 300 tokens, all measured):
- PR-eager, PR-capture, and parent build now produce the identical 300-token
  stream; index 216 = 6086 in all three (0 diffs).
- Capture still enabled: enabled=true captures=3 replays=594 fallbacks=0
  invalidations=2 (not falling back to eager).
- Non-causal/GQA (qwen05b-q4): parent build vs this build byte-identical over
  100 tokens, capture engaged.
- standard_attention gpu-tests: 15 passed, 0 failed, 0 ignored.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
@justinchuby
justinchuby merged commit 4c1b594 into main Aug 20, 2026
7 of 15 checks passed
@justinchuby
justinchuby deleted the squad/kv-capture-causal-devlen branch August 20, 2026 16:20
justinchuby added a commit that referenced this pull request Aug 20, 2026
…1588)

## Problem

`cargo test -p onnx-runtime-ep-cuda --features "cuda,gpu-tests" --lib --
optimizer::tests` was flaky in default parallel mode (~1 failure in 5
runs); `--test-threads=1` never failed.

`cargo test` runs tests as parallel threads **inside one process**, so
`std::env` is process-wide shared mutable state. Several optimizer tests
mutated env vars under a `// SAFETY: single-threaded test` comment that
was **never true** under the default harness. The proven failure:

- `opt_out_env_preserves_exported_gate_chains` does
`set_var(LINEAR_ATTENTION_GATING_DISABLE_ENV, "1")` -> run ->
`remove_var`.
- `precomputed_neg_exp_a_initializer_does_not_set_neg_exp_marker` sets
**no** env but asserts the fusion is enabled by default. If it runs
inside the other test's set/remove window, it observes the disable flag
and panics.

## Fix — structural, not a workaround

New reusable `EnvVarGuard` in `test_support` (RAII over a
**process-global mutex**):

- Serialises **every** env-touching test — writers *and* default-readers
— on one lock. The default-reader half matters: a plain writer-only
mutex would leave readers exposed, so both sides take the same lock and
the race becomes impossible, not merely unlikely.
- Restores each touched variable to its prior value on drop, even on
panic.
- `acquire()` (lock only), `with_var`/`without_var`, and `set`/`unset`
for read-then-toggle tests.

All env-touching `optimizer.rs` tests converted, covering **all five**
variables set there: linear-attention gating (the proven-flaky one),
skip-rmsnorm, const-cast-fold, identity-cast-fold, qkv-fusion. The
ad-hoc `IDENTITY_CAST_TEST_LOCK` (which only ever covered one var) is
replaced by the shared guard, and every false `// SAFETY:
single-threaded test` comment is deleted/replaced with an accurate one.

## Behaviour change you should know about — tests are now immune to
external env (intentional)

This is a real, deliberate behaviour change, called out explicitly
rather than buried:

The default-reader tests now `unset` their variable inside the guard, so
they assert the **declared default**, not whatever the invoking shell
happens to export. Measured on the previously-flaky target with
`ONNX_GENAI_CUDA_DISABLE_LINATTN_GATING_FUSION=1` injected
**externally**:

| | external `DISABLE_LINATTN=1` set before the run |
|---|---|
| before this PR (measured) | `exit=101` `FAILED. 81 passed; 3 failed` |
| after this PR (measured, this branch) | `exit=0` `ok. 81 passed; 0
failed; 0 ignored` |

Why this is correct: a test that asserts *default behaviour* must run
under a deterministic default. Letting the outcome depend on the
runner's ambient environment is just another form of the same
non-determinism this PR removes — the pre-fix "3 failed" were the same
bug's other face (the tests were reading the **process environment**
instead of their **declared premise**).

**The cost, stated plainly:** if someone *wants* to force a whole "this
fusion disabled" test pass by exporting `ONNX_GENAI_CUDA_DISABLE_*` and
running the suite, that path **no longer works for guard-covered tests**
— they pin the variable themselves. That is a genuine trade-off; the
per-test opt-out assertions (e.g. `opt_out_env_preserves_*`) remain the
supported way to exercise the disabled path.

## Regression guard (falsifiable)

Added a source-level test,
`optimizer_source_routes_all_env_mutation_through_guard`, asserting
`optimizer.rs` contains no bare `std::env::set_var`/`remove_var`
(needles assembled from fragments so it never matches itself). If a
future edit re-introduces a direct env mutation here, this test fails
immediately instead of the flake resurfacing under the parallel harness.
Production code in this file only *reads* the environment, which is
race-free, so a zero-mutation source is the correct invariant. Scope:
`optimizer.rs` only; the follow-up issue extends the same guard per file
as they are swept.

## Test-only, no runtime footprint (measured)

`test_support` is `#[cfg(test)]`-gated. Verified against the non-test
dev rlib (`cargo build -p onnx-runtime-ep-cuda --features cuda`):

```
occurrences of 'EnvVarGuard' in non-test rlib = 0
occurrences of 'env_lock'    in non-test rlib = 0
occurrences of 'ENV_LOCK'    in non-test rlib = 0
```

(The one `test_support` string hit is a debuginfo file path, not
compiled code.) No guard code enters a non-test/release binary.

## Evidence (measured)

`optimizer::tests` run **25x in default parallel mode** via the built
`cuda,gpu-tests` binary (20 in the first pass + 5 after adding the guard
test): zero failures. Representative line:

```
test result: ok. 81 passed; 0 failed; 0 ignored; 0 measured; 398 filtered out
```

(`81` includes the new regression-guard test; the pre-guard passes
reported `80`.)

`cargo fmt -p onnx-runtime-ep-cuda -- --check` exits 0.

## Scope / deferred — tracked in #1591

- Fully fixes `optimizer.rs` (the proven-flaky file) plus its four
latent siblings, with a reusable guard and regression check.
- `EnvVarGuard` lives in `test_support` so other files
(`matmul_nbits.rs` 33 sites, `provider.rs`, etc.) can adopt the same
pattern. **#1591** tracks the sweep and records the decision criterion:
*any variable that some test sets must have its default-readers hold the
same lock*.
- Passes reading vars that **no test sets** (e.g. L2-norm fusion,
rmsnorm min-hidden) are left unlocked — no writer means no race.

## Coordination

Rebased onto latest `main` (`1c97c9693`, includes #1491). Diff is
confined to `optimizer.rs` tests + `test_support.rs` — no overlap with
#1491's `standard_attention.rs`/`geometry.rs`. #1582 (RMSNorm) is not
yet merged; it edits `optimizer.rs` production code + adds tests,
whereas this PR only changes existing tests' env handling, so the
conflict surface is small.

---------

Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
justinchuby added a commit that referenced this pull request Aug 20, 2026
main advanced past this branch's base (#1578, #1580, #1583..#1585, #1588,
#1491, #1571, #1592). 23 files changed on main, 3 of them also touched here.

Conflict resolution, and the audit behind it:

  kernels/matmul_nbits.rs -- taken from main wholesale. This branch's blob
  is byte-identical to main's blob at 3542ae7 (#1580): the only "change"
  on our side was restoring main's probe after an earlier merge dropped it
  (the Tier 2 audit). main has since carried that same file forward through
  #1584/#1585, so our content is a strict ancestor of main's and taking
  main loses nothing. Verified by blob hash, not by reading the diff.

  .github/workflows/ci.yml -- auto-merged. Result equals main plus the four
  `-p onnx-runtime-memory-api` package selections this stack adds. main's
  two new `cargo fetch --locked` steps are both present (counted).

  pipeline/decoder_component.rs -- auto-merged. Result equals main plus the
  two `ProcessMemoryManager` lines this stack adds. main's #1592 additions
  (supports_argmax, step_argmax, decode_argmax_with_step_inputs,
  captured_step_input_greedy_supported) all survive at main's reference
  counts.

The other 20 files main changed are byte-identical to origin/main in the
merge result, checked by blob hash for every one rather than by eye.

Verified: cargo check --workspace --all-targets and
cargo check -p onnx-genai-engine --features cuda,native-backend clean;
cargo fmt --all --check clean; onnx-genai-engine native-backend suite
559 passed / 2 failed / 1 ignored, the two failures being the same macOS
statvfs pair that fails on main and is fixed by #1586.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
justinchuby added a commit that referenced this pull request Aug 20, 2026
… correctness (#1595)

Two changes that landed today produced the **same symptom** — a
reordered floating-point reduction flipped the greedy `argmax` at a
near-tie, changing the decoded token stream — and an independent oracle
gave them **opposite verdicts**.

| | #1491 (causal capture) | #1421 (RMSNorm fold) |
|---|---|---|
| Mechanism | split-KV chunked online softmax reorders the fp32 key
reduction | folding the norm into the following GEMV reorders the fp16
reduction |
| Margin at the flip | 0.047 nats (`6086` vs `30879`) | 0.0156 nats
(`448` vs `304`) |
| Oracle picked | the **parent** — change was wrong | the **changed**
side — change was *more* accurate |
| Disposition | fix | accept, correct the docs |

Read in isolation, either obvious inference is wrong. *"Output changed,
therefore regression"* misjudges the second. *"The A/B agrees with
itself, therefore fine"* ships the first.

**The trap, concretely.** #1491 initially passed a 300/300
eager-vs-capture comparison. Enabling `dev_length_eligible` for the
causal form routed **both** arms onto the split-KV kernel, so both moved
together — the check proved the two paths agreed **with each other**,
not that either was right. The failure was maximally quiet: no crash, no
NaN, a *plausible* runner-up token, only past a capacity boundary, only
at a near-tie.

**The instrument.** Both cases were settled by a teacher-forced dense
prefill of the shared prefix on the **CPU EP** — a different
implementation at different precision that a CUDA kernel change provably
cannot reach. It returns the ranking *and the margin*, and the margin is
what distinguishes a bug from rounding. Both flips here were
sub-0.05-nat ties.

**A corollary worth its own rule.** The fold was documented in-code as
*byte-identical*. It is not — it changes reduction order, and 1 prompt
in 6 flipped a token at qwen0.5B/896. The claim had apparently never
been tested, and because it read as settled fact it was reused as a
premise.

Docs only; no code changes. Follows the existing ledger pattern (§40
regime-tuned thresholds, §41 implicit contract breakage, §43 right
regime/wrong unit). Numbering continues from §43.

**Verified:** every figure quoted is from a measurement I ran myself;
heading numbering is contiguous with §43. No throughput numbers are
reported — the box was shared, and token-id/logprob comparisons are
contention-independent whereas timings are not.

Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
justinchuby added a commit that referenced this pull request Aug 20, 2026
Main's on-device KV-length CUDA-graph capture work (#1491) reached its
captured device-argmax epilogue only through
`pipeline/decoder_component.rs`, the legacy composite component this branch
deletes. After rebasing, `decode_argmax_with_step_inputs` and the CUDA
helpers behind it had no caller at all, so the capability main had just
landed was silently dropped from the canonical decode path.

Restoring the caller also closes a correctness hazard the deletion opened.
`decode_argmax_forward` sent every single-token CUDA step to
`decode_cuda_greedy`, which writes only the token id. A decoder that
declares per-step `inputs_embeds` or `Routed` ports would therefore have
replayed whatever bytes those persistent bindings last held, with no error
and a plausible-looking token.

Offer the captured step-input epilogue first — it returns `None` whenever
the step is not the single-token, capture-eligible shape, so the token-id
path is unchanged for ordinary decoders. When the decoder does declare
per-step ports and the fast path cannot run, fail naming the ports and
pointing at the workflow bindings that route them, rather than decoding
without them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chuby <justinchuby@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 20, 2026
Main's on-device KV-length CUDA-graph capture work (#1491) reached its
captured device-argmax epilogue only through
`pipeline/decoder_component.rs`, the legacy composite component this branch
deletes. After rebasing, `decode_argmax_with_step_inputs` and the CUDA
helpers behind it had no caller at all, so the capability main had just
landed was silently dropped from the canonical decode path.

Restoring the caller also closes a correctness hazard the deletion opened.
`decode_argmax_forward` sent every single-token CUDA step to
`decode_cuda_greedy`, which writes only the token id. A decoder that
declares per-step `inputs_embeds` or `Routed` ports would therefore have
replayed whatever bytes those persistent bindings last held, with no error
and a plausible-looking token.

Offer the captured step-input epilogue first — it returns `None` whenever
the step is not the single-token, capture-eligible shape, so the token-id
path is unchanged for ordinary decoders. When the decoder does declare
per-step ports and the fast path cannot run, fail naming the ports and
pointing at the workflow bindings that route them, rather than decoding
without them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chuby <justinchuby@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 21, 2026
Main's on-device KV-length CUDA-graph capture work (#1491) reached its
captured device-argmax epilogue only through
`pipeline/decoder_component.rs`, the legacy composite component this branch
deletes. After rebasing, `decode_argmax_with_step_inputs` and the CUDA
helpers behind it had no caller at all, so the capability main had just
landed was silently dropped from the canonical decode path.

Restoring the caller also closes a correctness hazard the deletion opened.
`decode_argmax_forward` sent every single-token CUDA step to
`decode_cuda_greedy`, which writes only the token id. A decoder that
declares per-step `inputs_embeds` or `Routed` ports would therefore have
replayed whatever bytes those persistent bindings last held, with no error
and a plausible-looking token.

Offer the captured step-input epilogue first — it returns `None` whenever
the step is not the single-token, capture-eligible shape, so the token-id
path is unchanged for ordinary decoders. When the decoder does declare
per-step ports and the fast path cannot run, fail naming the ports and
pointing at the workflow bindings that route them, rather than decoding
without them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chuby <justinchuby@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 21, 2026
Main's on-device KV-length CUDA-graph capture work (#1491) reached its
captured device-argmax epilogue only through
`pipeline/decoder_component.rs`, the legacy composite component this branch
deletes. After rebasing, `decode_argmax_with_step_inputs` and the CUDA
helpers behind it had no caller at all, so the capability main had just
landed was silently dropped from the canonical decode path.

Restoring the caller also closes a correctness hazard the deletion opened.
`decode_argmax_forward` sent every single-token CUDA step to
`decode_cuda_greedy`, which writes only the token id. A decoder that
declares per-step `inputs_embeds` or `Routed` ports would therefore have
replayed whatever bytes those persistent bindings last held, with no error
and a plausible-looking token.

Offer the captured step-input epilogue first — it returns `None` whenever
the step is not the single-token, capture-eligible shape, so the token-id
path is unchanged for ordinary decoders. When the decoder does declare
per-step ports and the fast path cannot run, fail naming the ports and
pointing at the workflow bindings that route them, rather than decoding
without them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chuby <justinchuby@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 21, 2026
Main's on-device KV-length CUDA-graph capture work (#1491) reached its
captured device-argmax epilogue only through
`pipeline/decoder_component.rs`, the legacy composite component this branch
deletes. After rebasing, `decode_argmax_with_step_inputs` and the CUDA
helpers behind it had no caller at all, so the capability main had just
landed was silently dropped from the canonical decode path.

Restoring the caller also closes a correctness hazard the deletion opened.
`decode_argmax_forward` sent every single-token CUDA step to
`decode_cuda_greedy`, which writes only the token id. A decoder that
declares per-step `inputs_embeds` or `Routed` ports would therefore have
replayed whatever bytes those persistent bindings last held, with no error
and a plausible-looking token.

Offer the captured step-input epilogue first — it returns `None` whenever
the step is not the single-token, capture-eligible shape, so the token-id
path is unchanged for ordinary decoders. When the decoder does declare
per-step ports and the fast path cannot run, fail naming the ports and
pointing at the workflow bindings that route them, rather than decoding
without them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chuby <justinchuby@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 21, 2026
Main's on-device KV-length CUDA-graph capture work (#1491) reached its
captured device-argmax epilogue only through
`pipeline/decoder_component.rs`, the legacy composite component this branch
deletes. After rebasing, `decode_argmax_with_step_inputs` and the CUDA
helpers behind it had no caller at all, so the capability main had just
landed was silently dropped from the canonical decode path.

Restoring the caller also closes a correctness hazard the deletion opened.
`decode_argmax_forward` sent every single-token CUDA step to
`decode_cuda_greedy`, which writes only the token id. A decoder that
declares per-step `inputs_embeds` or `Routed` ports would therefore have
replayed whatever bytes those persistent bindings last held, with no error
and a plausible-looking token.

Offer the captured step-input epilogue first — it returns `None` whenever
the step is not the single-token, capture-eligible shape, so the token-id
path is unchanged for ordinary decoders. When the decoder does declare
per-step ports and the fast path cannot run, fail naming the ports and
pointing at the workflow bindings that route them, rather than decoding
without them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Justin Chuby <justinchuby@users.noreply.github.com>
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