Repository navigation
fix(ci): restore both required checks — migrate to as_chunks for Rust 1.98, reformat main - #1603
Merged
Merged
Conversation
Rust 1.98.0 (released 2026-08-18) added chunks_exact_to_as_chunks to the
default clippy set. CI installs unpinned `stable`, so the required
`Fast (Linux x86_64)` check started failing on every PR in the repo the
moment the release landed -- clippy runs with -D warnings, and there were
70 hits across 28 files. This is not caused by any one branch; it
reproduces on pristine main.
DESIGN.md pins the MSRV policy to 'Latest stable', so pinning the
toolchain or blanket-allowing the lint would both contradict the stated
policy. The code is migrated instead.
as_chunks::<N>() yields &[T; N] rather than &[T], so the fallible
conversions call sites used to perform (try_into().unwrap(), or
.expect("chunks_exact(8) yields 8 bytes")) are not merely redundant, they
no longer type-check. Removing them is the bulk of the diff and is a
strict improvement: a panicking path that could never panic is gone.
Where the old code used ChunksExact::remainder(), as_chunks returns the
remainder as .1, so dot_u8_f32 and PipelineCacheKey::absorb now destructure
it directly instead of threading a by_ref() iterator.
One site is a genuine false positive and is annotated rather than changed:
onnx-runtime-ir's read_vec_le chunks by T::BYTE_SIZE, an associated const
of a generic parameter, which is not permitted as a const generic argument
on stable.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…site Two gates are currently red on pristine main (0da1fe0), independently of this branch: * `Rust quality` runs `cargo fmt --all -- --check`, and #1598 landed six files unformatted (decode/mod.rs, decode/state.rs, native_decode/{cuda, mod,tests}.rs). * `Fast` runs clippy with -D warnings, and #1576 added one more chunks_exact site in simd_activations.rs's ARM sweep test. Both are fixed here so the branch can prove the gates green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1603 +/- ##
==========================================
- Coverage 81.53% 80.97% -0.57%
==========================================
Files 383 383
Lines 179169 179249 +80
Branches 179169 179249 +80
==========================================
- Hits 146084 145138 -946
- Misses 28145 29173 +1028
+ Partials 4940 4938 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ice_fill
Review caught that the migration only covered code reachable under default
features. `Rust quality` has a dedicated step (ci.yml:416) that runs clippy
over onnx-genai-engine with --features native-backend, and native_decode is
compiled nowhere else -- so ten more chunks_exact sites in native_decode/
{tensor,mod,tests}.rs and native_component.rs kept that gate red.
The same step also fails on a second new 1.98 lint, clippy::manual_slice_fill,
in native_decode/cuda.rs. Two hand-rolled fill loops become slice::fill.
Clippy's own suggestion there (`&mut self.row_lens.fill(..)`) is malformed;
the borrow is not wanted.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…unks # Conflicts: # crates/onnx-runtime-ep-cpu/src/kernels/moe.rs
…take
`half_prefill_gebp_agrees_with_the_blocked_half_gemm_and_is_the_route`
asserted that bf16 prefill takes the fused widen-pack GEBP. It does not,
on any host with AVX-512 BF16: `half_gemm_tile` hands bf16 to the native
microkernel and returns before the GEBP is reached, so the counter is 0
and the test fails with
BFloat16 m=2: prefill did not take the fused widen-pack GEBP
left: 0
right: 1
which is precisely what `Fast (Linux x86_64)` reported. The runner pool is
heterogeneous, so this is deterministic per runner and looks flaky across
runs. It is not caused by any change in this PR -- the diff does not touch
matmul.rs -- but it keeps the required check red, which is what this PR is
for.
Confirmed by simulating the branch on a host without AVX-512: forcing the
bf16 early return reproduces the CI panic text exactly, and the f16 arm
(which has no such interception) passes throughout.
The assertion is redirected rather than dropped. A route counter for the
native kernel is added -- the sibling of the GEBP's -- so each host asserts
the route it is supposed to take and neither arm can silently stop running.
Skipping the check instead would have left "the GEBP did not run" as the
only claim, which is equally true of a kernel that did nothing at all.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…unks # Conflicts: # crates/onnx-runtime-ep-cpu/src/kernels/matmul.rs
This was referenced Aug 20, 2026
Closed
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
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.
Both required checks are currently red on pristine
mainThis is not caused by any one branch. On
0da1fe0bd, with a clean checkout:Fast (Linux x86_64)— Rust 1.98.0 was released on 2026-08-18 andadded
clippy::chunks_exact_to_as_chunksto the default set. CI installsunpinned
stable(rustup toolchain install stable, ci.yml:126), so the lintarrived on its own. The clippy gate runs
-D warnings, and there were 70hits across 28 files, so every PR in the repo now fails this check.
Rust quality—cargo fmt --all -- --checkfails: feat(native-decode): commit recurrent GDN state to accepted prefix in speculative decode #1598 landed six filesunformatted (
decode/mod.rs,decode/state.rs,native_decode/{cuda,mod,tests}.rs).I found this while trying to get
Fastgreen on an unrelated PR of mine. Itblocks everyone, so it is fixed here.
Why migrate rather than pin or allow
docs/architecture/DESIGN.md:818states the policy explicitly:| MSRV | Latest stable | No old Rust version compat needed |
Pinning the toolchain would contradict that, and a blanket
allow(chunks_exact_to_as_chunks)would suppress a lint that is pointing atreal code quality. So the code is migrated.
What the migration actually does
as_chunks::<N>()yields&[T; N]wherechunks_exact(N)yielded&[T]. Thathas a pleasant consequence: the fallible conversions call sites had to perform
are not merely redundant afterwards, they no longer type-check. So most of
this diff is deleting them:
Each one removed a
panic!branch that could never fire.onnx-runtime-comm'sreductions,
onnx-runtime-ep-cpu's quantization/qlinear paths,onnx-runtime-shape-inference,onnx-stdand the session tests all shed one.Where the old code used
ChunksExact::remainder(),as_chunksreturns theremainder as
.1, so the iterator no longer has to be threaded throughby_ref()and then re-consulted:(
dot_u8_f32inmatmul_nbits.rs, andPipelineCacheKey::absorbinonnx-genai-engine, which is a hash — the chunk sequence and the remainderhandling are unchanged, so the digest is unchanged.)
One genuine false positive is annotated, not changed.
onnx-runtime-ir'sread_vec_lechunks byT::BYTE_SIZE, an associated const of a genericparameter, which is not permitted as a const generic argument on stable —
clippy's own suggestion does not compile there. It carries an
#[allow]with areason.Two hazards worth flagging, since this was partly mechanical
chunks_exact_mutisas_chunks_mut::<N>().0.iter(),which silently drops the mutability the call site needs. Applying it verbatim
would not compile in
onnx-runtime-comm::reduction(the loop body callscopy_from_slice). Every mutable site uses.iter_mut()here.chunks_exactinside the same expression asanother is masked until the first is fixed, so the migration was driven to a
fixed point and the gate re-run from a forced-fresh analysis (clippy caches
diagnostics, and an unchanged tree reports nothing on a second run — which
will happily look like success over a dirty tree).
Local validation
The exact CI clippy command (all 30 crates,
--all-targets -- -D warnings)is clean, as is the same gate over
onnx-genai-cli,onnx-genai-engine,onnx-runtime-ep-pluginandonnx-runtime-ep-cpu-plugin.cargo fmt --all --checkis clean.
Tests, on
stable1.98.0:onnx-runtime-ep-cpu --libonnx-runtime-ep-plugin --libonnx-runtime-ep-cpu-plugin --test plugin_ort_e2e --include-ignoredonnx-runtime-sessiononnx-genai-engine --libonnx-runtime-comm,-ir,-shape-inference,-eager,-capi,onnx-std,onnx-genai-preprocessaarch64-unknown-linux-gnuunder qemu)onnx-runtime-ep-cpu --libThe kernel crates are where the numeric risk is, and 1551 kernel tests plus the
57-case ORT conformance run cover it.
Note on scope
This touches crates outside my usual area (
onnx-runtime-comm,onnx-std,onnx-genai-engine,onnx-runtime-session,onnx-runtime-shape-inference,onnx-runtime-capi,onnx-genai-preprocess). That is not empire-building — thegate is all-or-nothing, so
Fastcannot go green for anyone until every hit isaddressed. The changes there are mechanical and each is covered by that crate's
own tests.
Independent review found a BLOCKER, now fixed
Reviewed adversarially (Opus, independent context). It found a real defect I had
missed, and it is the interesting kind:
That step exists because of a previous incident — its comment says "without this
step a refactor can leave it uncompilable while every other job still passes".
It caught me the same way. Fixed in
83d7bc9ee, along with a second new-1.98lint the same step exposes,
clippy::manual_slice_fill, innative_decode/cuda.rs(clippy's suggestion there,
&mut self.row_lens.fill(..), is malformed — theborrow is not wanted,
self.row_lens.fill(..)is).Lesson recorded: for a lint migration, "clean under default features" is not the
gate. Every feature-gated lane has to be run.
The review verified and found no behavioural regression in the rest, in
particular the two sites I was most concerned about:
PipelineCacheKey::absorb(a cache-key hash) — same 8-byte words in thesame order, same zero-padded tail, so the digest is unchanged and cache keys
stay stable.
reduction.rs— mutability preserved (iter_mut, not clippy's suggestediter); confirmed by running the crate's own falsifiers, includingdistributed_all_reduce_matches_single_device_bitwise, which a non-mutatingiterator would turn into a no-op.
It also confirmed
dot_u8_f32's tail arithmetic, that every droppedtry_into()was a genuine
&[T; N]→[T; N], that theread_vec_le#[allow]justificationis true, that the reformatting commit is formatting-only, and that the ARM sweep
test's
coveredis unchanged.Full local validation
Gates, all on
stable1.98.0 (installed locally to match CI — my default was1.97.1, which is precisely why I could not see this lint at first):
Fastclippy (30 crates,--all-targets -- -D warnings)Rust qualityclippy (same set)-p onnx-genai-engine --features native-backendonnx-genai-cli,onnx-runtime-ep-plugin,onnx-runtime-ep-cpu-plugincargo fmt --all -- --checkscripts/check_cross_compile.sh(x86_64 + real aarch64 pass)Rust qualitypython gates (publish order, dispatch manifest/reachability, feature-gate coverage, env vars, …)Tests:
onnx-runtime-ep-cpu --libonnx-runtime-ep-cpu --features mlas kernels::moe::onnx-runtime-ep-cpu --features mlas kernels::qlinear_matmul::onnx-runtime-ep-cpu --features mlasregistry configonnx-genai-engine --lib --features native-backendonnx-genai-engine --libonnx-runtime-ep-cpu-plugin --test plugin_ort_e2e --include-ignoredonnx-runtime-ep-plugin --libonnx-runtime-session,-comm,-ir,-shape-inference,-eager,-capi,onnx-std,onnx-genai-preprocessDisclosure of missing scope:
onnx-genai-engine's integration test binaries(
iterative_pipeline_e2e,vlm_pipeline_e2e, …) could not be linked locally —the build host's disk is shared and repeatedly hit 100%, and
lddied with a buserror. The library tests for that crate ran (567 with
native-backend, 433without) and cover the changed code; CI will link the rest. No local red anywhere.
Why
mainlooks green when it is notWorth recording, because it misled me for a while and will mislead the next
person:
Fastis passing onmainright now, on rustc 1.98.0, with 108chunks_exactsites still in the linted crates.It is passing because its clippy step never checked those crates. The step
compiled 244 crates from a restored cache, and
onnx-runtime-ir,onnx-runtime-ep-cpuandonnx-runtime-commare not among them — they werealready fresh, and cargo does not re-emit diagnostics for a crate it did not
rebuild. The lint is a default-
warnlint promoted by-D warnings, so acached crate is indistinguishable from a clean one.
The moment a crate is genuinely re-checked, it fires. That is exactly what
happened on #1587, whose log shows the error arriving on the line after
Checking onnx-runtime-ir. So the situation is not "main is fine and this PR iscleanup" — it is that main is one cache eviction away from a repo-wide outage,
and in the meantime every PR that touches one of these crates goes red for a
reason that has nothing to do with its own change.
Both my own PRs died this way before I understood it.
Second blocker found while landing this: a bf16 route assertion (now fixed on main)
Faston this PR then failed in a place my diff does not touch (matmul.rs,zero lines changed):
half_prefill_gebp_agrees_with_the_blocked_half_gemm_and_is_the_routeassertedthat bf16 prefill takes the fused widen-pack GEBP. On any host with AVX-512
BF16 it does not:
half_gemm_tilehands bf16 to the native microkernel andreturns before the GEBP is reached, so the route counter is legitimately 0. The
f16 arm has no such interception, which is why only the bf16 rows failed.
My CPU has no AVX-512 at all, so it passed locally and failed on runners that
have the faster kernel -- deterministic per runner, indistinguishable from
flake across a heterogeneous pool. Confirmed rather than argued: forcing the
bf16 early return on my host reproduced the CI panic verbatim, including
m=2andleft: 0.I fixed it here, and then a fix for the same defect landed on
mainwhile thisPR was in the queue. Main's is better -- it derives the expectation from the
very predicates
half_gemm_tiledispatches on, instead of restating thehardware assumption, and it treats the no-AVX2 host as a legitimate
(0, 0)rather than a skip. So I took
main's version wholesale and dropped mine; thisPR now carries no change to
matmul.rsat all. Recording the diagnosis hereanyway, since the reproduction is the part that was expensive.
Re-validated after taking main's version: ep-cpu 1555 passed on x86_64,
1455 passed, 0 failed on aarch64 under qemu, ORT conformance 57 passed,
engine
--features native-backend568 passed; fmt and every clippy gatestill clean.