Repository navigation
ci: unbreak the Rust quality lane on main, a fourth time - #1393
Merged
Merged
Conversation
`main` is failing `cargo fmt --all -- --check` on `crates/onnx-runtime-ep-cuda/src/runtime.rs`, where a `map_or` closure exceeds the width rustfmt will keep inline. Every branch cut from main inherits the failure, so the required Rust quality check is red for work that did not cause it. Formatting only -- the output of `cargo fmt --all`, no behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
enabled auto-merge (squash)
August 19, 2026 04:21
This was referenced Aug 19, 2026
c5d19f7 landed three files that rustfmt disagrees with, so `cargo fmt --check` fails on main itself. That is a required check, so every open PR that merges main inherits the failure and cannot be merged. Pure rustfmt output, no hand edits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Refreshed onto latest main.
This matters more than the line count suggests: Commit is pure This is the fifth occurrence. The reason it keeps recurring is that the fmt gate only runs on PR branches, so a PR can be green when it is approved and still land a main that is red if it merged before a conflicting formatting change. Worth considering a push-triggered fmt job on main so the breakage is attributed to the commit that caused it rather than found by whoever merges next. |
Merging latest main brings a fifth unformatted site (qwen35_0_8b_text_decode_lock.rs). Pure rustfmt output; cargo fmt --check is clean on the merge result. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 19, 2026
Owner
Author
|
Refreshed onto latest main again;
I also audited this against #1382, which carries rustfmt fixes for three of the same files — full write-up in #1429 (comment). Summary: the aarch64 half of that cluster is already fixed on main by #1443, so #1382 and #1429 should not merge as repairs, which leaves this PR as the single fmt path with no duplication. Pure |
Owner
Author
REJECT — #1393 does not unbreak the lane it is named forIndependent audit (Pris, Tester) against The findingThe PR is titled "unbreak the Rust quality lane, a fourth time". Its merge result still fails that lane. git merge --no-edit origin/main # in a clean worktree of this branch
cargo clippy -q -p onnx-runtime-session --all-targets -- -D warnings; echo "EXIT=$?"
The blocker is - .map_or(true, |v| !matches!(v.as_str(), "0" | "false" | "off" | "no"))
+ .is_none_or(|v| !matches!(v.as_str(), "0" | "false" | "off" | "no"))
Running Knock-on
bash scripts/check_cross_compile.sh; echo "EXIT=$?"
# EXIT=1 — error: this `map_or` can be simplifiedThat is why the aarch64 defect in #1443 was never reported by the gate that exists to catch it. RecommendationClose in favour of #1382, which fixes this line, the four other Per the reviewer protocol's strict lockout, the original author should not own the revision. If a separate PR is still wanted here, suggest Resch or Gaff pick it up — though the recommendation is simply to close it. |
`clippy::unnecessary_map_or` fires on `map_or(true, ..)` and the Rust quality lane builds with `-D warnings`, so this is a hard error on main today -- reproduced on an unmodified `dbade34c1` checkout with the same stable 1.97.1 toolchain CI installs. Every PR that merges main inherits it, which is why nothing has been able to go green. `is_none_or(f)` is the exact rewrite the lint suggests and is semantically identical to `map_or(true, f)`: `None` yields `true`, `Some(v)` yields `f(v)`. Behaviour of `gqa_shape_capacity_bound_enabled` is unchanged -- unset stays enabled, and the falsey spellings stay disabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`clippy::inconsistent_digit_grouping` fires on `2_000_000_000_000_0` and the Fast (Linux x86_64) lane runs clippy with `--all-targets -D warnings`, so this is a hard error on main today -- reproduced on an unmodified `dbade34c1` checkout. It is invisible to a plain `cargo clippy` because the literal lives in a `#[cfg(test)]` module. `20_000_000_000_000` is the identical value (2e13), which is what the test's own comment says it is intending -- "2e13 FLOP / 2e13 = 1 s". `op_cost_takes_roofline_max` still asserts the compute term dominates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ain-4 # Conflicts: # crates/onnx-runtime-session/src/executor/dispatch.rs
The ONNX Runtime 1.27 -> 1.28 bump (#1481) added a field to `OrtEpFactory`, so `build_factory`'s struct literal no longer compiles: error[E0063]: missing field `SelectBestModelCandidate` --> crates/onnx-runtime-ep-plugin/src/factory.rs:131:17 This is a hard error on `main` at f6f3a3f -- reproduced on a pristine checkout -- so the whole workspace is currently uncompilable. The field is `Option<unsafe extern "C" fn(..)>`, i.e. ORT's encoding for an optional callback, and `None` is the honest value here. The hook only matters for EPs that publish several compiled variants of a single model and need to rank them; we publish one variant, so ORT falls back to `ValidateCompiledModelCompatibilityInfo`, which we already implement and which is the right answer for a single candidate. Also refresh the fail-closed diagnostic, which still hardcoded "version 27" while the version it actually enforces (`ort::ORT_API_VERSION`) is now 28 -- the check was right, the message it printed was not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
…e it (#1407) ## The UB `nested_dispatch_slot_pressure` (which I added in #1377) reconstructs a `&mut [u64]` over the **entire** row in every task and only then narrows: ```rust let row = unsafe { std::slice::from_raw_parts_mut(base as *mut u64, NEST_INNER) }; for slot in &mut row[start..end] { *slot += 1; } ``` The *stores* are disjoint. The *retags* are not. Under Stacked Borrows the violation is the retag: each task pushes a `Unique` covering all of `NEST_INNER`, popping the previous task's tag. Narrowing before the retag fixes it, and is the shape `parallel_output_rows_repeated` already uses in production. **Diagnosis and fix are Pris's, from #1385.** This PR carries them because that one is still a draft with auto-merge off and the UB is on `main` today. If #1385 goes ready first, close this and I will rebase the Miri half onto it — the two halves are independent. ## Why it survived, which is the part worth keeping CI's Miri lane runs `-p onnx-runtime-ep-cpu --lib task_runtime::`. **Integration tests under `tests/` are never Miri-checked at all.** Fixing this one instance would have left that gap open for the next one, so this also puts the shape under the lane as a lib test. That was not sufficient either, and the first attempt is the useful part: **the new test passed under Miri with the bad retag still in it.** Miri reports `available_parallelism() == 1`, so `resolve_width` builds a one-lane pool, every fan-out returns `Backend::Serial`, and no two tasks ever run against each other. Probe under Miri: ``` pool_width=1 backend=Serial tasks=1 ``` The lane has been type-checking the unsafe blocks in this module without exercising the concurrency they exist for. The workflow comment claims it "runs real threads under Stacked Borrows" — it runs one. So I would have shipped a canary that passes for a reason unrelated to its claim, which is precisely the class Pris named in #1385. Two fixes: 1. **The test asserts it actually fanned out** (`Backend::Native`), so it fails loudly if it ever degenerates to one task instead of passing silently. 2. **A dedicated lane step with `-Zmiri-num-cpus=4`**, scoped to that single test rather than all of `task_runtime::` — multi-CPU Miri multiplies the runtime of what is already the slowest step in the lane. The targeted step costs **18s**. ## Verified in both directions | retag shape | `-Zmiri-num-cpus=4` | result | | --- | --- | --- | | whole row (**#1377 as merged**) | yes | `error: Undefined Behavior: Data race detected between (1) retag write on thread task_runtime::t and (2) retag write of type [u64] on thread nxrt-task-0` | | whole row | **no** (lane default) | **passes** | | own range (**this PR**) | yes | passes | The middle row is the finding: without the flag the falsifier does not falsify. ## Scope Test-only plus one workflow step. No production change — `for_each_chunk_mut` was always correct. 30/30 `task_runtime::` lib tests pass natively and under Miri; the repaired benchmark still runs (`1 of 30 dispatches declined`); fmt and clippy clean. Includes the one-line rustfmt repair of `onnx-runtime-ep-cuda/src/runtime.rs` that `main` is currently failing on (same as #1393/#1395/#1398 — whichever lands first makes the rest a no-op). 🤖 Generated with [GitHub Copilot CLI](https://github.com/features/copilot/cli) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
## Why CI's Rust quality lane (`cargo fmt --all -- --check`) runs on Linux and is fine. The **local** gate that is supposed to catch rustfmt drift *before* it lands is non-functional on Windows — which is where this repo's agent workflow runs, from git worktrees. The quality lane on `main` has been repaired for rustfmt drift four times (#1260, #1320, #1393, #1400). *That the broken local gate is the cause of those four repairs is a plausible inference, not something I measured — I only verified that the local gate does not work.* This PR makes the local gate runnable on Windows. It does **not** claim to prevent future drift. ## Defects (each verified on this Windows box) Environment: `rustfmt 1.9.0-stable`, `cargo 1.97.1`, Git-for-Windows bash 5.3, `git config core.autocrlf = true`, workspace = **54 members / 972 tracked `.rs` files**, mixed-edition (**52 on edition 2024, 2 on 2021**). 1. **Shell scripts check out as CRLF.** `.gitattributes` only pinned `schema/inference_metadata.schema.json`; `*.sh` and the extension-less `scripts/hooks/*` were unprotected, so all 14 tracked `.sh` files + the hook showed `w/crlf`. Running one under bash printed `scripts/install-hooks.sh: line 10: $'\r': command not found` and `set: pipefail: invalid option name` — unrunnable. 2. **`install-hooks.sh` cannot work in a worktree.** It used `HOOKS_DST="$REPO_ROOT/.git/hooks"` and bailed if that dir was missing. In a worktree `.git` is a **file**, so it always errored "are you in a git repo?". 3. **`cargo fmt --all` fails on Windows regardless.** `cargo fmt --all -- --check` exits **1** with `The filename or extension is too long. (os error 206)`: cargo-fmt passes every path to one `rustfmt`, overflowing the Windows ~32 KB command-line limit. Linux CI is unaffected (`ARG_MAX` ~2 MB). The old `pre-commit` ran this with **`2>/dev/null`** and then told the user `Fix: cargo fmt --all` — a command that also fails with os error 206. *(This fails loudly with exit 1 — there is no false-green here.)* 4. **No hook was installed** in this checkout — a consequence of 1+2. **Mixed-edition trap (why the fix uses `cargo fmt -p`, not raw `rustfmt`):** with the wrong edition, `rustfmt` mis-parses 2024-only syntax (e.g. `let` chains: `error: let chains are only allowed in Rust 2024 or later`) **and fails**. Only cargo knows each package's declared edition, so driving the check per package is the only correct approach. *(An earlier claim of a silent exit-0 false green was traced to a measurement artifact — `rustfmt … | Select-Object -First N` truncates the pipeline and drops the native exit code — and has been withdrawn; the failure is exit 1.)* ## Changes - **`.gitattributes`** — pin `*.sh` and `scripts/hooks/*` to `eol=lf` (comment explains a CRLF bash script is unexecutable) and renormalize. All 15 files now report `i/lf w/lf attr/text eol=lf`. - **`scripts/install-hooks.sh`** — resolve the hooks dir via `git rev-parse --git-common-dir` (the shared gitdir used by the main checkout and every linked worktree), resolving a relative result to absolute. Keeps `--dry` and the "does not clobber foreign hooks" property. - **`scripts/hooks/pre-commit`** — map the staged `.rs` files to their owning workspace packages and run `cargo fmt -p <pkg> -- --check` only for those. Now **mirrors CI's scope exactly**: - Files whose crate is **not a workspace member** (e.g. the root-level `bench-*` crates) are **skipped with a warning**, because `cargo fmt --all` does not cover them either. Blocking on a non-member would recreate the os-error-206 failure shape (`cargo fmt -p <non-member>` → "not a member of the workspace") and wall people off behind drift they never introduced. Membership is taken from `cargo metadata --no-deps` (matched on `manifest_path`, which is unambiguous — bare `"name"` keys also appear on every dependency). - If `cargo metadata` itself fails, the hook **fails open** (warns, lets the commit through) — a format gate must not lock you out of the repo. - Stops suppressing stderr; the printed fix is `cargo fmt -p <pkg>` (works on Windows). - **`wiki/development/Testing and Verification.md`** — state plainly that `cargo fmt --all` does not work on Windows here; give the per-package alternative and `bash scripts/install-hooks.sh`. ## Verification (measured on this box) - **`install-hooks.sh --dry`** succeeds from the **worktree** and (relative-`.git` branch) from a **normal checkout**, both resolving to the same shared `…/onnx-genai/.git/hooks`. Run under **Git-for-Windows bash**, which actually executes hooks. *Note:* WSL bash cannot run git in a Windows-created worktree at all — the `.git` pointer holds a `C:/…` path WSL's git can't resolve; that is a WSL/Windows limitation affecting every git command there, not this script. - **End-to-end, against the committed hook:** - staged a mis-formatted **member** `.rs` → commit **blocked** (exit 1), diff shown, fix `cargo fmt -p onnx-runtime-cpuinfo` printed; ran it → commit **passed**. - staged only a **non-member** (`bench-seqmajor`) `.rs` → commit **passed** with the "not a workspace member … CI's cargo fmt --all does not cover them either" skip warning. - staged a mis-formatted **member** *and* a **non-member** together → commit **blocked**, and the block came **only** from the member; the non-member was skipped and the printed fix command works. - simulated `cargo metadata` failure (stub returning 101) → hook **exited 0** with the fail-open warning. - All test artifacts discarded; nothing committed. - **`main` is clean** by the new check: looping `cargo fmt -p <name> -- --check` over all 54 members → **checked 54, failed 0, ignored 0**. - **Hook wall time** on a realistic single-package staged change: **~1–2 s** (the hook only checks the staged packages, not all 54). - **Full-member confirmation timing** (this is the `main`-clean sweep, not the per-commit hook cost): two consecutive runs **26.1 s** then **25.2 s**, consistent with an independent 23.1 s measurement. An earlier one-off 87.5 s reading was a non-reproducible first-run outlier and is not representative. ## Not touched - `.github/workflows/ci.yml` — CI is not broken; this is a local-gate fix. - Anything under `.squad/`. ## Rebase (onto latest main) Rebased from base `4b1cabb8` onto `origin/main` at `1557a355` (which had advanced through #1482, #1173, #1420, #1487). The **only** conflict was in `wiki/development/Testing and Verification.md`: #1482 translated the whole wiki to Chinese (`lang: zh-CN`), so my originally-English Windows-formatting section collided with the now-Chinese baseline. Resolved by **following the new Chinese baseline** — the added formatting/pre-commit documentation is written in Chinese to match the surrounding prose, and none of #1482's translation was reverted. Per project rules, code, commit messages and this PR title/body stay in English; only that wiki body follows its file's language. Checked that #1487's `docs/benchmarks/windows-cuda-runbook.md` neither overlaps nor conflicts with the wiki formatting note (the runbook covers CUDA benchmarking and contains no formatting/hook content), so no cross-link was needed. After the rebase, re-ran the three end-to-end scenarios (member-block→fix→pass, non-member-only→pass+skip-warning, mixed→blocked-only-by-member) and the `main`-clean sweep (**checked 54, failed 0**) — all still correct. Test artifacts cleaned; working tree clean. Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
…nt4 (#1434) Building the decode pool is unconditional today: every borrowed-int4 call installs a 16-worker Rayon pool before dispatching, including the decode-shaped calls that then route all of their work to the task runtime and never touch it. Those workers are constructed, parked, and torn down once per call for nothing. This defers the construction for exactly one path — decode-shaped (`m == 1`) borrowed int4 — and hands the routing logic the width it *would* have installed, so the executor choice and the partition grain are both unchanged. ## What is actually covered The scope is deliberately narrow, and the narrowness is what makes it provable rather than measured: - Three call sites are converted: `packed_nbits_gemv`, `gemv_nk`, and the borrowed-int4 site gated on `m == 1`. - Those paths reach Rayon only through `parallel_output_rows_repeated` (`parallel_output_rows` delegates to it), which is the hookable dispatcher. - The `m == 1` gate is what makes this safe by construction, not by inspection: `borrowed_affine_int4_matmul_prefill` drives Rayon directly, and it is unreachable at `m == 1`. The other seven `with_decode_pool` sites are untouched, so the kernels that use Rayon directly — `packed_nbits_gemm`, `int8_matmul`, `int8_row`, `parallel_n16_output_rows`, `parallel_kai_output_rows` — keep eager installation. Routing is preserved rather than assumed. `flat_fan_out` branches on `rayon::current_num_threads()`, which equalled the decode width only because we were installed; `effective_fan_out_width()` reproduces exactly that width while deferred. The second commit extends the same helper to `output_chunk_len`, so the *grain* cannot drift from the installed grain either — without it a deferred call partitioned into 4096-row chunks where the installed one used 64. Five tests, two of them verified as falsifiers by deliberately breaking the thing they check: - relaxing the gate to `m >= 1` makes `only_the_decode_shaped_borrowed_int4_path_defers_the_pool` fail; - dropping the grain fix makes `a_deferred_fan_out_partitions_for_the_pool_it_would_have_installed` fail (chunk 64 vs 4096). ## Measured 16-core budget, interleaved with alternating arm order in a single session. | | before | after | |---|---|---| | process threads | 48 | **32** | | `onnx-genai-decode-*` workers | 16, 340-490 ms CPU | **none** | | voluntary ctxsw / iter | 24.61 | **16.12** | | total CPU | 3.660 cpu-s | 3.580 cpu-s | | dispatches / iter | 1.30 | 1.08 | **Latency is neutral, and that is the claim — not an improvement.** Six A/B reps gave 0.938, 1.180, 1.130, 1.115, 1.031, 1.026 (median 1.073), and an A/A null control measured in the same window gave a 0.83-1.21 band. Every ratio is inside the band, so this PR does not demonstrate a latency change in either direction. The win is structural: 16 fewer threads and a third fewer voluntary context switches. One number deserves an explicit caveat: total CPU is flat, not lower. The decode pool's CPU does not disappear, it reappears on the caller thread. That is attribution changing, not work being removed — the work was always the caller's, it was just being done by borrowed workers. `onnx-runtime-ep-cpu` lib suite: 1453 passed, 0 failed on merged latest main. fmt and clippy clean for this crate. (`cargo fmt --check` currently reports five sites repo-wide, all inherited from main and none in the one file this PR touches; #1393 repairs them.) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
added a commit
that referenced
this pull request
Aug 19, 2026
`cargo fmt --all -- --check` is a hard gate in both `Fast (Linux x86_64)` and `Rust quality`, and it fails on `main` @ `ba52d1702` with four diffs, all in `crates/onnx-runtime-ep-cuda/src/optimizer.rs` — one method chain at line 308, three in the `CudaRsqrtFusion` tests. Every PR that merges main inherits it, so nothing in the queue can go green until it is repaired. Same lane #1393 unbroke four days of breakages ago. Pure `cargo fmt --all` output. No semantic change. ## Local validation | gate | result | |------|--------| | `cargo fmt --all -- --check` | **0 diffs** | | `cargo test --locked $(… offline-linux)` | **3989 passed, 0 failed, exit 0** | | `cargo clippy --locked --all-targets $(… offline-linux) -- -D warnings` | exit 0 | | `cargo check -p onnx-runtime-ep-cuda` | clean | | `scripts/check_cross_compile.sh` | PASS (x86_64 + aarch64) | | 9 guard scripts | PASS | Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Process note re: the audit verdict (#1393 REJECTED — close without merge). This is not actionable: #1393 was already merged at 2026-08-19T16:16:59Z as Respecting the lockout: I am the original author, so I am not pushing any revision of this artifact, and I have not. I am also not reverting it, and want that decision visible rather than silent. Beyond the fmt/clippy hunks (which #1382 also carried, and which merged away as no-ops when I brought main into #1382), this commit is the only carrier of the fix for #1481's ORT 1.27→1.28 bump: Main did not compile at all on If the audit's objection is to the bundling — four unrelated repairs in one PR — that is a fair criticism I accept, and the right remedy is a follow-up that re-does any specific hunk under different authorship, not a revert. Happy to have someone else own that; I will not self-revise. cc @justinchuby |
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1393 +/- ##
==========================================
+ Coverage 82.10% 82.64% +0.54%
==========================================
Files 12 12
Lines 5471 5475 +4
Branches 5471 5475 +4
==========================================
+ Hits 4492 4525 +33
+ Misses 780 757 -23
+ Partials 199 193 -6
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 22, 2026
## The problem
Rayon does not name the workers `build_global` creates, and an **unnamed
thread's `comm` defaults to the process name**. So the pool that
`bound_process_to_decode_budget` builds appears in `ps`, `top` and
`/proc/<pid>/task/*/comm` as N extra copies of the host binary.
This is not a cosmetic gap. It was the largest unattributed block of
threads in any budgeted process, and it is what kept "the process has
roughly twice the budget in threads and we don't know whose they are"
open as a question across several phases of the CPU scheduler campaign.
A flat thread census does not merely *fail* to explain these threads —
it **misattributes them to the caller**, which is worse, because the
census looks complete.
## What the naming immediately revealed
Census on `gemm_nbits_qwen3_0p6b_qkv_t8`,
`ONNX_GENAI_CPU_DECODE_THREADS=16`, decode-only:
```
== thread census (48 threads) ==
name count cpu_ms
onnx-genai-deco 16 20.0
bench_decode_ga 1 40.0
nxgn-prefill-0 1 0.0
nxgn-prefill-1 1 0.0
... (16 total) all 0.0
```
Before this change the sixteen `nxgn-prefill-*` rows read as
`bench_decode_ga` — indistinguishable from the benchmark's own main
thread.
**All sixteen report 0.0 ms of CPU.** The pool is built eagerly at EP
init, is never used by a decode-only workload, and holds N threads for
the process lifetime.
That also quantifies the budget's real cost, which is larger than it
looks:
| configuration | threads | composition |
| --- | --- | --- |
| default (no budget) | 22 | 1 main + 6 decode + 15 task-runtime |
| budget 16 | 48 | 1 + **16 prefill Rayon** + 16 decode + 15
task-runtime |
| budget 32 | 80 | 1 + **32 prefill Rayon** + 32 decode + 15
task-runtime |
Setting an explicit budget of N does not size one pool to N — it sizes
the decode pool to N **and** builds a second, separate N-wide pool. Half
of that was previously anonymous.
A useful negative result falls out of the same census: **the ORT session
contributes exactly zero threads**. Whatever else is going on, our EP is
not double-provisioning against ORT.
## Why the names are short
Linux stores `comm` in 15 bytes. This crate's existing
`onnx-genai-`-prefixed convention (`decode_numa.rs`, `decode_spmd.rs`)
already exceeds that and collapses to `onnx-genai-deco` /
`onnx-genai-spmd`, losing the worker index — visible in the census
above, where all 16 decode workers fold into one row. `nxgn-prefill-15`
is exactly 15 bytes and survives intact, matching the short form the
task runtime already uses (`nxrt-task-N`).
The test asserts the *property* — fits in `comm`, keeps its prefix,
keeps its index, stays distinct — rather than the literal string, since
the string is not the thing that has to hold.
## Scope
Thread names only. No behaviour change, no sizing change, no scheduling
change. 1448/1448 `onnx-runtime-ep-cpu` lib tests pass; fmt and clippy
clean.
**Making the pool lazy is the real fix and is deliberately not attempted
here.** It would require every global-Rayon entry point in the default
build to route through a guard before first use, and missing one
silently un-bounds the user's explicit budget — a correctness regression
traded for a thread-count saving. Named as a measured cost with a known
shape (§41) so the decision is on the record rather than rediscovered.
Includes the one-line rustfmt repair of
`onnx-runtime-ep-cuda/src/runtime.rs` that main is currently failing on
(same as #1393/#1395 — whichever lands first makes the others a no-op).
Without it this branch cannot be green.
Follows #1395, which built the census that made this diagnosable.
🤖 Generated with [GitHub Copilot
CLI](https://github.com/features/copilot/cli)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Unbreak the two CI lanes that are red on
mainright now. Threeindependent breakages, all pre-existing and all reproduced on an unmodified
dbade34c1checkout with the same stable 1.97.1 toolchain CI installs:cargo fmt --all -- --checkRust qualityclippyclippy::unnecessary_map_or—dispatch.rs:26map_or(true, f)→is_none_or(f)Fast (Linux x86_64)clippy--all-targetsclippy::inconsistent_digit_grouping—cost-model/model.rs:3142_000_000_000_000_0→20_000_000_000_000Both lanes build with
RUSTFLAGS: -D warnings, so #2 and #3 are hard errors,not warnings. Every PR that merges
maininherits all three — verified on#1434 and #1420. Nothing in the queue can go green until this lands.
#3 is worth calling out: it is invisible to a plain
cargo clippybecause theliteral lives in a
#[cfg(test)]module. Only the--all-targetsinvocationin the Fast lane sees it.
Semantics
Both non-fmt changes are provably value-preserving:
is_none_or(f)is the rewrite the lint itself suggests, and is definitionallymap_or(true, f):None→true,Some(v)→f(v).gqa_shape_capacity_bound_enabled()is unchanged — unset stays enabled, thefalsey spellings stay disabled.
20_000_000_000_000 == 2_000_000_000_000_0(both 2e13), which is what thetest's own comment already claims — "2e13 FLOP / 2e13 = 1 s".
op_cost_takes_roofline_maxstill asserts the compute term dominates.Validation
Ran locally per the delayed-Actions directive, on this head merged with
origin/main@dbade34c1:cargo fmt --all -- --checkcargo clippy --locked --all-targets $(workspace_test_packages.py cargo-args offline-linux) -- -D warningscargo test --locked $(… offline-linux)scripts/check_cross_compile.shbenchmark_muse_native_local.py --self-test --require-numpycheck_publish_order.py/check_profile_table.py/check_platform_naming.pycheck_dispatch_reachability.py/check_feature_gate_coverage.pycheck_dispatch_manifest.py(--self-testand plain)workspace_test_packages.py verifyverify_documented_env_vars.py-p onnx-runtime-ep-cpu --no-default-features --features mlasmoe::19 passed ·qlinear_matmul::30 passed ·optimization_registry_excludes_nchwc_without_cnn_ops1 passedcargo clippy -p onnx-genai-engine --features native-backendcargo build -p onnx-runtime-ep-cpu-plugin --features mlasWindows ARM64: not validated locally — stated as a blocker, then bounded
I could not run
Rust (Windows ARM64)here and I am not claiming it as apass. Two routes were attempted, both fail identically on unmodified
main,so neither can discriminate this PR from baseline:
cargo-xwin/ clang-cl — installed, MSVC CRT + SDK downloaded, correctlytargeting
aarch64-pc-windows-msvc. Fails in vendoredmlasi.hon NEONintrinsics (
veorq_s32,vdupq_n_f32, …) that MSVC supplies but clang-cl inMSVC mode does not.
aarch64-unknown-linux-gnu+ GNU cross toolchain as an ARM64-NEON proxy —gets much further, compiles most of the ARM64 MLAS source set, then fails on
activate_fp16.cpp.What makes this safe to merge anyway is dependency-graph disjointness, not a
judgement call. That lane builds only
mlas-sysandonnx-runtime-ep-cpu-plugin --features mlas. This PR touchesonnx-genai-engine(tests),onnx-runtime-ep-cudaandonnx-runtime-session:Zero of the crates this PR modifies are in that lane's graph, so it cannot
observe this change.