Skip to content

fix(models/deepseek-v3): restore last layer, causal prefill, f16 clip - #618

Merged
inureyes merged 1 commit into
mainfrom
fix/issue-525-deepseek-v3-correctness
Jul 2, 2026
Merged

fix(models/deepseek-v3): restore last layer, causal prefill, f16 clip#618
inureyes merged 1 commit into
mainfrom
fix/issue-525-deepseek-v3-correctness

Conversation

@inureyes

@inureyes inureyes commented Jul 2, 2026

Copy link
Copy Markdown
Member

Follow-up hardening for #525 (round 3): the Kimi-VL port loaded and ran a full forward on the real kimi-vl-a3b-thinking-4bit checkpoint (after #608 and #614) but emitted garbage: a constant "!", which is token id 0 in the Kimi tokenizer, i.e. the argmax of NaN/degenerate logits. Root-causing against the real checkpoint (read-only safetensors header enumeration) and the references (mlx-lm deepseek_v3.py, mlx-vlm kimi_vl/language.py) surfaced four contract violations in the shared DeepSeek-V3 backbone. The tokenizer/chat-template contract (the #522 Moondream2 failure mode) was audited first and verified sound: chat_template.jinja is picked up by ChatTemplateProcessor::from_model_path, the special ids match tokenizer.json (<|media_pad|> = 163592 = config media_placeholder_token_id, <|im_end|> = 163584 in the config eos list), and the tokenizer adds no BOS (no post_processor template), so the prompt framing is not the bug.

Root causes and fixes

  1. Dropped final decoder layer (structural). sanitize_weights and from_weights treated index num_hidden_layers - 1 as the multi-token-prediction trailer: sanitize deleted model.layers.26.* (a REAL, fully populated decoder layer in this checkpoint; the shards have layers 0..=26 and no MTP trailer) and from_weights built only 26 of 27 layers. The reference builds range(config.num_hidden_layers) layers (kimi_vl/language.py line 377-380, mlx-lm deepseek_v3.py line 326), and mlx-lm's sanitize strips model.layers.61, the OUT-OF-RANGE index num_hidden_layers, not num_hidden_layers - 1 (deepseek_v3.py line 477). Fixed in sanitize_weights (all three per-layer loops plus the trailer removal, now model.layers.{num_hidden_layers}.) and from_weights, with the pipeline partitioner mirrors (local_runtime.rs, partition_profile.rs, stage executor) updated in lockstep.

  2. Non-causal prefill (correctness, poisons the KV cache). DeepSeekV3Attention feeds pe_scores as the additive SDPA mask, so causality exists only if baked into pe_scores, and the C++ fast_scaled_dot_product_attention wrapper applies no implicit causality for array masks. Both standard generation paths (CLI text prefill and the VLM embeddings prefill; merge_llava intentionally returns no mask) call the model with mask == None, expecting the model to self-apply a causal mask like the reference create_attention_mask(h, cache[0]). A multi-token prefill therefore attended bidirectionally, and every layer above the first wrote future-contaminated K/V into the cache, corrupting the entire generation, not just the first token. The attention now applies create_causal_mask(l, live_len_before) when mask == None && l > 1, the same pattern qwen3.rs and deepseek_v2.rs already use. youtu_vl_lm.rs shares this attention and silently gains the same fix (its "implicit causal path" comment is now true).

  3. Missing f16 overflow guard (the NaN source). The reference kimi_vl SwitchGLU passes activation=clipped_silu (clip(silu(gate), -100, 100) * up), added upstream specifically "to prevent fp16 from overflowing". This checkpoint stores float16 weights (scales/biases F16), so the whole model computes in f16 and the unclipped silu(gate) * up product overflows to inf; downstream RMSNorm turns NaN, and once a decode step writes NaN into the KV cache every subsequent step reads it back, locking generation onto argmax(NaN) = token 0 = "!", exactly the observed " a!!!!!!..." pattern (finite prefill, poisoned decode). The routed-expert activation now mirrors clipped_silu; the clip is the identity for |silu(gate)| <= 100, so bf16/f32 checkpoints in the normal activation range are numerically unchanged.

  4. Routing precision. Expert scores are now computed in float32 (sigmoid(gates.astype(f32))), mirroring group_expert_select in BOTH references (mlx-lm deepseek_v3.py line 203, kimi_vl/language.py line 263), instead of routing in f16 after the correction-bias add.

What changed

  • src/models/deepseek_v3.rs: causal-mask fallback in DeepSeekV3Attention::forward, clipped_swiglu helper wired into SwitchGLU::forward, f32 routing in MoEGate::forward, layer-count fixes in sanitize_weights/from_weights, and five new regression tests.
  • src/distributed/pipeline/local_runtime.rs, partition_profile.rs, partition_profile_tests.rs, stage_executor/deepseek_v3.rs: partitioner layer counts updated in lockstep with the model (all num_hidden_layers blocks are real).

Tests

  • sanitize_keeps_last_decoder_layer_and_strips_mtp_trailer: the last in-range layer survives (kv_b_proj decomposed) and layers.{num_hidden_layers}.* is stripped.
  • from_weights_builds_all_num_hidden_layers: tiny direct-q (q_lora_rank: null, the Kimi/Moonlight shape) model builds every layer and produces finite [1, 3, vocab] logits through forward_impl with mask == None.
  • prefill_is_causal_without_caller_mask: position 0 of a 2-token prefill equals a 1-token forward of the same token (verified to FAIL without the fallback; also cross-checks the absorbed decode path against the materialized prefill path).
  • clipped_swiglu_prevents_f16_overflow: asserts the unclipped f16 activation provably overflows to inf for the test input while the clipped one stays finite at the reference value, and that the clip is the identity in the normal range.
  • moe_gate_routes_in_f32_for_f16_activations: scores dtype is f32 for f16 activations/weights.
  • Updated: num_layers_counts_all_decoder_blocks_for_deepseek_v3 (61, was 60), build_profile_separates_dense_and_moe_in_deepseek (6 layers, was 5).

Verified locally: cargo test --lib deepseek_v3 (22 passed), cargo test --lib kimi_vl (16 passed), cargo test --test kimi_vl_parity (3 passed), cargo test --lib youtu (20), cargo test --lib pipeline (340), cargo check --lib --tests, cargo clippy --lib --tests -- -D warnings, cargo fmt. No GPU/real-model run was performed here; the orchestrator re-validates on device.

Notes for follow-up (out of scope here)

  • src/models/deepseek_v32.rs has the same "causality only via caller mask" structure in its attention (its pe_scores path); if any of its generation paths pass mask == None for a multi-token prefill it has the same latent non-causal-prefill bug and should get the same fallback.
  • Genuine DeepSeek-V3 text checkpoints were also affected by the layer off-by-one (they ran with 60 of 61 layers); this PR fixes that as a side effect.

