Repository navigation
fix(cuda): preserve the attention-mask prefix when a KV bucket grows - #1228
Merged
Merged
Conversation
Keep the valid attention-mask prefix when a VMM-backed CUDA KV bucket grows instead of clearing the full mask and resetting the derived past length. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1228 +/- ##
=========================================
+ Coverage 0 80.49% +80.49%
=========================================
Files 0 359 +359
Lines 0 157994 +157994
Branches 0 157994 +157994
=========================================
+ Hits 0 127184 +127184
- Misses 0 26194 +26194
- Partials 0 4616 +4616
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
justinchuby
added a commit
that referenced
this pull request
Aug 18, 2026
…red native-CUDA parity test) (#1264) ## What this fixes `main` currently has a **red end-to-end native-CUDA parity test**: `native_cuda_pipeline_decoder_matches_cpu_token_ids` fails during `DecodeCudaState::new()` with `full CUDA mask reservation size overflow`. ### Root cause `DecodeCudaState::new` computed the mask *reservation* extent unconditionally: ```rust let full_mask_bytes = batch .checked_mul(capacity.max_len) // <-- capacity.max_len == usize::MAX .and_then(|e| e.checked_mul(size_of::<i64>())) .context("full CUDA mask reservation size overflow")?; ``` For a model with **no `max_sequence_length` metadata**, `resolve_cuda_kv_capacity` returns `capacity.max_len == usize::MAX` — the deliberate *"unbounded until growth fails"* sentinel (added in #367, asserted by `cuda_kv_capacity_without_metadata_is_unbounded_until_growth_fails`). So the multiply overflows and aborts construction — **even on the non-VMM decode path, whose mask binding is a plain `[batch, max_len]` allocation that never reserves the full logical extent and never reads `full_mask_bytes` at all.** The `tiny-gemma4-vlm-cuda` fixture used by the parity test is exactly such a metadata-less model, which is why the test is red. ### First bad commit Bisected to **#682 (`9fd7ee56`, "Reserve CUDA KV with VMM committed ranges")**, which introduced the unconditional `full_mask_bytes`. The `usize::MAX` sentinel predates it (#367). My earlier suspects **#1221** (two-pass workspace reservation) and **#1228** (KV mask growth) were both **cleared with evidence** — they are much later and touch different code (#1228 only added the `grow_vmm_mask_in_place` shape assertion). ### The fix Compute `full_mask_bytes` (and the `mask_committed_bytes` it feeds) **only inside the `kv_commits_on_demand` (VMM) branch that actually consumes them.** The `usize::MAX` sentinel is intended design and is left untouched. Behaviour is **byte-identical on every previously-working path**: the non-VMM branch always committed `mask_bytes`, and `seq_major_fixed` implies `kv_commits_on_demand`, so `full_mask_bytes` was only ever live on the VMM path. ### Captured operands at the failure (RTX 4060, non-VMM path) ``` batch = 1 capacity.max_len = 18446744073709551615 (= usize::MAX) source = "unbounded (model.max_sequence_length unavailable)" bytes_per_token = 40 kv_commits_on_demand = false (non-VMM path -- full_mask_bytes is dead here) ``` ## Verification (RTX 4060, driver 591.55, CUDA 13 runtime) - **Regression test** `native_cuda_decoder_constructs_with_unbounded_metadata_less_capacity` (CUDA-gated, `ONNX_GENAI_RUN_CUDA_SMOKE=1`): constructs a metadata-less non-VMM decoder and asserts `new()` succeeds + `capacity.max_len == usize::MAX`. - **GREEN** with the fix. - **RED** with the exact `full CUDA mask reservation size overflow` when the computation is hoisted back out of the branch (falsified by revert). - **Parity test** `native_cuda_pipeline_decoder_matches_cpu_token_ids`: **RED on `main` → GREEN with this fix.** - Engine lib suite (`--features "cuda,native-backend" -- --test-threads=1`): **548 passed, 0 failed, 2 ignored** (default). ## Scope note — the VMM half is NOT fixed here This fix makes only the **non-VMM** path correct. On the **VMM** path (`ONNX_GENAI_CUDA_VMM=1`) a metadata-less model would still overflow for real, because `full_mask_bytes` — and `full_vmm_kv_allocation_bytes(..., capacity.max_len)` — become *live* values that genuinely need a concrete bound. Resolving the unbounded sentinel to a real capacity before the VMM path can reserve is a design decision, not an obvious mechanical fix, so it is **filed as a separate issue** rather than papered over here (see linked issue). ## Not covered by this PR Running the lib suite with `ONNX_GENAI_RUN_CUDA_SMOKE=1` surfaces **3 pre-existing failures** (`native_cuda_capture_replay_...`, `native_cuda_symbolic_batch_...`, `native_cuda_symbolic_total_seq_...`) that fail with `cannot resolve model.io.token_input ... 3 ports match`. These are a pre-existing test-harness `model.io` ambiguity unrelated to this change (this PR does not touch graph building or io resolution); they only surface because CI never sets the smoke env. Co-authored-by: justinchuby <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.
Unstacked, rebased replacement for #1163 (
split/native-kv-growth-mask). Same one-file change, +20/-14.What it changes
When a VMM-backed KV bucket grows, the attention-mask binding used to be cleared in full:
The mask is not rewritten from scratch each step --
extend_mask(start, end, expose_len)writes only the incremental[start, end)window of1s. So a full clear on growth erases every prior1, and anything that reads the mask to derive a past length sees zero. This replaces the blanket memset with a prefix-preserving copy plus a suffix clear, reusing the samecopy_kv_prefix_device_to_device_in_place/zero_kv_suffix_devicehelpers the KV bindings already use, and adds a shape guard that the binding really is[batch, max_len]before growth.Honest reporting: the claimed symptom did not reproduce
The original PR asserted that GQA derives a zero past length and produces silently wrong output. I could not reproduce that, and I would rather say so than merge on an unverified headline.
RTX 4060,
qwen05b-q4, forced growth viaONNX_GENAI_KV_MIN_BUCKET=8 ONNX_GENAI_CUDA_VMM=1 ONNX_GENAI_CUDA_GRAPH=0, 120 greedy tokens. Three genuine growth events on both builds:The generated text is character-identical with and without the fix, including across the
valid_len=64andvalid_len=128growths where a wiped prefix would have to bite. (I first got a false "verified" here twice: once because the worktree was rebased on a stalemainthat predated #1221, and once because aStart-Jobbuild died with its parent shell so I re-ran the old binary. Both are recorded so the next person does not repeat them.)So on this model the mask is evidently not the operand that carries past length, and the blanket memset is inert. That does not make the old code right -- preserving the valid prefix is unambiguously correct and clearing it is unambiguously wrong for any consumer that does read it. I am merging it as a correctness fix whose cost is one extra device-to-device copy on a rare event, not as a fix for a symptom I have seen.
Follow-up
Filed separately: find a model whose attention actually reads this mask, and pin the behaviour with a regression test. Until then this change is correct-by-construction but untested end to end.
Validation
cargo build -p onnx-genai-cli --features native-cuda: cleanmain(post-Fix native CUDA decode Attention workspace preparation (#1179) #1221), no conflictsSupersedes #1163.