Repository navigation
fix(ci): clear the remaining Rust 1.98.0 clippy failures (qmoe, ep-cpu) - #1609
Merged
Merged
Conversation
#1604 repaired most of the 1.98.0 fallout but could not see two things: - `onnx-runtime-ep-cuda/src/kernels/qmoe.rs` trips `collapsible_if` at five sites. This is the only thing keeping `main` red outside Windows: it fails `Clippy CUDA execution provider` in both `CUDA compile (Linux x86_64)` and `CUDA compile (Windows x86_64)`. The block came in with #1602, which merged before the 1.98.0 bump was understood. Collapsed into `let` chains (edition 2024), which is exactly clippy's own suggestion. - Three `onnx-runtime-ep-cpu` lint sites sit behind `cfg(all(target_arch = "aarch64", any(target_os = "macos", ...)))`, so the Linux quality gate never compiles them and no CI job lints macOS at all. They are latent rather than currently-red, but they break `cargo clippy` for anyone developing on Apple silicon: * `sdpa.rs` x6 — `unusual_byte_groupings` on RNG seeds * `accelerate_gemm.rs:943` — `manual_range_contains` * `matmul.rs:3424` — `collapsible_if` The six `sdpa.rs` literals are regrouped only; their values are asserted numerically identical (0xDEC0DE_A == 0xDEC_0DEA == 233573866, and so on), not eyeballed, so the deterministic test seeds are unchanged. Verified with rustc/clippy 1.98.0 (88d9e12ae 2026-08-18), the same build CI reports, each with a negative control confirming the check fails before the change and passes after: - `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings` reproduces CI's five errors at identical line:col, and is clean after. - the full offline-crate clippy gate from ci.yml, verbatim, `--all-targets` `-- -D warnings`: clean. - `cargo test -p onnx-runtime-ep-cpu`: 1525 passed, 0 failed. The thin-M NEON path changed in `matmul.rs` is aarch64/macOS, so it is actually exercised. - `cargo fmt --all -- --check`: clean. Refs #1600. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Reviewed. I re-derived the claims I could check independently rather than reading them, and they hold. Two of them correct me, which is the part worth recording. Independently verifiedThe six seed values are unchanged. I did not take the table on trust — I extracted every hex literal from both sides of the diff, stripped separators, and parsed base 16: Multisets identical. Since these feed The The cfg-gating claim. This is the one I most wanted to disprove, and could not. Where that corrects me, and why the correction matters more than the fixIn #1600 I attributed the The reproduction was real. The inference from it was wrong. I reproduced them on macOS, because that is the machine I have — so what I actually established was "pre-existing on main", not "this is what makes the Linux job red". Those are different claims, and my host's cfg profile silently supplied the difference. Reproducing on a clean tree controls for my branch; it does not control for my platform. I had no business calling that evidence strong for a Linux job. Your finding that Second correction: I had told you
ScopeNo conflict with #1579. It touches none of these four files, and I confirmed that mechanically rather than by inspection ( Your three "could not verify" items are stated at the right strength, particularly declining to claim the QMoE kernel still runs. A nesting collapse with no reordering is a strong argument, and you correctly labelled it an argument. The thing still not donePinning the toolchain. This is now the third PR in two days spent on symptoms of one unpinned toolchain, and the ledger of misattributions above — mine, twice — is the real cost: nobody could tell platform-gated from toolchain-gated from branch-introduced, because the toolchain was a free variable in every comparison. Land this, then pin it. |
This was referenced Aug 20, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1609 +/- ##
==========================================
+ Coverage 80.79% 80.94% +0.14%
==========================================
Files 380 382 +2
Lines 175073 178555 +3482
Branches 175073 178555 +3482
==========================================
+ Hits 141450 144528 +3078
- Misses 28727 29088 +361
- Partials 4896 4939 +43
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 20, 2026
…hosts cannot take
`Test cross-platform offline crates` fails on
`half_prefill_gebp_agrees_with_the_blocked_half_gemm_and_is_the_route`:
assertion `left == right` failed: BFloat16 m=2: prefill did not take the
fused widen-pack GEBP
left: 0
right: 1
The library is right and the test is wrong. `half_gemm_tile` tries the native
AVX-512 BF16 kernel *before* the fused widen-pack GEBP:
if format == HalfFormat::Bf16 && x86_bf16::native_available() { ...; return }
if half_prefill_gebp_selected(..) && half_prefill_gebp_enabled() { ... }
`native_available()` is a runtime `is_x86_feature_detected!("avx512bf16")`
probe, so on a host that has AVX-512 BF16 a bf16 tile legitimately never
reaches the GEBP and the counter stays 0. The test carved out the analogous
MLAS/f16 case but not this one.
That explains the shape of the failure, which otherwise looks like a
regression from whatever merged most recently: it tracks the runner's CPU, not
the commit. It passed on `eb8ce595` and failed on `a429538f` on Linux while
also failing on Windows x86_64 — different hardware, same tree. Nothing in
#1608 (CUDA VMM) could have touched a CPU bf16 GEBP route.
The precedence is already documented and already encoded for the decode side
in `half_decode_prefers_gebp_when`, which declines bf16 exactly when
`native_available()`. Only the prefill guardrail was missing it.
Rather than switch the assertion off on AVX-512 hosts, count both arms and
assert exactly one ran. A bare "GEBP was not taken" skip would stop catching a
silent fall-through to the portable blocked half GEMM on precisely the newest
hardware; the disjunction keeps the dispatch guarded on every CPU. The numeric
agreement check against `naive_matmul` runs either way, as before.
Verification, and its limits:
- `cargo fmt --all -- --check` clean, and `cargo clippy -p onnx-runtime-ep-cpu
--all-targets` reports only the pre-existing macOS lints fixed in #1609 —
none from this change.
- **The changed code is not compiled locally at all.** It is entirely
`#[cfg(target_arch = "x86_64")]` and this host is aarch64-apple-darwin. I
tried both x86_64 targets: `x86_64-apple-darwin` fails because upstream
publishes no `onnxruntime-osx-x86_64-1.28.0` asset, and
`x86_64-unknown-linux-gnu` fails in ort-sys bindgen with `'stdlib.h' file not
found` — the same environmental blocker #1604 hit on
`aarch64-pc-windows-msvc`. `onnx-genai-ort-sys` is a normal dependency of
`onnx-runtime-ep-cpu` via `onnx-runtime-ep-api`, so there is no `--lib`-only
escape. CI is the oracle for this one.
- Even a green CI run only proves the non-AVX-512 arm unless the runner
happens to have `avx512bf16`. The `(0, 1)` branch may go unexercised.
Refs #1600.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 20, 2026
…1381) (#1613) ## The finding: the in-tree gate was the bug, not the missing one `MatMul` sent every `m == 1` `f16`/`bf16` decode with `k * n >= HALF_PREFILL_GEBP_MIN_WEIGHT` (1 048 576) to the fused widen-pack GEBP. `Gemm` never did — its `m == 1` `f16` path stayed on the decode GEMV at every weight. That divergence is #1381, and the obvious reading was that `Gemm` was missing a gate. I implemented that reading first, benchmarked it, and it reversed: at the model shapes the GEBP was 19–26% **slower**. Escalating to the authoritative production harness showed why — it is `MatMul` that was carrying a bad gate. **This PR retires the handover.** `#1381` closes with `MatMul` adopting `Gemm`'s route rather than the reverse. ## Evidence All numbers through `crates/onnx-runtime-ep-cpu/benches/half_decode_gemv_ab.rs`, which drives `ExecutionProvider::get_kernel` + `Kernel::execute` (dispatch and output narrowing included). Arms are selected by `ONNX_GENAI_CPU_MM_HALF_GEMV`/`_GEBP`, which are process-wide `OnceLock`s, so one arm per process. Every ratio is divided by an `f32` control measured in the same two processes, because this host is heavily shared. The original gate's evidence was **32 threads and `k <= 2048` only**. - **8 threads — a loss at every `full`-set shape at or above the gate**, `/ctl` 0.26–0.76 (1.3x–3.8x), and in **20 of the 22 `f16` cells** of the threshold-neighbourhood `band` set over **two independent repetitions**, `/ctl` 0.20–1.14 (up to 5.0x). The GEBP's weight bandwidth is pinned at 20–34 GB/s independent of shape; the GEMV reaches 24–155 GB/s. - Reported against myself: the band re-run **does not reproduce** an earlier "no row above 1.00" claim. `w1.05M_k1024` — *exactly* at the retired threshold, at `k = 1024` — measured 1.14 then 0.89, and five `bf16` cells reach 1.00+ with their `f32` controls drifted to 0.55–0.94, which makes those cells unusable rather than favourable. Both are tabulated in the doc rather than dropped. The earlier 6.7x figure is likewise not reproducible and has been walked back to the supported 5.0x everywhere it was cited. - **32 threads — still a loss at every shape a 7B-class decode issues.** `k = 4096` qkv/mlp `/ctl` 0.51–0.89; a 136M `lm_head` 0.86. (The original table reported 1.18/1.37 for the last two; re-run they are 0.88/0.86.) `bf16` tracks `f16` at 0.41–0.88. - **The one corner it wins** — `k = 1024`, 1.05M–4.2M, 32 threads, `/ctl` 0.95–1.49 — is not a shape any such model emits, and its `k = 2048` neighbours are not reproducible run to run (6.3M measured 1.31 then 0.70 from the same binary). ## Why there was nothing to retune The GEBP earns its packing by reusing a `KC x NR` panel of `B` across the rows of `A` (`MR = 6, NR = 16, KC = 256`). At `m == 1`, `m_panels = 1` — every panel is consumed by exactly one microkernel pass, so **none** of the packing is repaid. Where the cost actually lands: the widen-pack writes `k*n` `f32` and reads it back, but per strip that is a `bpack` of at most `KC * 16 * NR * 4 B` = 256 KiB, i.e. **L2-resident** on this host — so that traffic is not a DRAM story. What does reach DRAM is **line amplification**: `pack_b_half` walks `B` column-strip-major and at the usual `panels_per_strip = 1` touches `NR * 2` = 32 bytes of every 64-byte line, ~2x the GEMV's read traffic. The rest is L2 bandwidth, the widening compute, and a fork/join over strips that one row of `A` cannot amortise. Both axes follow: pack work per unit of reuse rises with `k`, and it can only be overlapped if there are workers to overlap it with — which is why the arm collapses at 8 threads and merely loses at 32. A decode-specialised GEBP that skips packing `B` **is** the GEMV. The finding is structural. ## Changes - `matmul.rs`: dropped `&& !half_decode_prefers_gebp(..)` from the decode arm; deleted `half_decode_prefers_gebp` and `half_decode_prefers_gebp_when` (94 lines, incl. the superseded evidence table). - `gemm.rs`: **behaviourally unchanged** — it was right. Adds a test-only route counter and the comment recording #1381's resolution. - **Deliberately untouched:** `HALF_PREFILL_GEBP_MIN_WEIGHT` / `half_prefill_gebp_selected`. They are the *prefill* gate and they still serve a **batched** `m == 1` half MatMul, which the non-batched GEMV declines and whose only alternative is the row-blocked half GEMM at 16x–21x. Pinned by `a_batched_single_row_half_matmul_still_reaches_the_gebp`. - **Coverage repaired in the same change:** new `PROBE_SHAPE=band` puts 11 rows immediately below, at and above the retired threshold at three different `k`, so a `k`-dependent effect cannot hide inside a `k * n` gate again. Pins are now by **execution** rather than by asking a predicate — the predicate is what was deleted. - Docs: new `docs/benchmarks/2026-08-21-half-decode-gebp-retired.md`, new §14 in `docs/performance/CPU_MATMUL_ASSIGNMENT.md`, and a superseded banner on the 2026-08-19 bf16 record whose second finding this reverses. ## Review Reviewed twice by `claude-opus-4.8`, both times with an explicit instruction to argue the opposing case (that retiring the divert is wrong, or that a narrower gate should have been kept). It did, and concluded the case does not survive: a `k * n`-only gate — the only kind the retired predicate could express — cannot see thread count, and reinstating it to capture the `k = 1024` corner net-loses on the `k = 4096` and `lm_head` shapes that models actually issue. All five defects from the first pass are fixed in this head; the second pass found one further Low (a loss range that no longer matched its own table after rescoping), which is fixed, and prompted the band re-run above. ## Mutation test Re-inserting the divert (`&& geom.k * geom.n < 1_048_576` on the decode arm) fails exactly three tests — `no_half_decode_is_diverted_off_the_gemv_on_weight`, `f16_decode_at_the_retired_weight_gate_keeps_the_gemv`, `gemm_and_matmul_take_the_same_decode_route` — so the pins are not vacuous. ## Validation (local, on this head merged with `origin/main` 78058a4) `PASS=19 FAIL=1` over the full matrix: `fmt --all`, offline build, `-p onnx-runtime-ep-cpu --lib` (**1552 passed / 0 failed / 21 ignored**), clippy `-D warnings`, `--features mlas`, native-be clippy, `aarch64-unknown-linux-gnu` clippy, `--no-default-features`, `--all-features`, **no-MLAS default-artifact symbol scan**, `cross-compile.sh`, and all eight `scripts/` conformance checks (dispatch reachability, dispatch manifest, feature-gate coverage, documented env vars, publish order, profile table, platform naming, workspace test packages). The one failure is step **H** `aarch64-pc-windows-msvc`, which is environmental and pre-existing: `ort-sys`' bindgen cannot find `stdlib.h` without the Windows SDK headers. No code path in this diff is Windows-specific. Additionally, because the repo's quality lane is wider than that step's package list, I ran it separately: clippy `-D warnings --all-targets` over the full 31-package quality set — **green** — and `-p onnx-genai-cli` — **green**. > Out-of-scope observation: `-p onnx-runtime-ep-cuda --features cuda` clippy is **red on `main` > today** (`unusual_byte_groupings` in `device_argmax.rs`/`matmul_nbits.rs`, `type_complexity` in > `gqa_shared_prefix_parity_gpu.rs`, a manual `div_ceil`). Pre-existing 1.98.0 fallout in the lane > #1609 did not cover; this diff touches no CUDA file. Miri is **not** meaningful coverage for this EP's SIMD and this diff adds no `unsafe` — reporting it as *no coverage*, not as a pass. ## Remaining losses - The `k = 1024` / 32-thread corner gives up 1.1x–1.5x. Recovering it needs a thread-count- **and** `k`-dependent predicate; not attempted, because the measurement that would justify it is the least reproducible one on this host — and §12 already records a per-op result that inverted under the real pool. - The GEMV itself is not at the roofline: 24–155 GB/s against a 75.8 GB/s DRAM ceiling puts the large shapes at ~77% and leaves the small ones latency-bound. Separate line of work. - `Gemm`'s half fast path is still **`f16`-only**: `bf16` operands return `None` at the dtype check and fall into the portable blocked half GEMM, where `MatMul` serves them from the same GEMV. A distinct, separately mergeable gap — documented as open, not addressed here. Closes #1381. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 20, 2026
…hosts cannot take (#1610) Fixes the `Rust coverage` failure in `kernels::matmul::tests::half_prefill_gebp_agrees_with_the_blocked_half_gemm_and_is_the_route`. ## What the failure was ``` matmul.rs:5434: assertion `left == right` failed: BFloat16 m=2: prefill did not take the fused widen-pack GEBP left: 0 right: 1 ``` **This is a latent test defect, not a regression, and it is not attributable to any commit.** A naive bisect says otherwise — `Rust coverage (Linux x86_64)` is green at `eb8ce595` (#1604) and red at `a429538f` (#1608) — but #1608 changes exactly one file, `crates/onnx-runtime-ep-cuda/src/provider.rs`, and that coverage job's package list does not contain `-p onnx-runtime-ep-cuda` at all. The commit's only changed file is not compiled by the job that went red. The red tracks **which runner picked up the job**, not the code. `half_gemm_tile` tries the native AVX-512 BF16 kernel *before* the GEBP and returns: ```rust if format == HalfFormat::Bf16 && x86_bf16::native_available() { x86_bf16::gemm(a, b, c, m, k, n); return; // never reaches the GEBP route } if half_prefill_gebp_selected(format, m, k, n) && half_prefill_gebp_enabled() { ... } ``` On a host with `avx512bf16` (Cooper Lake / Sapphire Rapids and later) a bf16 tile legitimately never reaches the GEBP and the counter stays 0. GitHub's Linux x64 pool is a mix of Intel generations, so the same commit passes or fails depending on the runner. The precedence is already encoded for decode in `half_decode_prefers_gebp_when`; only prefill's guardrail lacked it. ## A second, latent hardware assumption in the same test The test already had `if !crate::backend::has_simd_x86() { continue; }`. That is a **`continue`**, so on a non-AVX2/FMA host it skipped the numeric comparison against `naive_matmul` as well as the route assertion — deleting the half of the test that is valuable on every host. It has never fired (all current runners have AVX2), so it was latent, but it is the same class of bug. ## What changed The expectation is now **derived from the predicates `half_gemm_tile` actually dispatches on**, rather than restating hardware conditions in the test: ```rust let expect_native_bf16 = format == HalfFormat::Bf16 && x86_bf16::native_available(); let expect_gebp = !expect_native_bf16 && half_prefill_gebp_selected(format, m, k, n) && half_prefill_gebp_enabled(); assert_eq!( (half_prefill_gebp_calls(), half_native_bf16_calls()), (u64::from(expect_gebp), u64::from(expect_native_bf16)), ...); ``` - A test-only `HALF_NATIVE_BF16_CALLS` counter on the native bf16 arm distinguishes "took the *other* fast route" from "silently fell through to the portable blocked half GEMM" — which is what the guardrail is for. Without it, the only way to tolerate the bf16 case is to stop asserting anything. - `(0, 0)` on a no-SIMD host is now an asserted-correct outcome instead of a skip. - **The numeric comparison against `naive_matmul` now runs unconditionally on every host.** - Because the expectation reuses the real predicates, it cannot drift from the dispatch order the way a transcribed condition would. ## How I verified it Transcribed `half_gemm_tile`'s dispatch order and the new expectation into a standalone program and checked them against each other across **all 16 combinations** of `native_available` × `has_simd_x86` × `half_prefill_gebp_enabled` × dtype. They agree on all 16. ## What I could not verify **I have not reproduced the original failure, and I have not run this test.** Stating that plainly rather than implying otherwise. This host is aarch64 macOS; the test is `#[cfg(target_arch = "x86_64")]` and `x86_bf16::native_available()` does not exist here. Cross-compiling is blocked in both directions: `x86_64-apple-darwin` because upstream publishes no `onnxruntime-osx-x86_64-1.28.0.tgz` (HTTP 404), and `x86_64-unknown-linux-gnu` because ort-sys bindgen fails on `'stdlib.h' file not found`. `onnx-genai-ort-sys` is a normal dependency via `onnx-runtime-ep-api`, so `--lib` does not avoid it. No Docker on this machine. So the 16-configuration check verifies the *logic* is consistent with the dispatch order. It does **not** verify that the code compiles under `target_arch = "x86_64"`, nor that the counter increments where I placed it. CI is the oracle for both. **A green run on this PR will not prove the fix.** It only exercises the `(1, 0)` branch unless the runner happens to have `avx512bf16`. The `(0, 1)` branch — the one that was actually failing — is reachable only on Cooper Lake or newer, and a runner cannot be requested. Corroborating evidence for the mechanism, though: #1609's run went green on `Rust coverage (Linux x86_64)` *and* `Rust coverage (Windows x86_64)` with the **old** assertion still in place, which is exactly what the CPU-dependence diagnosis predicts and is not evidence that the old test was fine. ## Notes - Touches only `crates/onnx-runtime-ep-cpu/src/kernels/matmul.rs`. No overlap with #1579's file set; nothing in `crates/onnx-genai-engine/src/{decode,native_decode}/`. - Based on `a429538f`, which predates #1609, so this branch inherits `main`'s `CUDA compile (Linux x86_64)` red at `Clippy CUDA execution provider`. That failure is **not** from this change — #1609 fixes it. Refs #1600. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 20, 2026
fix(ci): collapse the NVRTC cache lookup's nested `if let` `main` is red on both `CUDA compile (Linux x86_64)` and `CUDA compile (Windows x86_64)` at `81855858`. This is the third independent casualty of the Rust 1.98.0 clippy bump, after #1604 and #1609. Refs #1600. ## What is failing ``` error: this `if` statement can be collapsed --> crates/onnx-runtime-ep-cuda/src/runtime.rs:942:9 = note: `-D clippy::collapsible-if` implied by `-D warnings` error: could not compile `onnx-runtime-ep-cuda` (lib) due to 1 previous error ``` `clippy::collapsible_if` fires across `if let` chains in 1.98.0. The block it lands on is the PTX disk-cache lookup added by #1612, which merged before this lint's reach was understood -- the same way #1602's `float_widen_entry` block became #1609's problem. Nothing here is a regression from #1612's logic; the code is correct, the lint is new. I did not infer the location. I pulled the job log from run [32421805546](https://github.com/justinchuby/onnx-genai/actions/runs/32421805546) and read it. ## The fix Collapse to a let-chain, matching the form #1609 adopted in `qmoe.rs`. A let-chain is semantically identical to the nested form -- same short-circuit, same bindings, same scope -- so cache behaviour is unchanged. `kernel_cache::load` returning `None`, or the bytes failing UTF-8, still both fall through to a real NVRTC compile. Only one occurrence exists. The other `kernel_cache::load` call site in this file (line 973, the cubin path) is a `match`, not a nested `if let`, and does not trip the lint. ## Verified on this macOS host - `cargo clippy -p onnx-runtime-ep-cuda --features cuda --lib -- -D warnings` is clean with this change. - **Reverse control, which is the part that matters:** I reverted the file to `origin/main` and re-ran the same command. It fails with the exact CI error at the exact line -- `error: this if statement can be collapsed --> crates/onnx-runtime-ep-cuda/src/runtime.rs:942:9` -- then passes again once the change is restored. So this reproduces the CI failure locally and demonstrably clears it, rather than merely compiling. - `cargo fmt --all -- --check` clean. ## Not verified No CUDA hardware here, so nothing was executed -- but this job is a compile/lint gate, and the gate itself is what I reproduced. The runtime path is untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
This was referenced Aug 21, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 21, 2026
Closes the root cause behind the 1.98.0 CI incidents tracked in #1600. ## What the failure was Every blocking gate in `ci.yml` runs under `-D warnings` — either as `RUSTFLAGS` or as a trailing `-- -D warnings` on clippy. CI resolved `stable` at job time, so when runner images rolled forward to `rustc 1.98.0 (88d9e12ae 2026-08-18)`, every lint that release made warn-by-default became an instant failure on code that was clean the day before. No commit to blame, and a contributor on 1.97.0 could not reproduce it — clippy 0.1.97 does not carry those lints at all, so it does not even print them. Four consecutive incidents, all the same cause: | PR | Lint | Code from | |---|---|---| | #1604 | `cargo fmt` drift, `manual_slice_fill`, `needless_late_init` | assorted | | #1609 | `collapsible_if` ×5 in `qmoe.rs` + ep-cpu lints | #1602 | | #1615 | `collapsible_if` in `runtime.rs` | #1612 | | #1603 | `chunks_exact_to_as_chunks`, 12 crates | assorted | Each fixed symptoms; none could prevent the next. Formatting is the self-perpetuating one — an unpinned rustfmt means whoever formats next re-flows the file back the other way, so the tree oscillates. ## How I reproduced it I read the version out of a failing job log rather than guessing, then installed it: ``` rustup toolchain install 1.98.0 # -> rustc 1.98.0 (88d9e12ae 2026-08-18), byte-identical build hash ``` That is what turned "unreproducible locally" into reproducible, and it is what made #1609 and #1603 diagnosable at all. ## What I changed **`rust-toolchain.toml`** (new) — `channel = "1.98.0"`, `profile = "minimal"`, `components = ["clippy", "rustfmt", "llvm-tools-preview"]`. No `targets` key: `Rust (Windows ARM64)` runs natively on `windows-11-arm` and needs no cross target, and `scripts/check_cross_compile.sh` adds its own. **`ci.yml`** — the eight setup steps collapse from `rustup toolchain install stable --profile minimal --component …` + `rustup default stable` to a bare `rustup toolchain install`, which reads the file and installs the declared components. The `version=` cache-key output is unchanged. Policy comment rewritten. **`ci.yml`, `changes` job** — separate bug, fixed while here. It was gated on `github.event_name == 'pull_request' || 'push'`, with a comment claiming schedule and `workflow_dispatch` runs would skip only that job and still get full CI. **They did not.** Every job below is `needs: changes` with a plain `if:` (no `always()`/`!cancelled()`), and GitHub skips a job whose `needs` dependency was skipped regardless of its own `if:`. So the gate skipped the *entire workflow*: the nightly cron and every manual dispatch reported success without running a single check — a green with no verdict behind it, which is the same class of problem this PR is about. The classify step already leaves `docs_only=false` for any event it does not diff, so removing the gate makes the documented behaviour real. ## How I verified it All local, on this branch, with my rustup default left at 1.97.0 so the file is doing the work: **Negative control — the pin is what changes the resolution:** ``` without rust-toolchain.toml: rustc 1.97.0 clippy 0.1.97 rustfmt 1.9.0 (2d8144b788) with rust-toolchain.toml: rustc 1.98.0 clippy 0.1.98 rustfmt 1.9.0 (88d9e12ae1) ``` clippy `0.1.97` → `0.1.98` is precisely the gap that made these failures invisible to contributors. **Both gates, run with bare commands** (no `+1.98.0`), which is the point — a 1.97.0 contributor now gets CI's behaviour by default: - `cargo fmt --all -- --check` → clean - the verbatim 30-package clippy gate from `ci.yml` ~326, including the trailing `-- -D warnings` → **exit 0** **Component auto-install:** `llvm-tools` was absent from my 1.98.0 install and rustup fetched it on first use inside the repo, so dropping the per-job `--component` flags is safe. Confirmed end-to-end with `cargo llvm-cov --locked -p onnx-runtime-cpuinfo` → exit 0, which is the one job whose component requirement changed. **rustup override semantics, empirically checked** (I had this wrong earlier and said so on #1600): `rustup component add` and `rustup target add` **respect the directory pin** — I expected them to hit the default toolchain and silently break the coverage and cross-compile jobs. They do not; both landed on 1.98.0. Explicit `+toolchain` still beats the file (`rustc +stable -Vv` → 1.97.0 inside the repo), so miri's `cargo +nightly` is unaffected. **YAML:** all workflow files parse; `changes` confirmed to have no `if:` key. ## What I could not verify - **Everything about the CI runners themselves.** I am on macOS aarch64. That a Linux or Windows runner materializes 1.98.0 from this file, and that the cache key still resolves correctly there, is only checkable in CI. This PR's own run is the oracle. - **The `workflow_dispatch`/`schedule` fix.** I verified the mechanism by reading the gating (all 8 downstream jobs are `needs: changes` with plain `if:`), but I have not observed a dispatch run execute the matrix. Worth triggering one after merge to confirm. - **Other workflows.** `audit.yml`, `publish.yml`, `publish-ep-plugins.yml`, `wheels.yml` still say `rustup default stable`. They inherit the pin anyway — any `cargo` run inside the repo resolves through the file — so their behaviour is already correct and their explicit install is merely redundant. I left them rather than widen the blast radius onto release workflows. **One exception worth flagging:** `benchmark.yml` uses `dtolnay/rust-toolchain@stable`, which sets `RUSTUP_TOOLCHAIN` — that env var *overrides* the file, so benchmark is genuinely not pinned. It is non-blocking, but it is a real gap, not an oversight. ## Related, not fixed here **No CI job runs clippy on macOS.** `rust-coverage` matrixes macOS but only *installs* clippy (~line 458) and never runs it; all six clippy-running jobs are Linux/Windows. That means #1609's macOS-only fixes in `accelerate_gemm.rs` and `matmul.rs` (both `#[cfg(target_os = "macos")]`) were structurally unverifiable by CI. I ran CI's exact clippy package list on macOS under 1.98.0 → **exit 0**, so this is a prevention gap rather than an outstanding defect. Filing separately. ## Conflict risk with #1579 None. This PR touches only `.github/workflows/ci.yml` and a new root file. No overlap with `crates/onnx-runtime-ep-cpu`, `crates/onnx-runtime-ir`, or `crates/onnx-genai-engine/src/{decode,native_decode}/`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 08760f2f-160f-41e5-828d-9d9b6045c00d
This was referenced Aug 21, 2026
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
1 participant
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.
Finishes the job #1604 started. Refs #1600.
mainis still red ata429538f. #1604 fixed the fmt drift,manual_slice_fill,needless_late_init, and centrally allowedchunks_exact_to_as_chunks, which turnedRust quality,Fast (Linux),CLI ORT (Linux)and all three coverage jobs green. Two things it could not have seen are left.What was still failing
Clippy CUDA execution provider— the only non-Windows red left onmain.Not
chunks_exactand notapprox_constant, both of which were guesses on record in #1600. I pulled the job log rather than infer. Run 32406806834 (main @eb8ce595) and run 32406772158 (#1604's own branch) agree exactly:clippy::collapsible_ifnow fires acrossif letchains in 1.98.0. Thefloat_widen_entryblock it lands on arrived with #1602, which merged before the 1.98.0 bump was understood — so this is a second, independent 1.98.0 casualty, not a regression from #1604.Fixed by collapsing into
letchains, which is clippy's own suggested form and is available on edition 2024.1804and1805are one three-deep nest reported twice; it becomes a single chain.Three
onnx-runtime-ep-cpusites that no CI job can currently see.#1600 attributed these to
Fast (Linux)andRust quality. That attribution is wrong, and the correction matters for the ledger:sdpa.rs×6unusual_byte_groupingsare inside#[cfg(all(target_arch = "aarch64", any(target_os = "macos", target_os = "ios")))]accelerate_gemm.rs:943manual_range_containsis in the Accelerate-framework modulematmul.rs:3424collapsible_ifis inside#[cfg(any(target_os = "macos", target_os = "ios"))]The Linux gate never compiles any of them, and
ci.ymlruns no clippy on macOS at all —Rust coverage (macOS arm64)setsRUSTFLAGS: -D warnings, which catches rustc warnings but not clippy lints. So these are latent, not currently-red. They still breakcargo clippyfor anyone developing on Apple silicon, which is how they surfaced. I've fixed them here; the underlying gap is noted in #1600 rather than papered over.How I verified
I could reproduce all of this locally, including the CUDA gate that #1600 expected to need CI as its only oracle. The unlock: CI is on
rustc 1.98.0 (88d9e12ae 2026-08-18)— readable from the failing job log — andrustup install 1.98.0yields that exact build.cargo clippy -p onnx-runtime-ep-cuda --features cudaneeds no CUDA toolkit to run.Every check below is a negative control: confirmed failing before the change and passing after.
cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings(CI's exact command)sdpa.rslint set,-p onnx-runtime-ep-cpu --all-targetsci.yml's full offline-crate gate, package list verbatim,--all-targets -- -D warningsaccelerate_gemm,matmul)cargo test -p onnx-runtime-ep-cpucargo fmt --all -- --checkThe hex regrouping is asserted numerically, not by eye — I compiled both spellings and compared:
Those are seeds for
deterministic_values, so an unchanged value is what keeps the expected test outputs valid. The 1525-test run is the confirmation. Thematmul.rsthin-M NEON path I collapsed is aarch64/macOS, so it is genuinely exercised on this machine rather than merely compiled.What I could not verify
CUDA compile (Windows x86_64)fails on the identical fiveqmoe.rserrors as Linux, so the same fix should clear it, but I am asserting that from log equality, not from a Windows run.--features cudatype-checks the crate; it does not prove the QMoE kernel still runs. The change is a pureif-nesting collapse with no reordering of conditions and no change to short-circuit behaviour, but that is an argument, not a measurement.Test cross-platform offline cratesonRust (Windows ARM64)/Rust coverage (Windows x86_64). Deliberately kept separate per CI on main is red across 8 jobs, and has been since 08-03 — attribution table + reproductions #1600's ordering.On pinning — deliberately not done here
#1600 calls the missing pin the root cause. Before changing it I found this in
ci.yml, above every one of the 17rustupinstall sites:So the unpinned toolchain is an explicit, reasoned decision, not an oversight — and reversing a documented policy does not belong inside an unrelated repair PR. There are also two concrete traps that make a bare
rust-toolchain.tomlunsafe:rustup component add llvm-tools-previewand anyrustup target addapply to the default toolchain, not to a directory-pinned one, so a naive pin would quietly break the two coverage jobs and theaarch64-pc-windows-msvccross-compile. (Miri is safe — it invokescargo +nightlyexplicitly.)Worth doing, worth doing on purpose, and worth its own PR with that policy comment updated to match. Raised in #1600 with the tradeoff spelled out.
Conflict risk
Flagging for #1579 (the #1186 memory stack): this touches
crates/onnx-runtime-ep-cpu/src/kernels/{sdpa,accelerate_gemm,matmul}.rsandcrates/onnx-runtime-ep-cuda/src/kernels/qmoe.rs. All four hunks are small and local — six literals, one boolean, twoif-nesting collapses — so textual conflicts should be trivial to resolve. Nothing incrates/onnx-genai-engine/src/{decode,native_decode}/is touched.