The Kimi-VL port (#525) loaded and ran but emitted garbage (a constant "!" = token id 0, the argmax of NaN/degenerate logits). Root-causing against the real kimi-vl-a3b-thinking-4bit checkpoint and the references (mlx-lm deepseek_v3.py, mlx-vlm kimi_vl/language.py) surfaced four contract violations in the shared DeepSeek-V3 backbone:

1. Layer-count off-by-one: sanitize_weights and from_weights treated index num_hidden_layers - 1 as the multi-token-prediction trailer, deleting the last REAL decoder layer's weights and building one layer too few. The real checkpoint has 27 fully populated decoder layers (model.layers.0..=26) and no MTP trailer; genuine DeepSeek-V3 stores its MTP head at the OUT-OF-RANGE index model.layers.61 (num_hidden_layers = 61), which is exactly what mlx-lm's sanitize strips. Both references build range(num_hidden_layers) layers. The pipeline partitioner mirrors (local_runtime, partition_profile, stage executor) are updated in lockstep.

2. Non-causal prefill: DeepSeekV3Attention feeds pe_scores as the additive SDPA mask, so causality exists only if it is baked into pe_scores. The standard generation paths (CLI text prefill and the VLM embeddings prefill via merge_llava) pass mask == None, expecting the model to self-apply a causal mask like the reference create_attention_mask does, so a multi-token prefill attended bidirectionally and every layer above the first cached future-contaminated K/V, corrupting the whole generation. The attention now applies create_causal_mask(l, live_len) when no caller mask is provided and l > 1, matching the qwen3/deepseek_v2 in-repo pattern (and making the youtu_vl_lm "implicit causal path" comment true, since it shares this attention).

3. Missing f16 overflow guard: the reference kimi_vl SwitchGLU uses clipped_silu (clip(silu(gate), -100, 100) * up) specifically "to prevent fp16 from overflowing". This checkpoint stores f16 weights, so the unclipped silu(gate) * up product overflows to inf, downstream norms turn NaN, and argmax over NaN logits yields token 0 ("!"). The routed-expert activation now mirrors clipped_silu; the clip is the identity for |silu(gate)| <= 100, so bf16/f32 checkpoints in the normal range are numerically unchanged.

4. Routing precision: expert scores are now computed in float32 (sigmoid(gates.astype(f32))), mirroring group_expert_select in both references, instead of routing on f16 scores after the correction-bias add.

New regression tests: sanitize keeps the last decoder layer and strips the layers.{num_hidden_layers} trailer; from_weights builds all num_hidden_layers layers with finite logits on a tiny direct-q (q_lora_rank null) model; a 2-token prefill with mask == None matches the 1-token forward at position 0 (fails without the causal fallback); clipped_swiglu stays finite where the unclipped f16 activation provably overflows and is the identity in the normal range; MoE gate scores are float32 for f16 activations.
@inureyes inureyes added type:bug Bug fixes, error corrections, or issue resolutions priority:high High priority area:models Model architectures, weights, loading, metadata status:review Under review labels Jul 2, 2026
@inureyes
inureyes merged commit b9aa703 into main Jul 2, 2026
6 checks passed
@inureyes
inureyes deleted the fix/issue-525-deepseek-v3-correctness branch July 2, 2026 13:49
inureyes added a commit that referenced this pull request Jul 5, 2026
…t bidirectional

MLAAttention::forward applied causality only when the caller supplied a mask, and both standard generation paths pass mask == None for prefill (the offline CLI text prefill and the VLM embeddings prefill), so a multi-token prefill attended bidirectionally and wrote future-contaminated K/V into the cache; the DSA facets were also exposed (the lightning indexer's top-k could select future keys, and apply_sparse_prefill_mask's "pe_scores already carries the causal mask" invariant was false). This is the same class PR #618 fixed on the deepseek_v3 backbone and explicitly flagged here. The fix mirrors deepseek_v3: capture the pre-update live length (live_before) next to the rope offset, build create_causal_mask(l, live_before) once for any maskless multi-token forward, and route that effective mask to BOTH consumers, the indexer top_indices selection and the additive pe_scores mask; decode (l == 1) stays maskless since every cached position is causally valid. Four regression tests: the dense prefill causality test fails before the fix (position 0 contaminated by position 1, max diff 9e-3) and passes after; the sparse-indexer variant pins the kv_len > index_topk path; a decode-after-prefill test pins the maskless decode step; a sanitize test locks that only the MTP trailer at layer_idx == num_hidden_layers is stripped (the #618-class off-by-one). Real-checkpoint validation is not feasible on the 128 GB validation machine: every public deepseek_v32-backbone checkpoint (DeepSeek-V3.2, GLM-5 via glm_moe_dsa) exceeds it; the fix is a byte-for-byte mirror of the #618 pattern that was validated on real hardware via the deepseek_v3 backbone, with the fail-then-pass unit proof standing in here.
inureyes added a commit that referenced this pull request Jul 5, 2026
…t bidirectional (#667)

MLAAttention::forward applied causality only when the caller supplied a mask, and both standard generation paths pass mask == None for prefill (the offline CLI text prefill and the VLM embeddings prefill), so a multi-token prefill attended bidirectionally and wrote future-contaminated K/V into the cache; the DSA facets were also exposed (the lightning indexer's top-k could select future keys, and apply_sparse_prefill_mask's "pe_scores already carries the causal mask" invariant was false). This is the same class PR #618 fixed on the deepseek_v3 backbone and explicitly flagged here. The fix mirrors deepseek_v3: capture the pre-update live length (live_before) next to the rope offset, build create_causal_mask(l, live_before) once for any maskless multi-token forward, and route that effective mask to BOTH consumers, the indexer top_indices selection and the additive pe_scores mask; decode (l == 1) stays maskless since every cached position is causally valid. Four regression tests: the dense prefill causality test fails before the fix (position 0 contaminated by position 1, max diff 9e-3) and passes after; the sparse-indexer variant pins the kv_len > index_topk path; a decode-after-prefill test pins the maskless decode step; a sanitize test locks that only the MTP trailer at layer_idx == num_hidden_layers is stripped (the #618-class off-by-one). Real-checkpoint validation is not feasible on the 128 GB validation machine: every public deepseek_v32-backbone checkpoint (DeepSeek-V3.2, GLM-5 via glm_moe_dsa) exceeds it; the fix is a byte-for-byte mirror of the #618 pattern that was validated on real hardware via the deepseek_v3 backbone, with the fail-then-pass unit proof standing in here.
@inureyes inureyes self-assigned this Aug 31, 2026
@inureyes inureyes added status:done Completed and removed status:review Under review labels Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:models Model architectures, weights, loading, metadata priority:high High 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.

1 participant