Repository navigation
fix(rocke): name seqlen and tile when dense attention rejects a partial tile - #14335
Conversation
…clines a partial tile The dense _base_spec factories set ragged=True for any self-attention length that is not a tile multiple, so Sq = Skv = 384 or 1152 at block_m 256 is an ordinary graph that takes the on-chip ragged path. The gfx942 predicate then declined with "ragged not yet supported" and the shared spec validator with "ragged is not supported with sliding_window", both of which read as a varlen problem and name nothing the caller can act on. partial_tile_reason(spec) names the length and the tile. The gfx942 predicate now says "gfx942 attention_dense needs whole tiles: seqlen_q 384 is not a multiple of block_m 256 (the partial-tile path is not yet supported)" and the sliding-window check says "sliding_window needs whole tiles: <reason> (the partial-tile path has no window mask)". Varlen messages are unchanged. What could break: anything matching the old "ragged not yet supported" or "ragged is not supported with sliding_window" text. No other caller in the repo matches either string. Checked host only, no GPU: tests/dispatch/attention plus tests/test_attention_dense_gfx942.py give 752 passed, 75 skipped, 2600 subtests passed. With the two messages reverted, the three updated tests fail. black 25.12.0 --check is clean on the changed files.
…in the unified mask tests The sliding_window branch of AttentionDenseSpec.__post_init__ no longer says "ragged", so test_ragged_window_is_declined_not_served asserted text that this branch removed. It now checks the same three parts the gfx950 and gfx942 wiring tests check: the query length against block_m, the key length against block_n, and that the reason still names sliding_window and no longer says ragged.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
🟢 Approval recommended
The diagnostic-only changes preserve rejection behavior and include focused regression coverage, with no unresolved blocking issues.
No open issues 🎉
What changed in this PR
This PR makes rocKE dense-attention rejection messages actionable by naming sequence lengths and tile sizes instead of describing ordinary partial-tile inputs as “ragged.” Existing rejection conditions and varlen messages remain unchanged.
Changes:
- Adds a shared helper that explains which sequence lengths are not tile multiples.
- Updates gfx942 and sliding-window rejection messages to use that explanation.
- Expands regression assertions for partial-tile diagnostics through dispatch and kernel support checks.
| File | Description |
|---|---|
dnn-providers/hip-kernel-provider/rocke/library/tests/test_attention_dense_gfx942.py |
Checks the specific length-and-tile rejection message. |
dnn-providers/hip-kernel-provider/rocke/library/tests/dispatch/attention/test_unified_mask_wiring.py |
Verifies partial-tile window refusals identify both lengths and tiles. |
dnn-providers/hip-kernel-provider/rocke/library/tests/dispatch/attention/test_gfx950_dense_wiring.py |
Updates sliding-window diagnostic assertions. |
dnn-providers/hip-kernel-provider/rocke/library/tests/dispatch/attention/test_gfx942_dense_wiring.py |
Covers multiple partial-tile lengths and omits aligned dimensions from diagnostics. |
dnn-providers/hip-kernel-provider/rocke/library/kernels/gfx942/attention_dense.py |
Uses the shared explanation for partial-tile rejection. |
dnn-providers/hip-kernel-provider/rocke/library/kernels/common/attention_dense_spec.py |
Adds the diagnostic helper and updates sliding-window validation wording. |
Review effort: Balanced
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14335 — name seqlen and tile when dense attention rejects a partial tile (yassinsolim) — 🟢 First-round: no closer look needed — wording-only; every verdict matches develop; the red summaries are cancelled duplicate runs
Net-new: none. Utility: 🟢 Positive — refusals name the failing length and tile, not "ragged"; verdicts unchanged. Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted First-round review: a local qwen3.6 model made the independent first pass. The orchestrator (Claude Opus 5.5) verified it. On a clean merge with today's develop, it ran the native build and ctests, the C++/Python library parity, the differential goldens and the PR's four test files, plus a probe that compares every accept/reject verdict and message with develop's. It also checked develop's changes since the PR's base for drift, read the predecessor #13681's reviews, and read the CI jobs at head, including the two cancelled duplicate runs. The orchestrator owns this merge-worthiness call. It flags whether a closer look is needed; it is not an approval or a rejection. |
AnaghaRaoAMD
left a comment
There was a problem hiding this comment.
Merged into current develop (43e2cb7aa47), this branch fails 14 tests in the files it touches, while develop alone passes all 558. #14367 merged after this branch's base (28af48c) and made the dense block_m depend on the shape, so some shapes now build at 128. Because of that, the hardcoded block_m 256 expectations no longer hold, and gfx942 now accepts Sq = Skv = 384. CI here is green because it ran on the old base. The source change still applies cleanly and is still needed, since both old messages are still on develop. It needs a develop merge and test updates (details inline).
…l-tile tests Develop now builds these requests at block_m 128 instead of 256, so the hardcoded 256 and the Sq = Skv = 384 case no longer hold. The gfx942 test reads block_m and block_n from the default spec, the gfx950 tests parse them from the refusal message, and the 384 case is dropped since a 128-row tile accepts it.
|
Automated review: Model - qwen3.6:35b-a3b-coding-mtp-q4_K_M, effort: max PR #14335 — name seqlen and tile when dense attention rejects a partial tile (yassinsolim) — 🟠 Follow-up: no test pins the tile
Utility: 🟢 Positive — refusals name the failing length and tile; tests follow develop's picks Legend: 🔴 blocking · 🟠 should-fix · 🟡 minor nit · 🟢 info/good · ⚪ retracted Follow-up review: first pass by a local qwen3.6 model, verified by the orchestrator (Claude Opus 5.5). Checks: merge with today's develop, build and ctest, library parity, differential goldens, changed tests (source and installed), refusal-tile mutation, CI shard logs, code quality. Flags whether a closer look is needed; not an approval or rejection. |
… spec helpers Merges origin/develop. N1. #14335 rewrote the ragged + sliding_window reject: it now names the length and tile that leave a partial tile (partial_tile_reason). The C port still said "ragged is not supported with sliding_window", so on develop the vectors test failed, and a regenerate would have failed the C ctest. Ported partial_tile_reason and the new text; three cases cover its seqlen_kv-only, both and whole-tile branches (only seqlen_q was recorded). Vectors regenerated; the C ctest matches all four. N2. #14835 added rocke_spec_set_reasonf / ROCKE_SPEC_FAIL and rocke_floor_div to helpers/spec. The port's local reject() and py_floordiv() are gone: 58 rejects use ROCKE_SPEC_FAIL and the three floor divisions rocke_floor_div. rocke_spec_set_reasonf now marks a cut reason with "..." like rocke_spec_set_reason, as its header already promised ("same truncation"). Nit: the header's ZeroDivisionError note no longer quotes one CPython 3.12 text for both operators.
…al tile (#14335) Replaces #13681 with the same commit (1b17bab). GitHub closed #13681 when its branch moved to `users/ysoliman/rocke/` for the rocKE branch-naming rule, and a PR cannot be reopened once its branch is renamed. The review history, including the rocKE checklist reviews, stays on #13681. Carved out of #12928, which is closed. The rest of that PR is already on develop (#12710 landed the nhead_k guard, #12719 replaced the factories this touched and routes `_request_errors` through every dense candidate). ## Motivation The dense `_base_spec` factories set `ragged=True` for any self-attention length that is not a tile multiple, so Sq = Skv = 384 or 1152 at block_m 256 is an ordinary graph taking the on-chip ragged path. The gfx942 predicate then declined with "gfx942 attention_dense: ragged not yet supported" and the shared spec validator with "ragged is not supported with sliding_window". Both read as a varlen problem and name nothing the caller can act on. Found while running the hipDNN kernel-integration runbook on gfx942. ## Technical details - `kernels/common/attention_dense_spec.py`: new `partial_tile_reason(spec)` names the length and the tile, for example "seqlen_q 384 is not a multiple of block_m 256". The sliding-window branch of `__post_init__` now says "sliding_window needs whole tiles: <reason> (the partial-tile path has no window mask)". - `kernels/gfx942/attention_dense.py`: `supports_attention_dense` says "gfx942 attention_dense needs whole tiles: <reason> (the partial-tile path is not yet supported)". - Varlen messages are unchanged. What could break: anything matching the old "ragged not yet supported" or "ragged is not supported with sliding_window" text. A repo grep for those two strings finds only the two lines this PR rewrites, and that was not enough. One test matched on less than the whole string: `tests/dispatch/attention/test_unified_mask_wiring.py` asserted the substring "ragged" in the gfx950 window refusal, and this branch broke it. It is now `test_partial_tile_window_is_declined_not_served` and checks the length, the tile and the window, the same way the gfx942 and gfx950 wiring tests do. Anything outside this repo that keys on the word "ragged" has the same exposure. ## Test plan and result Host only, no GPU, Windows Python 3.12, from `dnn-providers/hip-kernel-provider/rocke/library` with `library` and `platform/python` on PYTHONPATH. - `tests/dispatch/attention` plus `tests/test_attention_dense_gfx942.py` at head `1b17babac3`: 882 passed, 121 skipped, 3296 subtests passed. The merge base, develop at 28af48c, gives 882 passed, 121 skipped, 3293 subtests; the 3 extra subtests are the new 384 / 1152 / 1000 cases. - Negative control: with the two messages reverted, `test_rejects_a_partial_tile_and_names_the_length`, `test_sliding_window_with_partial_tile_rejected_at_dispatch` and the gfx942 `supports` marker all fail. - black 25.12.0 `--check` is clean on the changed files. Refs #12920. ## Submission checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Merged develop 3f51737 and updated the three rocke attention wiring tests to match the new refusal message.
TheRock Submodule Bump ActivityNewest first
|
Replaces #13681 with the same commit (1b17bab). GitHub closed #13681 when its branch moved to
users/ysoliman/rocke/for the rocKE branch-naming rule, and a PR cannot be reopened once its branch is renamed. The review history, including the rocKE checklist reviews, stays on #13681.Carved out of #12928, which is closed. The rest of that PR is already on develop (#12710 landed the nhead_k guard, #12719 replaced the factories this touched and routes
_request_errorsthrough every dense candidate).Motivation
The dense
_base_specfactories setragged=Truefor any self-attention length that is not a tile multiple, so Sq = Skv = 384 or 1152 at block_m 256 is an ordinary graph taking the on-chip ragged path. The gfx942 predicate then declined with "gfx942 attention_dense: ragged not yet supported" and the shared spec validator with "ragged is not supported with sliding_window". Both read as a varlen problem and name nothing the caller can act on. Found while running the hipDNN kernel-integration runbook on gfx942.Technical details
kernels/common/attention_dense_spec.py: newpartial_tile_reason(spec)names the length and the tile, for example "seqlen_q 384 is not a multiple of block_m 256". The sliding-window branch of__post_init__now says "sliding_window needs whole tiles: (the partial-tile path has no window mask)".kernels/gfx942/attention_dense.py:supports_attention_densesays "gfx942 attention_dense needs whole tiles: (the partial-tile path is not yet supported)".What could break: anything matching the old "ragged not yet supported" or "ragged is not supported with sliding_window" text. A repo grep for those two strings finds only the two lines this PR rewrites, and that was not enough. One test matched on less than the whole string:
tests/dispatch/attention/test_unified_mask_wiring.pyasserted the substring "ragged" in the gfx950 window refusal, and this branch broke it. It is nowtest_partial_tile_window_is_declined_not_servedand checks the length, the tile and the window, the same way the gfx942 and gfx950 wiring tests do. Anything outside this repo that keys on the word "ragged" has the same exposure.Test plan and result
Host only, no GPU, Windows Python 3.12, from
dnn-providers/hip-kernel-provider/rocke/librarywithlibraryandplatform/pythonon PYTHONPATH.tests/dispatch/attentionplustests/test_attention_dense_gfx942.pyat head1b17babac3: 882 passed, 121 skipped, 3296 subtests passed. The merge base, develop at 28af48c, gives 882 passed, 121 skipped, 3293 subtests; the 3 extra subtests are the new 384 / 1152 / 1000 cases.test_rejects_a_partial_tile_and_names_the_length,test_sliding_window_with_partial_tile_rejected_at_dispatchand the gfx942supportsmarker all fail.--checkis clean on the changed files.Refs #12920.
Submission checklist
Merged develop 3f51737 and updated the three rocke attention wiring tests to match the new refusal message.