[Gemma4] Fix the bug of use_qk_norm accidently set to False in Gemma 4 configs - #4938
[Gemma4] Fix the bug of use_qk_norm accidently set to False in Gemma 4 configs #4938Dr-Left wants to merge 1 commit into
use_qk_norm accidently set to False in Gemma 4 configs #4938Conversation
There was a problem hiding this comment.
Code Review
This pull request enables QK normalization (use_qk_norm: true) across several Gemma 4 model configurations (26b, 31b, e2b, and e4b), disables layer scanning for the smaller variants, updates the attention layer's weight initialization logic, and adds a new unit test suite. The review feedback highlights a potential silent correctness bug where a mismatch can occur between weight initialization and the forward pass if self.use_qk_norm and self.config.use_qk_norm are inconsistent. Additionally, the feedback points out that Flax NNX variables in the new unit tests should access their underlying JAX arrays using .value instead of slicing with [...].
| if ( | ||
| self.use_qk_norm | ||
| or getattr(self.config, "use_qk_norm", False) | ||
| or (self.query_pre_attn_scalar is not None and self.query_pre_attn_scalar != 1.0) | ||
| ): |
There was a problem hiding this comment.
There is a potential silent correctness bug/inconsistency here.
If a model configuration has use_qk_norm: true (so self.config.use_qk_norm is True), but the attention layer is instantiated without explicitly passing use_qk_norm=True (meaning self.use_qk_norm defaults to False in Attention.__init__), then:
self.query_normandself.key_normwill beNone(QK normalization is NOT applied during the forward pass).- However,
init_query_wwill seegetattr(self.config, "use_qk_norm", False)asTrueand setdepth_scaling = 1.0(disabling depth scaling during weight initialization).
This results in a silent mismatch where weights are initialized as if QK normalization is active, but the forward pass does not actually apply QK normalization.
To prevent this, we should ensure that self.use_qk_norm in Attention.__init__ defaults to config.use_qk_norm when not explicitly provided, or at least raise an error/warning if they are inconsistent. Since Attention.__init__ is outside of this diff, please consider updating Attention.__init__ in a follow-up or expanding this PR's scope to make self.use_qk_norm the single source of truth (e.g., self.use_qk_norm = use_qk_norm or getattr(config, "use_qk_norm", False)).
There was a problem hiding this comment.
Fixed in new push. default to None for all attentions and override by config.use_qk_norm
|
|
||
| # Initial query weights should NOT be divided by sqrt(head_dim) = 16.0 | ||
| # Expected standard deviation for fan_in=2816 is 1/sqrt(2816) ~= 0.0188 | ||
| q_kernel = layer.self_attention.query.kernel[...] |
There was a problem hiding this comment.
In Flax NNX, nnx.Variable (and its subclass nnx.Param) does not support direct indexing/slicing via [...]. To access the underlying JAX array, you should use the .value attribute.
Please update this to use kernel.value instead.
| q_kernel = layer.self_attention.query.kernel[...] | |
| q_kernel = layer.self_attention.query.kernel.value |
There was a problem hiding this comment.
Replacing kernel[...] with kernel.value would re-introduce DeprecationWarnings across the codebase.
| self.assertEqual(layer.self_attention.query_pre_attn_scalar, 1.0) | ||
| self.assertTrue(layer.self_attention.use_qk_norm) | ||
|
|
||
| q_kernel = layer.self_attention.query.kernel[...] |
There was a problem hiding this comment.
In Flax NNX, nnx.Variable (and its subclass nnx.Param) does not support direct indexing/slicing via [...]. To access the underlying JAX array, you should use the .value attribute.
Please update this to use kernel.value instead.
| q_kernel = layer.self_attention.query.kernel[...] | |
| q_kernel = layer.self_attention.query.kernel.value |
There was a problem hiding this comment.
Replacing kernel[...] with kernel.value would re-introduce DeprecationWarnings across the codebase.
…ht init depth_scaling This change resolves an issue with Gemma 4 attention weight initialization and QK normalization: 1. In `src/maxtext/layers/attentions.py`, `init_query_w` now checks `self.use_qk_norm` in addition to `self.config.use_qk_norm`. When `use_qk_norm` is enabled (or `query_pre_attn_scalar != 1.0`), `depth_scaling` is set to 1.0 instead of `sqrt(head_dim)`. This prevents dividing the initial query projection weights by `sqrt(head_dim)`, ensuring unscaled canonical weight initialization for Gemma 4. 2. In Gemma 4 model configs (`gemma4-26b.yml`, `gemma4-31b.yml`, `gemma4-e2b.yml`, `gemma4-e4b.yml`), explicitly enabled `use_qk_norm: true`. 3. In `gemma4-e2b.yml` and `gemma4-e4b.yml`, set `scan_layers: false` since per-layer embeddings and variable layer configs are not compatible with layer scanning. 4. Added `tests/unit/gemma4_canonical_test.py` to verify that canonical attention scaling (`query_pre_attn_scalar = 1.0`), `use_qk_norm`, and unscaled weight initialization hold for both dense/MoE Gemma 4 and small Gemma 4 models. Added unit test: - `tests/unit/gemma4_canonical_test.py` TAG=agy CONV=f731b620-dff8-4a9f-b791-657890c4604f
c4b9418 to
f85570d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Description
This change resolves an issue with Gemma 4 attention weight initialization and QK normalization:
src/maxtext/layers/attentions.py,init_query_wnow checksself.use_qk_normin addition toself.config.use_qk_norm. Whenuse_qk_normis enabled (orquery_pre_attn_scalar != 1.0),depth_scalingis set to 1.0 instead ofsqrt(head_dim). This prevents dividing the initial query projection weights bysqrt(head_dim), ensuring unscaled canonical weight initialization for Gemma 4.gemma4-26b.yml,gemma4-31b.yml,gemma4-e2b.yml,gemma4-e4b.yml), explicitly enableduse_qk_norm: true.gemma4-e2b.ymlandgemma4-e4b.yml, setscan_layers: falsesince per-layer embeddings and variable layer configs are not compatible with layer scanning.tests/unit/gemma4_canonical_test.pyto verify that canonical attention scaling (query_pre_attn_scalar = 1.0),use_qk_norm, and unscaled weight initialization hold for both dense/MoE Gemma 4 and small Gemma 4 models.Tests
Smoke test on V5p-256 x 2 slices (Data parallel training of Gemma4-26B-A4B)
Before fix: http://tb/281018541
After fix: http://tb/281083608 log_link
Added unit test:
tests/unit/gemma4_canonical_test.pyChecklist
Before submitting this PR, please make sure (put X in square brackets):
gemini-reviewlabel.