Follow-up to #449: allowlist I/O dtype check on both SDPA paths by egilliam-nv · Pull Request #454 · NVIDIA/cudnn-frontend

egilliam-nv · 2026-07-30T17:56:25Z

Follow-up to #449 addressing review feedback. Together with #449 this should fully resolve #424 (which was re-opened because #449 alone wasn't fully satisfactory).

Fixes #424.

Changes

Check the Q/K/V/O I/O dtype on both the UNIFIED and COMPOSITE paths of verify_sdpa_support_surface_for_implementation, via a shared io_dtypes_within allowlist helper.
- Unified allows {fp16, bf16, fp8}.
- Composite additionally allows fp32 — matching the composite forward and backward engines, which support FP32/FP16/BF16/FP8 I/O and nothing else.
- Neither allows fp64 (or any other dtype).
Error "with what is allowed." The messages lead with the supported set and reject any other dtype generically, so e.g. fp64 is caught with a sensible message instead of prompting a follow-up bug.
Correct the unified dynamic-shape message — dynamic shape is not planned; override shape is the supported mechanism.

Behavior

fp32 is rejected on unified, accepted on composite; under AUTO, fp32 auto-selects composite (unchanged from Reject FP32 I/O in the unified SDPA node #449).
fp64 (and other unsupported dtypes) are rejected on both explicit paths; under AUTO, neither path accepts them so auto-select finds no implementation.

Testing

test/python/test_sdpa_fp32_rejected.py, addressing the review comments on #449/#454:

Each Q/K/V/O port is checked independently (one unsupported port, rest FP16), asserting the rejection comes from our unified/composite dtype check.
fp64 rejected on unified + composite; fp32 rejected on unified but accepted on composite/AUTO.
The fp32-composite and fp16/bf16-unified positive tests fail on a wrong-path rejection but skip on genuine engine/capability unavailability.

Verified on A100 (sm80) and H100 (sm90): 16 passed.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved SDPA validation for tensor data types across unified and composite implementations.
- Composite attention now supports FP32 input and output tensors while continuing to reject unsupported FP64 tensors.
- Unified attention consistently enforces its supported FP16, BF16, and FP8 data types.
- Updated dynamic-shape validation messaging for clearer guidance.
Tests
- Expanded coverage for per-port Q/K/V/output data-type validation.
- Added verification that automatic implementation selection routes FP32 workloads to the composite implementation.

Address review feedback on NVIDIA#449 (disallow fp32 for unified SDPA): - Check the Q/K/V/O I/O dtype on BOTH the UNIFIED and COMPOSITE paths of verify_sdpa_support_surface_for_implementation, via a shared io_dtypes_within allowlist helper. Unified allows {fp16, bf16, fp8}; composite additionally allows fp32 (matching the fort composite fwd + bwd engines, which support FP32/FP16/BF16/FP8 I/O and nothing else). Neither allows fp64. - Phrase the errors in terms of what IS allowed and reject any other dtype generically, so e.g. fp64 is caught with a sensible message rather than prompting a follow-up bug. - Extend the regression test: fp64 is rejected on the unified and composite paths; fp32 is rejected on unified but accepted on composite/AUTO; fp16/bf16 still build on unified. Also correct the unified dynamic-shape message (dynamic shape is not coming; override shape is the supported mechanism). Verified on A100 (sm80) and H100 (sm90): 7 passed. Signed-off-by: Emil Gilliam <egilliam@nvidia.com>

coderabbitai · 2026-07-30T17:56:49Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5446decb-a5f2-4479-8a22-61fc69857bae

📥 Commits

Reviewing files that changed from the base of the PR and between 932bf11 and 2cc23e4.

📒 Files selected for processing (1)

test/python/test_sdpa_fp32_rejected.py

📝 Walkthrough

Walkthrough

SDPA support validation now uses a shared I/O dtype helper for COMPOSITE and UNIFIED implementations. Tests add per-port FP64 rejection coverage and expand FP32, FP16, and BF16 behavior checks.

Changes

SDPA dtype validation

Layer / File(s)	Summary
Shared implementation dtype checks `include/cudnn_frontend/node/sdpa_support_surface.h`	COMPOSITE and UNIFIED validation use shared permitted-dtype checks, including FP32 support for COMPOSITE and updated rejection messages.
SDPA dtype regression coverage `test/python/test_sdpa_fp32_rejected.py`	Adds per-port FP64 and unified unsupported-dtype rejection tests, explicit output dtype coverage, and expands FP32 COMPOSITE/AUTO and FP16/BF16 unified coverage.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: anerudhan

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description covers the change, issue link, behavior, and testing, but it misses several template sections like Affected area and API impact.	Add the missing template sections: Before submitting, Affected area, Summary, Why, API and compatibility impact, and keep Testing exact.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding I/O dtype allowlist checks on both SDPA paths.
Linked Issues check	✅ Passed	The PR validates SDPA I/O dtypes on both paths and rejects unsupported float32/float64 cases, satisfying `#424`'s validation requirement.
Out of Scope Changes check	✅ Passed	The added tests and message updates are directly tied to the dtype-validation fix and do not introduce unrelated scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/python/test_sdpa_fp32_rejected.py`:
- Around line 80-84: Update test_fp16_bf16_unified_supported to guard the
UNIFIED FP16/BF16 capability before calling _build_sdpa, skipping when the
current cuDNN/GPU combination lacks support while retaining execution for
supported configurations and both parametrized dtypes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cf87c277-9062-4f7e-9eb5-dbf2d8067e9c

📥 Commits

Reviewing files that changed from the base of the PR and between 57dc579 and 2ca27de.

📒 Files selected for processing (2)

include/cudnn_frontend/node/sdpa_support_surface.h
test/python/test_sdpa_fp32_rejected.py

Address CodeRabbit review comments on NVIDIA#449 and NVIDIA#454: - Exercise each Q/K/V/O port independently: override exactly one port with an unsupported dtype (rest FP16) so a check that only looked at Q would be caught. Assert via pytest.raises(match=...) that the rejection is our unified/composite dtype check rather than an incidental one (the intended validation stage). - Don't mask a unified-misroute as a missing composite engine: in the FP32 composite/AUTO test, fail if the rejection came from the unified node; skip only on genuine composite-engine unavailability. - Capability-guard the positive FP16/BF16 unified test: skip when unified SDPA is unsupported on this cuDNN/GPU combo, but still fail if our dtype guard wrongly rejects FP16/BF16. Not addressing the FP8-coverage nitpick: FP8 needs the sdpa_fp8 path with descale/scale tensors, out of scope for this dtype-rejection test. Verified on A100 (sm80) and H100 (sm90): 16 passed. Signed-off-by: Emil Gilliam <egilliam@nvidia.com>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/python/test_sdpa_fp32_rejected.py`:
- Around line 123-127: Update the exception handling in
test_sdpa_fp32_rejected.py so a Composite SDPA node rejection is asserted as a
test failure rather than skipped. Before treating the exception as genuine
engine unavailability, assert that its message contains neither the unified-node
rejection nor the composite-engine rejection, preserving skips only for
unrelated unavailability errors.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: b48b4926-e9f2-438f-ab71-8df53674f5b0

📥 Commits

Reviewing files that changed from the base of the PR and between 2ca27de and 932bf11.

📒 Files selected for processing (1)

test/python/test_sdpa_fp32_rejected.py

egilliam-nv · 2026-07-30T18:12:45Z

@cudnn-ci-bot run

cudnn-ci-bot · 2026-07-30T18:13:22Z

🚀 Running mirror pipeline

Branch: cudnn-gh/pr-454-932bf11
Pipeline: 60341111

Address further CodeRabbit comments on NVIDIA#454: the FP16/BF16-unified and FP32-composite/AUTO positive tests skipped on any GRAPH_NOT_SUPPORTED, which would mask a real regression where our own support-surface allowlist wrongly rejects a dtype it should accept (e.g. composite rejecting FP32, or unified rejecting FP16/BF16). Add a shared _is_dtype_allowlist_rejection() helper (our unified/composite messages both contain "SDPA node supports only"); both positive tests now fail on such a rejection and skip only on a genuine engine/backend unavailability, which surfaces a different message. Verified on A100 (sm80) and H100 (sm90): 16 passed. Signed-off-by: Emil Gilliam <egilliam@nvidia.com>

egilliam-nv added cat-bug Reports of incorrect behavior, crashes, regressions, or unexpected results. mod-frontend cuDNN frontend APIs, operation graph construction, plans, and user-facing wrappers. orig-nv-eng Reported or requested by NVIDIA engineering. labels Jul 30, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Follow-up to #449: allowlist I/O dtype check on both SDPA paths - #454

Follow-up to #449: allowlist I/O dtype check on both SDPA paths#454
egilliam-nv wants to merge 3 commits into
NVIDIA:developfrom
egilliam-nv:egilliam/sdpa_fp32_followup

egilliam-nv commented Jul 30, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

egilliam-nv commented Jul 30, 2026

Uh oh!

cudnn-ci-bot commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

egilliam-nv commented Jul 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Behavior

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

egilliam-nv commented Jul 30, 2026

Uh oh!

cudnn-ci-bot commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

egilliam-nv commented Jul 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 30, 2026 •

edited

Loading