test: align test_fmha_v2_prefill SM gating with is_sm12x_supported - #3182
Conversation
📝 WalkthroughWalkthroughThe test file updates GPU support checks from SM120a-specific to SM12x-family support, changing from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/attention/test_fmha_v2_prefill.py (2)
492-502: Optional: hoist the repeatedis_sm12x_supportedcheck.
is_sm12x = is_sm12x_supported(torch.device("cuda"))is computed twice per test invocation (Line 492 and Line 758). Since the device is fixed for the run, you could compute it once at the top ofrun_trtllm_fmha_v2_prefill_caseand reuse it, eliminating the duplicate compute-capability lookup. Not a correctness concern.♻️ Suggested refactor
- if not is_sm90a_supported(torch.device("cuda")) and not is_sm12x_supported( - torch.device("cuda") - ): + is_sm12x = is_sm12x_supported(torch.device("cuda")) + if not is_sm90a_supported(torch.device("cuda")) and not is_sm12x: pytest.skip("FMHA v2 requires SM90+ (Hopper) or SM12x GPUs.") # Skip invalid combinations - is_sm12x = is_sm12x_supported(torch.device("cuda")) if dtype == torch.float8_e4m3fn and is_sm12x: @@ - is_sm12x = is_sm12x_supported(torch.device("cuda")) if logits_soft_cap > 0:Also applies to: 756-767
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/attention/test_fmha_v2_prefill.py` around lines 492 - 502, The test recomputes is_sm12x_supported(torch.device("cuda")) multiple times; hoist this call to a single local variable at the start of run_trtllm_fmha_v2_prefill_case and replace the duplicate calls with that variable (e.g., keep the existing is_sm12x variable and remove the second call), referencing is_sm12x_supported and is_sm12x so all conditional skips use the precomputed is_sm12x value.
740-755: Doc comment mentions SM90/SM120 explicitly while logic now covers SM12x.The block comment (Lines 742-755) describes formats as "SM90 (Hopper)" vs "SM120 (Blackwell) tiled", but the runtime branch at Line 760 keys off
is_sm12x(which includes SM121+). Consider updating the wording to "SM12x" for consistency with the gate. Minor doc nit; behavior is correct assuming all SM12x variants share the sameSoftmax_saversemantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/attention/test_fmha_v2_prefill.py` around lines 740 - 755, Update the doc comment to match the runtime branch naming: replace occurrences of "SM120 (Blackwell)" with "SM12x (Blackwell family)" (and any other explicit "SM120" mentions) so the description aligns with the gating variable is_sm12x; keep the explanation of how max is stored and how lse is computed for the two cases (SM90 vs SM12x) and ensure phrases like "SM12x tiled" or "SM12x (Blackwell)" are used consistently to reflect all SM12x variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/attention/test_fmha_v2_prefill.py`:
- Around line 492-502: The test recomputes
is_sm12x_supported(torch.device("cuda")) multiple times; hoist this call to a
single local variable at the start of run_trtllm_fmha_v2_prefill_case and
replace the duplicate calls with that variable (e.g., keep the existing is_sm12x
variable and remove the second call), referencing is_sm12x_supported and
is_sm12x so all conditional skips use the precomputed is_sm12x value.
- Around line 740-755: Update the doc comment to match the runtime branch
naming: replace occurrences of "SM120 (Blackwell)" with "SM12x (Blackwell
family)" (and any other explicit "SM120" mentions) so the description aligns
with the gating variable is_sm12x; keep the explanation of how max is stored and
how lse is computed for the two cases (SM90 vs SM12x) and ensure phrases like
"SM12x tiled" or "SM12x (Blackwell)" are used consistently to reflect all SM12x
variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ba88841-b2d9-4214-a141-79f3f243c6ae
📒 Files selected for processing (1)
tests/attention/test_fmha_v2_prefill.py
There was a problem hiding this comment.
Code Review
This pull request refactors the GPU architecture checks in the FMHA v2 prefill tests by replacing the specific is_sm120a_supported function with the more general is_sm12x_supported. Correspondingly, variable names and test skip messages have been updated to reference SM12x instead of SM120+. I have no feedback to provide.
|
/bot run |
📌 Description
The function-level skip in
test_fmha_v2_prefill.pychecksis_sm120a_supported, which only matches sm_120, but the actual FMHAv2 dispatch (prefill.py:4498,:4540) is gated onis_sm12x_supported—so SM121 ends up skipped from these tests even though the kernel path supports it.This swaps the test gates to
is_sm12x_supported. The top-level skip message already said"SM12x"(only the gate was lagging). The three sub-skips (FP8 / SEPARATE_Q_K_V / SLIDING_WINDOW) previously said"SM120+"; they're updated to"SM12x"to match what the gate actually means.sm_120behavior is unchanged: both helpers evaluate True for sm_120 on CUDA ≥ 12.8 (is_sm120a_supported:major == 12 and minor==0;is_sm12x_supported:major == 12, with a CUDA-version branch that collapses to ≥ 12.8 on minor 0).Verified on RTX Pro 6000 (sm_120, CUDA 12.9): 233 passed, 2112 skipped,0 failed.
🔍 Related Issues
Refs #3170 (Action Item 1 / T1).
🚀 Pull Request Checklist
✅ Pre-commit Checks
pre-commitby runningpip install pre-commit(or used your preferred method).pre-commit install.pre-commit run --all-filesand fixed any reported issues.🧪 Tests
unittest, etc.).Summary by CodeRabbit