Skip to content

Make order-dependent test results loud instead of silent (#797, #801, #804) - #807

Merged
justinchuby merged 2 commits into
mainfrom
squad/test-isolation-guard
Aug 12, 2026
Merged

justinchuby merged 2 commits into
mainfrom
squad/test-isolation-guard

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Why

This class of defect produced two wrong conclusions this week, both of which cost real work before being caught. Both are the same family: state that is frozen or warmed once per process, making results depend on test order. On a measurement-driven subsystem a wrong number does not just fail — it redirects the design.

This is the same telemetry-that-misleads lineage as #719 / #715 (documented in MEMORY_ARCHITECTURE.md's "third failure mode"). After the second instance in a week, the response is a guard, not more vigilance.

This PR is test infrastructure + documentation only. No production behavior changes.

What

1. A guard for process-frozen configuration (onnx-genai-runtime-config)

runtime_config() records the environment it froze the RuntimeConfig snapshot from (captured through the same lookup from_env uses, so the set can never drift from the parsed fields). On every later call it re-reads those variables and panics loudly — with the frozen and current values, and a pointer to #804 — if any now differs. A test can no longer silently believe it set a knob that was already frozen.

2. A helper for stream-correct device test code (onnx-runtime-cuda-memory::test_support::TestStream)

A shared, test-only (#[cfg(any(test, feature = "gpu-tests"))]) RAII wrapper around one non-blocking stream. Every device op a test needs — fill, write, read, plus raw() for graph launches/memsets — flows through that stream, so a single cuStreamSynchronize is a total order. The module doc explains the legacy-default-vs-non-blocking exemption at the call site so the next author does not rediscover #797 the hard way. vmm_kv_layout_residency_gpu.rs (the exact #797 site) is converted to it and its four local stream helpers removed; 8/8 reliable in isolation and green in the full single-threaded suite.

3. The pattern, documented where it will be read (docs/MEMORY_ARCHITECTURE.md)

A tight "a fourth failure mode: order-dependent test state" subsection next to the existing third-failure-mode material: both case studies, the two mechanisms (frozen process config; context warmed by an earlier sibling), the two remedies (process isolation for anything that reads process-frozen config; single-stream discipline for device tests), and the one-line discipline that names the defect — run any test you touch in isolation as well as in the full suite. Cites #797/#801/#804/#794/#719/#715 and links rather than duplicating (#791 consolidated this doc to stop drift).

Inventory of other order-dependent tests found (not fixed here)

Per scope discipline, these are listed, not converted. The TestStream helper and the config guard make converting them mechanical follow-ups.

Device tests that mix a created stream with default-stream copies (#797 hazard)

Each creates a non-blocking stream and issues default-stream cuMemcpyHtoD_v2/cuMemcpyDtoH_v2 copies. Where those touch the same buffers as stream-ordered work on a cold context, they are exposed to the exact #797 race. They should migrate to test_support::TestStream:

  • crates/onnx-runtime-cuda-memory/tests/vmm_commit_shared_prefix_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_dummy_page_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_dummy_write_protect_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_granularity_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_graph_remap_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_kv_contiguous_tail_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_prefix_share_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_prefix_share_write_protect_gpu.rs
  • crates/onnx-runtime-cuda-memory/tests/vmm_stable_va_weight_slot_gpu.rs

(Non-vmm siblings virtual_memory_gpu.rs and physical_pool_production_gpu.rs also use default-stream copies; they pass but are the same latent shape.)

Tests that mutate a RuntimeConfig-feeding env var within a process

These change a variable that feeds the frozen snapshot (ONNX_GENAI_CUDA_GRAPH, ONNX_GENAI_EP) across phases. The correct pattern is child-process-per-phase; the new guard will now fire loudly on any that mutate in-process and then read runtime_config():

  • crates/onnx-genai-engine/tests/mobius_seqmajor_growth_parity_native_cuda.rs — already correct (spawns a child per mode; its own doc comment states ONNX_GENAI_CUDA_GRAPH is parsed once). This is the reference pattern.
  • crates/onnx-genai-engine/tests/stable_va_capture_offload_e2e.rs — sets ONNX_GENAI_CUDA_GRAPH per capture phase.
  • crates/onnx-genai-engine/tests/gemma4_e2b_native_cuda_pipeline_bench.rs — sets ONNX_GENAI_CUDA_GRAPH to 1 then 0.
  • crates/onnx-genai-engine/tests/qwen2_5_1_5b_divergence.rs — switches ONNX_GENAI_EP between cpu and cuda.
  • crates/onnx-genai-engine/tests/qwen3_0_6b_capture_step_inputs_decline.rs and gemma3n_native_cuda_capture_realmodel.rs — toggle the native-decoder capture env per phase.

Session-crate tests that toggle env (ONNX_GENAI_DECODE_MEMO, ONNX_GENAI_SCAN_INLINE_SINGLE_TRIP, ONNX_RT_SIBLING_MERGE, the EP-CPU weight-offload knob) read those variables directly at their call sites, not through RuntimeConfig, so they are not affected by the freeze guard and the full onnx-runtime-session suite stays green.

Validation (all local; not waiting for CI)

  • cargo fmt -p onnx-genai-runtime-config -p onnx-runtime-cuda-memory -- --check ✅ (--all hits the Windows os error 206 long-command artifact, not a formatting issue)
  • cargo clippy --locked --all-targets -p onnx-genai-engine --features native-backend -- -D warnings ✅
  • cargo clippy -p onnx-runtime-cuda-memory --features gpu-tests --all-targets -- -D warnings ✅
  • cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings ✅
  • cargo test -p onnx-runtime-session ✅ (full suite)
  • cargo test -p onnx-runtime-cuda-memory --features gpu-tests -- --test-threads=1 ✅ (all vmm_*/gpu suites, RTX 4060)
  • The converted seq_major_fixed_stride_grows_under_captured_replay_without_recapture run 8× in isolation: 8/8 pass — the property this PR is about.

Notes

  • Kept strictly to test infra + the docs subsection so the two other active workstreams (GQA pooled QKV staging in group_query_attention.rs; head-major vs seq-major re-capture measurement) merge cleanly — no overlap.
  • Does not merge and does not close any issue.

Copilot AI added 2 commits August 12, 2026 02:56
…804)

Two wrong conclusions this week came from state frozen or warmed once per
process, making results depend on test order:

- #804 (carried into #794/#801): ONNX_GENAI_CUDA_GRAPH set after the
  process-wide RuntimeConfig OnceLock froze, so captures=0 was misattributed
  to a model declining CUDA graph capture.
- #797: a readback on the legacy default stream raced a memset on a created
  non-blocking stream (mutually exempt from implicit sync); it only 'passed'
  because an alphabetically-earlier sibling subtest warmed the context.

This adds the guards, not more vigilance (cf. #719, #715 telemetry lineage):

1. onnx-genai-runtime-config: runtime_config() records the environment it
   froze from and, under debug_assertions only (release path untouched and
   cost-free), panics loudly if any variable that fed the frozen snapshot is
   later observed to differ. debug_assertions (not cfg(test)) so the guard is
   live for downstream integration-test binaries where #804 actually lived.
   Pure find_changed() helper is unit-tested.

2. onnx-runtime-cuda-memory::test_support::TestStream: a shared single-stream
   test helper (gated to test/gpu-tests) that makes stream-correct device test
   code the easy path, documenting the default-vs-non-blocking exemption at the
   call site. vmm_kv_layout_residency_gpu.rs (the #797 site) converted to it;
   40/40 reliable in isolation, and the other vmm_* tests that still mix a
   created stream with default-stream copies are inventoried in the PR.

3. docs/MEMORY_ARCHITECTURE.md: a 'fourth failure mode: order-dependent test
   state' subsection with both case studies, the two mechanisms, and the two
   remedies (process isolation for process-frozen config; single-stream
   discipline for device tests).

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

codecov Bot commented Aug 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.55%. Comparing base (39c2935) to head (e300b10).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #807       +/-   ##
===========================================
+ Coverage   79.21%   83.55%    +4.33%     
===========================================
  Files         331       12      -319     
  Lines      138203     5192   -133011     
  Branches   138203     5192   -133011     
===========================================
- Hits       109484     4338   -105146     
+ Misses      24372      677    -23695     
+ Partials     4347      177     -4170     
Flag Coverage Δ
cli-ort-linux 83.50% <ø> (ø)
cli-ort-windows 83.09% <ø> (+0.09%) ⬆️
mlas ?
offline ?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 319 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@justinchuby

Copy link
Copy Markdown
Owner Author

Coordinator pre-merge validation on the merged branch: origin/main merged (no
conflicts); cargo fmt -p onnx-genai-runtime-config -p onnx-runtime-cuda-memory -- --check clean; full cargo test -p onnx-runtime-session: 153 passed;
onnx-genai-runtime-config: 19 passed.

I checked the one thing that could have made this unmergeable — it touches two
production crates, so I verified the guard really is compiled out of release:

#[cfg(debug_assertions)]      { ... freeze_guard::assert_frozen_env_unchanged(); }
#[cfg(not(debug_assertions))] { // identical to the historical behaviour }

Release is byte-for-byte the old path. And the reasoning for debug_assertions
over cfg(test) is correct rather than convenient: the #804 mutation lived in a
downstream crate's integration test, where this crate's cfg(test) is never
set, so cfg(test) would have guarded nothing.

Why this was worth doing at all

This class of defect produced two wrong conclusions this week, and the cost was
not a failing test — it was misdirected design:

Then #805 measured the actual cause of the re-capture question — unconditional
growth invalidation — proving that two of the three intermediate explanations had
been measurement artifacts. On a measurement-driven line, a wrong number does not
merely fail; it redirects the design. Making this class loud is worth a PR.

The caveat is the right call, not a defect

The guard fires on any in-process change to a config-feeding variable,
including five engine e2e tests that already do phase mutation. Those are
#[ignore] without gpu-tests and were outside the required validation set, so
they will trip the guard when run.

That is the intended signal — those tests need child-process isolation, exactly as
the mobius_... reference test already does. Inventorying them (9 vmm_* tests
mixing created and default streams; 5 engine tests mutating config-feeding env
vars) rather than silently converting them is the right scope call: a silent
conversion would have hidden how widespread the pattern is.

I am merging with that inventory as a known follow-up rather than blocking on it.
The guard existing and being loud is what prevents the next two-PR misattribution;
converting the five can follow.

Merging.

@justinchuby
justinchuby merged commit 2f39c38 into main Aug 12, 2026
1 check passed
@justinchuby
justinchuby deleted the squad/test-isolation-guard branch August 12, 2026 10:08
@github-actions

Copy link
Copy Markdown

✅ Benchmarks — No Regression

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
✅ add/small_f16_threads=1-internal/1024 13.49 µs 15.47 µs +14.7%
✅ logit_processing/seven_processor_chain_per_step 329.98 µs 377.59 µs +14.4%
✅ add/small_f32_threads=1-internal/1024 204.8 ns 232.3 ns +13.4%
✅ sampling_latency/min_p_per_token 217.67 µs 246.50 µs +13.2%
✅ sampling_latency/top_p_per_token 432.54 µs 480.14 µs +11.0%
✅ grammar_masking/llguidance_compute_mask/32 84.20 µs 91.67 µs +8.9%
✅ gather/medium_f16_threads=1-internal/32768 2.61 µs 2.84 µs +8.8%
✅ matmul/large_generic_f16_threads=8/32x1024x1024 88.73 µs 95.97 µs +8.2%
✅ gather/medium_bf16_threads=1-internal/32768 2.62 µs 2.80 µs +7.0%
✅ qwen3_sampling_processors/top_p_fast_after_top_k 569.19 µs 600.64 µs +5.5%
✅ matmul/large_generic_bf16_threads=1/32x1024x1024 2.03 ms 2.13 ms +5.2%
✅ tokenization/encode_tokens_per_second 394.94 µs 414.65 µs +5.0%
✅ qwen3_sampling_processors/top_k_top_p_full_sort_baseline 6.39 ms 6.58 ms +3.0%
✅ sampling_latency/top_k_per_token 64.00 µs 64.76 µs +1.2%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 80.40 µs 79.74 µs -0.8%
✅ add/medium_f16_threads=1-internal/262144 2.62 ms 2.59 ms -1.0%
✅ qwen3_sampling_processors/top_k_top_p_fast 767.67 µs 756.61 µs -1.4%
✅ add/small_bf16_threads=1-internal/1024 14.14 µs 13.83 µs -2.2%
✅ tokenization/decode_tokens_per_second 7.12 ms 6.96 ms -2.3%
✅ add/medium_f32_threads=1-internal/262144 2.66 ms 2.59 ms -2.6%
✅ add/medium_bf16_threads=1-internal/262144 2.71 ms 2.62 ms -3.3%
✅ matmul/medium_generic_f32_threads=1/32x512x512 2.39 ms 2.30 ms -3.4%
✅ qwen3_sampling_processors/top_k_partial_selection 175.24 µs 169.20 µs -3.4%
✅ add/large_f32_threads=1-internal/4194304 42.03 ms 40.13 ms -4.5%
✅ matmul/large_generic_bf16_threads=8/32x1024x1024 1.43 ms 1.34 ms -6.0%
✅ kv_cache/alloc_dealloc_pages 49.68 µs 45.47 µs -8.5%
✅ block_quantized_moe_cached_dense/mxfp4_cached_dense_expert_repeated_call/rows=1,H=256,I=256,E=4,top_k=1 87.02 µs 77.67 µs -10.8%
✅ sampling_latency/greedy_per_token 3.83 µs 3.42 µs -10.8%
✅ matmul/small_generic_bf16_threads=8/1x256x256 34.93 µs 31.15 µs -10.8%
✅ qwen3_sampling_processors/top_k_full_sort_baseline 2.76 ms 2.45 ms -11.5%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.13 ms 997.38 µs -12.1%
✅ gather/small_f32_threads=1-internal/4096 768.3 ns 669.7 ns -12.8%
✅ add/large_bf16_threads=1-internal/4194304 48.38 ms 41.95 ms -13.3%
✅ matmul/large_generic_f32_threads=1/32x1024x1024 11.00 ms 9.42 ms -14.3%
✅ gather/small_bf16_threads=1-internal/4096 556.6 ns 474.8 ns -14.7%
🟢 matmul/medium_generic_f16_threads=1/32x512x512 37.04 µs 31.31 µs -15.4%
🟢 matmul/medium_generic_bf16_threads=1/32x512x512 623.05 µs 526.20 µs -15.5%
🟢 add/large_f16_threads=1-internal/4194304 49.29 ms 41.35 ms -16.1%
🟢 matmul/large_generic_f32_threads=8/32x1024x1024 4.66 ms 3.88 ms -16.7%
🟢 matmul/small_generic_f16_threads=1/1x256x256 37.30 µs 30.67 µs -17.8%
🟢 matmul/medium_generic_f16_threads=8/32x512x512 37.70 µs 30.99 µs -17.8%
🟢 gather/large_f32_threads=1-internal/131072 35.69 µs 29.13 µs -18.4%
🟢 matmul/small_generic_bf16_threads=1/1x256x256 39.70 µs 31.41 µs -20.9%
🟢 gather/small_f16_threads=1-internal/4096 617.7 ns 485.3 ns -21.4%
🟢 matmul/medium_generic_bf16_threads=8/32x512x512 483.94 µs 375.81 µs -22.3%
🟢 matmul/small_generic_f16_threads=8/1x256x256 40.23 µs 30.75 µs -23.6%
🟢 qwen3_sampling_processors/top_p_full_sort_after_top_k_baseline 5.61 ms 4.26 ms -24.0%
🟢 block_quantized_matmul_cached_dense/mxfp4_preexpanded_dense_oncelock_like_proxy/1x1024x1024 60.59 µs 44.54 µs -26.5%
🟢 reduce_mean/medium_f32_threads=1-internal/65536 338.67 µs 248.94 µs -26.5%
🟢 block_quantized_matmul_cached_dense/mxfp4_uncached_dequant_each_call/1x1024x1024 740.94 µs 535.70 µs -27.7%
🟢 reduce_mean/small_f32_threads=1-internal/4096 20.68 µs 14.85 µs -28.2%
🟢 matmul/small_generic_f32_threads=1/1x256x256 53.78 µs 37.73 µs -29.8%
🟢 block_quantized_moe_cached_dense/mxfp4_uncached_expert_dequant_each_call/rows=1,H=256,I=256,E=4,top_k=1 576.57 µs 379.58 µs -34.2%
🟢 matmul/small_generic_f32_threads=8/1x256x256 56.74 µs 35.11 µs -38.1%
🟢 gather/large_f16_threads=1-internal/131072 20.90 µs 11.98 µs -42.7%
🟢 matmul/medium_generic_f32_threads=8/32x512x512 1.59 ms 895.85 µs -43.7%
🟢 gather/medium_f32_threads=1-internal/32768 6.76 µs 3.74 µs -44.7%
🟢 block_quantized_matmul_cached_dense/mxfp4_cached_dense_repeated_call/1x1024x1024 82.96 µs 44.92 µs -45.9%
🟢 gather/large_bf16_threads=1-internal/131072 27.77 µs 12.00 µs -56.8%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.5.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 3.71 3.72 6.16 }
What this cannot catch
  • Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
  • Sub-threshold regressions that compound over multiple PRs
  • Performance changes that only manifest under GPU execution
  • Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)

justinchuby added a commit that referenced this pull request Aug 12, 2026
…m fixes

Syncs with main (11 commits) so CI's automatic PR-merge preview builds
against current upstream, and pulls in the test_support cfg-gating bug
(#807) that this branch needs to fix forward (see follow-up commit).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 12, 2026
## What

Documentation-only sync of five memory/KV design docs to the merged,
executable-test-backed facts about VMM commit geometry, KV layout,
weight offload, CUDA-graph capture, and plugin export. No code or tests
changed.

`MEMORY_ARCHITECTURE.md` is treated as the single source for KV geometry
and the audit table; the other four link into it rather than restating
(per #791).

## Most misleading claim found

`MEMORY_ARCHITECTURE.md` — the document explicitly designated the single
source of truth — still asserted the engine *"explicitly invalidates the
graph on every KV bucket growth for both layouts"* and that *"the engine
does not yet use that fact to retain the captured graph,"* with a table
showing seq-major at `captures=4, invalidations=4`. That was falsified
by **#811**: conditional invalidation has landed, seq-major fixed-stride
growth now **keeps** the captured graph (growth-attributable
invalidations **3 → 0**, `captures=1/invalidations=1`, matching the
no-growth control), verified by
`mobius_seqmajor_growth_parity_native_cuda.rs` including a negative
oracle. Because this doc is the single source others link to, the stale
claim had the widest blast radius.

## Changes

- **MEMORY_ARCHITECTURE.md** — correct the #811 conditional-invalidation
prose + table (verified against `cuda.rs`
`binding_growth_signature`/`record_growth_decision` and the parity
test); add the ORT-backend KV-routing + #762 plugin-export note
(verified `allocator.rs:232` `for_session_device` selects by device, not
implementation); thread #822 (granule queried), #812 (engine-side
consumer gate), and a #800/#802 record correcting this doc's earlier
fp16 sizing of Phase-2a scores (verified `attention.rs:368` uses
`dtype.element_size()`).
- **CUDA_GRAPH_CAPTURE.md** — replace the mutual-exclusion result with
#796 (stable VA removes it), #798 (managed no-spill default), #811
(conditional invalidation), and the #804/#807 `captures=0`
process-artifact correction.
- **WEIGHT_OFFLOAD.md** — correct the "mutually exclusive today" claim
(#796) and the north-star status for the managed no-spill default
(#798).
- **KV_INSERTION_DESIGN.md** (most stale, 2026-07-15) — add a
synchronization banner linking to `MEMORY_ARCHITECTURE.md`; note the
layout line post-dates the memo, prefix sharing is a landed primitive
(#777), and block-table paged attention remains a proposal (verified: no
`block_table` in any CUDA attention kernel).
- **DESIGN.md** — forward-pointer in the already-superseded §39.5 noting
layout (seq-major/token-major, #787/#792) is now the primary residency
lever.

## Method

Every retained performance claim cites the PR that measured it; measured
fact / design intent / not-yet-built are distinguished; superseded
designs are labelled, not deleted. The "still open" item (engine commits
flat buckets, not the dense live-prefix ranges `kv_commit.rs` computes,
so the `layers×2` floor is not yet realised through the engine) is
described as in-flight, not delivered.

## Note on #762

The task framing had #762 (EP plugin export) as open; `gh pr view 762`
shows it **merged** today. It is described accurately per that ground
truth: the plugin-EP mechanism landed for the CPU EP (validated
end-to-end), while the CUDA plugin shim compiles but is unvalidated on
hardware — so plugin-EP-hosted VMM KV remains in-progress design intent,
not a shipped capability.

## Validation

Documentation only — `git diff --stat` touches exactly the five `.md`
files; working tree otherwise clean. **Not merged; no issue closed.**

Refs #736, #762, #776, #777, #783, #787, #791, #792, #794, #796, #797,
#798, #801, #802, #804, #807, #811, #812, #822

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 12, 2026
…-up to #762's CI fix) (#818)

## Context

PR #762 merged into `main` at `e9c0ab6a4` and already carries the fix
for the
CI-wide compile break introduced by #807:
`crates/onnx-runtime-cuda-memory/src/lib.rs`
gates `pub mod test_support;` on `#[cfg(feature = "gpu-tests")]` only
(previously
`#[cfg(any(test, feature = "gpu-tests"))]`), and the one integration
test that
uses `TestStream` (`tests/vmm_kv_layout_residency_gpu.rs`) cfg-gates its
import
and body accordingly.

**Root cause recap:** `cfg(test)` is only set when a crate compiles as
its own
unit-test binary. Integration tests under `tests/*.rs` link the crate as
a
regular (non-`--test`) dependency, so `cfg(test)` never applies to them.
The
old gate `any(test, feature = "gpu-tests")` therefore never actually
exposed
`test_support` to `tests/vmm_kv_layout_residency_gpu.rs` unless
`gpu-tests` was
passed explicitly — which is exactly what broke CI's honesty-checker
base
(cuda-only, no `gpu-tests`) build for 7 of 9 failed jobs on #762's own
CI run.

## What this PR does

Since the functional fix already reached `main` via #762, **this PR is a
minimal, standalone doc correction**: `test_support.rs`'s module doc
still said
the module was compiled "behind the `gpu-tests` feature (and the crate's
own
`cfg(test)`)" — stale wording that contradicts the actual (correct) gate
in
`lib.rs` and could mislead a future reader into re-introducing the same
bug by
assuming `cfg(test)` alone helps here. Updated the doc to explicitly
state the
gate is `gpu-tests`-only and explain why `cfg(test)` doesn't apply to
integration tests.

**No functional/behavioral change.** No `.squad/` state included.

## Delta vs. Opus-approved fix (afbd2c1 on #762)

`git diff afbd2c1 origin/main --
crates/onnx-runtime-cuda-memory/src/lib.rs
crates/onnx-runtime-cuda-memory/tests/vmm_kv_layout_residency_gpu.rs`
is **empty** — main already has byte-identical content for both files
via the
#762 merge. The only delta introduced by this branch beyond what Opus
already
reviewed is the `test_support.rs` doc-comment correction (7 insertions /
3
deletions), which was never part of the reviewed diff. Flagging this so
a tiny
follow-up look is optional but available before merge.

## Validation (local, CPU-only host, matching CI's commands)

- `cargo test --locked -p onnx-runtime-cuda-memory --features cuda
--tests --no-run` — compiles clean (base/no-gpu-tests config)
- `cargo test --locked -p onnx-runtime-cuda-memory --features
cuda,gpu-tests --tests --no-run` — compiles clean (gpu-tests config)
- `python .github/scripts/verify_cuda_test_honesty.py` — passes: 416
tests/66 targets identical in both configs (416 ignored without
gpu-tests; 412 fail-loud/4 ignored/0 passed with gpu-tests on this
no-CUDA host)
- `cargo clippy --locked -p onnx-runtime-cuda-memory --features cuda
--all-targets -- -D warnings` — clean
- `cargo clippy --locked -p onnx-runtime-cuda-memory --features
cuda,gpu-tests --all-targets -- -D warnings` — clean
- `cargo check --locked -p onnx-runtime-ep-cuda -p onnx-runtime-python
--features onnx-runtime-python/cuda` — clean
- `cargo test --locked -p onnx-runtime-ep-cuda --features cuda --test
capture_sync_contract` — 1 passed
- `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings`
— clean
- `cargo fmt --all -- --check` — clean

## Notes

- #762 (branch `squad/ep-plugin-parity-cuda`) temporarily carried this
same
cfg-gating fix directly on its own branch (to unblock its own CI) and
has
now merged into `main`, so `main`'s CI should already be green going
forward.
  This standalone branch exists per explicit request so the repo-wide CI
  unblock has its own paper trail, separate from #762's larger diff.
- Kept scope minimal: no CI workflow changes, no other files touched.
- Leaving as **Draft** until CI is confirmed green on this PR's own run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 12, 2026
…ative) + #384 metadata generator (#834)

## Summary

Demonstrates the KV-cache committed-**physical**-bytes floor end-to-end
on **qwen14b** via the native CUDA decode path, comparing **head-major
(BNSH)** vs **seq-major (BSNH)** layouts in the same session, swept
across context capacities that bracket the 2 MiB-per-stripe granule
threshold (#776). Token IDs are **byte-identical** between layouts at
every point.

This is the first time this line (chased since #797) has been driven
through the *engine* on a model whose head stripe actually reaches a
full granule (qwen14b @ 8192 ctx: `8192 x 128 x 2 = 2,097,152 B =
exactly 2 MiB`), rather than the sub-granule qwen0.5b of #827.

## Headline result — honest negative (per #794 / #812 / #827 precedent)

Head-major and seq-major commit **byte-identical** physical KV bytes at
every swept capacity:

| KV capacity (tokens) | committed bytes (both layouts) | granules |
ratio seq/head |
|---:|---:|---:|---:|
| 1024 | 402,653,184 | 192 | 1.00x |
| 2048 | 603,979,776 | 288 | 1.00x |
| 4096 | 1,006,632,960 | 480 | 1.00x |
| 8192 | 1,811,939,328 | 864 | 1.00x |

Expected-if-it-separated: head-major `layers x 2 x kv_heads` = **768
granules (~1.5 GiB)** vs seq-major `layers x 2` = **96 granules (~192
MiB)** — the 8x that never appeared.

## Why it does not separate

`growth_events == 0` and `committed_len == capacity` for **both**
layouts: the engine commits the full KV bucket **eagerly** on
reservation — reservation *is* the commit, so layout cannot change the
total. Seq-major's fixed full-context stride *is* active and correct
(`max_len == 8192` at every capacity vs head-major `max_len ==
capacity`, tokens match), which proves the seq-major kernel/layout
resolution works — but a fixed stride only makes **growth** free (#797's
0 bytes moved) and preserves the captured graph (#811/#812); it does not
shrink an eagerly-committed full bucket.

The 768-vs-96 (8x) floor stays a **driver-level** property
(`vmm_kv_layout_residency_gpu`) realized only by a **live-prefix
commit-on-demand** path, which the engine does not do on initial
reservation. The new guardrail assertions (`growth_events==0`,
`committed_len==capacity`, `head==seq committed`) will **turn red** the
moment a future change makes seq-major commit less — i.e. they are a
tripwire for the desired win.

Measurement used the in-place **VMM** allocator
(`ONNX_GENAI_CUDA_VMM=1`), which reports granule-rounded **physical**
bytes; the legacy/governor allocator reports nominal bytes and was
deliberately avoided. Capture was declined throughout
(`ONNX_GENAI_CUDA_GRAPH=0`); offload/weight-streaming (16.6 GB model vs
7.7 GB budget, #796) was active and did not perturb the KV commit
counters.

## Deliverables

1.
**`crates/onnx-genai-engine/tests/qwen14b_kv_floor_sweep_native_cuda.rs`**
— child-process-isolated sweep harness (mirrors
`mobius_seqmajor_growth_parity_native_cuda`), honoring process-frozen
config (#804/#807). Self-execs per `(layout, capacity)`; asserts token
parity + layout resolution + the deterministic guardrails above.
`#[ignore]`, gated on `cuda,native-backend`.
2. **`scripts/gen_inference_metadata.py`** (#384) — emits
`inference_metadata.yaml` from `genai_config.json` for any such export.
Reproduces the hand-written qwen14b metadata **byte-for-byte**. Turns
the one-off local `qwen14b-zp` fix (which unblocked native loading for
this whole measurement) into a reproducible artifact; the model file
itself stays local (models live outside the repo).
3. **Docs** — `MEMORY_ARCHITECTURE.md` floor-progression table +
eager-full-bucket explanation; `MODEL_METADATA.md` #384 generator
section.

## Method notes

- Seq-major was obtained by patching a **copy** of the model
(`kv_layout=1` on all 48 GQA nodes), as #801/#805 did for mobius — the
original model directory was never modified.
- Reported committed **physical** bytes throughout, never nominal
content bytes.
- Deterministic counters lead; wall-clock is omitted (this box's
identical runs ranged 3.9-28 tok/s) and no number was extrapolated that
was not measured.

## Unrelated pre-existing fix (disclosed)

`crates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs`: added a
`#[cfg(not(target_arch = "aarch64"))] let _ = dot_kernel;` guard in
`borrowed_affine_int4_matmul` to silence a pre-existing x86
unused-variable lint that blocked `-D warnings` (matches sibling
functions at 3779/4272/4448). Behavior-neutral; the mobius ignored test
still passes (2 passed).

## Validation

- `cargo fmt -p onnx-genai-engine -p onnx-runtime-ep-cpu -- --check`
(per-package; `--all` hits Windows `os error 206`)
- `cargo clippy --locked --all-targets -p onnx-genai-engine --features
native-backend -- -D warnings` — pass
- `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings`
— pass
- `cargo test -p onnx-runtime-session` (full) — pass
- `cargo test -p onnx-genai-engine --features cuda,native-backend --test
mobius_seqmajor_growth_parity_native_cuda -- --ignored --test-threads=1`
(#814 policy) — 2 passed
- New sweep harness run twice; all guardrails passed.

Refs #797 #787 #827 #384 #777. Do **not** merge; does **not** close any
issue.

---------

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 12, 2026
…8x separation does NOT appear (mechanism identified) (#841)

## The question

Since #797 the line of work has chased: does the **seq-major (BSNH)** KV
layout commit **~8× fewer** physical VMM granules than **head-major
(BNSH)** on a real model? #834 reported "byte-identical, engine commits
the full bucket eagerly" — but that was a **measurement artifact**: it
pinned `ONNX_GENAI_KV_MIN_BUCKET == capacity`, and since
`kv_capacity_bucket(len) =
len.next_power_of_two().max(min_bucket).min(hard_max)`, that *forces*
`initial_bucket_len == capacity ⇒ committed_len == capacity` and
`growth_events == 0`, disabling the very on-demand-commit mechanism that
could separate the layouts. It then "discovered" the flat commit it had
itself forced.

This PR re-runs the experiment with the mechanism **enabled**
(`min_bucket = 256` default), asserting **token-ID parity**, each
`(layout, config)` in its **own child process** (config is
process-frozen on first read, #804/#807), under the **in-place VMM
allocator** (`ONNX_GENAI_CUDA_VMM=1`, granule-accurate committed bytes —
never nominal).

## Answer: NO — the 8× separation does not appear (and, from the code,
never will on the engine's real commit path)

Measured on this box (RTX 4060 Laptop, 8 GiB; qwen14b: 48 layers,
kv_heads=8, head_dim=128, fp16, 96 KV bindings; granule 2 MiB),
**on-demand commit ACTIVE** (`growth_events ≥ 1`):

| prefix | bucket | head-major committed | seq-major committed | ratio |
tokens |

|-------:|-------:|---------------------:|--------------------:|:-----:|:------:|
| 422 | 512 | 402,653,184 B | 402,653,184 B | **1.00×** | identical |
| 547 | 1024 | 402,653,184 B | 402,653,184 B | **1.00×** | identical |

Byte-identical committed physical KV at every shared prefix, with growth
firing (directly refuting #834's "eager full bucket"). seq-major
reserves the full fixed 8192 stride (nominal per-binding 16 MiB) yet
commits the **same granules** as head-major's packed 512/1024 bucket.

## Mechanism (from the commit path + counters, not a knob)

- **head-major** grows a **packed bucket**: `vmm_growth_requests`
commits the flat range `0..(bucket × kv_heads × head_dim × elem)` with
per-head stride = the *current bucket* (`new_shape[2] = new_capacity`),
and **re-strides + re-captures** on growth (`kv_bytes_moved =
100,667,392 B` and `invalidations` at the 512→1024 step).
- **seq-major** commits the same dense prefix `0..(committed_len ×
kv_heads × head_dim × elem)` on a **fixed 8192 stride**, moving **0
bytes** and **keeping** its captured graph (`growth_keeps`).
- Because `kv_capacity_bucket(required)` is identical for both, both
land the **same dense-from-zero granules** ⇒ byte-identical committed
KV.

The 8× floor of `kv_commit::live_prefix_committed_bytes` is a property
of a **fixed full-context-stride head-major** layout (each head stripe
`max_len × head_dim × elem` apart, short prefix ⇒ 1 granule scattered
per head). The engine's head-major path **deliberately never
instantiates** that — it re-packs its growing bucket dense. Head-major
therefore sits on its **dense floor (192 granules) = 0.25× of the 8×
scatter floor (768 granules)** at every reachable bucket:

| bucket | head-major committed | 8× fixed-stride floor | dense/floor |
|-------:|---------------------:|----------------------:|:-----------:|
| 512 | 402,653,184 B (192 gr) | 1,610,612,736 B (768 gr) | 0.25× |
| 1024 | 402,653,184 B (192 gr) | 1,610,612,736 B (768 gr) | 0.25× |

## The 8192 crossover regime is physically unreachable on this box
(three hard constraints, all reported)

1. **seq-major prefill > 512 tokens is unconverted** —
`selected_backend_for_shape` only auto-selects the fused/flash prefill
path at `valid_sequence_length ≤ 512`; longer prompts fall to Phase2a
unfused-decode-prep, which `require_converted_path_support` rejects for
BSNH (hard `KernelFailed`).
2. **head-major prefill > ~600 tokens OOMs the attention workspace** — 8
GiB card, 16.6 GB weights lease ~7.5 GB, leaving no workspace headroom;
bucket 2048 (~2000-token prompt) already fails with `cuMemMap: growing
physical handle pool lease failed`.
3. **decode is weight-offload bound (~0.25 tok/s)** — reaching an 8k
prefix by decode is infeasible.

So the max bucket reachable by **both** layouts is ~1024, exactly where
they're byte-identical. The crossover (bucket 8192, where per-head
stripe = one granule and dense == scatter) is left as a symbolic result
(already proved by `kv_commit.rs`'s unit tests), not a hardware run.

## Guardrails added

- **Byte-equality guard** (`head == seq`) — if seq-major ever commits
strictly *less*, it fails loudly with instructions to record the win in
`MEMORY_ARCHITECTURE.md`.
- **Mechanism guards** — `seq.kv_bytes_moved == 0`, `seq.growth_keeps ≥
1`, `seq.max_len ≥ 8192`, `head.max_len == growing bucket`; if a
regression flips head-major to a fixed stride (which *would* separate
the floor) these break.
- **Instrument-integrity guards** — `logical_len > min_bucket`,
`growth_events ≥ 1`, `committed_bucket > min_bucket` so a future
re-pinning of `min_bucket` can't silently re-disable the mechanism.
- **Dense-floor guard** — head-major committed `<` the 8× fixed-stride
floor at every reachable bucket.

## Validation (local, per the brief — CI not waited on)

- `cargo fmt -p onnx-genai-engine -- --check` ✓
- `cargo clippy --locked --all-targets -p onnx-genai-engine --features
native-backend -- -D warnings` ✓
- `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings`
✓
- `cargo clippy --all-targets -p onnx-genai-engine --features
cuda,native-backend -- -D warnings` (self-lint of the cuda-gated test) ✓
- **MANDATORY** `cargo test -p onnx-genai-engine --features
cuda,native-backend --test mobius_seqmajor_growth_parity_native_cuda --
--ignored --test-threads=1` ✓ (2 passed)
- Full harness `qwen14b_kv_longprefix_native_cuda -- --ignored
--test-threads=1` ✓ (passed, table above)

No assertions inside `Drop`. New test is `#[ignore]`d (requires the
qwen14b head + `kv_layout=1` exports and a CUDA device).

## Bottom line

The 8× floor separation is a **fixed-stride head-major** property the
engine never instantiates; #834's "eager full bucket" was a `min_bucket
== capacity` artifact. With the mechanism enabled, on-demand commit
**is** active, yet head-major and seq-major commit **byte-identical**
physical KV because head-major packs its growing bucket dense — and the
long-context regime where the geometry *could* diverge is unreachable on
this hardware. **Negative result, real mechanism identified from
counters + code.**

Part of the #797 / #834 line. Does not close any issue; opened for
review, not merge.

Co-authored-by: justinchuby <223556219+Copilot@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