fix: harden sliding-window over-window prefill across models#412
Merged
Conversation
A single-pass prefill longer than a model's sliding window degenerated to NaN or `<pad>` (or crashed on a mask/key shape mismatch) on every sliding-window model except gemma3/gemma4. When `seq_len > window` a fresh RotatingKVCache (and a dense KVCache) keep ALL prefill keys, but the shared `causal_attention` wrapper and the per-model prefill mask builders capped the windowed mask to `[seq_len, window]` and sliced K/V to the trailing `window` keys. That strands the earliest query rows (logical position `< seq_len - window`) with an all-`-inf` mask row that softmaxes to NaN; models that pass full K/V with a capped mask hit a hard shape mismatch instead. PR #405 fixed only gemma3/gemma4. General fix in `causal_attention` (src/lib/mlxcel-core/src/lib.rs): for a multi-token prefill that exceeds the window (`q_len > 1 && k_len > window`), build the full-width windowed-causal mask over ALL keys via `create_causal_mask_with_window_full` instead of slicing K/V. The single-query decode path (`q_len == 1`) keeps the trailing-window K/V slice byte-for-byte; non-windowed callers (`window_size == 0`) and within-window prefills are untouched. Shared helper `create_sliding_window_prefill_mask` (src/lib/mlxcel-core/src/utils.rs) selects the uncapped full mask for a fresh over-window prefill (`size > window && sliding_offset == 0`, the gemma3 prior-art gate) and the clamped mask otherwise, byte-identical to the per-model `sliding_offset.min((window - size).max(0))` construction it replaces. Wired into the prefill mask builders for gpt_oss, mellum, exaone4, exaone_moe, ministral3, step3p5 (RotatingKVCache, full K/V), and cohere2, gemma3n (dense KVCache with a model-level K/V slice, now sliced to the mask's key axis instead of blindly to `window` so a full mask keeps all keys). baichuan reaches the shared `causal_attention` prefill path directly, so the general fix covers it. recurrent_gemma (full causal mask, no window cap) and diffusion_gemma (already uses an uncapped dense windowed mask) are unaffected; gemma3/gemma4 keep their #405 helpers. Validated on M1 Ultra at --temp 0: gpt-oss-20b (window 128) with a ~260-token prompt now answers coherently where it previously broke; gemma-4-12b (window 1024) with a ~1600-token prompt stays coherent (#405 not regressed); short prompts and decode on gemma-4-12b, gpt-oss-20b, and non-windowed qwen3-0.6b are unchanged. Tests cover the `create_sliding_window_prefill_mask` selector and a `q_len > 1, k_len > window` `causal_attention` regression asserting a finite full-mask result.
Member
Author
Implementation Review SummaryIntent
Findings Addressed
Verification
Review notes (verified correct, no change needed)
Out of scope (pre-existing, flagged for a separate chore)
|
olmo3 is a registered sliding-window model on a dense KVCache that was missed in the initial #408 audit. It built the clamped create_causal_mask_with_window and sliced K/V to the trailing window_size, so a fresh single-pass prefill longer than sliding_window (4096) stranded the earliest query rows with an all-(-inf) mask row -> NaN/<pad>. It is not covered by the general causal_attention fix because its windowed prefill goes through the explicit-mask branch. Mirror the cohere2/gemma3n dense-cache fix: build via create_sliding_window_prefill_mask and slice K/V to the mask key axis (mask_klen) instead of window_size, so a full mask keeps every key and a clamped mask drops the oldest. Correct the helper Used-by list (add Olmo3; remove Baichuan, which relies on the general causal_attention path).
4 tasks
… hardening The tensor-parallel runtime re-implements gemma3 sliding-window mask construction (gemma3_masks) and still used the clamped create_causal_mask_with_window, so a fresh single-pass prefill longer than the sliding window crashed with a [broadcast_shapes] mismatch (the same DoS the single-process fix removed for gpt-oss): gemma3's RotatingKVCache returns all keys and the consumer passes the mask straight to attention_from_ptr with no trim_mask_to_keys fallback. Tensor-parallel gemma4 was already fixed incidentally, since its attend() trims and falls back to the patched causal_attention. Swap gemma3_masks onto the shared create_sliding_window_prefill_mask, mirroring the single-process gemma3 sliding_prefill_mask: full windowed mask for a fresh over-window prefill, clamped mask otherwise (byte-identical to the prior effective_offset construction for every working case). tensor_parallel_gemma3_matches_full_model_logits passes.
Merged
4 tasks
inureyes
added a commit
that referenced
this pull request
Jun 24, 2026
…ll retained keys (#413) (#418) * fix(models): full windowed mask for dense over-window prefill (#413) Dense-`KVCache` sliding-window models (cohere2, gemma3n, olmo3) retain every key and slice K/V to the mask's key axis, but they routed mask construction through `create_sliding_window_prefill_mask`, whose full-mask gate is `size > window && sliding_offset == 0`. A rolled-over or chunked over-window prefill (`sliding_offset > 0 && size > window`) therefore fell to the clamped `[size, window]` mask. The clamped `tril` diagonal `window - size < 0` strands the earliest query rows (logical position `< size - window`) with an all-`-inf` row, which softmaxes to NaN and decodes to `<pad>`. This is a latent degeneration, not a regression from #412/#408: the behavior is byte-identical to the pre-#408 path. `RotatingKVCache` models (gpt_oss, mellum, exaone4, exaone_moe, ministral3, step3p5, gemma3, gemma4) and the gemma3 tensor-parallel path legitimately NEED the clamped path once the cache has rolled over, because the rotating cache then returns at most `window` keys. So the shared helper must not change behavior for them. Add a sibling helper `create_sliding_window_prefill_mask_dense` that builds the full `[size, size + sliding_offset]` windowed-causal mask for any `size > window` (at any offset), keeping the clamped path only for within-window prefills. For `size <= window` it is byte-identical to the rotating helper (same clamped builder and offset clamp), so within-window greedy output is unchanged. Reroute the five dense call sites (cohere2 x2, gemma3n x2, olmo3 x1) to the dense helper and leave the rotating-cache call sites untouched. Add three mask-layer regression tests: the in-scope offset>0 over-window fix (full mask, no all-`-inf` row), parity with the rotating helper for a fresh over-window prefill, and byte-identical output for within-window prefills including the within-window rolled-over clamped path. * fix(models): dense prefill always uses full windowed mask (#413) Generalize the dense-cache fix to cover the adjacent latent case in the same change. A dense `KVCache` retains every key and the consumer slices K/V to the mask's key axis, so the correct prefill mask is ALWAYS the full `[size, size + sliding_offset]` windowed-causal mask over all retained keys, for every size/offset combination, not only `size > window`. This drops the `if size > window` branch from `create_sliding_window_prefill_mask_dense`; it now unconditionally calls `create_causal_mask_with_window_full`. For `size + sliding_offset <= window` (the common within-window prefill) this is byte-identical to the rotating helper: `create_causal_mask_with_window` takes its non-capped path there and produces the same `tril(offset)` intersect `triu(offset - window + 1)` band over `[size, size + sliding_offset]`, so greedy output is unchanged. The unconditional full mask fixes both dense-cache degeneration cases the clamped path caused once the total exceeded the window: (1) `size > window` (any offset) where the clamped `[size, window]` mask stranded the earliest query rows with an all-`-inf` row (NaN then `<pad>`), and (2) `size <= window && size + sliding_offset > window` where the clamped path's trailing-window K/V slice dropped the OLDEST in-window keys for the earliest query rows (coherent-but-wrong output, not NaN). RotatingKVCache models (gpt_oss, mellum, exaone4, exaone_moe, ministral3, step3p5, gemma3, gemma4) and the gemma3 tensor-parallel path keep `create_sliding_window_prefill_mask` and its clamped path, because a rotating cache physically returns at most `window` keys once rolled over. Tests: change `sliding_window_prefill_mask_dense_within_window_byte_identical` to within-total inputs only (`(3, 0, 8)` and `(3, 2, 8)`), since `(2, 9, 4)` is now the latent-fix case. Add `sliding_window_prefill_mask_dense_full_when_within_window_over_total` asserting the full `[2, 11]` mask (not clamped `[2, 4]`) and that row 0 (logical position 9) attends to its oldest in-window key (column 6) that the clamped path dropped. Update the five dense call-site comments to describe the unconditional full mask.
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
A single-pass prefill longer than a model's sliding window degenerated to NaN /
<pad>(or hard-crashed on a mask/key shape mismatch) on every sliding-window model except gemma3/gemma4. Whenseq_len > window, a freshRotatingKVCache(and a denseKVCache) keep ALL prefill keys, but the sharedcausal_attentionwrapper and the per-model prefill mask builders capped the windowed mask to[seq_len, window]and sliced K/V to the trailingwindowkeys. That strands the earliest query rows (logical position< seq_len - window) with an all--infmask row that softmaxes to NaN; models that pass full K/V with a capped mask hit a hard[broadcast_shapes]crash instead. PR #405 fixed only gemma3/gemma4.Chosen approach: general fix + shared helper (no loud-guard fallback needed)
The general
causal_attentionfix landed cleanly and is bit-exact for every currently-correct path, so the loud-guard fallback was not required.General fix in
causal_attention(src/lib/mlxcel-core/src/lib.rs): for a multi-token prefill that exceeds the window (q_len > 1 && k_len > window), build the full-width windowed-causal mask over ALL keys viacreate_causal_mask_with_window_fullinstead of slicing K/V. The single-query decode path (q_len == 1) keeps the trailing-window K/V slice byte-for-byte; non-windowed callers (window_size == 0) and within-window prefills are untouched. This is the shared safety net and directly fixes baichuan (which reaches this path withmask.is_none()), and incidentally also fixes the tensor-parallel gemma4 path (which routes throughcausal_attention).Shared helper
create_sliding_window_prefill_mask(size, sliding_offset, window)(src/lib/mlxcel-core/src/utils.rs): selects the uncapped full mask for a fresh over-window prefill (size > window && sliding_offset == 0, the gemma3 prior-art gate) and the clamped mask otherwise. Its else-branch is byte-identical to the per-modelsliding_offset.min((window - size).max(0))construction it replaces (the capped tril offsetwindow - sizeis offset-independent, and the helper's non-capped triu is a no-op forsize <= window), so greedy temp-0 output is unchanged for every non-degenerate case.Per-model wiring of the helper into the prefill mask builders (see audit table). gemma3/gemma4 keep their fix(models): correct gemma sliding-window prefill beyond the window #405
sliding_prefill_maskhelpers as-is; refactor: hoist duplicated sliding_prefill_mask helper from gemma3/gemma4 to a shared module #410 will consolidate them onto the shared helper.Tensor-parallel gemma3 path (
src/distributed/tensor_parallel/llama_runtime.rs,gemma3_masks): the TP path independently computed its own clamped offset and calledcreate_causal_mask_with_window, so a TP gemma3 >window prefill hit the same NaN/crash as the single-process path. Replaced withcreate_sliding_window_prefill_mask, mirroring the single-process fix. TP gemma4 was incidentally fixed by the generalcausal_attentionchange in item 1.Per-model audit (every
RotatingKVCache/create_causal_mask_with_windowsliding-window caller)>windowprefill[N,128]vs scores[..,N,N])window)window)causal_attention(mask=None)causal_attentionfix (no model edit)gemma3_masksinllama_runtime.rscreate_sliding_window_prefill_maskingemma3_maskstensor_parallel_gemma3_matches_full_model_logits)causal_attentioncausal_attentionfix (item 1 above)causal_attentionfixwindow_size = 0dense_windowed_causal_mask(uncapped)sliding_prefill_maskministral-3b-4bit and exaone4-1.2b-4bit ship with
sliding_window: null(no windowed layers), so those local checkpoints do not exercise the windowed path; their fix rests on the unit test, the byte-identical helper proof, and the gpt-oss real-model result for the identical code shape.Real-model validation (M1 Ultra,
--temp 0)Bug existed on
main(HEAD 346018a), gpt-oss-20b-mxfp4, 298-token prompt (window 128):Same prompt on this branch:
gemma-4-12b-it-4bit, ~1600-token prompt (window 1024), this branch (#405 not regressed):
Regression (byte-identical
mainvs this branch, generated text diffed):<128, prefill + decode): BYTE-IDENTICALThis confirms decode (
q_len == 1), non-windowed (window_size == 0), and within-window prefill paths are unchanged; only the previously-degenerate>windowprefill changed (crash/NaN -> coherent).Tests (fast, no model loads)
utils::tests::sliding_window_prefill_mask_selects_full_when_fresh_over_window/_clamps_when_rolled_over/_within_window_matches_capped_builder(the selector's three regimes).layers::tests::causal_attention_multi_token_over_window_uses_full_windowed_mask(theq_len > 1, k_len > windowcase: finite, matches the full windowed-mask reference; the prior clamp+slice path produced NaN). Updated from the old test that asserted the now-removed sliced behavior.utils::tests(30) andlayers::tests(28), and gemma3/gemma4sliding_prefill_masktests still pass.tensor_parallel_gemma3_matches_full_model_logitsvalidates the TP gemma3 mask path.Test plan
cargo clippy --features metal,accelerate -p mlxcel-core -p mlxcel --lib --tests -- -D warnings(clean)cargo test --release -p mlxcel-core sliding_window_prefill_maskandcausal_attention(all pass)cargo test --release -p mlxcel-core "utils::tests"/"layers::tests"(30 / 28 pass)cargo test --release -p mlxcel sliding_prefill_mask(gemma3/gemma4, 4 pass)cargo test --release -p mlxcel tensor_parallel_gemma3(logit-match test passes)>window(crash on main -> coherent here), gemma-4-12b>windowcoherent, byte-identical short/decode regressionKnown follow-ups
offset > 0rolled-over over-window path; in that case the cache already trims towindowkeys so the clamped mask is the correct shape, but this has not been confirmed safe under every model's K/V trimming logic.sliding_prefill_maskonto the sharedcreate_sliding_window_prefill_mask.window_size = 0(window not enforced by the mask); orthogonal to this issue, worth a separate look.Closes #408