🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/172671

• 📄 Preview Python docs built from this PR
• 📄 Preview C++ docs built from this PR
• ❓ Need help or want to give feedback on the CI? Visit the bot commands wiki

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit b56ea73 with merge base cee5acf ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@claude can you review this one please look for any cargo cult patterns that shouldnt apply

Claude finished @drisspg's task in 1m 17s —— View job

PR Review: Cargo Cult Patterns in test_fa3.py

• Read the test file and understand the structure
• Identify cargo cult patterns that don't apply to FA3
• Check for code that could be shared/made DRY
• Provide detailed review feedback

I've compared test_fa3.py with test_fa4.py and analyzed the code for cargo cult patterns. Here are my findings:

🔴 Major DRY Violations - Identical Code Between test_fa3.py and test_fa4.py

The following code is nearly 100% identical between both files and should be extracted to a common utility:

1. SdpaShape namedtuple (line 28)

   • Exact duplicate in both files

2. cuda_kernel_profiler context manager (lines 44-54)

   • Nearly identical, with minor differences:
     • FA3 version includes ProfilerActivity.CPU in addition to CUDA
     • FA4 filters by evt.device_type == torch.autograd.DeviceType.CUDA
   • The CPU activity inclusion in FA3 may be cargo cult - Fix this →

3. flash_vs_math function (lines 57-123)

   • Exactly identical - 67 lines of duplicated code

4. DummyHandle class (lines 126-132)

   • Identical helper class for testing

5. _assert_flash_matches_math method (lines 144-209)

   • Identical except for skipUnless decorator

6. test_flash_attention_matches_math (lines 227-237)

   • Nearly identical, but FA4 has an extra test_backward = head_dim == 128 and dtype == torch.float16 condition due to a known issue. FA3 always tests backward.

7. test_*_kernel_called (lines 241-281)

   • Similar structure but different kernel patterns

8. test_multiple_activate (lines 284-321)

   • Identical except for the final activate_flash_attention_impl("FA3/FA4") calls

9. test_compiled_sdpa_*_metadata (lines 325-349)

   • Identical

10. test_compiled_sdpa_*_matches_math (lines 353-401)

    • Identical

11. test_compiled_sdpa_*_backward_matches_math (lines 405-484)

    • Nearly identical. FA3 adds min_atol handling (lines 467, 473-476) which FA4 lacks

12. test_attention_preserves_query_layout (lines 487-522)

    • Identical

🟡 Potential Cargo Cult Patterns

1. SM90 check in _fa3_dependencies_available (line 35):

   if major != 9:  # FA3 requires Hopper (SM90)

   FA4 checks major not in (9, 10) (supports both Hopper and Blackwell). Verify if FA3 truly only supports SM90 or if this is overly restrictive.

2. Hardcoded rtol values (multiple locations):

   • rtol=2 appears throughout both files. If this tolerance is correct for one implementation, it may not be optimal for another.

3. min_atol in backward test (lines 467, 473-476 in FA3):

   min_atol = 0.05 if dtype == torch.bfloat16 else 0.01

   FA3 adds this but FA4 doesn't have it. Either FA4 should also have this, or it's unnecessary cargo cult.

🟢 FA3-Specific Code That Looks Correct

The FP8 tests (lines 524-782) appear to be FA3-specific functionality and are appropriately not duplicated from FA4.

📋 Recommendations

Per Driss's review comment, I'd suggest:

1. Create a common test utilities module at test/nn/attention/_sdpa_test_utils.py:

   • Move SdpaShape, cuda_kernel_profiler, flash_vs_math, DummyHandle
   • Consider creating a base test class SDPATestMixin with shared test methods

2. Parameterize the implementation-specific parts:

   • Dependency check function
   • Kernel pattern strings
   • Activation function name ("FA3" vs "FA4")

3. Example refactor structure:

   # test/nn/attention/_sdpa_test_utils.py
   SdpaShape = namedtuple("Sdpa_Shape", ["batch", "num_heads", "seq_len", "head_dim"])
   
   @contextmanager
   def cuda_kernel_profiler(kernel_pattern): ...
   
   def flash_vs_math(test_case, q, k, v, is_causal=False, rtol=2): ...
   
   class DummyHandle: ...

   # test/nn/attention/test_fa3.py
   from _sdpa_test_utils import SdpaShape, cuda_kernel_profiler, flash_vs_math, DummyHandle

This would reduce the ~350 lines of duplicated code significantly.

looks like alot of cargo cult from other test file which is good maybe see if there are pieces that are very easily reusable between this fa4 and test_transformers even which has a bulk of sdap tests and make a lil more DRY if possible

Makes sense, I used a lot of the same tests as fa4, so I put it in a common file and referenced it in both test files. I didn't want to mess with the test_transformers file too much since that's less experimental

@pytorchbot merge

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team