fix(gemma3n): share KV independently of the cache (#1674) - #1702
Open
Anai-Guo wants to merge 1 commit into
Open
Conversation
KV-shared layers only reused the source layer's keys/values when a cache was present, and when they did they took their RoPE offset from the source layer's cache *after* that layer had already appended the current tokens. Two consequences: - a no-cache forward (LoRA training, `mlx_lm.evaluate`) computed its own k_proj/v_proj on the shared layers, so training did not match generation; - with a cache, the shared layers rotated their queries at `offset + L` while the concrete layers used `offset`, so prefilling a prompt in one step did not match decoding it token by token. Thread the source layer's keys/values and offset through the decoder loop instead, the way `gemma4_text` already does, so sharing no longer depends on a cache being present.
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.
Closes #1674.
Problem
In
Gemma3nAttention.__call__, KV-shared layers only reused the source layer's keys/values when a cache was present:Coupling KV sharing to the cache produces two distinct defects.
1. No-cache forwards don't share KV (the reported issue). Generation always runs with a cache, so it is unaffected, but any cache-free forward —
mlx_lm.evaluate/perplexity and, notably, LoRA fine-tuning — falls into theelsebranch and computes the shared layers' ownk_proj/v_proj. The model being trained is not the model used at inference. For reference,transformersthreads ashared_kv_statesdict through the forward and never consults a cache for this, and it does not even buildk_proj/v_projon shared layers.2. With a cache, the shared layers rotate their queries at the wrong offset.
cachehere is the source layer's cache, and by the time a shared layer runs, that layer has already calledupdate_and_fetch, socache.offsetisoffset + L, notoffset. The concrete layers rotate their queries atoffset. So prefilling a prompt in one step does not agree with decoding it token by token.Both are visible on the
gemma3nconfig already intests/test_models.py(random weights, so magnitudes are arbitrary — the point is that quantities which must agree don't):num_kv_shared_layers=2)max abs(model(x) - model(x, cache=fresh))max abs(prefill_6_tokens - decode_6x1_tokens)With
num_kv_shared_layers=0(sharing off) both are0.0before and after, which is the control that pins the KV-shared path as the cause.Change
Thread the source layer's
(keys, values)and its RoPE offset through the decoder loop instead of recovering them from a cache — the same shapegemma4_textalready uses (Attention.__call__(..., shared_kv=..., offset=...)returning(out, (keys, values), offset), withGemma4TextModelkeeping anintermediateslist).Gemma3nModelalready computeslayer_idx_to_cache_idx, which is exactly the "source layer of the same type" mapping, so it doubles as the index intointermediatesand no new bookkeeping is needed.Sharing now behaves identically with and without a cache, and the shared layers rotate their queries at the same offset as the layer whose K/V they reuse.
Deliberately not changed: shared layers keep their (now unused in the shared path)
k_proj/v_proj/k_norm/v_normparameters, so weight loading is untouched. Dropping them the waygemma4_textdoes would change whatstrict=Trueaccepts and belongs in a separate change.Tests
New
test_gemma3n_kv_sharing_is_cache_independentasserts both invariants, fornum_kv_shared_layersof 2 and 0.Verified with
mlx-cuda0.30.0 on the CPU backend (no Apple hardware available here):main(assertTrue(mx.allclose(no_cache, cached, ...))→array(False)), passes with this change.tests/test_models.py: 78 tests, the same 3 errors before and after (test_bitnet,test_gated_delta,test_gated_delta_masked— custom-Metal-kernel tests that don't run on this backend). No new failures;test_all_modelscoversgemma3nand passes.black25.1.0 andisort6.0.0 (--profile=black) report both files unchanged.🤖 Generated with Claude Code