Repository navigation
refactor(ep-plugin): share first native shape rules - #2064
Merged
Merged
Conversation
Route ConstantOfShape, Expand, and Tile through the native inference registry while retaining plugin fallbacks for symbolic inputs and stricter native rejections. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Pin the exact shared-rule census, exercise every tri-state fallback and version/domain selector, and keep test-only hooks out of shipped builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
justinchuby
force-pushed
the
refactor/shared-shape-rules
branch
from
August 25, 2026 03:31
f255ba3 to
6f23d5e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2064 +/- ##
==========================================
+ Coverage 80.35% 80.70% +0.35%
==========================================
Files 409 427 +18
Lines 191136 211901 +20765
Branches 191136 211901 +20765
==========================================
+ Hits 153593 171025 +17432
- Misses 32103 35185 +3082
- Partials 5440 5691 +251
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
## Summary - implement `ai.onnx::STFT` on the native CPU EP - share DFT's radix-2/vDSP/scalar transform core through a reusable per-execution plan - route plugin output sizing through the native shape-rule adapter from #2064 - add strict dtype/shape/value validation and dynamic native/plugin shape tests ## ONNX contract This implements **STFT since_version 17**: - inputs: `signal: T1`, required scalar `frame_step: T2`, optional rank-1 `window: T1`, optional scalar `frame_length: T2` - at least one of `window` or `frame_length` is required; when both are present their lengths must match, so ONNX STFT has no separate window-length/FFT-length mode - `onesided` is integer 0/1 and defaults to 1 - canonical signal shape is `[batch, signal_length, 1|2]`; complex input (`...x2`) requires `onesided=0` - output is `[batch, floor((signal_length-frame_length)/frame_step)+1, bins, 2]`, with `bins=frame_length/2+1` for onesided and `bins=frame_length` otherwise - frames are complete and unpadded: no centering and no implicit padding; a signal shorter than one frame is rejected The schema permits f16/f32/f64/bf16. This CPU kernel deliberately claims **f16, bf16, and f32 only**: inputs are widened once, computed in f32, and narrowed once. f64 is rejected at claim and execution rather than pretending f32 arithmetic is f64 support. Length inputs accept int32/int64. ## Algorithm and allocation behavior For `N=frame_length`, power-of-two frames use the existing radix-2 FFT (or Accelerate vDSP where available), for `O(batch * frames * N log N)`. Arbitrary lengths currently use the existing scalar DFT, `O(batch * frames * N^2)`; no faster arbitrary-length claim is made. Signal/window strided views are materialized once. The output is allocated once. Four `N`-element scratch vectors and one DFT/vDSP plan are reused across all frames and batches; there is no per-frame `Vec` or vDSP-plan allocation. ## Tests - CPU STFT focused suite: **11 passed** (real/complex, overlap, explicit nontrivial window, strided signal/window, onesided/full conjugacy and Nyquist, exact/short/multiple frames, arbitrary length, f16/bf16, invalid contracts) - existing DFT focused suite: **5 passed** - native shape-inference STFT tests: **3 passed** - shared native/plugin rule census + agreement: **4 passed** - plugin shape-coverage suite: **4 passed** - native runtime dynamic STFT sizing: **1 passed** - CPU EP lib suite with the pre-existing Windows-only affinity test excluded: **1780 passed, 23 ignored, 1 filtered** - shape-inference full suite: **283 passed** plus **59 auxiliary/integration tests** - warnings-denied Clippy: changed five crates, all targets, green - package-scoped rustfmt check: green Mutation checks each went red independently: 1. ignoring `window` failed the strided nontrivial-window oracle (`10` vs `13.75` DC) 2. incrementing frames by `frame_length` instead of `frame_step` failed the overlap test 3. dropping the final `+1` frame failed the three-frame shape assertion 4. using `N/2` instead of `N/2+1` failed the Nyquist/shape test ## CUDA reuse A future CUDA STFT can reuse the v17 validation contract, native shape rule, shared plugin adapter, independent f64 reference formula, frame-count/onesided mutation tests, and the fixed-size transform-plan/scratch ownership pattern. This PR does not touch CUDA code or Leon's cuFFT work. ## Not verified - CUDA STFT or GPU execution (out of scope) - macOS/iOS vDSP execution on this Windows host; existing DFT vDSP tests remain in place and the setup is now reusable across STFT frames - a loaded ORT plugin end-to-end STFT session; the plugin claim coverage, shared-rule census, concrete shared shape agreement, and CPU execution paths are locally green - throughput numbers; complexity and allocation behavior are established, but arbitrary-length acceleration/benchmarking remains follow-up work The only excluded CPU-lib test is `a_default_width_pool_on_leader_cpus_uses_every_core_it_was_given`, which is Linux-affinity-specific and fails on Windows because process-wide affinity masking is intentionally unsupported there. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
This was referenced Aug 25, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 25, 2026
…2058) (#2098) Closes #2058. 21 crates are compiled and tested by CI and linted by nothing. This derives the clippy package lists from the same source the test lanes already use, and adds a guard so the two cannot drift apart again. ## The correction I owe first #2058 claimed two live `clippy::unnecessary_cast` denials on `main` in `onnx-runtime-ep-plugin/src/compute.rs`. **They are gone, and I did not fix them.** At `bc715c329`: ``` cargo clippy --locked --all-targets -p onnx-runtime-ep-plugin -- -D warnings -> 0 ``` `git log 67d3aa5..HEAD -- crates/onnx-runtime-ep-plugin/` is three feature/refactor commits (#2049, #2064, #2083); none mentions clippy or the cast. The lines were deleted incidentally by a refactor. That weakens the issue's headline and strengthens its actual point. The gap admits defects **and releases them unobserved** — nobody knows what is in there at any moment without running the lint themselves. The sample was never the argument; the mechanism is. ## The rule that created the gap was false `ci.yml` instructed: *"To add a crate, first confirm its normal+dev dependency tree contains no ort-sys/CUDA dependency."* That rule was already violated by the list it annotated. Measured: ``` crates in offline-linux whose normal+dev tree contains onnx-genai-ort-sys: 13 of those, already on the clippy list: onnx-runtime-ep-cpu, onnx-runtime-ep-api ``` And it does not matter which way you resolve that, because **`cargo clippy` only ever checks** — it never links and never runs a test binary. The offline/ort-backed split exists for `cargo test`; it has no force for lint. `onnx-genai-ort-sys` itself compiles here with no network. In #2058 I wrote that I did not know whether the premise was stale or whether `ep-plugin` should be out of `offline-linux`, and would rather flag it than guess. This is the measurement I said I would not substitute a guess for: **the premise was false.** ## What changed - All three clippy `-p` lists — **byte-identical to each other, 31 packages, repeated in three jobs** — become `$(python .github/scripts/workspace_test_packages.py cargo-args lint)`. The new `lint` lane is "every package some test lane compiles" (55). Windows ARM64 takes `offline-linux` (49), still a strict superset of the 31 it linted before, and no ORT crates on that target. - `verify` gains a **lint-coverage half**: it fails if any tested package is reached by no `cargo clippy` invocation anywhere in `.github/workflows`. Generator calls are **expanded, not skipped** — a computed `-p` list counts, and a hand-written one cannot hide behind some other step computing one. - A **self-test step** actually exercises the controls. ## Evidence **The guard detects the real defect.** Run against unmodified `main`, before the `ci.yml` change, it reports the gap by name: ``` Workspace lint coverage check failed. Package(s) are compiled and tested by CI and linted by nothing: - onnx-genai - onnx-runtime-ep-nxrt-abi - onnx-runtime-memory-abi - onnx-genai-capi - onnx-runtime-ep-nxrt-host - onnx-runtime-memory-host - onnx-genai-ort - onnx-runtime-ep-nxrt-testplugin - onnx-runtime-memory-testplugin ... 21 total ``` **The fix closes it, and the pass is not an empty selection.** The exact command `ci.yml` now runs: ``` cargo clippy --locked --all-targets $(python ... cargo-args lint) -- -D warnings -> RC=0 ``` parsed with `--message-format=json`: **wanted 55, seen 55, MISSING: none.** A clippy run that selected nothing would also exit 0, so the package set is confirmed present in the lint graph rather than inferred from the exit code. **Mutations.** | mutation | result | |---|---| | `--simulate-unlinted onnx-runtime-ep-cpu` | rc=1, names that crate | | `--simulate-missing onnx-runtime-ir` (pre-existing half, after refactor) | rc=1 | | both Linux clippy steps reverted to lane `offline-linux` | rc=1, names `onnx-genai`, `onnx-genai-capi`, `onnx-genai-ort` | | a **comment** claiming `cargo clippy -p onnx-genai ...` added, lanes narrowed | rc=1 — prose cannot buy coverage | | clean tree | rc=0 | ## Two defects the mutations found in my own work Recording both, because in each case the check was passing at the time. **1. The scanner counted a YAML comment as an invocation.** After I rewrote the explanatory comment — which contains the words `cargo clippy` — the reported invocation count went `10 -> 11`. Nothing failed; the only symptom was a number moving that I had no reason to expect to move. A comment reading `# cargo clippy -p foo` would have granted `foo` lint coverage. The scanner now skips comment lines, the count is back to 10, and the mutation table above has a cell for exactly this. **2. My first self-test passed because `python` was not on PATH.** It was written as `if cmd ...; then fail; fi` — a bare non-zero check. Command-not-found is `127`, which is non-zero, so it read as *"the guard correctly failed"*. It passed loudest in precisely the case where nothing ran. It now requires **exit 1 specifically, plus the matching failure message**: ``` interpreter present -> rc=0 both guards correctly fail interpreter MISSING -> rc=1 ::error::verify ... exited 127, expected 1 guard neutered -> rc=1 ::error::verify ... exited 0, expected 1 ``` This is the same shape as `QEMU_LD_PREFIX` and `| head`: **a weaker check fails in the direction of passing.** It is also why the self-test step exists at all — `--simulate-missing` had shipped with this guard since it was written and CI had **never once invoked it**. A control nobody runs is not a control. ## Scope and risk - `Fast (Linux x86_64)` and `Rust quality` gain 24 crates each. Verified locally: the whole 55-package set is clean at `-D warnings --all-targets`. - **`Rust (Windows ARM64)` gains 18 crates on `aarch64-pc-windows-msvc`, which I cannot run locally.** If that lane goes red this is mine and I will narrow it before merging. That lane is **not required**, which — as of last night's two merged defects that green required checks did not catch — is exactly the kind of lane I intend to read rather than assume. **I will wait for it.** - No Rust source changes. No behaviour change. Lint coverage only. Normal `--squash --auto`. No admin bypass. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
origin/mainat5e8ef116b(the merged feat(ep-plugin): claim six more ops whose shape is carried in input values #2049 foundation); all four duplicated logical feat(ep-plugin): claim six more ops whose shape is carried in input values #2049 commits droppedConstantOfShape,Expand, andTileSymbolicOrUnknownandRejectedtestutilfeature;shared_shapesis private in shipped buildsRebased commit list
8dc155b58—refactor(ep-plugin): share first native shape rules5d1d4b80a—docs(test): name remaining shape-rule duplication6f23d5e2e—test(ep-plugin): harden shared shape contractsThe diff against
origin/mainis only the incremental adapter, tests, manifests/lockfile, and related documentation: 7 files, 701 insertions, 47 deletions. It contains none of #2049's duplicated implementation commits.Selection and fallback contract
Resolvedconcrete native shapes are used directly.SymbolicOrUnknownfalls back because the ORT plugin must allocate before kernel execution.Rejectedalso falls back in this first slice, preserving historical plugin permissiveness.Tests now discriminate:
Expand@7=>SymbolicOrUnknown, then compatibility fallback;Expand@8=>ResolvedExpand=> notSharedNativeExpandshape operand => symbolic native result, then a safe plugin error without dereferenceTilerepeats => nativeRejected, while the pre-existing permissive plugin fallback still returns[6, 6]SharedNative, preventing recursionThe adapter documentation explicitly records that production receives ORT
Node_GetSinceVersion(the selected schema/kernelsince_version), not necessarily the graph-level opset. Future version-sensitive migrations must account for that distinction.Anti-vacuity and edge coverage
The shared-rule sweep asserts the exact independent census
ConstantOfShape, Expand, Tile, then asserts the number compared. The shape-preserving sweep can no longer skip rejected/unresolved cases and assertscompared == CASES.len().Added agreement fixtures for:
Expandtarget extent0Tilerepeat0ConstantOfShapeinput => scalarDependency finding
Empirical
cargo tree -p onnx-runtime-ep-plugin -e normalfindings:onnx-runtime-shape-inferenceis a thin direct dependency (onnx-runtime-ir+thiserror)prostwas already in the shipped plugin graph throughonnx-runtime-ep-api -> onnx-runtime-loader/ tracer before this PRTherefore no shared-rules crate split is justified for this slice: protobuf is pre-existing, not introduced by the adapter. The remaining binary-size risk is compiled shape-registry code itself, noted below.
Validation
All commands were run after restoring every mutation:
cargo test --manifest-path C:\Users\justinchu\dev\ng-shape-dry\Cargo.toml --locked --no-fail-fast -p onnx-runtime-ep-plugin— 356 passed, 3 ignoredcargo test --manifest-path C:\Users\justinchu\dev\ng-shape-dry\Cargo.toml --locked --no-fail-fast -p onnx-runtime-ep-cpu-plugin— 101 passed, 1 ignoredcargo test --manifest-path C:\Users\justinchu\dev\ng-shape-dry\Cargo.toml --locked --no-fail-fast -p onnx-runtime-shape-inference— 343 passedcargo test --manifest-path C:\Users\justinchu\dev\ng-shape-dry\Cargo.toml --locked -p onnx-runtime-session warm_decode_seeding_admits_previously_unresolved_capture_safe_node -- --nocapture— 1 passed; this is the genuine runtime JIT-sizing proofcargo clippy --manifest-path C:\Users\justinchu\dev\ng-shape-dry\Cargo.toml --locked -p onnx-runtime-ep-plugin -p onnx-runtime-ep-cpu-plugin -p onnx-runtime-shape-inference -p onnx-runtime-session --all-targets -- -D warnings— passedMutation matrix
TilefromSharedNativeShapeRule::ALLALLempty[]vs expected threeExpandreturn the target[1, 4]vs native[3, 4]Rejectedinto a hard errorTilerejection instead of returning[6, 6]Each mutation exited 101 and was reverted; the clean suites above then passed.
Remaining risks / intentionally deferred
Node_GetSinceVersionis not guaranteed to equal graph opset; this is documented and pinned, but every future version-sensitive migration needs its own selection fixture.