Repository navigation
Fix sliding-window mask layer_idx in Gemma3/Gemma4 create_masks_for_vision_model - #48482
Conversation
…odel `create_masks_for_vision_model` pads `block_sequence_ids` using the KV sizes of cache layer 0, but then calls `create_causal_mask` without `layer_idx`, which defaults to the first `full_attention` layer. On Gemma3/Gemma4 checkpoints layer 0 is a `sliding_attention` layer, so with a hybrid cache the two lengths differ: * DynamicCache: the sliding mask is built with the full layer's kv_length, so it no longer matches the sliding KV cache. * StaticCache: the full layer's kv_length is the padded `max_cache_len`, so `blockwise_overlay` indexes `block_sequence_ids` out of bounds. This raises an IndexError on CPU and triggers a device-side assert (crashing the process) on CUDA and XPU. Pin both the padding and the mask itself to the same `sliding_attention` layer. Also add a regression test covering a decode step with both a dynamic and a static hybrid cache. The existing `test_attention_mask_composition` only checks prefill with `past_key_values=None`, and the tester config places `full_attention` at layer 0, so neither exercised this path.
The crash actually happens on the first (prefill) forward: Gemma3's `prepare_inputs_for_generation` clears `token_type_ids` after the first step, so decode never reaches `create_masks_for_vision_model`. With an empty StaticCache the sliding layer reports `kv_length == q_len` while the full layer reports the padded `max_cache_len`, which is exactly the divergence that made `blockwise_overlay` read out of bounds. Rename the test accordingly and parameterize it over prefill/static, decode/static and decode/dynamic.
The Gemma4 checkpoints that reach create_masks_for_vision_model (use_bidirectional_attention="vision": the 31B and 26B-A4B variants) use layer_types starting with sliding_attention and sliding_window=1024, so they hit exactly the same layer_idx mismatch as Gemma3. Mirror the gemma3 test on Gemma4Vision2TextModelTest. Verified: 3 of the 4 parameterizations fail before the fix and all pass after.
|
run-slow: gemma3, gemma4, gemma4_unified |
|
This comment contains models: ["models/gemma3", "models/gemma4", "models/gemma4_unified"] |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
| @parameterized.expand([("prefill_static",), ("decode_static",), ("decode_dynamic",)]) | ||
| def test_attention_mask_composition_with_hybrid_cache(self, scenario: str): | ||
| """The sliding mask must be sized against a sliding layer, not the first full attention one. | ||
|
|
||
| Regression test: on real Gemma3 checkpoints layer 0 is a `sliding_attention` layer, and once the | ||
| cache outgrows the sliding window its `kv_length` differs from the first `full_attention` layer's. | ||
| Sizing the sliding mask against the wrong layer yields a mask that mismatches the cache or indexes | ||
| `block_sequence_ids` out of bounds. | ||
| """ |
There was a problem hiding this comment.
so iiuc bug was found with a real ShieldGemma ckpt when we feed a very long input, and it throws a cuda index error
Why not just create a tiny test in shield gemma and call model with a long input, or add a slow test_generation_beyond_sliding_window for this failing ckpt? So we don't have to manually engineer inputs and rely on not-so-reliable create_masks_for_vision_model - utilities tend to change over time
There was a problem hiding this comment.
Fixed. We use model level tests now.
|
[For maintainers] Suggested jobs to run (before merge) run-slow: gemma3, gemma4, gemma4_unified |
CI recapDashboard: View test results in Grafana |
…r_vision_model` (huggingface#48482) * Fix sliding mask layer_idx in Gemma3/Gemma4 create_masks_for_vision_model `create_masks_for_vision_model` pads `block_sequence_ids` using the KV sizes of cache layer 0, but then calls `create_causal_mask` without `layer_idx`, which defaults to the first `full_attention` layer. On Gemma3/Gemma4 checkpoints layer 0 is a `sliding_attention` layer, so with a hybrid cache the two lengths differ: * DynamicCache: the sliding mask is built with the full layer's kv_length, so it no longer matches the sliding KV cache. * StaticCache: the full layer's kv_length is the padded `max_cache_len`, so `blockwise_overlay` indexes `block_sequence_ids` out of bounds. This raises an IndexError on CPU and triggers a device-side assert (crashing the process) on CUDA and XPU. Pin both the padding and the mask itself to the same `sliding_attention` layer. Also add a regression test covering a decode step with both a dynamic and a static hybrid cache. The existing `test_attention_mask_composition` only checks prefill with `past_key_values=None`, and the tester config places `full_attention` at layer 0, so neither exercised this path. * Cover the prefill path in the vision sliding mask regression test The crash actually happens on the first (prefill) forward: Gemma3's `prepare_inputs_for_generation` clears `token_type_ids` after the first step, so decode never reaches `create_masks_for_vision_model`. With an empty StaticCache the sliding layer reports `kv_length == q_len` while the full layer reports the padded `max_cache_len`, which is exactly the divergence that made `blockwise_overlay` read out of bounds. Rename the test accordingly and parameterize it over prefill/static, decode/static and decode/dynamic. * Cover the vision sliding mask regression on Gemma4 as well The Gemma4 checkpoints that reach create_masks_for_vision_model (use_bidirectional_attention="vision": the 31B and 26B-A4B variants) use layer_types starting with sliding_attention and sliding_window=1024, so they hit exactly the same layer_idx mismatch as Gemma3. Mirror the gemma3 test on Gemma4Vision2TextModelTest. Verified: 3 of the 4 parameterizations fail before the fix and all pass after. * Test the vision sliding mask regression through the model instead of the mask utility
Overview
create_masks_for_vision_model()padsblock_sequence_idswithlayer_idx=0, but builds the sliding mask viacreate_causal_mask()without passinglayer_idx, which defaults to the firstfull_attentionlayer. On real checkpoints layer 0 issliding_attention, so padding and mask are sized against different layers andblockwise_overlayindexes out of bounds.The two lengths only diverge when
max_cache_len > sliding_window; below that the bug is invisible.Introduced by #46850.
Reproduction
google/shieldgemma-2-4b-it,StaticCache(max_cache_len=sliding_window + 256), one 896x896 image. Before the fix, on A100:After the fix: exit 0, generates
'No'. (On XPU the same run aborts with SIGABRT instead of raising — same root cause, different surfacing.)CPU-only, no checkpoint, the bug has two faces depending on layer order:
layer_types["sliding_attention", "full_attention"]IndexError: index 13 is out of bounds for dimension 1 with size 13["full_attention", "sliding_attention"]kv_length=32, cache wants13, no exceptionThe second face — a silently wrong-shaped mask — is the more dangerous one.
Fix
Pin both the padding and the sliding mask to a sliding layer:
getattr(..., None)rather thanhasattr:past_key_values=Noneand an emptyis_slidingboth fall back to0, preserving current behaviour for models without sliding layers.Affected models
token_type_ids is not None.use_bidirectional_attention == "vision", set for the 31B and 26B-A4B variants, which uselayer_types = ["sliding_attention"] * 5 + ["full_attention"]andsliding_window=1024, i.e. the same trigger conditions. E2B/E4B haveuse_bidirectional_attention=Noneand never reach this path.Reproduced directly on both gemma3 and gemma4.
Tests
test_attention_mask_composition_with_hybrid_cache, added toGemma3Vision2TextModelTestandGemma4Vision2TextModelTest, parameterized over three scenarios. The assertions are just:Everything before them exists to reach a state where
sliding_kv_length != full_kv_length, since the bug is invisible otherwise. TheassertNotEqualguard makes a future config change that turns those assertions into tautologies fail loudly.prefill_staticIndexError: index 13 out of bounds ... size 13decode_staticIndexError: index 14 out of bounds ... size 14decode_dynamicIdentical numbers on gemma3 and gemma4. Before the fix: 3 failed, 1 passed. After: 4 passed.
Decode is reachable through
generate():prepare_inputs_for_generationclearstoken_type_idsonly aftersuper()returns, and the mask is already built inside that super call. Tracinggenerate(max_new_tokens=8)with aStaticCacheshows two calls intocreate_masks_for_vision_model,q_len=419andq_len=1.decode_dynamicis only reachable through a manualmodel(...)call, but it is the only scenario covering the silent wrong-shape face.Existing CI missed this because
test_attention_mask_compositionpassespast_key_values=None(no cache, no divergence), and the gemma3 tester config happens to putfull_attentionat layer 0.Regressions
tests/models/gemma3 + gemma4 + gemma4_unifiedmodeling tests on A100: baseline 43 failed / 1101 passed, this PR 40 failed / 1107 passed. No regressions. The 40 failures are pre-existing onmain. The 3-test delta is flaky, not fixed here — on the unpatched tree they pass 3/3 in isolation, and the failure is a1.5e-5mismatch againstatol=1e-5.ruff check,ruff format --checkandutils/check_modular_conversion.pyall pass.Files changed