Fix MLXVLM Gemma4 loader to honor num_kv_shared_layers (E-series targets)#384
Conversation
|
Independently hit this exact bug and can confirm the fix. Repro: Loading Confirmation: With this PR applied, As a bonus this also unblocks the E-series Thanks for the fix! |
davidkoski
left a comment
There was a problem hiding this comment.
Changes look good but it is failing swift-format -- please rerun that. Thank you!
…Tests CI's lint job builds swift-format from releases/latest (603.0.0), whose OrderedImports rule places attributed imports (@_spi/@testable) after the plain imports in a separate group — differing from 602.x, which the repo's `language: system` pre-commit hook picks up locally. This left MTPSpeculativeTokenIteratorTests.swift (unrelated to this PR's change) drifted on main, tripping the whole-repo `pre-commit run --all` lint on every PR. Reformat it to satisfy CI's 603. Requested by @davidkoski on ml-explore#384.
122f671 to
785f963
Compare
|
Done — rerun and pushed (785f963). The failing file was actually an unrelated one: Heads-up for the maintainers: because CI tracks |
…Tests Same pre-existing drift as fixed on ml-explore#384: CI's lint builds swift-format from releases/latest (603.0.0), whose OrderedImports groups attributed imports (@_spi/@testable) into a trailing block — differing from the 602.x that the repo's `language: system` pre-commit hook picks up locally. The unrelated file MTPSpeculativeTokenIteratorTests.swift had drifted on main, tripping the whole-repo `pre-commit run --all` lint on this PR too. Reformat to satisfy 603.
Cherry-pick of the loader fix from ml-explore#384 onto bd4b743 (our pinned base). Gates the K/V-projection build on isKVSharedLayer so the module tree matches E-series checkpoints whose shared tail ships no k_proj (E2B layer 15 / E4B layer 24) — fixes keyNotFound: layers.24.self_attn.k_proj.weight.
|
Ugh, trying to get CI to rerun on this. |
|
Claude thinks it sees the issue with the CI, PR incoming. |
The VLM Gemma4 text backbone built every decoder layer with the default
kvSharedOnly=false, so Gemma4TextAttention.init constructed k_proj/v_proj/
k_norm/v_norm on the KV-shared tail layers too — even though it already
computed isKVSharedLayer and the checkpoints ship no such weights there.
Loading any KV-sharing Gemma 4 checkpoint (the E2B/E4B "effective" targets,
which route through VLMModelFactory because they carry vision_config) then
failed `keyNotFound layers.{15,24}.self_attn.k_proj.weight` — independent of
MTP (speculative=false fails identically). Dense/MoE (num_kv_shared_layers=0)
were unaffected.
Gate the K/V projection construction on `!isKVSharedLayer` as well, matching
the text-only backbone (MLXLLM/Models/Gemma4Text.swift). The runtime already
passes sharedKV to these layers and sanitize already drops redundant K/V
weights for PTQ checkpoints, so both QAT (omit) and PTQ (ship+drop) layouts
load. The bug is upstream drift: MLXVLM duplicates the LM backbone because it
cannot import MLXLLM, and the fix landed only in the text copy.
Validated by testGemma4TextSharedLayersDeclareNoKProj (no checkpoint): a
4-layer model with 2 KV-shared layers declares k_proj only on layers 0,1.
Relates to ml-explore#282 (Gemma 4 family loader gaps).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Real-checkpoint regression for the num_kv_shared_layers loader fix: loads the E2B/E4B 4bit targets through VLMModelFactory (the path that threw keyNotFound on the KV-shared tail layers pre-fix) and runs a short greedy generate, which also exercises the runtime shared-KV forward. Cache-gated per model id, so it runs where the weights are present and disables (not fails) otherwise. Verified locally: both E2B and E4B targets load and generate non-empty output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2d4c149 to
79a2b58
Compare
davidkoski
left a comment
There was a problem hiding this comment.
Changes look good, thank you!
Proposed changes
Fixes loading of the Gemma 4 E-series targets (E2B/E4B), which currently fail
with
keyNotFound … layers.{15,24}.self_attn.k_proj.weight. This is independentof MTP — plain loading (
speculative=false) fails identically.Root cause. The E-series checkpoints use
num_kv_shared_layers > 0: the lastN decoder layers reuse an earlier layer's K/V and ship no
k_proj/v_proj. Theycarry
vision_config, so they load throughVLMModelFactory→MLXVLM/Models/Gemma4.swift. That backbone builds every decoder layer with thedefault
kvSharedOnly: false, andGemma4TextAttention.initgated K/V projectionconstruction on
kvSharedOnlyalone — even though it already computesisKVSharedLayer. So the shared-tail layers declared ak_projthe checkpointdoesn't have, and
loadWeightsthrew. The text-only backbone(
MLXLLM/Models/Gemma4Text.swift) already gates on the shared-layer predicate;this was drift between the two copies.
Fix. Gate K/V projection construction on
!kvSharedOnly && !isKVSharedLayer,matching the text backbone. The runtime already passes
sharedKVto these layersand
sanitizealready drops redundant K/V weights for PTQ checkpoints, so bothQAT (omit) and PTQ (ship+drop) layouts load. Dense / 26B-A4B MoE
(
num_kv_shared_layers = 0) are unaffected.Relates to #282 (Gemma 4 family loader gaps). Complements the E-series MTP
centroid-embedder PR — that fixes the drafter; this fixes the target it pairs with.
Checklist
pre-commit run --all-filesto format my codetestGemma4TextSharedLayersDeclareNoKProj(unit, no checkpoint): a 4-layermodel with 2 KV-shared layers declares
k_projonly on the KV-owning layers.Gemma4EseriesLoadIntegrationTests(cache-gated): the real E2B/E4B 4bittargets load through
VLMModelFactoryand generate. Verified locally — bothload and produce output (the path that threw pre-fix).