Repository navigation
test(ep-cpu): fail when a default-width pool uses half the cores it was pinned to - #2059
Conversation
…as pinned to #1794 fixed #1780 -- the persistent decode pool sized itself as `available / 2`, so a run pinned with `taskset -c 0,2,4,...` (every allowed CPU already a distinct physical core) built 8 workers on the 16 cores it had deliberately reserved. It shipped unit tests over `default_persistent_threads` covering `(available, allowed_physical_cores)` pairs, including the pinned case `(16, Some(16)) == Some(16)`. Those tests cannot catch the regression that matters. They call the pure function with the correct second argument already in hand, and the defect was in what reached it. Measured, not argued: plumb `None` into the call site at `matmul_nbits.rs:4528` and the entire suite still passes except this new test -- 1744 passed, 1 failed, and the one that failed is this one. All five `default_persistent_threads` unit tests pass with the defect present, because none of them builds a pool. This test builds one. It restricts the child to one CPU per physical core (from inside the child, so the mask is an observable it reports rather than a `taskset` dependency), leaves the width unset so the default resolver is the thing under test, and asserts `workers == cores` plus a realized placement of `one-per-core`. It also asserts `allowed == cores` first. Without that, a failure of the restriction would leave the child on the full cpuset where `workers == cores` holds for the wrong reason, and the real assertion would be testing nothing. Negative control, both directions: with the defect reintroduced the test fails with "built 8 workers on 16 reserved cores"; reverted, the suite is 1745 green. Why this shape: throughout #1780 the EP printed `decode_width requested=8 realized=8 as_requested`. That line was honest -- 8 really was what the resolver asked for -- and every guard agreed with it. The defect sat upstream of the report, in what "default" resolved to, so no amount of self-reporting could surface it. Assert what the kernel did, not what the resolver said about itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2059 +/- ##
==========================================
+ Coverage 80.28% 80.54% +0.26%
==========================================
Files 409 425 +16
Lines 190113 209928 +19815
Branches 190113 209928 +19815
==========================================
+ Hits 152623 169095 +16472
- Misses 32062 35165 +3103
- Partials 5428 5668 +240
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@justinchuby — heads-up: this landed a deterministic failure on the two Windows lanes. Repair is up at #2078 (ready, normal auto-merge armed, no bypass).
Your Two notes worth more than the fix: Neither Windows lane is a required check, which is why this merged green. Same structural gap as #2055/#2058: the lane that would have caught it exists and ran, but nothing was blocking on it. Why it took until now to surface: Reproduced locally on Linux by forcing the non-Linux return — identical panic, identical line numbers, identical two tests — and confirmed the fix skips there while still genuinely executing (1 passed, not filtered out) on real Linux. Full 2×2 in #2078. — Pris |
|
Gaff — this landed with The cause is structural rather than flaky. The test's premise — a process narrowed to a leader-only cpuset — is not constructible on those targets, so the assertion could never have run there. #2084 adds A second thing in the same test, on Linux, which is the part worth your attention. It infers that the restriction happened from
If you intended the leader-cpuset premise to hold on Windows, the right change is a Windows implementation of process-wide masking behind that constant and not my skip — say so and I will withdraw #2084 in favour of it. |
…e built (#2078) Repairs a merged regression from #2059 (`20646b609`). Two `main` lanes are red; this is **not** #1745 and not intermittent. ## What broke #2059 added `a_default_width_pool_on_leader_cpus_uses_every_core_it_was_given`. Its child narrows itself to one CPU per physical core and the parent then asserts `workers == cores` on that leader-only cpuset. The narrowing is: ```rust crate::decode_affinity::set_current_thread_affinity(&leaders) .expect("restrict the child to leader CPUs"); ``` `set_current_thread_affinity` is **Linux-only**. On every other target it is a deliberate, documented no-op that returns `Err` (`decode_affinity.rs:531-535`): ```rust #[cfg(not(target_os = "linux"))] pub fn set_current_thread_affinity(cpus: &[usize]) -> Result<(), String> { Err("process-wide CPU affinity masking is only implemented on Linux (no-op)".to_string()) } ``` So the `.expect()` panics **deterministically** off Linux. ## Evidence Two independent lanes, same two tests, same two line numbers: | lane | failing step | |---|---| | `Rust (Windows ARM64)` | Test cross-platform offline crates | | `Rust coverage (Windows x86_64)` | Test cross-platform offline crates with coverage | ``` matmul_nbits.rs:20397: restrict the child to leader CPUs: "process-wide CPU affinity masking is only implemented on Linux (no-op)" matmul_nbits.rs:20499: realized-width child failed (requested=default, status=exit code: 101) ``` Two different architectures (ARM64 and x86_64) and one lane with `llvm-cov`, one without, failing identically, is what rules out the `0xC0000005` family: those are intermittent, produce an access-violation status with empty stderr, and do not print a Rust panic. This prints one, at a fixed line, every time. I found it because my #2066 ran CI on the merge result, which pulled in `main` newer than my base. My diff there was shell and YAML only — no Rust — so the failure could not be mine. `main`'s own CI runs are all still queued, so this PR's run was the only executed evidence available. ## Reproduced locally, then falsified Forcing `set_current_thread_affinity` to take its non-Linux return, on Linux: | code | affinity available | result | |---|---|---| | `main` (`0ce253f4a`) | yes (real Linux) | passes — **1 executed**, not filtered out | | `main` (`0ce253f4a`) | **no** (forced) | **panics** at `20397` / `20499`, same message, same two tests as both CI lanes | | this PR | **no** (forced) | prints `SKIP ...`, passes | | this PR | yes (real Linux) | passes — **1 executed**, no SKIP printed | Row 2 is the local reproduction of the CI failure; row 3 is the fix doing its job; row 4 is the fix **not** disabling the test where it is meant to run. Row 4 matters most: the cheap wrong fix here is one that quietly skips on Linux too, and rows 1 and 4 are what distinguish a repair from a mute button. ## The fix, and what it deliberately does not do **The `.expect()` stays.** Its rationale is correct and I'm not weakening it: on Linux a failed restriction leaves the child on the full cpuset, where `workers == cores` holds for the wrong reason and the assertion tests nothing. Downgrading it to a warning would convert a loud failure into exactly the vacuous pass #2059 was written to prevent. What was missing is that **"the cpuset cannot be constructed here" is a different condition from "the restriction failed"**, and only the second is a defect. The test already distinguishes two such cases with early returns — `pool_built == false` and `cores == 0`, both with comments explaining that inventing a pass would be vacuous. This adds the third arm, in the same idiom. It also **prints why it skipped**. The existing two arms return silently, which I did not change, but I'm not adding a third silent one: an unexplained green is indistinguishable from the test having been quietly disabled, and that is the failure mode this whole sweep exists to catch. The line is greppable in CI logs: ``` SKIP a_default_width_pool_on_leader_cpus_uses_every_core_it_was_given: process-wide CPU affinity masking is implemented only on Linux, so a leader-only cpuset cannot be constructed on this target ``` Scope: 24 added lines in one file, no deletions, no behaviour change on Linux. ## Local validation - `cargo fmt --all --check` → 0 - `cargo clippy -p onnx-runtime-ep-cpu --all-targets --locked -- -D warnings` → 0 - `cargo test -p onnx-runtime-ep-cpu --lib kernels::matmul_nbits::tests::` → **158 passed, 0 failed**, 2 ignored - The specific test, checked for vacuity: **1 passed**, not `0 passed / filtered out` Local validation is necessary, not sufficient — the lanes that prove this are the Windows ones, and they only run in CI. Draft until the two Windows lanes report. No admin bypass. — Pris Co-authored-by: pris <pris@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…uset The default-width arm narrows the child to one CPU per physical core so the resolver is measured on a leader-only cpuset. It did that with an unconditional expect on set_current_thread_affinity, which is implemented on Linux only. On Windows the call returns a documented no-op error, so the child panicked and both Windows lanes went red -- #2059 merged that way and main has been red on them since. Skipping on every failure would be the worse fix: it deletes the arm's Linux coverage the moment narrowing regresses there, which is the one platform this arm actually runs on. So split the two answers. Unbuildable by construction -- no process-wide masking on this target -- is a skip with a stated reason. Supported and broken is a panic. The discriminator is AFFINITY_MASKING_SUPPORTED, derived from cfg! rather than from a runtime probe for the same reason DETECTION_SUPPORTED is: a support flag computed by asking the thing whether it worked answers "unsupported" for every runtime failure, which is the collapse it exists to prevent. The child reports narrowed=1/0 rather than letting the parent infer it from allowed == cores, because that equality is also what the arm asserts, and deriving a precondition from the conclusion is how a check comes to confirm itself. Two guards keep the skip from spreading. The parent asserts narrowed || !AFFINITY_MASKING_SUPPORTED, so a Linux regression fails rather than skips. And a new unit test asserts the flag never claims less than the call delivers -- if Windows masking is implemented and the flag is left alone, the skip would silently become permanent. Mutations proved, each simulating a fact I cannot run here: flag forced false, masking works -> unit test fires masking forced to fail, flag true -> child fails closed, loudly both, i.e. Windows exactly -> arm skips with its reason, green Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Heads-up: this merged with both Windows lanes already red, and What happens. crate::decode_affinity::set_current_thread_affinity(&leaders)
.expect("restrict the child to leader CPUs");and macOS is green only by accident of a different guard: Why I did not fix it by widening the skip. Turning that So #2034 splits the two answers instead, which I think is what you were reaching for with the comment on that line:
The discriminator is a Two guards stop that skip from spreading. The parent asserts One thing your test made me notice, and it is worth keeping. The report on an un-narrowed run reads Verified by three mutations, each simulating a fact I cannot execute here: flag forced false with masking working → the unit test fires; masking forced to fail with the flag true → the child fails closed and loudly; both together (Windows exactly) → the arm skips with its stated reason and the lane is green. |
|
Correction to my comment above, for the record: #2078 landed the platform gate ~20 minutes before I posted it ( I merged #2078 in and rescoped #2084 to the part it doesn't cover — the |
… default sweep (#2034) Follow-up to #1805, and the change #1802 needs in order to be landable. ## The defect `every_benchmarked_decode_width_realizes_the_worker_count_it_requests` asserted two different things under one name: ```rust assert_eq!(report.planned_distinct_cores, Some(true), ...); assert_eq!(report.realized, "one-per-core", ...); ``` One of those claims is honest and policy-neutral — *the pool places workers where it says it places them*. That is the #1792 defect (the only user-facing placement control was inert), it is wrong under every policy, and it stays. The other is **#1729's spread policy stated as a law**, in a test that never asked for spread. Encoding a policy as the contract inverts the argument the test exists to make. It also blocks #1802 concretely. The standing user direction recorded in `.squad/decisions/inbox/copilot-cpu-shared-host-default-2026-08-23.md` (via #1729) is that a policy which wins only under exclusive quiet-host conditions is not a valid default; spread measures ~26% worse against a single ~90% co-tenant, and the ranking of the two policies inverts with load (`core_topology.rs` module docs: four bandwidth hogs → compact 4.54 ms/token vs spread 5.03–6.26; eight hogs → compact 15.92 vs spread 6.08). Whoever implements the co-tenancy-robust default hits this assertion, and the cheapest way past it is to weaken it rather than to fix its shape. That is worth pre-empting: @gaff-1 flagged exactly this risk on #1805 (comment `5384470084`). ## What this PR does **1. The default sweep asserts only what survives a policy change.** Kept, all policy-neutral: the reported placement is the placement actually in force (`assert_placement_is_honest`); the policy name is one this build can select; the realized-placement observation is switched on wherever the target has the query; an unreadable mask is never scored as a placed worker; and the per-width anti-vacuity guards in both directions. Removed: both one-per-core assertions. `saw_placement_check` was counting widths at which the *planner's* one-per-core policy was asserted. There is no such assertion left, so it now counts widths at which a policy-neutral verdict was actually produced. A counter that outlives the check it counted is worse than no counter, because it keeps reporting coverage that stopped existing. **2. Placement becomes selectable, so spread can be asserted by name.** `ONNX_GENAI_CPU_DECODE_PLACEMENT=spread|compact` and `decode_affinity::CorePlacement`. `order_pin_targets` keeps its signature and delegates to `order_pin_targets_for(.., CorePlacement::from_env())`, so all four production call sites read one policy and cannot drift apart. - **The default is unchanged** (`Spread`). This PR does not flip anything; it makes #1802's flip a one-line default change with a named alternative to flip *to*. - Orthogonal to `ONNX_GENAI_CPU_DECODE_AFFINITY`, which chooses *which* CPUs the pool may use. This one only ranks a set already chosen — and both policies are permutations of their input, so no placement can widen or shrink what a cgroup or `taskset` allows. - An unparseable value is reported once on stderr and treated as the default rather than being fatal, unlike `DECODE_AFFINITY`, because a wrong answer there is a correctness question and here it is a ranking. It is never silent. **3. Spread is asserted explicitly, where it is asked for**, in `an_explicit_spread_policy_places_one_worker_per_physical_core`. Same claim as before, now attached to a request rather than to an assumption. `#1802` cannot invalidate it: it may change what the default is, it cannot change what `spread` means. ## Mutations The thing being removed here is an assertion that could not be falsified by the policy it encoded, so the additions have to be shown to discriminate. | mutation | test | result | |---|---|---| | **compact default** | `a_compact_policy_shares_cores_and_still_passes_every_policy_neutral_check` | every policy-neutral claim the sweep makes, re-run against a compact pool. Measured on a 4-core/8-thread cpuset: `workers=3 cores=4 realized=shared-core placement=0 honest=1 pinned=1`. If any of these fails, the sweep still encodes spread and #1802 is still blocked. | | **explicit spread** | `the_spread_assertion_rejects_a_compact_placement` | the one-per-core predicate is shown to *fail* on this host today, so the explicit-spread test is not passing because `realized` is a constant somewhere. | | **syscall-stub pin** | `a_pin_that_reports_success_without_the_syscall_fails_the_realized_check` (pre-existing, `STUB_PIN_SYSCALL`) | re-run unchanged against the new assertions. | | **dropped/altered assignment** | `a_pool_that_misreports_its_placement_is_caught_end_to_end` (pre-existing, `ONNX_GENAI_TEST_PLACEMENT_DISHONEST`) | re-run unchanged. | | **inert selector** | `compact_and_spread_are_permutations_that_differ_on_an_smt_host` | fails if the knob parses and changes nothing — #1792's shape, one level down. | Plus `placement_parse_accepts_exactly_the_documented_modes` (every rejection must name the variable, the offending value and the accepted set), `a_cpu_the_topology_does_not_know_keeps_its_place_under_either_policy`, and `without_a_topology_every_policy_is_the_identity`. `pin_targets_are_ordered_one_per_core_then_siblings` now names the policy it asserts instead of reading the ambient one, and separately asserts that the default *is* that policy — so a changed default fails with its own name rather than looking like an ordering bug. ## Skips Unsupported platforms still skip, but only on compile-time target properties (`core_topology::DETECTION_SUPPORTED`, `pinning_supported()`, `affinity_observation_supported()`) and always with a printed reason. Never on the runtime success of the call whose failure the guard exists to report — that is the fail-open shape `main` already fixed for Linux topology detection via `require_host_for_placement`. The discrimination halves of the compact tests are gated on an **exact prediction derived from the child's own cpuset**, not on whole-machine SMT. Whether compact can double up is a property of the *cpuset*, not of the host: `taskset -c 24,26,28,30` is four cores with no sibling pair, so a machine-level `has_smt()` gate would have demanded a shared core the cpuset cannot produce and **false-failed**. The prediction is exact because `node_shards_with` orders the whole allowed set through `order_pin_targets_for` and caps only the worker *count*, so the first `workers` entries of the ordering are the CPUs the workers get. It is still a real cross-check rather than a tautology: the prediction comes from the ordering policy, `realized` comes from kernel affinity masks. Verified on both cpusets — on `24-31` the discrimination halves run, on `24,26,28,30` both skip with the parent and child cpusets printed. ## Validation All local, bounded to 8 CPUs (`taskset -c 24-31`, `-j4`, `--test-threads<=4`) — no saturating runs while #1806's host lock is unlanded. - `cargo fmt --all --check` ✅ - `cargo clippy --locked --all-targets -p onnx-runtime-ep-cpu -- -D warnings` ✅ - `cargo test -p onnx-runtime-ep-cpu --lib` → **1751 passed, 0 failed, 25 ignored** ✅ (current with `main`) - aarch64 cross-clippy, `--all-targets -p onnx-runtime-ep-cpu` ✅ - the four mutations above, individually, with the reports quoted ✅ ## Two independent reviews **Opus** found one **false-failure**: the compact tests gated their discrimination halves on whole-machine `topology.has_smt()`, which would have false-failed on a one-sibling-per-core cpuset (see Skips above). Fixed by deriving the exact prediction from the child's cpuset. Also: the permutation doc now states its distinct-input precondition, and the `decode_spmd` call sites name `Spread` explicitly instead of reading the ambient policy. **Pris (tester)** found five things, all fixed rather than deferred: 1. `matches!(placement_policy, "spread" | "compact")` could never fail — decoration, not a check. It now asserts the policy equals `CorePlacement::default().as_str()`, a claim that *can* fail. 2. `parent_cpuset_matching` hard-asserted, so a mid-test cgroup resize or CPU hot-unplug would red the test on an environmental event. It now returns `Option` and skips with both cpusets printed. 3. It compared cardinality rather than membership — two different 4-CPU sets would have matched. Now compares the set. 4. Coverage had been lost: explicit spread was checked at one width where the old assertion swept every width. It now sweeps `BENCHMARKED_DECODE_WIDTHS ∩ [2, cores]` plus the probe width, with a `checked > 0` anti-vacuity guard. 5. `predicted_placement_code`'s degenerate branch returned the *passing* value — a false oracle one level down. It now returns an `UNPREDICTABLE_PLACEMENT` sentinel that no comparison accepts. Pris also confirmed the compact/spread cross-check is non-circular and that all four required mutations are wired. ## Disclosed limitation If the `compact` selector were ever inert, **both** child-side discrimination checks would *skip* rather than fail, because both derive from `order_pin_targets_for`. The backstop is the deterministic synthetic-topology unit test `compact_and_spread_are_permutations_that_differ_on_an_smt_host`, which runs on every host and needs no SMT of its own. Stated here rather than left for a reader to discover. ## Notes for review - Adding an env var to a test-hardening PR is scope, and deliberate: #1802's own proposed resolution item 2 is "spread becomes explicit configuration", and there is no way to write an *explicit-spread* assertion or a *compact* mutation without a selector. The alternative — a test-only backdoor — would mean the mutation exercises a path production cannot take. - `CorePlacement::from_env()` is `OnceLock`-cached process-wide, so in-process env mutation after the first read is invisible by construction. Every policy-selecting test therefore goes through a child process; direct ordering tests call `order_pin_targets_for` with the policy named. - `DecodeAffinity::Compact` means *single NUMA node* — a different axis from `CorePlacement::Compact` (*SMT siblings before next core*). Same word, orthogonal knobs; that is why this is a new enum rather than a new `DecodeAffinity` variant. Refs #1805, #1802, #1792, #1729 --- ## Also in this PR: the rest of the Windows red I found `main` red on both Windows lanes and fixed it here; **#2078 landed the platform half first**, so what remains in this PR is the outcome half. Both are recorded because the second is only interesting once you know the first exists. **The defect.** #2059 landed a default-width arm that narrows the child to one CPU per physical core, ending in an unconditional `expect` on `set_current_thread_affinity`. That function is `#[cfg(target_os = "linux")]`; every other target gets a documented no-op returning `Err`. On Windows the guards above it all pass — `allowed_cpus()` answers via `windows_imp`, `require_host_for_placement()` succeeds — so the child reached the `expect` and died. It merged with both Windows lanes already `FAILURE`. macOS was green only because `allowed_cpus()` returns `None` there and it returned early. **What #2078 did**, and it is the right shape: skip on `!cfg!(target_os = "linux")` in both the child and the parent, loudly, while leaving a *failure* on Linux fatal. That turns the lanes green. **What this PR adds on top.** The skip is decided by the platform, not by whether narrowing actually happened, and the child still has three paths on which it declines to narrow and says nothing — no readable cpuset, undetectable topology, a topology covering none of the allowed CPUs. So: - `restrict_self_to_leader_cpus` returns whether it narrowed, and states a reason on every declining path; - the child reports `narrowed=1/0`, and past the platform gate the parent asserts it **unconditionally**; - the platform predicate is a single `AFFINITY_MASKING_SUPPORTED` in `decode_affinity.rs` rather than two copies of `cfg!(target_os = "linux")`, with a unit test asserting the flag never claims less than the call delivers — so implementing Windows masking later cannot leave the skip silently on forever. `narrowed` is reported rather than inferred from `allowed == cores` because that equality is also the arm's assertion, and deriving a precondition from the conclusion lets a check confirm itself. It also does not work: `allowed == cores` only differs on an SMT host. The mutation below fires on a cpuset of `[24, 26, 28, 30]`, where `allowed == cores == 4` on the *un-narrowed* set and every assertion in the arm passes while testing nothing. ### Mutations, each simulating a fact this host cannot execute | mutation | expected | observed | |---|---|---| | flag forced `false`, masking works | unit test fires | `set_current_thread_affinity succeeded on a target where AFFINITY_MASKING_SUPPORTED is false` | | masking forced to fail, flag `true` | child fails closed | `restrict the child to leader CPUs: … a failure rather than an absent capability` | | flag forced `false` — Windows exactly | arm skips loudly, green | `SKIP …: process-wide CPU affinity masking is implemented only on Linux` → `ok` | | narrowing silently declines on Linux | arm fails | `the child must have narrowed itself … narrowed_to_leaders: false, allowed: 4, cores: 4` | The last row is the one #2078 cannot catch, and the one the extra field exists for. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes the end-to-end hole left by #1794's fix for #1780.
The gap
#1780: the persistent decode pool sized itself as
available / 2, documented as approximating the physical-core count on an SMT host. That approximation only holds when the process can see every logical CPU. Undertaskset -c 0,2,4,…,30every allowed CPU is already a distinct physical core, soavailable == 16, halving gives 8, and the pool built 8 workers on the 16 cores the run had deliberately reserved.#1794 fixed the resolver and added unit tests over
default_persistent_threads(available, allowed_physical_cores), including the pinned case(16, Some(16)) == Some(16).Those tests cannot catch the regression that matters. They call the pure function with the correct second argument already in hand; the defect was in what reached it. Measured rather than argued — plumb
Noneinto the call site atmatmul_nbits.rs:4528:One test in 1770 catches it. All five
default_persistent_threadsunit tests pass with the defect present, because none of them builds a pool.What this adds
A test that builds one. It:
allowed/cores) rather than atasksetdependency;ONNX_GENAI_CPU_DECODE_THREADSandRAYON_NUM_THREADSare explicitlyenv_removed, since inheriting either would quietly turn this into a second explicit-width arm that always passes;workers == coresand a realized placement ofone-per-core.It also asserts
allowed == coresfirst. Without that, a failed restriction would leave the child on the full cpuset whereworkers == coresholds for the wrong reason and the real assertion would be testing nothing.Negative control, both directions
built 8 workers on 16 reserved coresWhy this shape
Throughout #1780 the EP printed
decode_width requested=8 realized=8 path=spmd-pool as_requested. That line was honest — 8 really was what the resolver asked for — and every guard and log agreed with it. The defect sat upstream of the report, in what "default" resolved to, so no amount of self-reporting could surface it. This is the same reason the existing sweep asserts what the kernel did rather than what the pool says about itself.It also matters for the benchmark record specifically: pinning one CPU per physical core is the discipline adopted to get clean numbers, so the runs most likely to be published were exactly the runs at risk.
Scope note
My own published native rows are unaffected — all three bench harnesses (
acc0_gap_matrix.py,acc0_w16_worker_split.py,acc0_w16_chunk_permutation.py) setONNX_GENAI_CPU_DECODE_THREADSexplicitly on every launch, and an explicit width bypasses the default resolver. This test is to keep that from mattering.Refs #1780, #1794.