Skip to content

fix(gemma3): apply rope_scaling to global attention layers - #1709

Open
1qh wants to merge 4 commits into
Blaizzy:mainfrom
1qh:fix/gemma3-rope-scaling
Open

fix(gemma3): apply rope_scaling to global attention layers#1709
1qh wants to merge 4 commits into
Blaizzy:mainfrom
1qh:fix/gemma3-rope-scaling

Conversation

@1qh

@1qh 1qh commented Jul 24, 2026

Copy link
Copy Markdown

Problem

Gemma 3 ships rope_scaling in text_config and applies it to the global-attention layers only:

"rope_scaling": {"factor": 8.0, "rope_type": "linear"}

TextConfig declares the field, but Attention.__init__ builds a plain nn.RoPE and never passes it, so global layers run unscaled positions (effective scale=1.0 where the model expects 1/8).

The error is negligible at short context and compounds with length. On mlx-community/gemma-3-4b-it-4bit, first-token logits for a 2916-token prompt (mlx-vlm vs the reference mlx_lm.models.gemma3):

token mlx-lm (reference) mlx-vlm (before)
Từ (correct continuation) 33.75 — argmax 35.00 — 2nd
Đối 31.62 — 2nd 36.25 — argmax

Greedy decoding therefore picks a different token and degenerates into a repetition loop, while the same weights under mlx_lm.generate answer correctly. Short prompts are unaffected (top-100 logit rank-correlation 0.96 at 195 tokens vs 0.53 at 2916), which is why this hides in ordinary use.

Fix

Use the existing rope_utils.initialize_rope, matching gemma4 in this repo and gemma3_text in mlx-lm. Sliding layers keep unscaled local RoPE; scaling applies to global layers only.

After the fix, mlx-vlm reproduces the reference logits exactly:

top5: [(33.75, 'Từ'), (31.62, 'Đối'), (29.38, 'T'), (25.75, 'From'), (25.62, 'Với')]   # mlx-vlm, patched
top5: [(33.75, 'Từ'), (31.62, 'Đối'), (29.38, 'T'), (25.75, 'From'), (25.62, 'Với')]   # mlx-lm reference

Scope

  • gemma3 — the reported defect; gemma3_text inherits the fix through gemma3.language.LanguageModel.
  • gemma3n — same shape (declares rope_scaling, built a plain nn.RoPE). With no rope_scaling in the config the behaviour is unchanged (scale=1.0), so this is a no-op for current checkpoints and correct if one declares scaling.

Tests

Two regression tests added to test_models.py:

  • test_gemma3_rope_scaling_applies_to_global_layers — asserts global layers get 1/factor, sliding layers stay 1.0. Fails on main with AssertionError: 1.0 != 0.125.
  • test_gemma3_without_rope_scaling_is_unscaled — asserts the no-scaling path is untouched.

python -m unittest mlx_vlm.tests.test_models passes (all 20 gemma tests included).

Gemma 3 ships rope_scaling (linear, factor 8) in text_config and applies it to
the global-attention layers only. The Attention module declared rope_scaling on
TextConfig but built a plain nn.RoPE without it, so global layers ran unscaled
positions. The error is negligible at short context and compounds with length:
on a 2916-token prompt gemma-3-4b-it-4bit picks a different greedy token than
the reference implementation and degenerates into a repetition loop.

Use the existing rope_utils.initialize_rope, matching gemma4 in this repo and
gemma3_text in mlx-lm. Sliding layers keep unscaled local RoPE. gemma3_text
inherits the fix through gemma3.language.LanguageModel. gemma3n carried the same
shape and is fixed alongside; with no rope_scaling declared the behaviour is
unchanged (scale 1.0).
@1qh

1qh commented Jul 24, 2026

Copy link
Copy Markdown
Author

Follow-up: I swept the rest of the repo for the same shape and found ten more models that declare rope_scaling and build a plain nn.RoPE without it — including mllama, which affects Llama-3.2-Vision today. That is #1710.

The two are the same defect class and can be reviewed together or merged in either order; they touch disjoint files. This one is separate because it is verified end to end against a real gemma-3 checkpoint (patched mlx-vlm reproduces mlx-lm's reference logits exactly and a served repetition loop disappears), whereas #1710's evidence for mllama is a numerical comparison of the required Llama3RoPE against the current plain RoPE rather than a full model run. Happy to squash them into one PR if you would rather review a single change.

@Blaizzy
Blaizzy requested a review from Lazarus-931 July 25, 2026 15:29
@Blaizzy
Blaizzy removed the request for review from Lazarus-931 July 25, 2026 15:30

@Blaizzy Blaizzy left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@lucasnewman

Copy link
Copy Markdown
Collaborator

@1qh Can you sign your commits and force push so we can clear the merge requirements?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants