Fix Attention is_causal bottom-right alignment for external KV cache (onnx#8068, #28904) - #28958
Conversation
Review summary — bottom-right
|
1d4db98 to
16028d8
Compare
… fully-masked Y=0 + mode-3=0 The cuda-attention-kernel-patterns skill doc gave guidance opposite to the shipped behavior in #28958 (onnx/onnx#8068 errata): - is_causal=1 with an external/static KV cache (nonpad_kv_seqlen, no past_key) uses bottom-right (offset-aware) alignment and IS valid for decode — the doc previously called it spec-invalid and told models to use is_causal=0. - Fully-masked query rows output Y=0 on BOTH EPs (CPU Bug-2 guard + CUDA ZeroFullyMaskedRowsKernel); removed the stale mean(V) 'spec reference' claim and the resolved cross-EP TODO. - Documented qk_matmul_output mode-3 fully-masked row = 0 (mandated, consistent with Y), and that CUDA returns NOT_IMPLEMENTED for mode-3 (CPU-only path). - Corrected the bottom-right key-range bullet (was backwards) to the j <= i + offset form matching the rest of the doc and the kernel. Docs-only; no code/kernel change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com> (cherry picked from commit fb0f4b3)
Multi-agent review synthesis — re-verified against the onnx#8068 referenceUpdated: I re-ran the review team against the actual onnx#8068 reference ( Verdict: The core spec alignment is correct. The three Major items are real but, after grounding in the reference, two are latent (not exercised by any onnx#8068 conformance test) and the third has a narrow hard-failure set (3 tests on CUDA). Confirmed correct against
|
tianleiwu
left a comment
There was a problem hiding this comment.
Thanks for tightening the bottom-right/nonpad behavior and adding CUDA coverage across Flash, MEA, and unfused. I found one CPU/CUDA consistency issue in the fully-masked-row detector that should be fixed before this lands.
…ly-masked detection (onnx#28958 review) Addresses tianleiwu's CHANGES_REQUESTED review on #28958: (1) CPU float-mask -inf predicate (merge blocker), core/providers/cpu/llm/attention.cc. The fully-masked-row guard previously classified a position as masked only when it equaled the internal finite sentinel (mask_filter_value<T>()), so a user-supplied IEEE -inf in an additive float attn_mask was treated as unmasked and propagated to a NaN row. Now a position is masked iff it is the sentinel OR an IEEE negative infinity, matching the CUDA guard (bias <= masked_bias_value) and the onnx#8068 reference (isneginf). Finite large-negative user biases (e.g. -40000) stay unmasked, so existing bottom-right / fully-masked=0 / mode-3=0 behavior is byte-identical for all non-(-inf) inputs. A fully-masked float--inf row now yields Y=0 and qk_matmul_output mode-3=0 on CPU instead of NaN. Added AttentionTest.Attention4DAttnMaskFloatNegInfFullyMasked (CPU) asserting the zero rows + no NaN + mode-3=0. (2) Perf (correctness-neutral), contrib_ops/cuda/bert/unfused_attention.cu. Replaced the second unconditional float BlockReduce used only to detect a fully-masked row with a single __syncthreads_or() over the per-thread retained predicate (no shared memory, no cub float reduce). The fully-masked detection result is identical, only cheaper, and the call also publishes s_max as the block barrier (removes one redundant __syncthreads). Validation (CUDA Debug): AttentionTest 74/74 pass; MultiHeadAttentionTest + GroupQueryAttentionTest 62 pass / 6 WebGPU-skipped (shared unfused path unchanged). mode-3=0 logic untouched; CUDA mode-3 remains NOT_IMPLEMENTED. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com>
|
Thanks @tianleiwu for the careful review — all three points were helpful. Addressed below; 1. CPU float-
|
Posted in error to the wrong PR (this review text belongs to a different change about CUDA reduction integer accumulators). Dismissing; the correct review for #28958 follows.
tianleiwu
left a comment
There was a problem hiding this comment.
Re-reviewed the latest head. Thanks for addressing the three items from the previous round:
- CPU float
-infpredicate — thestd::isinf(mask_value_f) && mask_value_f < 0branch now classifies a user-infas masked alongside the finite sentinel; the fp32/fp16NegInfFullyMaskedgtests pin it. Resolved. - Unfused fully-masked detection — switched to
__syncthreads_or(thread_retained), which both publishess_maxand reduces the retained predicate with no extra shared memory / cub reduce. Resolved. causal_from_top_leftdecode change — confirmed it is scoped to thenonpad_kv_seqlen != nullptrbranch only; the no-cache /past_keyMEA branch keeps(past_sequence_length == 0), so decode/prefill on those paths is byte-unchanged and the external-cache path correctly uses bottom-right. Resolved.
Core bottom-right math (offset = nonpad_kv_seqlen[b] - q_seq, j <= i + offset, frontier clamped at 0) and the select-not-multiply zeroing look correct and are well covered on CPU / MEA / unfused.
Two remaining items (non-blocking) I'd like tracked before the onnx pin bump — both are latent on the current v1.21.0 pin but become live once the #8068 tests run:
-
Flash path has no fully-masked-row guard.
LaunchZeroFullyMaskedRowsruns only in the MEA and unfused paths. The eligibility change now routesis_causal + nonpad_kv_seqlen(nopast_key) to Flash, and forS_q > 1withnonpad_kv_seqlen[b] < S_qthe negative offset leaves early query rows structurally empty. Decode (S_q == 1) is safe (offset>= 0), and the fp32 /head_size == 4tests route to MEA — so nothing currently exercises Flash here. A user fp16/bf16 model with a Flash-eligiblehead_sizeandnonpad < S_qwould depend on FA2 emitting0(not NaN) for anlse == -infrow, which is unverified. Suggest either an explicit post-Flash guard or a gtest with a Flash-eligiblehead_sizeandnonpad < S_qto lock the behavior. -
Preemptive skip-list omits the mode-3 tests that hard-fail on CUDA. The purpose of the preemptive skip block is to keep CI green at the onnx pin bump, but the
qk_matmul_output_mode3families are deferred to a TODO. CUDA returnsNOT_IMPLEMENTEDfor modes beyondkQK, sotest_attention_(23|24)_*qk_matmul_output_mode3*will hard-fail the moment the pin includes #8068. Adding the^test_attention_(23|24)_.*qk_matmul_output_mode3skip now (guarded by the #27712 / #28994 reference) to both this jsonc andTestCase.ccwould make the block self-consistent and avoid a known future break.
Splits the **bug-fix** half of #340 into its own reviewable, mergeable PR, per @justinchuby's review request ("could you isolate bug fixes into potentially another PR"). ## Fixes 1. **fp16 GQA export emitting fp32 packed weights.** When building fp16 models, `_cast_module_dtype` casts params to fp16 but the folded initializer `Value`s lost their declared `.dtype` (None) while `const_value` stayed fp16. `FoldConcatInitializersPass` / `FoldTransposedInitializerPass` then defaulted the packed/transposed initializer to `FLOAT`, serializing fp32 weights and making ORT reject the model with a fp16/fp32 `MatMul` type-parameter error on both CPU and CUDA EPs. New shared helper `mobius._passes._dtype_utils.initializer_dtype()` resolves the effective dtype from `const_value` when the type annotation was dropped. Also strips dead pre-pack weights via `graph.remove(node, safe=True)` so DCE drops the orphaned q/k/v_proj source initializers. 2. **GQA `present.*` head_dim mis-declaration.** `_register_kv_cache_outputs` now stamps explicit `present.*` KV-cache output shapes/dtypes for GQA instead of relying on the known-wrong shape-inference path. 3. **Fail-closed `_register_kv_cache_outputs`** (closes #341). A partial set of present-shape parameters now **raises `ValueError`** (naming provided + missing params) instead of logging a warning and shipping a structurally-wrong model. All-six (stamp) or none (infer) are unaffected. Includes regression tests for all three (155 tests; an e2e ORT CPU-EP load test reproduces the original fp16/fp32 `MatMul` failure without the fix). ## Scope note The **Option-Y** static-cache graph workaround from #340 (`is_causal=0` + explicit causal mask + `nonpad_kv_seqlen`, forcing MEA) is **dropped, not landed**. The maskless `is_causal=1` + `nonpad_kv_seqlen` end-state (Flash-eligible, fewer nodes) will be emitted directly once onnx/onnx#8068 + microsoft/onnxruntime#28958 ship in a pinnable ORT release and mobius bumps its ORT pin — tracked by #345. Supersedes the bug-fix portion of #340. --------- Signed-off-by: titaiwang <titaiwang@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @tianleiwu — both items addressed, and verified on an A100. 1. Flash fully-masked-row guard. Rather than add a redundant host-side guard, I added regression gtests that drive a structurally-empty (negative-offset,
All gate 2. Preemptive skip-list for mode-3. Promoted the
CI stays green at the onnx pin bump and the block is self-consistent. |
tianleiwu
left a comment
There was a problem hiding this comment.
Re-reviewed at cc34d0b9. Both of my previously open threads are addressed, and I've resolved them:
- Flash empty-row coverage (
cuda/llm/attention.cc:1395): my ask for an explicit guard or a gtest with a Flash-eligiblehead_sizeandnonpad < S_qis satisfied by the three new Flash regression tests —Attention_Causal_NonPadKVSeqLen_FlashStructuralEmptyRows_Zero_{FP16,BF16}_CUDA(single-tile epilogue) and..._SplitKV_FP16_CUDA(cross-split combine). They lock that FA2 emits a zero row (not NaN) for anlse == -inffrontier. Resolved. - Mode-3 skip self-consistency (
onnx_backend_test_series_filters.jsonc): the^test_attention_(23|24)_.*qk_matmul_output_mode3regex and the matchingTestCase.ccCUDA-scoped entries were added now (not deferred), so the onnx pin bump can't break CUDA CI. Resolved.
The core fix looks correct and is exceptionally well-tested:
int32_t causal_diagonal_offset(signed) plus thestatic_cast<int32_t>(query_start)hardening inkernel_forward.hcorrectly preserves the negativenum_keys - num_queriesoffset; thehead_size=512MEA tests and the mixed-batch{-2, +2}offset test exercise the per-batchRightPaddingBatchHookpath directly.- The fully-masked-row → 0 guard uses an exact structural predicate (sentinel-equality / IEEE
-inf) on both EPs rather than the prior magnitude heuristic; theFiniteNegativeBiasfp16 regression tests pin that a finite-40000bias is no longer wrongly zeroed. __syncthreads_orfor the per-row retained flag (replacing the float reduce) is correct — it doubles as thes_maxpublish barrier.
Non-blocking suggestion (no need to hold the PR): the new CUDA MEA fully-masked-row tests all drive the nonpad_kv_seqlen (seqlens_k != nullptr) branch of LaunchZeroFullyMaskedRows. The standard MEA path call site (attn_bias != nullptr, seqlens_k == nullptr — e.g. an all-false bool mask on an MEA-eligible shape with total_sequence_length % 4 == 0) doesn't appear to have a dedicated CUDA test; Attention4DAttnMaskBoolAllFalse is CPU/DML-only. A small fp16 all-masked test on that shape would close the seqlens_k == nullptr kernel branch. Approving.
…gned-offset, false-green, rebuild) Standalone, docs-only knowledge capture distilled from debugging the MEA causal_diagonal_offset uint32 wraparound (ORT #28904 / onnx#8068, fixed in #28958). No source or test changes — this branch is based on the fix's parent so the diff is docs only. General meta-lessons land in broadly-triggered homes; operator-specific reference stays in the attention skill and cross-links up: - AGENTS.md (C++ Conventions): the signed-vs-unsigned wrap bug class on negative-capable differences (a - b that can be negative must be signed; unsigned wraps to ~4.29e9 and silently satisfies/skips a relational guard). - ort-build (Agent tips): build flags can silently reroute which kernel/code path runs (QUICK_BUILD compiles Flash hdim128-only -> head!=128 dispatches to MEA, arch-independent); plus the QUICK_BUILD flag row. - ort-test: a five-part false-green taxonomy (zero-match filter, stale incremental binary, wrong-artifact mtime, correct-fallback-masks-intended-path, arch-portability/H100-only verification), a "verify which path/kernel actually executed" subsection, and the two distinct attention_op_test.cc files (providers/cpu/llm ONNX Attention vs contrib_ops MHA/GQA). - cuda-attention-kernel-patterns: the Flash/MEA/unfused eligibility table (symbols + lines), the head_size=512 MEA-eligibility caveat (the predicate has no shared-memory check, so the MEA launch is arch-fragile and dies on small-smem GPUs like sm86 -- live bug #28388; use ORT_DISABLE_FLASH_ATTENTION + a small head_size to force MEA portably), and the §12 FMHA signed-offset fix-site table + rules (pinned symbol+line to cc34d0b), cross-linking up. - cuda-cutlass-fmha-incremental-rebuild (newly tracked): nvcc depfiles don't track cutlass_fmha/*.h, so check the .so/.o mtime, not the dlopen'd test exe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update: CI fix + regression-test migration + docsWhy the previous CI run was red (and why it wasn't the offset fix): the int32 Test migration: the 5 head_size=512 MEA regression tests are replaced with head_size=64 + Note: forcing MEA now relies on the env-var mechanism rather than head_size=512's by-construction routing; the tests verbose-confirm MEA was actually selected, and under QUICK_BUILD head_size=64 routes to MEA by construction (Flash is hdim128-only there). Docs: this PR also bundles the knowledge-capture skill/doc updates from debugging this issue (attention dispatch + CUTLASS FMHA rebuild gotchas, plus a general build-flag / false-green / signed-offset lesson set). Happy to split these into a separate docs-only PR if reviewers would prefer to keep this PR scoped to the kernel fix + tests — just say the word. |
…+ fully-masked-row zeroing (onnx#8068) Aligns the opset-24 ONNX-domain Attention kernels (CPU + CUDA) with the onnx/onnx#8068 errata for the external static KV-cache path (nonpad_kv_seqlen input, no past_key): - Bottom-right is_causal: per batch, offset[b] = nonpad_kv_seqlen[b] - q_seqlen; a query at in-block index i attends key j iff j <= i + offset[b]. Applied on CPU and on the CUDA Flash / Memory-Efficient (MEA) / unfused paths. The MEA causal-alignment selector is offset-aware (no unconditional top-left when an external cache is present); Flash's native bottom-right + per-batch seqlens_k is used where eligible. - Fully-masked-row -> 0 (Bug-2): a query row with no allowed key now outputs a zero row instead of mean-of-V (finite-sentinel softmax). Detected with an exact per-key structural predicate (isneginf-equivalent), zeroed with select (not multiply) before P@V so 0@V = 0. Added on CPU and the CUDA MEA path; the Flash is_causal + seqlens_k path (offset >= 0) cannot produce a fully- masked row and is left unguarded. - Removes the CUDA NOT_IMPLEMENTED reject for is_causal + nonpad_kv_seqlen with S_q != total_kv and no past_key (the spec now defines this result). - Full-prefill (offset 0) and past_key decode paths remain bit-identical; contrib MultiHeadAttention / GroupQueryAttention behavior is unchanged (they consume the shared FMHA kernels, not retargeted here). - C++ AttentionTest gtests added: bottom-right offset, structural-empty causal row -> 0 (CPU + CUDA), and fp16 fully-masked-row goldens. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com>
…_kv_seqlen Replaces the stale negative-reject assertion (is_causal + nonpad_kv_seqlen was NOT_IMPLEMENTED) with a positive test asserting the bottom-right result now computed per onnx/onnx#8068. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com>
…(no-op on v1.21.0) Adds the new onnx/onnx#8068 Attention node tests to the backend filter list (onnx_backend_test_series_filters.jsonc) and the C++ GetBrokenTests (TestCase.cc) so they are skipped until cmake/external/onnx is bumped to a release containing onnx#8068. No-op on the current onnx pin (v1.21.0); remove BOTH skip lists once the pin advances past onnx#8068. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com>
… fully-masked Y=0 + mode-3=0 The cuda-attention-kernel-patterns skill doc gave guidance opposite to the shipped behavior in #28958 (onnx/onnx#8068 errata): - is_causal=1 with an external/static KV cache (nonpad_kv_seqlen, no past_key) uses bottom-right (offset-aware) alignment and IS valid for decode — the doc previously called it spec-invalid and told models to use is_causal=0. - Fully-masked query rows output Y=0 on BOTH EPs (CPU Bug-2 guard + CUDA ZeroFullyMaskedRowsKernel); removed the stale mean(V) 'spec reference' claim and the resolved cross-EP TODO. - Documented qk_matmul_output mode-3 fully-masked row = 0 (mandated, consistent with Y), and that CUDA returns NOT_IMPLEMENTED for mode-3 (CPU-only path). - Corrected the bottom-right key-range bullet (was backwards) to the j <= i + offset form matching the rest of the doc and the kernel. Docs-only; no code/kernel change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com> (cherry picked from commit fb0f4b3)
…28994) Adds inline TODO(#28994) reminders in the two onnx-backend skip lists (onnx_backend_test_series_filters.jsonc + TestCase.cc::GetBrokenTests) noting that when cmake/external/onnx is bumped past onnx/onnx#8068, the new qk_matmul_output mode-3 conformance tests must be added to the skip lists: CUDA Attention returns NOT_IMPLEMENTED for qk_matmul_output_mode beyond kNone/kQK (core/providers/cuda/llm/attention.cc:1477). This capture gap is tracked by #28994. Comment-only; no skip regex is added to the live filters yet (that lands at pin-bump per #28994 Section A). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com>
…ly-masked detection (onnx#28958 review) Addresses tianleiwu's CHANGES_REQUESTED review on #28958: (1) CPU float-mask -inf predicate (merge blocker), core/providers/cpu/llm/attention.cc. The fully-masked-row guard previously classified a position as masked only when it equaled the internal finite sentinel (mask_filter_value<T>()), so a user-supplied IEEE -inf in an additive float attn_mask was treated as unmasked and propagated to a NaN row. Now a position is masked iff it is the sentinel OR an IEEE negative infinity, matching the CUDA guard (bias <= masked_bias_value) and the onnx#8068 reference (isneginf). Finite large-negative user biases (e.g. -40000) stay unmasked, so existing bottom-right / fully-masked=0 / mode-3=0 behavior is byte-identical for all non-(-inf) inputs. A fully-masked float--inf row now yields Y=0 and qk_matmul_output mode-3=0 on CPU instead of NaN. Added AttentionTest.Attention4DAttnMaskFloatNegInfFullyMasked (CPU) asserting the zero rows + no NaN + mode-3=0. (2) Perf (correctness-neutral), contrib_ops/cuda/bert/unfused_attention.cu. Replaced the second unconditional float BlockReduce used only to detect a fully-masked row with a single __syncthreads_or() over the per-thread retained predicate (no shared memory, no cub float reduce). The fully-masked detection result is identical, only cheaper, and the call also publishes s_max as the block barrier (removes one redundant __syncthreads). Validation (CUDA Debug): AttentionTest 74/74 pass; MultiHeadAttentionTest + GroupQueryAttentionTest 62 pass / 6 WebGPU-skipped (shared unfused path unchanged). mode-3=0 logic untouched; CUDA mode-3 remains NOT_IMPLEMENTED. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Ti-Tai Wang <titaiwang@microsoft.com>
…eemptive skips (onnx#28958 review) TASK A: regression tests driving structural fully-masked leading-row cases (is_causal + nonpad_kv_seqlen < q_seq, head_size=64, no attn_mask, no past_key) through the Flash path (confirmed via verbose log "using Flash Attention", attention.cc:1400), asserting leading offset<0 query rows == 0 (not NaN): - _FP16_/_BF16_ (total_sequence_length=4 -> num_splits=1): lock the FA2 single-tile epilogue (flash_attention/softmax.h:182-186, inv_sum=1 when sum==0 and acc_o=0). - _SplitKV_FP16_ (total_sequence_length=257 -> num_n_blocks=2 -> num_splits=2, SM-count-independent for batch*heads*m_blocks=1): locks the higher-risk split-KV combine (flash_attention/flash_fwd_kernel.h:1125/1135/1153, lse==-inf -> scale=expf(-inf)=0 -> O=0), which the single-tile cases never reach. All three gate on SM 8.0+ (Flash requirement; otherwise it falls to MEA and the FA2-epilogue intent is lost). The Flash path has no host-side LaunchZeroFullyMaskedRows guard (MEA/unfused only). TASK B: Promote the two TODO-only mode-3 qk_matmul_output skips to active preemptive skips. CUDA returns NOT_IMPLEMENTED for modes beyond kQK (cuda/llm/attention.cc:1477); CPU implements mode 3 and passes, so the TestCase.cc entries are scoped to the provider_name=="cuda" block (not the provider-agnostic set) to preserve CPU mode-3 conformance after the pin bump. jsonc (no per-EP scoping) keeps the global regex ^test_attention_(23|24)_.*qk_matmul_output_mode3. The 6 exact CUDA names are sourced from the onnx#8068 generated corpus. Keeps #28994/#27712 references. Validated on A100 (SM 8.0): 26/26 AttentionTest nonpad/structural/fullymasked tests pass, including the 3 new Flash tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…right causal offset CUTLASS Memory-Efficient Attention (MEA) stored causal_diagonal_offset as uint32_t. For the bottom-right causal case it is set to (num_keys - num_queries), which is NEGATIVE in the external-KV-cache regime where nonpad_kv_seqlen < q_sequence_length. The negative value wrapped to a huge unsigned number, so the per-element causal-mask guard (min(...) >= query_start + offset) was always false and the mask was skipped entirely -- a boundary query row then over-attended one extra key, producing wrong outputs (onnx#8068 / ORT #28904). Fix: - Make causal_diagonal_offset int32_t so the negative offset is preserved. - Cast query_start to int32_t at the causal-mask guard (query_start is uint32_t, so uint32_t + int32_t would otherwise re-promote the comparison to unsigned). - Harden the lower-left sliding-window mask guard (L957) with the same static_cast<int32_t>(query_start) pattern. It is dormant today (window_size > 0 never co-occurs with a negative offset: opset-24 Attention hard-codes window=-1, GQA/Paged keep offset >= 0) and bit-identical, but this prevents the same unsigned-wrap landmine if a future sliding-window / KV-trimming caller combines window_size > 0 with a negative offset. Tests (onnxruntime/test/providers/cpu/llm/attention_op_test.cc), 7 MEA regression tests forcing the MEA path by construction (head_size=512 is Flash-ineligible in all builds, MEA-eligible): - head_size=512 structural-MEA negative-offset primary, FP16 and BF16. - Zero-offset and positive-offset defense-in-depth (offset >= 0 stays bit-identical through MEA). - ForceFlashDisabled head_size=64 FP16/BF16 cross-check (ORT_DISABLE_FLASH_ATTENTION=1) pinning the exact CI QUICK_BUILD regime. - MixedBatchOffsets batch=2 with mixed nonpad_kv_seqlen {2,6} to exercise the per-batch RightPaddingBatchHook offset path (one negative, one positive offset in a single launch). - A SKIP_IF_MEA_NOT_COMPILED guard makes these tests GTEST_SKIP() rather than silently pass via the (correct) unfused fallback when MEA is compiled out, avoiding a false-green. Follow-up: the query_start > 0 (multi-query-block) path is additive-only future coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…64 force-MEA The head_size=512 MEA negative-offset tests crash at kernel launch on sm86/sm89 CI GPUs: the cutlass FMHA SharedStorage exceeds the ~99KB dynamic shared-memory opt-in cap, and fmha_launch_template.h sets cudaFuncSetAttribute(MaxDynamicSharedMemorySize) unchecked (gated only by sm>=70, not actual capacity), so the launch fails with 'CUDA failure 1: invalid argument'. The tests passed only on sm90 (227KB opt-in). The cap is non-monotonic in SM version, so no clean version guard exists. Convert the offset coverage (negative/zero/positive/mixed-batch) to head_size=64 + ORT_DISABLE_FLASH_ATTENTION=1, the same force-MEA mechanism the existing ForceFlashDisabled tests use. head_size=64 needs only a few KB of static shared memory (no opt-in path), so the tests run on every architecture while still forcing RunMemoryEfficientAttention and asserting the int32 causal_diagonal_offset fix. The two head_size=512 negative-offset tests were duplicates of the existing ForceFlashDisabled tests and are dropped. Verified AttentionTest.* = 83/83 on H100, all 5 tests MEA-routed. Agent-signed-off: Developer (e712d664) [claude-opus-4.8 via copilot] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…gned-offset, false-green, rebuild) Standalone, docs-only knowledge capture distilled from debugging the MEA causal_diagonal_offset uint32 wraparound (ORT #28904 / onnx#8068, fixed in #28958). No source or test changes — this branch is based on the fix's parent so the diff is docs only. General meta-lessons land in broadly-triggered homes; operator-specific reference stays in the attention skill and cross-links up: - AGENTS.md (C++ Conventions): the signed-vs-unsigned wrap bug class on negative-capable differences (a - b that can be negative must be signed; unsigned wraps to ~4.29e9 and silently satisfies/skips a relational guard). - ort-build (Agent tips): build flags can silently reroute which kernel/code path runs (QUICK_BUILD compiles Flash hdim128-only -> head!=128 dispatches to MEA, arch-independent); plus the QUICK_BUILD flag row. - ort-test: a five-part false-green taxonomy (zero-match filter, stale incremental binary, wrong-artifact mtime, correct-fallback-masks-intended-path, arch-portability/H100-only verification), a "verify which path/kernel actually executed" subsection, and the two distinct attention_op_test.cc files (providers/cpu/llm ONNX Attention vs contrib_ops MHA/GQA). - cuda-attention-kernel-patterns: the Flash/MEA/unfused eligibility table (symbols + lines), the head_size=512 MEA-eligibility caveat (the predicate has no shared-memory check, so the MEA launch is arch-fragile and dies on small-smem GPUs like sm86 -- live bug #28388; use ORT_DISABLE_FLASH_ATTENTION + a small head_size to force MEA portably), and the §12 FMHA signed-offset fix-site table + rules (pinned symbol+line to cc34d0b), cross-linking up. - cuda-cutlass-fmha-incremental-rebuild (newly tracked): nvcc depfiles don't track cutlass_fmha/*.h, so check the .so/.o mtime, not the dlopen'd test exe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9146d7c to
3ae9af5
Compare
@tianleiwu For couple of new commits: I fixed some tests that were accidentally using head_size=512 to test MEA kernel which does not officially support 512. Also, docs updated for ai-agents and human reading. As well as the resolve conflicts with main branch. Details are in above message! |
…edBiasSentinel) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address Copilot review comments on PR #364: - tests/static_cache_flash_e2e_test.py: the module-level `skipif` evaluated `supports_static_cache_flash()` eagerly at import/collection time, which runs a CUDA build + serialize + session.run. Replace it with an autouse fixture that calls `static_cache_flash_skip_reason()` at test setup, so importing the module and `pytest --collect-only` are side-effect-free. The probe is lru-cached (runs at most once) and the helper reports the true skip cause (no CUDA EP, missing microsoft/onnxruntime#28958, or an unexpected probe failure). The remaining Copilot comments are already satisfied at HEAD: the opset-lowering gate already excludes the CPU and default EPs (_builder.py), and the probe only treats an ORT `Fail` as the expected pre-#28958 reject when its message carries the reject signature, routing generic failures (OOM, drift) to PROBE_ERROR (ort_capabilities.py). The _builder.py:216 comment was a PR-description note, not a code defect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: titaiwang <titaiwang@microsoft.com>
…ORT #28958) (#364) ## What mobius `main` already emits the correct maskless `is_causal=1` + `nonpad_kv_seqlen` + `TensorScatter` static-cache decoder graph. This PR adds the runtime **enablement, verification, and CI** for that path on the CUDA Flash-attention kernel — it is **not** graph surgery. Nothing in the emitted graph is reverted or rewritten. The path becomes runnable once an ONNX Runtime build containing [microsoft/onnxruntime#28958](microsoft/onnxruntime#28958) is installed (Flash eligibility widening for the bottom-right-causal errata [onnx/onnx#8068](onnx/onnx#8068)). ## Issues - Closes #329 — this PR fully delivers the static-cache numerical-parity CI coverage that #329 asks for (`tests/static_cache_parity_test.py`: static vs dynamic vs HuggingFace, chunked-prefill zero-guard, V convex-hull valid-row invariant, exact-triangle `nonpad == q_seq`). - Part of #345 — this PR delivers the code/probe/CI half of #345 ("emit maskless `is_causal=1` + `nonpad_kv_seqlen` static-cache graph once upstream lands"). The ORT pin bump + node-count rebaseline + ONNX 1.22 pin + skill docs remain deferred until an official PyPI ORT release with microsoft/onnxruntime#28958, so #345 stays open (intentionally `Part of`, not `Closes`). ## Changes - **(a) Conditional opset 24→23 lowering** in `_builder.py` via `_graph_requires_opset24` (a **recursive** subgraph scan) + `_apply_opset_lowering`, so graphs carrying `TensorScatter` or the `Attention` `nonpad_kv_seqlen` input correctly **stay at opset 24**. Flag-gated, default off. (`_builder_test.py` exercises the real branch.) - **(b) Canonical capability probe** `src/mobius/_testing/ort_capabilities.py` — `supports_static_cache_flash()` is a **functional, fail-closed-but-loud runtime probe** (not a version-string check). It builds a minimal `TensorScatter` + maskless `Attention` graph and runs it on CUDA. A **known-answer value check** closes a latent CPU-fallback fail-open: because ORT implicitly appends the CPU EP, a CUDA build that declines the node would silently run on CPU with wrong (top-left) values; the probe's deterministic reference (`2.0`) rejects that → `NEEDS_FIX` → `False`. A structured `_ProbeOutcome` enum distinguishes the expected pre-#28958 reject from unexpected probe errors (logged with `exc_info`). - **(c) Static-cache parity test (#329)** `tests/static_cache_parity_test.py` — static vs dynamic vs HuggingFace, chunked-prefill zero-guard, V convex-hull valid-row invariant, exact-triangle `nonpad == q_seq`. - **(d) e2e CUDA Flash-dispatch test** `tests/static_cache_flash_e2e_test.py` — asserts the ONNX-domain `Attention` actually routes to **Flash** (via VERBOSE dispatch capture), gated on SM ≥ 8.0 (`_flash_capable_gpu`) and `onnxruntime_QUICK_BUILD`. - **(e) Probe consolidation** — deleted `tests/_static_cache_support.py`; one canonical probe module, no shim. ## Gating All new GPU tests **skip automatically** unless the installed ORT can actually run the path (probe-gated). CI stays green today and **flips green automatically** once an official ORT release containing microsoft/onnxruntime#28958 is installed — **zero code change needed** to enable. ## Verified - **5/5** static-cache tests pass on an A100 (SM 8.0) with a post-#28958 ORT (full targeted suite: 14 passed including builder tests). - **Fail-closed on pre-#28958 confirmed two ways**: (1) source — the CUDA kernel guard `causal_cross_no_past = is_causal && (q_seq != total_seq) && (past == 0)` in `Attention<T>::ComputeInternal` (`llm/attention.cc`) raises `NOT_IMPLEMENTED` for the `S_q=1` decode shape (no fast-path bypass); (2) empirically — a real `onnxruntime-gpu==1.27.0` isolated venv raises `NotImplemented` → `supports_static_cache_flash() == False`. ## Explicitly out of scope (intentionally held, separate follow-ups) - ORT dependency pin bump + node-count rebaseline + ONNX 1.22 pin + skill docs — parked until an official PyPI ORT release with #28958 exists. - `examples/static_cache_generation.py` nonpad-before-scatter check (`verify-example-nonpad`). - Pre-existing `RUF067` ruff version-skew in `pyproject.toml`. - onnxruntime-genai#2204-blocked bias-decoder external-KV work (#349). ## References - [onnx/onnx#8068](onnx/onnx#8068) — causal top-left → bottom-right errata - [microsoft/onnxruntime#28958](microsoft/onnxruntime#28958) — Flash eligibility widening --- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Signed-off-by: titaiwang <titaiwang@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Aligns the opset-24 ONNX-domain
Attentionkernels (CPU + CUDA) with the ONNX errata onnx/onnx#8068 (tracking RFC onnx/onnx#8054) for the external static KV-cache path — keyed bynonpad_kv_seqlen(input #6), nopast_key.Addresses #28904.
What changed
is_causalalignment. Per batch,offset[b] = nonpad_kv_seqlen[b] − q_sequence_length; a query at in-block indexiattends keyjiffj <= i + offset[b]. Applied on CPU and on the CUDA Flash / Memory-Efficient (MEA) / unfused paths. The MEA causal-alignment selector is now offset-aware (no unconditional top-left when an external cache is present); Flash's native bottom-right + per-batchseqlens_kis used where eligible.isneginf-equivalent) and zeroed with select (not multiply) beforeP @ V, so0 @ V = 0. Added on CPU and the CUDA MEA path. The Flashis_causal+seqlens_kpath (offset >= 0) cannot produce a fully-masked row and is intentionally left unguarded. Bool-mask conversion was already select-not-multiply on both EPs (Bug-1 satisfied; no change needed).NOT_IMPLEMENTEDreject foris_causal+nonpad_kv_seqlenwithS_q != total_kvand nopast_key— the spec now defines this result, so the op computes it rather than rejecting.Full-prefill (
offset = 0) andpast_keydecode paths remain bit-identical. ContribMultiHeadAttention/GroupQueryAttentionconsume the shared FMHA kernels and are unchanged — only the ONNX-domainAttentiondispatch is retargeted.Test coverage
AttentionTestgtests: 73/73 pass, including new bottom-right-offset, structural-empty causal row → 0 (CPU + CUDA), and fp16 fully-masked-row goldens.test_onnx_attention: 277/0 — includes the updatedtest_tensorscatter_attention.py(stale negative-reject → positive bottom-right acceptance).Preemptive onnx#8068 node-test skips (de-skip TODO)
This branch adds the new onnx/onnx#8068
Attentionbackend node tests to both skip lists so they don't fail before the onnx dependency is bumped:onnxruntime/test/testdata/onnx_backend_test_series_filters.jsonconnxruntime/test/onnx/TestCase.cc(C++GetBrokenTests)These are a no-op on the current onnx pin (v1.21.0). TODO (de-skip): remove both skip lists once
cmake/external/onnxis bumped to a release containing onnx#8068.Deferred follow-up
q_seq > 1Python bottom-right parity coverage requires upgrading thetest_onnx_attentionsuite's numpy/torch reference functions from total-kv-relative causal (offset = kv_seq − q_seq) to nonpad-relative bottom-right (offset = nonpad_kv_seqlen − q_seq); a naiveis_causal=1flip on the current refs is a no-op or a false failure against the correct kernel. Theq_seq > 1/nonpad < q_seqbehavior (including structural-empty rows) is already locked by the C++ gtest goldens. Tracked as follow-up.References
is_causalon thenonpad_kv_seqlen/no-past_keypath + composedis_causal+attn_maskNaN robustness). Separately pushed, CI green, awaiting SIG review.