Skip to content

fix(models): size dense prefill masks from live_len for mistral4/nemotron_nas/qwen-vl under --max-kv-size (#421)#422

Merged
inureyes merged 1 commit into
mainfrom
fix/issue-421-dense-mask-live-len-migration
Jun 24, 2026
Merged

fix(models): size dense prefill masks from live_len for mistral4/nemotron_nas/qwen-vl under --max-kv-size (#421)#422
inureyes merged 1 commit into
mainfrom
fix/issue-421-dense-mask-live-len-migration

Conversation

@inureyes

Copy link
Copy Markdown
Member

Summary

Completes the offset -> live_len prefill-mask migration started in #419 / PR #420, applying it to the five remaining dense Vec<KVCache> scheduler-pool models that still built their multi-token (l > 1) prefill causal mask from the cache's monotonic offset.

The bug

Under the server --max-kv-size path, enforce_max_kv_size_for (src/server/batch/scheduler.rs) calls KVCache::trim_front, which advances live_start while offset keeps growing monotonically (the RoPE invariant). KVCache::update_and_fetch then returns only the live window of live_len = offset - live_start keys, not offset keys. A model-level mask sized from offset is therefore WIDER than the live K/V after a trim. MLX broadcast_shapes throws, and because the throw crosses the cxx FFI boundary as a non-Result C++ exception it reaches std::terminate and ABORTS the whole server process. This is a remote-triggerable full-server abort: one long prompt plus a multi-token continuation chunk crashes every session.

The fix (mask-only)

For mask construction only, size the prefill causal mask from cache.live_len() (alias of seq_len(), both = offset - live_start) instead of cache.offset. RoPE, position_ids, and the Llama-4 attention scale keep reading the monotonic offset. With no trim (live_start == 0), live_len == offset, so greedy output on the normal untrimmed path is byte-identical.

What changed

  • src/models/mistral4.rs: Mistral4Model::transformer_body builds the prefill mask from caches[0].live_len(). The separate let offset = caches[0].offset that feeds the Llama-4 attention scale (get_llama4_attn_scale) is left monotonic and unchanged.
  • src/models/nemotron_nas.rs: LanguageModel::forward no-mask branch (the server path) builds the mask from caches.first().map(|c| c.live_len()). The per-layer RoPE offset in the attention block is untouched.
  • src/models/qwen2_vl.rs, src/models/qwen3_vl.rs, src/models/qwen3_vl_moe.rs: only the auto_mask = create_causal_mask(seq_len, ...) argument switches to caches[0].live_len(). The cache_offset variable stays caches[0].offset and continues to drive position_ids / MRoPE grid slicing and the deepstack cache_offset == 0 checks, because position_ids and RoPE need absolute (monotonic) positions.

Scope: confirmed affected vs excluded

The five models above each store attention K/V as a plain Vec<KVCache> in the scheduler pool, so enforce_max_kv_size_for trims them and the offset-sized mask is a live (remotely triggerable) bug. nemotron_h and jamba were investigated and EXCLUDED: they use ModelOwnedSequenceState, so CachePool::get_caches_mut returns an empty slice and the trim loop never runs against their attention caches. For them the offset-based mask is a latent inconsistency, not reachable via --max-kv-size, and is left unchanged. glm4_moe_lite and minicpm3 already size from seq_len() and are the reference pattern.

Test plan

  • cargo fmt --check
  • cargo check --lib --features metal,accelerate (compiles all five changed models)
  • cargo clippy --lib --tests --features metal,accelerate -- -D warnings (clean)
  • cargo test --release -p mlxcel-core --features metal,accelerate cache:: (402 passed) including the shared invariant guard kv_cache_continuation_mask_sizes_from_live_len_not_offset_after_trim (added in fix(models): size dense prefill masks from live_len under trim (#419) #420), which pins the trimmed-cache (live_start > 0) invariant these five models now rely on.

No per-model unit test is added: exercising each model's mask construction requires a fully loaded real checkpoint (heavy, not in CI), and the underlying primitive is already pinned by the shared cache-level invariant test above.

Real-model E2E validation status will be added by the orchestrator: nemotron_nas / qwen2_vl / qwen3_vl / qwen3_vl_moe validated locally under a small --max-kv-size with a multi-token continuation chunk; mistral4 (Mistral-Small-4-119B) validated when its checkpoint is available.

Closes #421

@inureyes inureyes added type:bug Bug fixes, error corrections, or issue resolutions priority:medium Medium priority area:models Model architectures, weights, loading, metadata area:inference Generation, sampling, decoding (incl. speculative, DRY) status:review Under review labels Jun 24, 2026
…tron_nas/qwen-vl under --max-kv-size (#421)

Completes the offset->live_len prefill-mask migration started in #419/#420 for the remaining dense Vec<KVCache> scheduler-pool models that build their multi-token prefill mask from the cache's monotonic offset.

Under the server --max-kv-size path, enforce_max_kv_size_for calls KVCache::trim_front, which advances live_start while offset keeps growing for RoPE. update_and_fetch then returns only live_len = offset - live_start keys, so a mask sized from offset is wider than the live K/V, MLX broadcast_shapes throws, and the non-Result C++ exception crosses the cxx boundary to std::terminate and aborts the whole server. A remote client can crash all sessions with one long prompt plus a multi-token continuation chunk.

Fix is mask-only: build the prefill causal mask from cache.live_len() (alias of seq_len(), both = offset - live_start) instead of offset. RoPE, position_ids, and the Llama-4 attention scale keep reading the monotonic offset. Byte-identical when untrimmed (live_start == 0, so live_len == offset).

Models changed:

- mistral4: transformer_body mask uses caches[0].live_len(); the Llama-4 attention scale below keeps caches[0].offset.
- nemotron_nas: forward no-mask branch uses caches.first().map(|c| c.live_len()); per-layer RoPE offset untouched.
- qwen2_vl / qwen3_vl / qwen3_vl_moe: only the auto_mask create_causal_mask argument switches to caches[0].live_len(); cache_offset stays monotonic for position_ids/RoPE grid slicing.

nemotron_h and jamba were investigated and excluded: they are ModelOwnedSequenceState caches, so get_caches_mut returns an empty slice and enforce_max_kv_size_for never trims them (latent inconsistency only, not remotely triggerable today).
@inureyes
inureyes force-pushed the fix/issue-421-dense-mask-live-len-migration branch from f0d5f90 to af05dc7 Compare June 24, 2026 15:35
@inureyes

Copy link
Copy Markdown
Member Author

PR Finalization Complete

Summary

  • Tests: No new tests added. The shared invariant kv_cache_continuation_mask_sizes_from_live_len_not_offset_after_trim (added in fix(models): size dense prefill masks from live_len under trim (#419) #420, lives in src/lib/mlxcel-core/src/cache.rs:8157) pins the trim_front / live_len / create_causal_mask contract all five models now rely on; it passes. None of the five model files have any existing unit tests (consistent with all inference model files in the codebase), and per-model mask-construction tests require a fully loaded checkpoint, so no cheap unit test exists. No gap to fill.
  • Documentation: No docs in docs/ reference --max-kv-size, live_len, trim_front, or dense-cache mask sizing behavior. The change is internal to model forward passes. No doc update warranted.
  • Lint/Format: Clean. cargo check --lib, cargo clippy --lib --tests -- -D warnings, cargo fmt --check, and cargo test --release -p mlxcel-core cache:: (402 passed) all green on current HEAD af05dc748.

No commits were added; the branch is already production-ready. Not merged.

@inureyes

Copy link
Copy Markdown
Member Author

Real-model validation (all 5 models, before/after under --max-kv-size 1024 --prefill-chunk-size 512)

Same bug class as #419/#420: a trim_front mid chunked-prefill leaves an offset-sized mask wider than the live K/V → MLX broadcast_shapes throw → cxx std::terminate → server abort. before = the merged main binary (loads all models but mask-buggy for these 5); after = this branch.

  • qwen2-vl-2b: before = abort (broadcast_shapes (512,2048) vs (1,12,512,1536)), after = HTTP 200. Directly reproduced crash→fix. Untrimmed control coherent ("...evolution of computing from early mechanical calculators...").
  • qwen3-vl-2b / qwen3-vl-30b-a3b: no-regression. These route text-only generation through forward_text_only; the fixed forward_impl auto-mask is the multimodal path. Byte-identical before==after; untrimmed controls coherent. They share qwen2_vl's exact one-line mask fix.
  • nemotron-nas-30b: no-regression, coherent, byte-identical (crash not reproduced even at --max-kv-size 600 --prefill-chunk-size 200; it does not hit a mid-prefill trim + continuation, so the fix is defensive).
  • mistral4 (Mistral-Small-4-119B): the mask fix is byte-identical and correct, and mistral4 now loads and generates text (via fix(loading): route Mistral3-VLM mistral4 (MLA) text backbone to the Mistral4 loader (#423) #424). Its mask crash→fix cannot be demonstrated because a SEPARATE bug aborts every mistral4 offset>0 forward first ([reshape] ... size 2097152 into (1,1,1), reachable both via image tokens and chunked prefill), tracked as fix(models): mistral4-VLM image+text path aborts in multimodal prefill (reshape size 12480512 into (1,1,1)) #425. mistral4 text-only without --max-kv-size (single-pass, offset==0) generates coherently, confirming the untrimmed path is unaffected.

Net: the fix is correct and byte-identical for all 5; qwen2_vl is the directly-reproduced crash→fix; the remaining 4 are no-regression (defensive / multimodal / blocked-by-#425). The shared cache invariant test from #420 pins the primitive.

@inureyes inureyes added status:done Completed and removed status:review Under review labels Jun 24, 2026
@inureyes
inureyes merged commit c1c1ae4 into main Jun 24, 2026
5 checks passed
@inureyes
inureyes deleted the fix/issue-421-dense-mask-live-len-migration branch June 24, 2026 16:21
inureyes added a commit that referenced this pull request Jun 24, 2026
…-max-kv-size (#431)

* fix(models): size gemma3/gemma4/exaone_moe prefill mask from live_len

gemma3, gemma4, and exaone_moe built their multi-token (l > 1) prefill attention masks from the cache's monotonic offset instead of the live window, for both the global (full-attention) causal mask and the sliding-window prefill mask. That is correct only while no trim has happened.

Under the server --max-kv-size path, enforce_max_kv_size_for calls KVCache::trim_front, which advances live_start while offset keeps growing for RoPE; update_and_fetch then returns only live_len = offset - live_start keys, so a mask sized from offset is wider than the live K/V, MLX broadcast_shapes throws, and the non-Result cxx boundary turns the throw into std::terminate and aborts the whole server. This completes the offset->live_len prefill-mask migration started in #419/#420 and #421/#422 for the three Gemma-family / ExaOne dense-cache models they missed.

Fix is mask-only: size the prefill masks from the cache's live window (live_len() for a Standard KVCache, seq_len() for a Rotating cache, both equal to the keys update_and_fetch returns) instead of offset. RoPE and position bookkeeping keep reading the monotonic offset. Byte-identical when untrimmed (live_start == 0, so live_len == offset).

Models changed:

- gemma3: the seq_len > 1 prefill branch sizes the global mask from caches[global_idx].live_len() and the sliding mask from caches[0].live_len(); a live_len() accessor was added to the model's CacheInterface trait (KVCache -> live_len(), RotatingKVCache -> seq_len()).
- gemma4: a first_cache_live_len helper (free function plus method form) mirrors first_cache_offset and feeds the two genuine prefill-mask sites (the non-ragged l > 1 branch and the stage-forward execute_hidden path). The batched-MTP sites that use the offset as a per_row_valid_end column coordinate (divergent verify, has_padding, mask_stale_key_gap) intentionally keep the monotonic first_cache_offset; that regime enforces live_len == offset (slot_base == 0) anyway.
- exaone_moe: the global and sliding prefill masks size from a new AnyKVCache::live_len() (Standard -> live_len(), Rotating -> seq_len()) instead of offset().

gemma4_mtp_target was verified and needs no change: it is not a registered LanguageModel, drives forward through caller-owned caches that never enter the scheduler pool, and its only mask builder sizes from a fixed offset 0 prefill.

Adds per-model unit tests (gemma3_mask_tests, gemma4_unified_mask_tests, exaone_moe_mask_tests) that trim a Standard cache so live_start > 0, then assert the live_len-sized prefill mask key axis matches the continuation chunk's returned K/V (live_len + m) while the offset-sized mask is strictly wider, plus a Rotating-cache case where live_len equals the returned key axis. The shared cache invariant is already pinned by kv_cache_continuation_mask_sizes_from_live_len_not_offset_after_trim in cache.rs (#419).

* fix(models): size gemma3 stage prefill mask from live_len

The Gemma3StageModel::execute_hidden pipeline-stage path still sized both the global causal mask and the sliding-window prefill mask from the cache's monotonic offset, while the gemma4 stage analog (Gemma4StageModel::execute_hidden) and the gemma3 single-process path (Gemma3Model::forward_with_caches) were already switched to live_len in this PR. This completes the offset to live_len prefill-mask migration for gemma3 so both stage paths match.

Like the rest of #430, this is byte-identical on the untrimmed path: the stage executor's PointerOwnedCacheStore syncs only offset (never live_start) into the model-owned internal caches, so live_start stays 0 and live_len == offset there today. The change uses the existing as_interface().live_len() accessor (KVCache to live_len, RotatingKVCache to seq_len), and keeps RoPE on the monotonic offset.

* docs(changelog): record gemma3/gemma4/exaone_moe prefill-mask consistency fix (#430)
inureyes added a commit that referenced this pull request Jun 25, 2026
The initial v0.3.3 cut (4d5f4fb) was never tagged, and 16 more commits
landed on main afterward. Fold those fixes into the v0.3.3 CHANGELOG and
debian/changelog entries and set the release date to 2026-06-25:

- N-gram loop detection (#433) and Nemotron-H Nano Omni audio input (#443)
- Prefill masks sized from the live window under --max-kv-size trim
  (#418, #420, #422, #431)
- Double-transpose crash on mlx-community conv checkpoints (#429)
- conv1d/conv2d and nemotron audio-encoder convs fallible at the FFI
  boundary (#434, #439)
- Gemma 4 audio placed in the user turn (#438, #440)
- mistral4 MLA backbone routing and 2D MoE token flattening (#423, #425)
- Bump actions/checkout from 6 to 7 (#395)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:inference Generation, sampling, decoding (incl. speculative, DRY) area:models Model architectures, weights, loading, metadata priority:medium Medium priority status:done Completed type:bug Bug fixes, error corrections, or issue resolutions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(models): complete offset->live_len prefill-mask migration for dense-KVCache models under --max-kv-size (mistral4/nemotron_nas/qwen*-vl)

1 participant