Repository navigation
fix(engine): bind fixed CUDA decoder state by shape - #436
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 81.31% 80.58% -0.74%
==========================================
Files 314 314
Lines 122493 122493
Branches 122493 122493
==========================================
- Hits 99606 98711 -895
- Misses 18862 19763 +901
+ Partials 4025 4019 -6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
|
VERDICT: APPROVE Independent review by Lori (opus) — I did not author this code. Reviewed against What I verified1. KV (rank-4 seq-growing) path is preserved — no regression
2. Generalizes by metadata, not by name
3. Allocation correctness (rank-3 FP16)
4. Tests are meaningful and pass
5. fmt / clippy
Minor, non-blocking observations (no fix required to merge)
Note for future runsThe suggested I did not run the full 27B E2E (optional); the rank-3 FP16 binding test is the core deliverable and it passes on GPU. |
## Summary - add rank-3 NCL Conv support with an output-owned NVRTC kernel - support groups/depthwise convolution, stride, dilation, optional bias, and asymmetric causal padding - preserve the existing rank-4 NCHW cuDNN path unchanged - add GPU-vs-CPU EP parity for basic, depthwise causal FP16, and grouped strided/dilated Conv1D ## Implementation choice Rank-3 tensors cannot always be lifted directly into the existing cuDNN path because the cuDNN legacy forward API used here requires symmetric padding, while hybrid LLM blocks require asymmetric causal padding such as [3, 0]. A native output-owned kernel handles the complete 1-D ONNX geometry without staging buffers or special-casing model names. ## Qwen3.6-27B evidence The native CUDA probe now clears the former rank-3 convolution failure at __fn0_Conv_node_12. Execution proceeds into the layer-0 linear-attention block and stops at the next independent blocker: no inferred shape for the Silu output v_model.layers.0.linear_attn.conv1d.CausalConvWithState_56_0. ## Validation - cargo test -p onnx-runtime-ep-cuda --test conv_gpu - cargo test -p onnx-runtime-ep-cuda --test cuda_conformance_gpu every_covered_op_has_a_conformance_entry - cargo test -p onnx-runtime-ep-cuda --test cuda_conformance_gpu conformance_sweep_matches_cpu - cargo fmt --all -- --check - cargo clippy -p onnx-runtime-ep-cuda --lib -- -D warnings - all-target clippy remains at the same 48 pre-existing errors, with no new Conv1D warning ## Stack This PR is based on and requires #436. Merge order: #436, then this PR. References #384 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This is a follow-up build hotfix for the native-backend clippy check failure. **Root cause:** LoopStatePair import at the module level is only used inside the #[cfg(feature = "cuda")] build_cuda_decoder_with_fixed_state function. In non-cuda builds (native-backend), the import is unused, causing `clippy -D warnings` to fail with: ``` error: unused import: LoopStatePair ``` **Fix:** Move the LoopStatePair import behind #[cfg(feature = "cuda")] to match its actual usage, following the same cfg-gate fix pattern from #436. **CI status:** Verified that `cargo clippy --locked --all-targets -p onnx-genai-engine --features native-backend -- -D warnings` passes with this fix. Fixes #438 native-backend CI build failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y exclusive (#446) ## Summary Fast-follow to the merged live GPU weight offload (#444, closing #63): make **weight offload** and **CUDA graph capture** explicitly **mutually exclusive**. ### Why Live weight offload pages weights host↔device with `cuMemAlloc` / `cuMemcpyHtoD` / `cuMemFree` — all **illegal during CUDA graph capture**. The previous code only skipped the residency stream-sync while capturing (`if !is_capturing()`), which *treated capture as a supported-but-degraded mode* even though the alloc/copy/free ops it guards can never legally run under capture. So enabling `ONNX_GENAI_WEIGHT_OFFLOAD=1` together with graph capture (which auto-enables for owned CUDA KV) was a latent foot-gun — flagged in Lori's review of #444. ### What changed - **Mutual exclusion at the decision point.** `resolve_graph_capture_enabled` (native decode session load) now takes a `weight_offload_enabled` input with **highest precedence**: when offload is on, capture resolves to **OFF** — beating structural auto-enable, an explicit `ONNX_GENAI_CUDA_GRAPH=1`, and a programmatic `Some(true)`. Graceful: **offload wins, capture is skipped, logged once** (`std::sync::Once`): `"weight offload is incompatible with CUDA graph capture; capture disabled"`. - **Dropped the dead capture branch.** With capture now impossible while offload runs, the `is_capturing()` guard in `CudaWeightResidency::admit()` is unreachable — removed; `admit()` now always synchronizes before eviction (the sync, and the paging ops it protects, are never capture-illegal anymore). - **cfg-correct across feature sets.** The offload query is a CUDA-EP feature; behind `#[cfg(feature = "cuda")]` it reads `DeviceOffloadPolicy::from_env().enabled`, and is a plain `false` on the non-cuda (`native-backend`-only) build — no unused-import-across-feature-sets breakage (the class that bit #436/#441). ### Before / After | | offload off | offload on | |---|---|---| | **Before** | capture per structural/env/programmatic | capture may still auto-enable → paging ops run under capture = UB/foot-gun | | **After** | capture per structural/env/programmatic (unchanged) | capture forced OFF, one-time log; `admit()` always syncs safely | ## Validation - **New test** `weight_offload_forces_graph_capture_off`: offload beats safe structure, explicit env=1, and programmatic `Some(true)`; with offload off the same safe structure still enables capture (proves the exclusion is genuinely offload-caused). ✅ - Existing resolver tests updated for the new parameter; all 5 pass. ✅ - **GPU** (`CUDA_VISIBLE_DEVICES=0 taskset -c 0`): `weight_offload_gpu` — 5/5 pass with the simplified `admit()` sync (residency page-in / reuse / eviction / referenced-page-pin all still correct). ✅ - `cargo fmt --all --check` clean. ✅ - `cargo clippy` clean for changed files on **both** `cuda,native-backend` **and** `native-backend`-only (non-cuda) feature sets. ✅ ## Notes - Draft — **do not merge**. Stacked as a small safety fast-follow on the merged #444. - No change to the non-offload default fast path; capture behavior is byte-identical when offload is disabled. Refs #444, #63 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Root cause
DecodeCudaState treated every past/present pair as a rank-4 KV cache. It forced axis 2 to the KV capacity bucket and later changed axis 2 on every decode step and capacity growth. Metadata state_pairs already identify fixed replace-semantics recurrent tensors, but that distinction was discarded before CUDA binding allocation.
Real-model evidence
Qwen3.6-27B INT4 previously failed while allocating past_key_values.12.conv_state as rank-3 FP16. With this change it clears CUDA state allocation and begins the forward pass. The next blocker is the CUDA Conv kernel declining rank-3 1-D convolution at node __fn0_Conv_node_12; the old line-480 state allocation error is gone.
Validation
References #384