Skip to content

frost(sdpa): fix sm120 split_kv test after #720 combine_rows change - #758

Merged
Aneureka merged 1 commit into
NVIDIA:developfrom
vedaanta:vagarwalla/sm120-split-kv-combine-rows
Aug 26, 2026
Merged

frost(sdpa): fix sm120 split_kv test after #720 combine_rows change#758
Aneureka merged 1 commit into
NVIDIA:developfrom
vedaanta:vagarwalla/sm120-split-kv-combine-rows

Conversation

@vedaanta

@vedaanta vedaanta commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

Test plan

  • py_compile on the changed file
  • black -l 160 clean
  • CI: frost:rel:sdpa:sm120 green

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Updated split-key/value attention test expectations to match the combine grid dimensions.
    • Improves test accuracy for batched and multi-query attention scenarios.

choose_split_kv gained a required combine_rows keyword-only argument for
the combine-pass cost term (NVIDIA#720), but the sm120 expected-split helper
wasn't updated to match, unlike its sm100 sibling in
test_sdpa_fwd_split_kv_sm100.py which already passes it. The squash-merge
of NVIDIA#720 dropped the follow-up fix (yanzhuo607#1), so
develop's sm120 CI (frost:rel:sdpa:sm120) is broken again:

    TypeError: choose_split_kv() missing 1 required keyword-only argument: 'combine_rows'

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 2ebd4579-0734-43a1-893a-75d9b9bf195f

📥 Commits

Reviewing files that changed from the base of the PR and between 0ee056c and 8afe31e.

📒 Files selected for processing (1)
  • test/python/sdpa/frost/test_sdpa_fwd_split_kv_sm120.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The SDPA forward split-KV test now passes the calculated combine-grid row count to choose_split_kv.

Changes

SDPA split-KV validation

Layer / File(s) Summary
Pass combine-grid rows to split selection
test/python/sdpa/frost/test_sdpa_fwd_split_kv_sm120.py
_expected_split passes api.batch_size * api.h_q * api.s_q_max as combine_rows to choose_split_kv.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 8afe3

This localized test-only change updates the sm120 helper to provide the required combine_rows value, with no actionable merge-blocking risk remaining after normal checks and review.

Suggested reviewers: aneureka

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the problem, the fix, and testing status. It does not include the required Affected area, Why, Related issues, or API and compatibility impact sections, and it does not docume… Add the missing template sections. Select the affected area, explain why the approach is appropriate, state related issues using the required format, document API and compatibility impact or write None, and complete the repository checklist…
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the sm120 split-KV test fix and its cause after the combine_rows change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, the fix, and testing status. It does not include the required Affected area, Why, Related issues, or API and compatibility impact sections, and it does not document the required repository checklist items.

Resolution

Add the missing template sections. Select the affected area, explain why the approach is appropriate, state related issues using the required format, document API and compatibility impact or write None, and complete the repository checklist items. Keep the exact test commands and results, including the pending CI status.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@vedaanta

Copy link
Copy Markdown
Collaborator Author

@cudnn-ci-bot run frost

@cudnn-ci-bot

cudnn-ci-bot commented Aug 26, 2026

Copy link
Copy Markdown

🏁 Pipeline finished

SHA: 8afe31e
Targets: frost
Branch: cudnn-gh/pr-758-8afe31e
Pipeline: 64603680
Last updated: 2026-08-26 05:32 UTC

@Aneureka Aneureka added orig-nv-eng Reported or requested by NVIDIA engineering. mod-frost cat-bugfix labels Aug 26, 2026
@Aneureka Aneureka added this to the Frontend 1.29.0 milestone Aug 26, 2026
@Aneureka
Aneureka merged commit 8acc777 into NVIDIA:develop Aug 26, 2026
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat-bugfix mod-frost orig-nv-eng Reported or requested by NVIDIA engineering.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants