Repository navigation
fix(cuda): accept fp16/bf16 router_probs, scales and biases in QMoE - #1602
Merged
Merged
Conversation
The native CUDA `com.microsoft::QMoE` kernel hardcoded `router_probs`,
fc1/fc2/fc3 `scales`, expert biases and the optional aggregation weights to
`Float32`, so an all-fp16 fused QMoE export (e.g. the Mobius fp16 build) was
rejected at runtime (`router_probs requires Float32, got Float16`) even though
its activations were already handled by the fp16 kernels. router_probs was
merely the first check to fail; scales would have failed next.
ORT's `com.microsoft::QMoE` binds input, router_probs, scales, biases and
output to a single type parameter `T` = {float, float16, bfloat16}, so a valid
fp16 graph carries fp16 versions of all these operands. Classify each T-typed
float operand independently: f32 operands keep their original pointers (the
pure-f32 path and the pre-existing "fp16 activations + f32 scales" graphs stay
byte-identical), while fp16/bf16 operands are losslessly widened to f32 scratch
via new `qmoe_widen_{f16,bf16}_f32` kernels and fed to the unchanged f32
routing/dequant kernels. Routing (max/expf/sum/normalize/top-k) still
accumulates in f32.
Adds an all-fp16/bf16 GPU regression (router_probs + all scales + biases +
aggregation weights half-typed) asserting conformance to a half-rounded CPU
reference across decode and prefill.
Measured: `qwen15-moe-qmoe-mobius` (all fp16) now loads and decodes coherent
text natively on CUDA with graph capture (captures=1 replays=22 fallbacks=0);
`qwen15-moe-qmoe-f32` greedy token ids are byte-identical to the pre-change
binary; qmoe_gpu suite 33 passed / 0 failed / 0 ignored under cuda,gpu-tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1602 +/- ##
==========================================
+ Coverage 80.69% 80.79% +0.09%
==========================================
Files 379 380 +1
Lines 170734 175074 +4340
Branches 170734 175074 +4340
==========================================
+ Hits 137782 141451 +3669
- Misses 28071 28727 +656
- Partials 4881 4896 +15
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
Picks up #1586, #1601 and #1602. No conflicts; Cargo.lock was the only file both sides touched and it auto-merged. Audited the same way as the previous two syncs rather than by reading the diff: every main-only file verified byte-identical to origin/main by blob hash, and for Cargo.lock every line main added over the merge-base confirmed present in the result, with this stack's three onnx-runtime-memory-api entries still intact. #1586 is the reason this sync matters beyond staying mergeable. It corrects the macOS `struct statvfs` FFI layout, which is what made `platform_capacity::tests::disk_capacity_is_measured_for_the_working_directory` and `engine::tests::an_explicit_byte_limit_is_honored_without_a_device_query` fail on every local run of this branch. Those two were the only failures in the crate suite here, and every "559 passed / 2 failed" figure reported against this PR was that pair. They are expected to be gone now; verified in the following run rather than assumed. 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 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
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
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes the native CUDA
com.microsoft::QMoEkernel accept fp16/bf16router_probs, scales, biases and aggregation weights, so an all-fp16 fusedQMoE export runs natively.
Root cause (corrects the task premise)
The blocker was broader than
router_probs. The kernel hardcodedrouter_probs,fc1_scales,fc2_scales(and, latently, fc3 scales, biasesand the optional aggregation weights) to
Float32.router_probswas only thefirst check to fail on the
qwen15-moe-qmoe-mobiusfixture; its scales arefp16 too and would have failed next. The old "mixed-precision loader blocker"
framing is stale: the Mobius export is now internally type-consistent (all
fp16) and already satisfies ORT's single-
Tbinding — the only remainingblocker was our own kernel. See
.squad/decisions/inbox/copilot-qmoe-fp16-router-probs-resolved.md.Design
ORT's
com.microsoft::QMoEbinds input, router_probs, scales, biases and outputto a single type parameter
T= {float, float16, bfloat16} (verified againstthe ORT schema). Each T-typed float operand is classified independently:
pre-existing "fp16 activations + f32 scales" synthetic graphs are byte-for-byte
unchanged.
qmoe_widen_{f16,bf16}_f32grid-strided kernels) and fed to the unchangedf32 routing/dequant kernels.
Accumulation precision: routing (max /
expf/ sum / normalize / top-k)still runs in f32 in
qmoe_route; dequant uses f32 scales. No softmax/top-kis ever done in fp16. fp16→f32 widening is exact, so no numerical regression.
This is a strict superset of ORT's single-
Trule; the native backend is thesole validator (it skips ORT
Session::new).Verification (measured on CUDA)
qwen15-moe-qmoe-mobius(all fp16) loads + decodes coherent text natively:generated_text = ": I am trying to find the best way to get the current date in a format that is easy to read. I";generated_token_ids = [25, 358, 1079, 4460, 311, 1477, 279, 1850, 1616, 311, 633, 279, 1482, 2400, 304, 264, 3561, 429, 374, 4135, 311, 1349, 13, 358].qwen15-moe-qmoe-f32byte-identical: same greedy token ids from thepre-change control binary (69329c9) and the fixed binary.
enabled=true captures=1 replays=22 fallbacks=0 invalidations=0.widely across the full expert range (e.g.
{22,33,47,52},{0,19,41,42},{16,17,25,41}, …) — not collapsed to one expert.qmoe_gpusuite 33 passed / 0 failed / 0 ignored under--features cuda,gpu-tests, incl. a new all-fp16/bf16 regression(
qmoe_all_half_router_scales_bias_match_rounded_cpu, decode + prefill).cargo fmt -p onnx-runtime-ep-cuda -- --checkexits 0.Performance is out of scope and not measured (the fixture's throughput was a
cold-NVRTC-cache artifact).
Notes
member should review before merging.