Skip to content

[CUDA] Commit and enforce phase-aware GQA dispatch policies - #33316

Open
Chi Lo (chilo-ms) wants to merge 12 commits into
mainfrom
chilo/gqa-committed-policy
Open

Chi Lo (chilo-ms) wants to merge 12 commits into
mainfrom
chilo/gqa-committed-policy

Conversation

@chilo-ms

@chilo-ms Chi Lo (chilo-ms) commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Description

Resolve session.kernel_dispatch_policy for CUDA GroupQueryAttention into runtime-enforced route masks for prefill, decode, and chunked prefill.

  • auto preserves existing phase-applicable dispatch behavior.
  • latency preserves the existing per-phase priority: XQA -> cuDNN -> Flash/fast-decode -> MEA -> unfused.
  • memory independently selects the minimum-workspace covering route set for each phase, including a phase-local covering fallback when needed. Decode does not inherit higher-workspace prefill selections.
  • safe restricts dispatch to routes with analytical workspace recipes and excludes cuDNN. It remains usable without a declared shape/envelope workspace bound; this route restriction is not itself a workspace-bound commitment.

Every runtime dispatch branch checks the current phase's permitted mask. A non-auto policy returns an explicit phase-specific error when no permitted route can serve the run, rather than escaping to an unaccounted route.

Accounting and eligibility

  • L1 and L2 resolve the same policy and budget the maximum across admitted phases and their permitted routes, not the sum of phase workspaces. L2 commits the masks after kernel creation and PrePack. The code now documents initialization-before-Run publication and the absence of concurrent re-declaration support; the sized set is the union of committed routes, not every existentially reachable route.
  • Exact head geometry from executable-graph metadata is kept separate from max-shape hints. Flash reachability and coverage use the matching FP16/BF16 predicate; exact-static heads are checked directly, while unknown heads retain the conservative admitted-domain sweep, including ORT_QUICK_BUILD restrictions.
  • Runtime-only rejection, such as non-aliasing caches disabling XQA, may use only a fallback explicitly committed for that phase. Decode-only routes do not leak into prefill or chunked prefill.
  • memory requires successful Level-2 commitment and is rejected at GQA initialization in CUDA-plugin, CUDA-minimal, and ORT-minimal configurations where that commitment cannot run. safe retains its constructor-installed route restrictions if estimation is unavailable; capacity-aware placement still requires a proven estimate.
  • cuDNN has no graph-free workspace oracle: auto/latency preserve its dispatch behavior, while strict capacity-aware accounting declines an estimate when it is reachable.

Regression coverage

Added scalar, node-adapter, Level-2, plugin-initialization, and production InferenceSession::Run() regressions for phase-local decode selection, exact-static head-128 Flash coverage across FP16/BF16 and quantized/nonquantized caches, quick-build versus unknown-head behavior, and Safe with missing envelopes or dynamic sequence shapes. Runtime cases assert CUDA assignment, permitted masks, selected routes, numerical output, and explicit no-permitted-route failures. Safe/cuDNN cases include an Auto control and explicit unsupported-device/build/version skips. A focused initialization regression also verifies that non-windowed Memory without an envelope identifies the required session option explicitly.

Validation

Passed on the refreshed 15-file C++ PR delta, using the existing isolated environment and CI-pinned clang-format 20.1.8 / lintrunner 0.13.1:

. .\.venv\Scripts\Activate.ps1
$files = @(git diff --name-only origin/main -- include onnxruntime | Where-Object { $_ -match '\.(cc|h)$' } | ForEach-Object { $_.Replace('/', '\') })
& .\.venv\Scripts\lintrunner.exe @files

Result: passed, ok No lint issues.

  • git diff --check origin/main -- include onnxruntime - passed.
  • git diff --cached --check origin/main -- include onnxruntime before the merge commit - passed.
  • Source audit of the policy delta - passed: no conflict markers or duplicate test definitions; strict parser matches merged main; the raw shared parser has no graph/config facade dependency; main's missing-profile Level-1 accounting behavior and the review regressions are retained.

Not run, per request: local ORT/CUDA builds or C++/GPU test execution. Compilation and CUDA/quick-build regression execution require new-head CI confirmation; lint/source checks are not substitutes for those results.

Stack and scope

#33042 has merged. This PR now targets main directly and includes main at 314ff8d4e8, integrated in 8fc52ed4ec. The merge preserves strict dispatch-policy validation, explicit forwarding, and main's missing-profile Level-1 accounting fix. The Level-2 GQA envelope plumbing from #33240 is included. The coordinated design is documented in #32944.

GQA only; other kernels are unchanged. latency is an ordered heuristic, not a calibrated timing model. Shape/KV envelopes remain estimation declarations rather than runtime-enforced input limits. Whole-graph lifetime accounting and allocator/runtime reserves remain separate work; this does not provide a whole-session no-OOM guarantee.

Motivation and Context

Make the selected phase-aware policy enforceable at runtime without admitting an unaccounted fallback. Address review findings that could inflate decode workspace through prefill-route inheritance, reject statically supported Flash-only nodes under quick builds, or inconsistently require a workspace envelope for Safe in ordinary versus plugin/minimal builds.

@chilo-ms Chi Lo (chilo-ms) changed the title [CUDA] Commit and enforce GQA dispatch policies [CUDA] Commit and enforce phase-aware GQA dispatch policies Oct 12, 2026
@titaiwangms
Ti-Tai Wang (titaiwangms) requested a balanced review from Copilot October 13, 2026 23:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The memory policy leaves CUDA plugin GQA kernels with permanently empty route masks, causing every run to fail.

1 open finding
What changed in this PR

Adds phase-aware CUDA GQA dispatch policies tied to workspace accounting and runtime enforcement.

Changes:

  • Resolves auto, latency, memory, and safe into phase-specific backend masks.
  • Enforces committed masks at runtime and validates GQA KV envelopes.
  • Expands policy, phase, accounting, and Level-2 commitment tests.
File Description
group_query_attention_workspace_header_test.cc Adds phase and ABI assertions.
group_query_attention_workspace_estimate_test.cc Tests policy resolution and accounting.
resource_accountant_test.cc Tests INT32 envelope validation.
cuda_execution_provider.cc Passes policy into L1 estimation.
resource_accountant.cc Centralizes envelope parsing.
group_query_attention.h Stores committed phase masks.
group_query_attention.cc Enforces masks during dispatch.
group_query_attention_workspace_estimate.h Documents resolved policy semantics.
group_query_attention_workspace_estimate.cc Builds phase-aware bounds.
group_query_attention_workspace_bounds.h Defines phases and committed masks.
group_query_attention_workspace_bounds.cc Resolves policies and aggregates phase bounds.
onnxruntime_session_options_config_keys.h Documents policy and envelope behavior.
resource_accountant.h Exposes the validated envelope parser.
kernel_dispatch_policy.h Adds policy-name formatting.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +142 to +144
case KernelDispatchPolicy::Memory: {
StoreCommittedBackends({});
break;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in c4cd281 and 769ac8f. memory is rejected during GQA kernel initialization wherever Level-2 commitment cannot run, rather than constructing a kernel with empty masks. This covers CUDA plugin builds and minimal configurations; the follow-up also covers ORT_MINIMAL_BUILD, where the host skips Level-2 declarations. auto, latency, and safe remain available.

The plugin regressions cover the initialization-time error and an unrelated Mul control so this does not become a global rejection of the session option. No local build/tests were run, as requested; validation is left to the new-head CI.

@titaiwangms Ti-Tai Wang (titaiwangms) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed f148b46dc8c60bbd6d3008b6c99975a5cde3e4da -> 3526b5a8329c2ae8b763bb0acaeaea51bc4f6c7c with the full five-role team. Requesting changes for two implementation defects and one important regression-test gap; inline comments provide triggering conditions and minimal corrections.

ID Severity Raised by Finding Disposition
F1 Major Critical, Integration Host-only parser dependency crosses the ordinary CUDA shared-provider boundary Confirmed by source/link/bridge inspection; no executed link/load repro
F2 Major Code Added tests do not execute production runtime policy gates/fallback/no-route behavior Confirmed missing-test finding, not a claimed route escape
F5 Major Deep Existence-based head reachability becomes an invalid sole-route Memory commitment Confirmed by pinned integer recipes and runtime predicates; no GPU execution

The F5 head512/envelope64 example is an availability regression (no permitted route despite supported alternatives), not a demonstrated OOM. The declared envelope is still caller-trusted and is not enforced as a runtime input limit.

Minority/disposition notes: Integration's F1 Critical rating is normalized to Major because this is a blocking unmerged build/load regression, not a demonstrated production incident. Readability's proposed Major for unknown policy tokens is rejected as a correctness blocker: fallback to Auto is the explicit parser contract; a typo warning would be a non-blocking improvement. Readability's generic-diagnostics Major is downgraded to Minor; please preserve the missing envelope/shape/oracle reason where practical. Integration retracted the proposed stack-tip merge-conflict/new-regression finding after confirming the bad test access was inherited and normal merging retains the parent fix.

Non-blocking open questions: Critical/Integration ask whether normal/plugin Safe initialization prerequisites intentionally differ when proof is unavailable, and whether a supported ORT_MINIMAL_BUILD configuration can retain GQA while excluding L2 without constructor rejection. Deep/Integration request clarification of L1-to-L2 selection consistency when shape bounds and candidate feasibility change; no additional counterexample beyond F5 was established. Deep also leaves Unfused's BNSH feasibility capture and an inherited head512 XQA recipe question unverified, neither used as a new blocker. CUDA decline/CPU fallback semantics need intent clarification, not universal CPU policy enforcement. Readability's custom-accountant policy drift was not established on the normal factory path; the Latency no-accountant fail-closed question was resolved by the final non-Auto error guard.

Other non-blocking improvements: document constructor/L2/Run mask lifecycle and relaxed-atomic assumptions; distinguish feasible, committed and sized backend sets; explain per-phase aggregation and Auto versus Latency. Failed-XQA-scratch/fallback overlap was not established: the inspected rejection precedes scratch allocation, and cuDNN returns instead of falling through.

Validation/exclusions: no local build, provider-load repro, Run tests, sanitizer, benchmark, concurrency stress or physical-memory/OOM measurement. There is no qualifying exact-head CUDA CI evidence for this child; #33016's framework/plugin results do not validate these new runtime paths. Readability's root coverage was partial; this is not an exhaustive whole-graph accounting or refreshed-stack review. Threat-model coverage concerns availability/resource admission and provider boundaries; no new privilege or information-disclosure issue was established.

case KernelDispatchPolicy::Latency:
break;
}
#if !defined(USE_CUDA_MINIMAL) && !defined(DISABLE_CONTRIB_OPS) && !defined(BUILD_CUDA_EP_AS_PLUGIN)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major: this introduces an unresolved host-framework dependency into the ordinary CUDA shared provider. ParseCudaGqaWorkspaceMaxTotalSequenceLength is an out-of-line function implemented in core/framework/resource_accountant.cc, but the ordinary CUDA provider source/link lists do not include that implementation or link onnxruntime_framework, and the inspected provider-host bridge/export surfaces do not expose it. Every normal-build GQA constructor reaches this call, including Auto with the envelope unset. Plugin builds compile this call out, so plugin-only CI cannot cover the regression.

Depending on the platform/linker, this blocks linking or provider loading. This is a static boundary finding, not an executed link/load claim. Please move lexical/range validation into a provider-safe helper accepting the raw string while keeping config retrieval on each side, or add an explicit provider-host bridge. Linking the host framework into the provider or passing the provider-side ConfigOptions facade directly to a native host implementation is not a safe boundary fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 769ac8f. The shared lexical/range validator is now header-only in core/framework/cuda_gqa_workspace_config.h and accepts only the raw string and integer output. The host wrapper and CUDA constructor each retrieve their own config entry and then call that helper. The CUDA constructor no longer includes resource_accountant.h or calls the native, out-of-line ConfigOptions overload. No framework linkage or provider-host bridge was added.

Unset still means zero; explicit values retain the same decimal/INT32 range validation and INVALID_ARGUMENT error. Added a raw-value regression including boundary values and unchanged output on failure. This is a source-level boundary fix; no local link/load test was run per the no-build request, and new-head CI is pending.

GQAExecutionPhase::ChunkedPrefill}) {
if (!IsGQAPhaseAdmitted(bounds, phase)) continue;
const auto selected =
SelectMinimumWorkspaceRoute(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major: candidate reachability does not establish coverage of the actual node geometry before committing a sole route. The inherited Flash predicate asks whether some supported head exists below the bound, and Flash sizing clamps the head to 256. That over-approximation was harmless for the earlier all-route maximum, but is not sufficient for this minimum-route commitment.

Concrete static counterexample: SM80/108 SMs, fp16, Flash/MEA enabled, BNSH, B=1, S=4, Hq=8, Hkv=2, head512, query [1,4,4096], K/V [1,4,1024], past [1,2,64,512], envelope64, Memory, and no quantization/bias/sink/rotary/packed-QKV/QK-norm. For Prefill and ChunkedPrefill, all candidates have 12 preparation bytes (backend offset256) and 131,072 preservation bytes. Complete candidate totals are Flash131,456, Unfused213,248 and MEA1,245,440, so the committed set is {Flash}. Runtime Flash eligibility requires head<=256; actual head512 fails. XQA is decode-only, and MEA/Unfused are excluded by the mask, so the new non-Auto no-permitted-route error follows despite supported alternatives.

These are pinned-source integer/control-flow results, not measured Auto success or OOM. Please require exact-static/full-admitted-domain eligibility before selecting a route. For nonexact bounds, retain a covering fallback set or decline commitment rather than treating an existence predicate as coverage. Add a head512/envelope64 regression.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 769ac8f. Per-phase covering_backends proofs are now separate from existential reachable_backends. Coverage checks the real Flash/MEA eligibility predicates across admitted head multiples, including quick-build head restrictions and MEA's head512 decode preference. A partial-coverage candidate is sized together with a proven covering fallback; commitment is declined if no covering route can be proven. XQA and Flash fast decode cannot be sole covering routes because cache aliasing remains a runtime fact.

The exact SM80/108-SM, B1/S4/head512/envelope64 regression now asserts {Flash, Unfused} for Prefill and ChunkedPrefill, with a complete Memory bound of 213,248 bytes versus the Auto maximum of 1,245,440 bytes. Added the no-covering-fallback decline case and CUDA Run regressions asserting Unfused execution for this head512 workload. These are added assertions, not locally measured results: no local CUDA build/execution was performed, and CI is pending.

EXPECT_TRUE(requirements.empty());
}

TEST(GroupQueryAttentionWorkspaceEstimateTest,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major regression-test gap: this tests initialization/declaration, not the runtime contract introduced by this PR. The added tests exercise resolver/helpers or initialize a session and inspect committed requirements; they do not Run() through the new Memory/Safe permission gates, allowed fallback, or no-permitted-route error. Removing those runtime gates would leave the added assertions intact. The head512/envelope64 counterexample in the bounds comment illustrates why resolver-only coverage is insufficient.

Please add production-path Prefill, Decode and ChunkedPrefill tests with committed-route observability, an allowed fallback after runtime-only eligibility rejection, a no-permitted-route failure, and an Auto control/no-accountant case. Ensure they exercise CUDA rather than silently falling back to CPU. This finding is missing enforcement coverage, not a claim that an unpermitted route has been observed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 769ac8f with production InferenceSession::Run() tests, not just resolver/declaration checks:

  • Prefill, Decode, and ChunkedPrefill under Memory, Latency, Safe, and Auto.
  • Explicit independent GPU cache outputs to reject XQA's runtime aliasing condition while retaining and executing the committed Unfused fallback.
  • A no-permitted-ChunkedPrefill failure after narrowing the phase set through the production declaration API, plus an Auto control.
  • Auto with neither resource accounting nor a declared envelope.
  • Safe excluding cuDNN while an otherwise-identical Auto decode control selects cuDNN, with explicit skips for unsupported device/build/version combinations.

Every Run asserts CUDA node assignment, permitted-mask/phase/selected-route diagnostics, non-aliasing, and numerical output (or the expected no-route error). The opt-in ORT_ENABLE_ATTENTION_KERNEL_DEBUG_INFO path exposes those facts without adding global mutable test hooks. No local build/tests were run as requested; the new-head CI must execute these regressions. Threads are left open for reviewer confirmation.

Chi Lo (chilo-ms) and others added 2 commits October 16, 2026 10:05
… estimator (reader-only)

Adds the hardware-neutral session option session.attention_dispatch_policy and
threads its value through to the CUDA GroupQueryAttention Level-1 workspace
estimator. This is the Tier-1 intent knob from the policy-based workspace
estimation design: "latency" favors high-workspace fast routes, "memory"
favors low-workspace routes (so more nodes fit on GPU), and "safe" assumes the
bounded fallback everywhere. Unset (default) or any unrecognized value means
"auto" and keeps the current route-aware behavior.

This is a pure no-functional-change reader. The raw string is parsed
(WorkspaceEstimatorConfig::attention_dispatch_policy ->
AttentionDispatchPolicy enum via ParseAttentionDispatchPolicy) and stored on
GQAWorkspaceEstimateConfig::dispatch_policy, but the estimator does not yet
consume it, so every policy value produces the identical estimate. A follow-up
PR biases route selection on the committed policy (coverage validation and the
bounded fallback land in their own PRs).

Plumbing mirrors the ep.cuda.gqa_workspace_max_total_sequence_length reader.
The enum/parser live in the CUDA GQA estimator header because it is the only
consumer today; hoist to a neutral header when a second op consumes it.

Tests: extends the FactoryRetainsNarrowWorkspaceEstimatorConfig framework test
(config string -> WorkspaceEstimatorConfig field, CPU CI) and adds a
provider-test asserting the token parsing (exact-match, unknown -> Auto) and
the reader-only invariant (no policy value changes the estimate).

Part of the policy-based workspace estimation effort; see
docs/annotated_partitioning/policy_based_workspace_estimation.md and the PR
chain (#32944, #32945, #33001, #33014, #33016). Base retargets toward main as
the stack merges.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
Rename the unreleased option to session.kernel_dispatch_policy. Move the shared enum and parser into a CUDA-independent framework header and retain GQA as the first reader-only consumer. Move token tests into the framework suite and preserve estimator behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
@chilo-ms
Chi Lo (chilo-ms) force-pushed the chilo/attention-dispatch-policy-reader branch from f148b46 to 9f1c163 Compare October 16, 2026 17:10
Chi Lo and others added 5 commits October 16, 2026 10:25
Default the optional reader policy explicitly to std::nullopt so partial WorkspaceEstimatorConfig aggregate initializers do not trigger -Wmissing-field-initializers under GCC and Clang. Add a regression for the omitted-policy initialization pattern while preserving reader-only behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
Resolve latency, memory, and safe intents to analytically accounted route masks shared by Level 1, Level 2, and runtime dispatch. Runtime fallback is restricted to the committed mask and non-auto policies fail closed when no permitted route is available.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
Resolve and enforce separate prefill, decode, and chunked-prefill route masks while budgeting the maximum across admitted phases.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
Reject memory dispatch during kernel initialization when the Level-2 commitment hook is unavailable, including CUDA plugin builds. Preserve other policies, cover plugin initialization and unrelated kernels, and document the restriction.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
Keep envelope validation provider-safe with a shared raw-value parser and
local configuration retrieval. Distinguish route reachability from full-domain
eligibility and retain a proven covering fallback before committing a policy.

Add CUDA Run coverage for every phase, non-aliasing decode fallback, Safe's
cuDNN exclusion, no-route errors, and Auto without resource accounting.
Reject memory policy in ORT-minimal builds that skip Level-2 commitment.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9

@tianleiwu Tianlei Wu (tianleiwu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong implementation of phase-aware GQA dispatch policy commitment and runtime enforcement, including the separation of existential reachable_backends from full-domain covering_backends and the header-only ParseCudaGqaWorkspaceMaxTotalSequenceLength validator. Left three inline comments on decode_fallbacks unioning in Memory policy decode selection, ORT_QUICK_BUILD / BFloat16 handling in BuildBounds's coverage loop, and Safe policy initialization consistency across normal vs. plugin/minimal builds.

Comment thread onnxruntime/contrib_ops/cuda/bert/group_query_attention_workspace_estimate.cc Outdated
Comment thread onnxruntime/contrib_ops/cuda/bert/group_query_attention.cc
Chi Lo and others added 3 commits October 16, 2026 16:21
Reject unknown non-empty session.kernel_dispatch_policy values with INVALID_ARGUMENT before creating accountants, including when partitioning is disabled. Keep empty and auto defaults, retain validated raw tokens, and enforce the CUDA consumer invariant. Forward policy through the GQA config helper and cover parser rejection, factory validation, and node-adapter defaults/forwarding.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 24b20465-fd4d-4a64-8174-aa0622c5164d
Keep provider-safe GQA envelope parsing and phase-aware policy enforcement while accepting the reader's strict parser and centralized config construction. Update the GQA runtime and node-adapter regression to the strict parser and enforcing consumer contract.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 24b20465-fd4d-4a64-8174-aa0622c5164d
Keep Memory decode fallbacks independent of prefill selections. Prove Flash coverage with exact graph head metadata and the matching FP16/BF16 predicate, including quick builds. Retain Safe route restrictions when no workspace envelope can be estimated.

Add scalar, node-adapter, Level-2, and CUDA Run regressions, and align plugin initialization expectations with strict policy validation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 24b20465-fd4d-4a64-8174-aa0622c5164d

@titaiwangms Ti-Tai Wang (titaiwangms) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the actual stacked delta ce93b4849f438c1e45061e317e1bc376cf70c38b -> 8c746850cea0130b98c66d563992ca0118d1db2a with the full five-role team. No new substantiated implementation blocker was established in the bounded review. The previous findings are addressed in the inspected source, but I am leaving a COMMENT rather than approval because the exact-head CUDA build/Run evidence remains outstanding. This PR still targets the #33042 reader branch, not main, and is currently draft.

Prior-finding dispositions:

  • Shared-provider parser dependency: addressed by the raw-string/scalar header helper. Host/provider retrieve config locally; the normal CUDA constructor no longer calls the native host ConfigOptions overload. No new framework linkage/bridge is used.
  • Existential Flash eligibility used as sole-route coverage: addressed by separating reachable from covering backends, retaining a proven phase-local fallback, and using executable-graph static-head provenance and the correct FP16/BF16 predicates. The previous unknown-head-bound-512 counterexample now retains Flash|Unfused with the 213,248-byte bound; exact static geometry is checked directly rather than inferred from a smaller eligible head.
  • Missing production enforcement tests: coverage added. Tests now call InferenceSession::Run and check CUDA assignment, phase/mask/selected-route diagnostics, independent output-cache alias rejection, output or intentional no-route failure, and Auto/Safe controls.
  • Decode fallback and initialization: decode commits its own selected.backends, not prefill selections. Safe may retain route restrictions without a proven workspace bound; capacity-aware placement still requires proof. Memory rejects unavailable commitment, including the inspected plugin/minimal cases.

Validation limitation: this exact head has only seven lint/docs/policy checks, with no qualifying CUDA build or test result. QA verified that the requested selectors exist, but none of the six available local binaries has proven reviewed-head provenance. No stale binary was executed as evidence and no full rebuild was started. The needed production batch is:

GTEST_FILTER='GroupQueryAttentionDispatchPolicyTest.*:GroupQueryAttentionWorkspaceEstimateTest.KernelCommits*' \
GTEST_COLOR=no timeout 300s \
./onnxruntime_provider_test --gtest_filter=CUDA_EP_Unittest.All

This requires an exact-head nonminimal/non-plugin CUDA internal-test build; Flash/MEA/cuDNN controls require their corresponding build/device support. Please retain actual OK versus SKIPPED versus compile-excluded distinctions. Source assertions do not establish numerical output, actual backend dispatch, or link/load success.

Non-blocking questions/follow-ups: consider nonuniform inputs and returned-cache assertions in a follow-up numerical/cache-sensitive case; clarify Auto versus Latency and the constructor/L2/Run mask lifecycle/commitment side effect; document reachable/covering/committed/permitted terminology. The explicit-empty numeric-envelope versus empty-policy difference is inherited contract behavior, not a new blocker. Cross-build initialization and token-validation behavior still need relevant execution coverage.

No Critical/Major finding was overruled for this PR in the new round. The Critical lane covered availability/resource boundaries and opt-in diagnostic metadata, not a broader exploit audit. Excluded: local build/Run/sanitizer, measured VRAM/OOM, numerical/performance/concurrency certification and exhaustive transitive consumers. Level-2 is still verification/commitment, not an automatic repartition loop; the caller-declared envelope remains an estimation input rather than runtime admission enforcement or a whole-session no-OOM guarantee. This comment records source-level closure of the previous issues, not runtime clearance or dismissal of the previous review.

Chi Lo (chilo-ms) added a commit that referenced this pull request Oct 21, 2026
### Description
<!-- Describe your changes. -->

Add reader-only plumbing for the hardware-neutral session option
`session.kernel_dispatch_policy`.

The intent is generic across operators and EPs. `KernelDispatchPolicy`
and the shared, header-only `TryParseKernelDispatchPolicy` live in
`core/framework/kernel_dispatch_policy.h`, with no CUDA or graph
dependencies. CUDA GQA is the first consumer, not the owner of the
policy.

For valid values, **no policy changes runtime dispatch, workspace
estimates, or partitioning** in this PR. `latency`, `memory`, and `safe`
remain intent metadata for future policy resolution. In particular,
`memory` does not reduce the reservation, and `safe` does not select a
bounded fallback or provide a no-OOM guarantee.

#### Reader contract

- Unset, empty, and explicit `auto` resolve to `Auto`. The other valid
tokens are exactly `latency`, `memory`, and `safe`.
- Unknown, differently cased, whitespace-only, and whitespace-padded
non-empty values fail session initialization with `INVALID_ARGUMENT`;
they never silently resolve to `Auto`. Validation runs even when
partitioning is disabled.
- The accountant factory retains the validated raw optional string
without changing other settings. The CUDA consumer enforces successful
parsing rather than silently accepting an invalid configuration.
- `GetGroupQueryAttentionWorkspaceEstimateConfig` accepts
`dispatch_policy = KernelDispatchPolicy::Auto` alongside the envelope
and initializes both fields in one place. The node-estimator overload
and CUDA test bridge forward the policy through that helper.
- Setting the policy alone does not enable partitioning or non-windowed
GQA estimation without a KV envelope.
- This replaces the earlier, unreleased attention-specific option name;
there is no compatibility alias.

#### Integration and regression coverage

Preserve #33014/#33016's envelope validation against `[0, INT32_MAX]`,
invalid-value diagnostics, and fail-closed GQA placement when no proven
estimate is available. Reuse the exported graph-config helper instead of
duplicating node parsing. Keep envelopes, eligibility, and sizing local
to GQA; other operators and EPs can adopt the shared intent
independently.

Framework regressions cover valid-token parsing, rejected tokens
(including `Memory`, `SAFE`, `memroy`, and ` memory `), unchanged parser
output on failure, validation with partitioning enabled and disabled,
raw-token retention, unset defaults, and policy-only configuration not
enabling partitioning. Existing KV-envelope boundary tests remain
intact.

CUDA metadata regressions cover default and explicit policy forwarding
through the node-config helper/test bridge, identical bytes/backend sets
for windowed and non-windowed estimation, and unchanged decline without
a non-windowed envelope.

#### Validation for this review update

- Changed-file lintrunner/clang-format check: passed for all 10 edited
C++ files.
- `git diff --check`: passed.
- No full ORT build or C++ regression suites were run locally for this
update, as requested. Earlier-head build evidence does not validate this
head; full framework/CUDA validation is left to CI.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->

Provide a generic policy representation and reader without coupling
other consumers to GQA or CUDA. Rejecting typos now prevents future
enforcement (including #33316's committed policies) from silently
falling back to `Auto` and bypassing the requested restrictions.

This PR builds on #33016, which is now merged, and targets `main`.
Committed policies, operator/EP-specific resolution, route restrictions,
runtime coverage/envelope enforcement, bounded fallback, and reduced
policy-bound estimates remain separate follow-ups.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Chi Lo <lochi@microsoft.com>
Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
Copilot-Session: 24b20465-fd4d-4a64-8174-aa0622c5164d
Base automatically changed from chilo/attention-dispatch-policy-reader to main October 21, 2026 15:54
Preserve strict policy validation and profile-missing Level-1 accounting from main while retaining phase-local route commitments and runtime regressions. Explain missing non-windowed Memory envelopes and document initialization-only mask publication and workspace aggregation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
@chilo-ms
Chi Lo (chilo-ms) marked this pull request as ready for review October 21, 2026 16:05
@chilo-ms

Copy link
Copy Markdown
Contributor Author

Ready for re-review at 8fc52ed. Merged current main after #33042 landed and resolved the conflicts while preserving strict token validation, main's missing-profile Level-1 accounting, and all seven inline-comment fixes/regressions.

Also documented initialization-before-Run / relaxed-atomic mask assumptions and max-of-phase workspace aggregation, and added a focused initialization regression for the explicit missing non-windowed Memory-envelope diagnostic.

Repository lintrunner passed on the final 15-file C++ delta, as did policy-delta whitespace and source-invariant checks. No local builds or C++/GPU test execution were performed, per request; CUDA/quick-build execution still requires new-head CI. The four original threads remain open for reviewer confirmation; their existing fix explanations are retained. The three later threads were already resolved by the reviewer.

Use logging supplied by the CUDA provider bridge instead of including native host logging types. Pass a typed output-name array to the session Run string-span overload in the production dispatch-policy tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b95559c4-5cbe-4787-8dd7-ffa45a2157f9
@chilo-ms

Copy link
Copy Markdown
Contributor Author

CI fixes pushed in e1ec319.

  • Ordinary CUDA and TensorRT jobs on both Linux and Windows diagnosed native/provider logging type and macro redefinitions in group_query_attention.cc. Removed the direct host logging.h include; cuda_common.h already supplies the appropriate provider logging surface.
  • Windows CUDA also diagnosed the new dispatch test's braced output-name list failing conversion to gsl::span<const std::string>. The test now passes a typed std::array<std::string, 3> with unchanged output order and assertions.
  • The separate Linux no-cuDNN failure was an HTTP 504 downloading cpp_client_telemetry during configure, before compilation. No unrelated dependency/workflow workaround was added.

Passed: . .\.venv\Scripts\Activate.ps1; & .\.venv\Scripts\lintrunner.exe onnxruntime\contrib_ops\cuda\bert\group_query_attention.cc onnxruntime\test\providers\cuda\test_cases\group_query_attention_workspace_estimate_test.cc and staged/working diff whitespace checks. No local builds or C++/GPU execution were performed, per request. The push starts new-head CI, including a fresh attempt of the no-cuDNN dependency download; no stale-head reruns were queued. Compilation and regression execution still need CI confirmation.

@tianleiwu Tianlei Wu (tianleiwu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the three findings from the previous round (Memory decode fallback independence, static_head_size / BFloat16 Flash coverage under ORT_QUICK_BUILD, and Safe policy retention without a workspace bound).

In the current CUDA CI runs (Test Linux CUDA x64 Release, Windows GPU CUDA CI Pipeline Test Job, Test Linux TensorRT x64 Release), 5 unit tests in CUDA_EP_Unittest.All are failing:

  1. GroupQueryAttentionWorkspaceEstimateTest.H512EstimatorDominatesCompleteRuntimeWorkspace (line 542), GroupQueryAttentionWorkspaceEstimateTest.GetCapabilityBudgetUsesLevel1Estimate (line 730), and GroupQueryAttentionWorkspaceEstimateTest.KernelDeclaresPrepackedHeadSinkRoot (line 1686):
    • See inline comment on AggregateCommittedPhases in group_query_attention_workspace_bounds.cc:476: when head_sink is present (has_sink == true, excluding Unfused/MEA) and FlashAttention is either disabled (kMath) or does not cover the head size (head = 512, or head = 64 in CI's ORT_QUICK_BUILD=ON where Flash only compiles head = 128), bounds.reachable_backends is {GQAReachableBackend::Xqa} (decode-only), so committed_backends.prefill is None and AggregateCommittedPhases fails with Unavailable.
  2. GroupQueryAttentionWorkspaceEstimateTest.SingleTokenShapeDoesNotAdmitChunkedPrefill (line 1001):
    • See inline comment on group_query_attention_workspace_estimate_test.cc:1000: Config() defaults to config.enable_xqa = true, so committed_backends.decode under Safe is GQAReachableBackend::Xqa | GQAReachableBackend::Unfused (0x11) rather than GQAReachableBackend::Unfused (0x10).
  3. GroupQueryAttentionWorkspaceBoundsTest.AggregatesExclusiveRoutesWithMax (group_query_attention_workspace_estimate_test.cc:1376):
    • Because GetGQAWorkspaceAggregateForPermittedPhase (group_query_attention_workspace_bounds.cc:282) now sizes GQAExecutionPhase::Decode with sequence_length = 1 instead of bounds.sequence_length_bound (4), unfused_only.total_workspace_bytes at S = 1 (37,120 bytes) is now smaller than xqa_only.total_workspace_bytes (140,030 bytes for 108 SMs), failing ASSERT_GT(unfused_only.total_workspace_bytes, xqa_only.total_workspace_bytes). Increasing xqa.present_kv_cache_capacity_bound (e.g., to 2048) or xqa.batch_size_bound in that test will make unfused_only exceed xqa_only at S = 1.

return aggregate;
}

const auto phase_aggregate =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check causes 3 existing unit tests in group_query_attention_workspace_estimate_test.cc to fail in CI (H512EstimatorDominatesCompleteRuntimeWorkspace at line 542, GetCapabilityBudgetUsesLevel1Estimate at line 730, and KernelDeclaresPrepackedHeadSinkRoot at line 1686):

  • In H512EstimatorDominatesCompleteRuntimeWorkspace (kMath, head = 512, head_sink present) and GetCapabilityBudgetUsesLevel1Estimate (kMath, head = 64, head_sink present), has_sink excludes Unfused and MemoryEfficient, and kMath disables Flash, leaving bounds.reachable_backends == GQAReachableBackend::Xqa (decode-only).
  • Similarly, in KernelDeclaresPrepackedHeadSinkRoot, CI builds with onnxruntime_QUICK_BUILD=ON, where Flash only supports head == 128, so the default head = 64 model with head_sink also has bounds.reachable_backends == GQAReachableBackend::Xqa.
  • Because BuildBounds sets bounds.prefill_reachable = true (and chunked_prefill_reachable = sequence_bound > 1), GetGQAReachableBackendsForPhase(Xqa, Prefill) is None, and AggregateCommittedPhases now returns Unavailable("The GQA dispatch policy has no route for an admitted phase.") even under KernelDispatchPolicy::Auto.

Either Auto/Latency/Safe should skip phases that have no reachable backend in bounds.reachable_backends (as long as at least one admitted phase is sized, or when Auto is used), or those three tests need a general-phase-eligible configuration (e.g. kFlash with head = 128 for GetCapabilityBudgetUsesLevel1Estimate / KernelDeclaresPrepackedHeadSinkRoot, and decode-only bounds or a general route for H512EstimatorDominatesCompleteRuntimeWorkspace).

config, SeparateShapes(/*sequence=*/1), Device(), options);
ASSERT_TRUE(estimate.has_value());
EXPECT_EQ(estimate->committed_backends.prefill,
GQAReachableBackend::Unfused);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SingleTokenShapeDoesNotAdmitChunkedPrefill fails here in CI (Actual: 0x11 vs Expected: 0x10):
Config() sets config.enable_xqa = true by default, and options.InitializeOnce(kMath, true) only disables Flash and MEA, so Xqa remains reachable in Decode and Safe commits GQAReachableBackend::Xqa | GQAReachableBackend::Unfused (0x11). Set config.enable_xqa = false; above (or expect GQAReachableBackend::Xqa | GQAReachableBackend::Unfused).

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants