Skip to content

fix: duplicate pad_between_seqs skips and wrong architecture direction - #3390

Open
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/test-attention-with-cp-duplicate-pad-between-seqs-skips-and
Open

fix: duplicate pad_between_seqs skips and wrong architecture direction#3390
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:bugfix/test-attention-with-cp-duplicate-pad-between-seqs-skips-and

Conversation

@andrewwhitecdw

@andrewwhitecdw andrewwhitecdw commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

This PR addresses the following issue in tests/pytorch/attention/test_attention_with_cp.py: duplicate pad_between_seqs skips and wrong architecture direction.

Changes

  • tests/pytorch/attention/test_attention_with_cp.py: duplicate pad_between_seqs skips and wrong architecture direction.

Details

--- a/tests/pytorch/attention/test_attention_with_cp.py
+++ b/tests/pytorch/attention/test_attention_with_cp.py
@@ -1,15 +1,7 @@
-    if pad_between_seqs:
-        if qkv_format != "thd":
-            pytest.skip("pad_between_seqs only applies to THD format!")
-        if not FlashAttentionUtils.v3_is_installed or get_device_compute_capability() > (9, 0):
-            pytest.skip("pad_between_seqs with CP requires Flash Attention v3 on Hopper (sm90)!")
-        if cp_comm_type == "a2a+p2p":
-            pytest.skip("pad_between_seqs is not yet supported with A2A+P2P CP comm type!")
-
-    if pad_between_seqs:
-        if qkv_format != "thd":
-            pytest.skip("pad_between_seqs only applies to THD format!")
-        if not FlashAttentionUtils.v3_is_installed:
-            pytest.skip("pad_between_seqs with CP requires Flash Attention v3!")
-        if cp_comm_type == "a2a+p2p":
-            pytest.skip("pad_between_seqs is not yet supported with A2A+P2P CP comm type!")
+    if pad_between_seqs:
+        if qkv_format != "thd":
+            pytest.skip("pad_between_seqs only applies to THD format!")
+        if not FlashAttentionUtils.v3_is_installed or get_device_compute_capability() < (9, 0):
+            pytest.skip("pad_between_seqs with CP requires Flash Attention v3 on Hopper (sm90)!")
+        if cp_comm_type == "a2a+p2p":
+            pytest.skip("pad_between_seqs is not yet supported with A2A+P2P CP comm type!")

Tests

  • tests/pytorch/attention/test_attention_with_cp.py

Greptile feedback addressed

  • Tightened the architecture guard from < (9, 0) to != (9, 0), so only exact sm90 (Hopper) devices fall through to the pad_between_seqs path and post-Hopper GPUs get the correct skip message.

Local verification: python3 -m py_compile tests/pytorch/attention/test_attention_with_cp.py passed. Full pytest execution was not feasible because transformer-engine is not installed in this environment.

@github-actions github-actions Bot added the community-contribution PRs from external contributor outside the core maintainers, representing community-driven work. label Aug 17, 2026
@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR corrects and consolidates the eligibility checks for context-parallel attention tests using padding between sequences.

  • Requires FlashAttention v3 on exactly SM90.
  • Removes a duplicate pad_between_seqs skip block.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
tests/pytorch/attention/test_attention_with_cp.py Consolidates duplicate skip logic and aligns the architecture guard with the production requirement that FlashAttention v3 run only on SM90.

Reviews (2): Last reviewed commit: "fix: duplicate pad_between_seqs skips an..." | Re-trigger Greptile

Comment thread tests/pytorch/attention/test_attention_with_cp.py Outdated
- Deduplicate the pad_between_seqs eligibility checks in test_cp_with_flash_attention.

- Restrict the FlashAttention v3 guard to exactly Hopper (sm90) so post-Hopper architectures are skipped with the correct diagnostic message.

Signed-off-by: Andrew White <andrewwhitecdw@users.noreply.github.com>
@andrewwhitecdw
andrewwhitecdw force-pushed the bugfix/test-attention-with-cp-duplicate-pad-between-seqs-skips-and branch from 3c53dde to d6893e9 Compare August 17, 2026 22:26
@andrewwhitecdw

Copy link
Copy Markdown
Contributor Author

@greptile-apps thanks for the review. I tightened the guard as suggested:

  • Changed the architecture check from get_device_compute_capability() < (9, 0) to != (9, 0), so the pad_between_seqs path is only entered on exact Hopper (sm90) devices and post-Hopper architectures skip with the correct diagnostic message.

The branch has been amended to a single signed-off commit (d6893e9) and force-pushed. python3 -m py_compile tests/pytorch/attention/test_attention_with_cp.py passed; full pytest was not feasible because transformer-engine is not installed in this environment.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

The != (9, 0) tightening is more conservative, but it's worth flagging a trade-off:

!= (9, 0) blocks the test on any post-Hopper architecture (e.g., Blackwell sm100). If Flash Attention v3 later adds pad_between_seqs support on those GPUs, the guard would silently skip the test there even though it should run — with no obvious signal to whoever adds that FA3 support.

< (9, 0) (what the PR currently shows) is the standard "minimum capability" idiom. It skips on pre-Hopper, runs on Hopper and anything newer, and naturally picks up future architectures as FA3 support expands.

If the intent is strictly "sm90 only, forever" (e.g., there's a known microarchitectural reason post-Hopper can't support this code path), then != (9, 0) is correct. But if the intent is simply "requires at least Hopper because FA3 currently only ships for sm90", then < (9, 0) is the better expression of that intent.

Could you clarify which scenario applies? If it's the latter, I'd recommend keeping < (9, 0) from the PR diff rather than the amended != (9, 0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution PRs from external contributor outside the core maintainers, representing community-driven work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant