Repository navigation
fix(ci): repair main for stable 1.98.0 — fmt, two new lints, one central allow - #1604
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1604 +/- ##
==========================================
+ Coverage 80.94% 81.30% +0.36%
==========================================
Files 382 380 -2
Lines 178557 175504 -3053
Branches 178557 175504 -3053
==========================================
- Hits 144531 142692 -1839
+ Misses 29087 27888 -1199
+ Partials 4939 4924 -15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This was referenced Aug 20, 2026
…ral allow `main` does not pass its own gates on a current stable toolchain. Three independent causes, all found by running the CI matrix locally: 1. #1598 landed unformatted. `cargo fmt --all --check` reports six diffs across `decode/mod.rs`, `decode/state.rs` and `native_decode/{cuda,mod,tests}.rs`. Fixed by running `cargo fmt --all`; no logic change. 2. rustc/clippy 1.98.0 (2026-08-18) turned on `clippy::manual_slice_fill` and `clippy::needless_late_init` findings that this tree trips: - `native_decode/cuda.rs` (two `for row_len in &mut self.row_lens` loops from #1598) now use `self.row_lens.fill(..)`. Note clippy's own suggestion text (`&mut self.row_lens.fill(..)`) does not compile; the `&mut` is spurious. - `kernels/moe.rs`'s SwiGLU split rewrites the deferred `let linear_part; let gate_part;` pair as a single `let (gate_part, linear_part) = if ..`, which is what the lint asks for and reads better besides. Both are behaviour-preserving. 3. The same release added `clippy::chunks_exact_to_as_chunks`, warn-by-default. This repo pins no toolchain, so every `-D warnings` gate (Linux offline, quality lane, native-backend, aarch64, `check_cross_compile.sh`) goes red as soon as a runner picks up the new stable — which this host already has. That lint fires at 201 call sites across 12 crates, so it is not a local defect, and it is not uniformly auto-fixable: in `onnx-runtime-ir::read_vec_le` the chunk size is `T::BYTE_SIZE`, an associated const of a type parameter, and the suggested `as_chunks::<T::BYTE_SIZE>()` fails to compile — "generic parameters may not be used in const operations" (checked against rustc 1.98.0 directly rather than assumed). So it is allowed in the existing `[workspace.lints.clippy]` table that all 52 crates already opt into, keeping a toolchain bump from gating unrelated work. Migrating the sites that genuinely benefit — `as_chunks` yields `&[T; N]`, dropping fallible `try_into` and helping codegen — belongs with each crate's owner, not in one 201-site emergency rewrite. Cases 2's two lints are fixed rather than allowed precisely because they are small and local. Verified locally: fmt; offline build; ep-cpu tests (default, `--no-default-features`, `--features mlas`, `--all-features`); clippy `-D warnings` over the Linux-offline set, the full quality-lane set plus `onnx-genai-cli`, `onnx-genai-engine --features native-backend`, `onnx-runtime-ep-cuda --features cuda`, and `--target aarch64-unknown-linux-gnu`; the MLAS-free artifact gate; `check_cross_compile.sh`; and all eight repo lint scripts. 19/20 — the sole failure is `--target aarch64-pc-windows-msvc`, which needs Windows SDK headers this container lacks (`fatal error: 'stdlib.h' file not found`) and fails identically on unmodified `main`. Pre-existing and deliberately not widened into this repair: `clippy::approx_constant` denies in `onnx-runtime-ep-cuda`'s lib *tests* (`optimizer.rs:4726`). CI runs that crate's clippy without `--all-targets`, so no gate sees it today. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
force-pushed
the
squad/roy-main-fmt-clippy-repair
branch
from
August 20, 2026 19:05
1aa3513 to
a609163
Compare
justinchuby
marked this pull request as ready for review
August 20, 2026 19:05
justinchuby
added a commit
that referenced
this pull request
Aug 20, 2026
Brings in #1604 (CI repair for stable 1.98.0) and #1608 (VMM reservation ladder under managed no-spill). One content conflict: crates/onnx-runtime-ep-cuda/src/provider.rs. Resolved to this branch's side, because the defect #1608 fixes cannot occur on this branch. #1608 fixed a loop that resolved every ladder rung through `resolve_vmm_initialization(auto_dynamic_lending, ..)?`, so under managed no-spill the *first* rung's failure returned Err and the smaller rungs were never attempted. Its fix resolves each rung in fallback mode and defers the fatal verdict to after the loop. This branch removed the two-mechanism selection entirely: the arena is the only built-in mechanism, `resolve_vmm_initialization` does not exist here (0 occurrences), and the loop calls `build_arena` directly, recording `Err` into `last_error` and continuing. The ladder therefore always descends fully, in every configuration, and only exhausting it is fatal. #1608's intent is satisfied unconditionally rather than conditionally. The two sides differ in one deliberate way, which is not a merge loss: when the ladder is exhausted and no-spill was not requested, main warns and falls back to the eager allocator, whereas this branch fails the provider. That is the point of the stack -- there is no second mechanism to fall back to. The error path is `vmm_unavailable`, which states the support boundary and appends the requested managed limit when one was set, so the no-spill case is still reported as such. main made no other change to this file; its diff against the merge base is exactly the two hunks above. Audit, mechanical rather than by eye: - every file main changed outside the overlap set: blob hash compared against origin/main, all identical - the five files #1604 reformatted for 1.98.0: now byte-identical to origin/main, so this branch's earlier 1.97 formatting of the same files is superseded rather than mixed - Cargo.toml: every line main added is present in the merged result, including the central `chunks_exact_to_as_chunks` allow; this branch's three onnx-runtime-memory-api entries survive, and that crate already carries `[lints] workspace = true`, so the allow reaches it Not addressed here, on purpose: main pins no toolchain, which is the cause behind both #1604 symptoms. Tracked in #1600, not widened into this branch.
This was referenced Aug 20, 2026
Closed
justinchuby
added a commit
that referenced
this pull request
Aug 20, 2026
…u) (#1609) Finishes the job #1604 started. Refs #1600. `main` is still red at `a429538f`. #1604 fixed the fmt drift, `manual_slice_fill`, `needless_late_init`, and centrally allowed `chunks_exact_to_as_chunks`, which turned `Rust 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 on `main`.** Not `chunks_exact` and not `approx_constant`, both of which were guesses on record in #1600. I pulled the job log rather than infer. Run [32406806834](https://github.com/justinchuby/onnx-genai/actions/runs/32406806834) (main @ `eb8ce595`) and run [32406772158](https://github.com/justinchuby/onnx-genai/actions/runs/32406772158) (#1604's own branch) agree exactly: ``` error: this `if` statement can be collapsed --> crates/onnx-runtime-ep-cuda/src/kernels/qmoe.rs:1768:9 --> crates/onnx-runtime-ep-cuda/src/kernels/qmoe.rs:1780:9 --> crates/onnx-runtime-ep-cuda/src/kernels/qmoe.rs:1792:9 --> crates/onnx-runtime-ep-cuda/src/kernels/qmoe.rs:1804:9 --> crates/onnx-runtime-ep-cuda/src/kernels/qmoe.rs:1805:13 error: could not compile `onnx-runtime-ep-cuda` (lib) due to 5 previous errors ``` `clippy::collapsible_if` now fires across `if let` chains in 1.98.0. The `float_widen_entry` block 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 `let` chains, which is clippy's own suggested form and is available on edition 2024. `1804` and `1805` are one three-deep nest reported twice; it becomes a single chain. **Three `onnx-runtime-ep-cpu` sites that no CI job can currently see.** #1600 attributed these to `Fast (Linux)` and `Rust quality`. That attribution is wrong, and the correction matters for the ledger: - `sdpa.rs` ×6 `unusual_byte_groupings` are inside `#[cfg(all(target_arch = "aarch64", any(target_os = "macos", target_os = "ios")))]` - `accelerate_gemm.rs:943` `manual_range_contains` is in the Accelerate-framework module - `matmul.rs:3424` `collapsible_if` is inside `#[cfg(any(target_os = "macos", target_os = "ios"))]` The Linux gate never compiles any of them, and **`ci.yml` runs no clippy on macOS at all** — `Rust coverage (macOS arm64)` sets `RUSTFLAGS: -D warnings`, which catches rustc warnings but not clippy lints. So these are latent, not currently-red. They still break `cargo clippy` for 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 — and `rustup install 1.98.0` yields that exact build. `cargo clippy -p onnx-runtime-ep-cuda --features cuda` needs no CUDA toolkit to run. Every check below is a negative control: confirmed failing *before* the change and passing *after*. | check | before | after | |---|---|---| | `cargo clippy -p onnx-runtime-ep-cuda --features cuda -- -D warnings` (CI's exact command) | 5 errors, identical line:col to CI | clean | | `sdpa.rs` lint set, `-p onnx-runtime-ep-cpu --all-targets` | 6 errors at 2542/2543/2544/2587/2588/2589 | clean | | `ci.yml`'s full offline-crate gate, package list verbatim, `--all-targets -- -D warnings` | 2 errors (`accelerate_gemm`, `matmul`) | clean, exit 0 | | `cargo test -p onnx-runtime-ep-cpu` | — | 1525 passed, 0 failed | | `cargo fmt --all -- --check` | — | clean | The hex regrouping is asserted numerically, not by eye — I compiled both spellings and compared: ``` DEC0DE_A: old=233573866 (0xDEC0DEA) new=233573866 identical=true DEC0DE_B: old=233573867 (0xDEC0DEB) new=233573867 identical=true DEC0DE_C: old=233573868 (0xDEC0DEC) new=233573868 identical=true A1CE_0A: old=10604042 (0xA1CE0A) new=10604042 identical=true A1CE_0B: old=10604043 (0xA1CE0B) new=10604043 identical=true A1CE_0C: old=10604044 (0xA1CE0C) new=10604044 identical=true ``` 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. The `matmul.rs` thin-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 - **Nothing was run on Windows.** `CUDA compile (Windows x86_64)` fails on the identical five `qmoe.rs` errors as Linux, so the same fix should clear it, but I am asserting that from log equality, not from a Windows run. - **CUDA is lint-checked, never built or executed here.** No GPU and no CUDA toolkit. `--features cuda` type-checks the crate; it does not prove the QMoE kernel still runs. The change is a pure `if`-nesting collapse with no reordering of conditions and no change to short-circuit behaviour, but that is an argument, not a measurement. - **This does not address `Test cross-platform offline crates`** on `Rust (Windows ARM64)` / `Rust coverage (Windows x86_64)`. Deliberately kept separate per #1600's ordering. - **This does not pin the toolchain.** See below. ## 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 17 `rustup` install sites: > Keep CI on Rust stable deliberately: this follows Rust's stability promise without freezing security fixes; the cache key records the resolved rustc release. 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.toml` unsafe: `rustup component add llvm-tools-preview` and any `rustup target add` apply to the *default* toolchain, not to a directory-pinned one, so a naive pin would quietly break the two coverage jobs and the `aarch64-pc-windows-msvc` cross-compile. (Miri is safe — it invokes `cargo +nightly` explicitly.) 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}.rs` and `crates/onnx-runtime-ep-cuda/src/kernels/qmoe.rs`. All four hunks are small and local — six literals, one boolean, two `if`-nesting collapses — so textual conflicts should be trivial to resolve. Nothing in `crates/onnx-genai-engine/src/{decode,native_decode}/` is touched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
…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
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 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.
mainis red on stable 1.98.0I ran the CI matrix locally against unmodified
origin/main(3041c4c0f) before starting my own work and found it failing 6/20 steps. Three independent causes:1. #1598 landed unformatted.
cargo fmt --all --checkreports six diffs acrossdecode/mod.rs,decode/state.rs,native_decode/{cuda,mod,tests}.rs. Fixed withcargo fmt --all.2. Two new 1.98.0 clippy findings, fixed at the source:
manual_slice_fill— twofor row_len in &mut self.row_lensloops innative_decode/cuda.rs(also from feat(native-decode): commit recurrent GDN state to accepted prefix in speculative decode #1598) becomeself.row_lens.fill(..). Clippy's own suggestion text (&mut self.row_lens.fill(..)) does not compile; the&mutis spurious.needless_late_init—kernels/moe.rs's SwiGLU split rewriteslet linear_part; let gate_part;+if/elseaslet (gate_part, linear_part) = if ...Both behaviour-preserving;
cargo test -p onnx-runtime-ep-cpucovers the MoE path.3.
chunks_exact_to_as_chunks— allowed centrally, not rewritten.rustc/clippy 1.98.0 (built 2026-08-18) added this warn-by-default lint. The repo pins no toolchain, so every
-D warningsgate — Linux offline, quality lane, native-backend, aarch64, andcheck_cross_compile.sh— goes red the moment a runner picks up the new stable. This host already has it.Two reasons not to mass-rewrite:
It fires at 201 call sites across 12 crates (123 in
ep-cuda, 46 inep-cpu). That is a toolchain event, not a defect, and rewriting other owners' hot paths under time pressure is how subtle bugs land.It is not uniformly auto-fixable. In
onnx-runtime-ir::read_vec_lethe chunk size isT::BYTE_SIZE, an associated const of a type parameter, and clippy's suggestedas_chunks::<T::BYTE_SIZE>()does not compile:I compiled that against rustc 1.98.0 to check rather than assuming it.
So it goes in the existing
[workspace.lints.clippy]table that all 52 crates already opt into, with the reasoning recorded inline. Migrating the sites that genuinely benefit (as_chunksyields&[T; N], dropping fallibletry_intoand helping codegen) is real work worth doing per-crate by each owner. The two small lints in §2 are fixed rather than allowed precisely because they are small and local — the allow is for the 201-site case only.Validation
Local CI matrix, 19/20:
cargo fmt --all --checkcargo test -p onnx-runtime-ep-cpu-D warningscargo test -p onnx-runtime-ep-cpu --features mlasonnx-genai-engine --features native-backend--target aarch64-unknown-linux-gnucargo check --target aarch64-pc-windows-msvc--no-default-featurestests--all-featurescheckdefault_artifacts_are_mlas_freescripts/check_cross_compile.shPlus, beyond the script: clippy
-D warningsover the full quality-lane package set +onnx-genai-cli, andonnx-runtime-ep-cuda --features cuda(the exact CI invocation). Both clean.Step H fails with
onnxruntime_c_api.h:34:10: fatal error: 'stdlib.h' file not found— bindgen needs Windows SDK headers this container does not have. It fails identically on unmodifiedmain, so it is not a regression from this PR.Not fixed here
clippy::approx_constantdenies inonnx-runtime-ep-cuda's lib tests (optimizer.rs:4726). CI runs that crate's clippy without--all-targets, so no gate sees it today — butcargo clippy --workspace --all-targetsdoes fail on it. Left for that crate's owner rather than widened into a repair PR.Why I'm filing this
I hit these while re-validating my own branch after merging latest
main, and confirmed against a cleanorigin/maincheckout that they are not mine. Unblockingmainfirst, then rebasing my stack onto it.