Gemma4: fix failed test cases#45568
Conversation
Signed-off-by: Liu, Kaixuan <kaixuan.liu@intel.com>
Signed-off-by: Liu, Kaixuan <kaixuan.liu@intel.com>
Signed-off-by: Liu, Kaixuan <kaixuan.liu@intel.com>
Signed-off-by: Liu, Kaixuan <kaixuan.liu@intel.com>
Signed-off-by: Liu, Kaixuan <kaixuan.liu@intel.com>
Signed-off-by: Liu, Kaixuan <kaixuan.liu@intel.com>
|
[For maintainers] Suggested jobs to run (before merge) run-slow: gemma4 |
| if attention_mask is not None: | ||
| attention_mask = self._convert_4d_mask_to_blocked_5d(attention_mask) |
There was a problem hiding this comment.
@Cyrilvallez any opinion.
From PR descriptioin
Fix bug when attention_mask is None(tests/models/gemma4/test_modeling_gemma4.py::Gemma4Audio2TextModelTest::test_eager_matches_fa2_generate)
| @unittest.skip( | ||
| "Under non-bf16 dtypes, MoE grouped_mm falls back to " | ||
| "_grouped_mm_fallback_backward which is incompatible with torch.compile." | ||
| ) | ||
| def test_flash_attn_2_can_compile_with_attention_mask_None_without_graph_break(self): | ||
| pass | ||
|
|
||
| @unittest.skip( | ||
| "Under non-bf16 dtypes, MoE grouped_mm falls back to " | ||
| "_grouped_mm_fallback_backward which is incompatible with torch.compile." | ||
| ) | ||
| def test_torch_compile_for_training(self): | ||
| pass |
There was a problem hiding this comment.
OK for me. Just let one of @Cyrilvallez or @vasqu to also valid or comment
There was a problem hiding this comment.
They are indeed failing on our CI too
There was a problem hiding this comment.
Hmm, iirc the fallback should be compile compatible cc @IlyasMoutawwakil
There was a problem hiding this comment.
Which torch version is CI now at btw?
There was a problem hiding this comment.
it is torch compileable, just not any mode that uses cuda graphs (like max-autotune), where the torch.grouped_mm also fails on <sm90 (it also uses the fallback path)
| @require_flash_attn | ||
| @require_torch_accelerator | ||
| @mark.flash_attn_test | ||
| @slow | ||
| def test_flash_attn_2_from_config(self): | ||
| # Gemma4 requires mm_token_type_ids in train mode, so we test in eval mode | ||
| self.flash_attn_from_config(attn_implementation="flash_attention_2", test_fwd_in_train=False) | ||
|
|
||
| @require_flash_attn_3 | ||
| @require_torch_gpu | ||
| @mark.flash_attn_3_test | ||
| @slow | ||
| def test_flash_attn_3_from_config(self): | ||
| # Gemma4 requires mm_token_type_ids in train mode, so we test in eval mode | ||
| self.flash_attn_from_config(attn_implementation="flash_attention_3", test_fwd_in_train=False) |
There was a problem hiding this comment.
@kaixuanliu I didn't see these 2 failing on our Flash Attn CI job.
Could you share more info / error logs ?
There was a problem hiding this comment.
Our flash attn ci doesn have FA3 - I think it's hard to install because you need to compile from source and it's much longer than FA2 build from source
Maybe we could add a separate FA4 CI - not sure how stable it is tho since it's still in beta
There was a problem hiding this comment.
Well, for FA3 and FA4, on my env they are skipped as well. I can delete these two.
There was a problem hiding this comment.
No, I mean for
test_flash_attn_2_from_config
our CI is [PASSED]. So I am not sure why we need this fix, at least for FA2.
Our CI runner don't have FA3 or FA4, so they are skipped. But the question may still valid: do we really this fix?
| pass | ||
|
|
||
| @unittest.skip("The base test does not pass image_position_ids and mm_token_type_ids required by Gemma4") | ||
| def test_flash_attn_4_inference_equivalence_right_padding(self): |
There was a problem hiding this comment.
Can we have something like
transformers/tests/models/dia/test_modeling_dia.py
Lines 258 to 271 in 727741f
But for FA? Imo it will always be quite a lot to skip these manually like that
What does this PR do?
This PR did several things:
attention_maskis None(tests/models/gemma4/test_modeling_gemma4.py::Gemma4Audio2TextModelTest::test_eager_matches_fa2_generate)test_flash_attn_x_from_configFixes # (issue)
Code Agent Policy
Who can review?
@ydshieh pls help review