Repository navigation
test(ep-cpu): assert placement honesty, not the spread policy, in the default sweep - #2034
Merged
Merged
Conversation
… default sweep `every_benchmarked_decode_width_realizes_the_worker_count_it_requests` asserted two different things under one name. One was honest and policy-neutral -- the pool places workers where it says it places them, which is the #1792 defect and holds under every policy. The other was `planned_distinct_cores == Some(true)` and `realized == "one-per-core"`: that is #1729's *spread* policy stated as a law, in a test that never asked for it. Encoding the policy as the contract inverts the argument the test exists to make, and it blocks #1802. 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 on an exclusive quiet host is not a valid default; spread measures ~26% worse with a single ~90% co-tenant. So whoever lands the co-tenancy-robust default hits this assertion, and the path of least resistance is to weaken it rather than fix its shape. What changes: * The sweep now asserts only claims that survive a policy change: the reported placement is the placement in force, the policy name is one this build can select, the realized-placement observation is switched on where the target supports it, and an unreadable mask is never scored as a placed worker. `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. * Placement becomes selectable: `ONNX_GENAI_CPU_DECODE_PLACEMENT=spread|compact` and `CorePlacement`. `order_pin_targets` keeps its signature and now delegates to `order_pin_targets_for(.., CorePlacement::from_env())`, so all four production call sites read one policy and cannot drift. **The default is unchanged** (`Spread`), which makes #1802's flip a one-line default change with a named alternative to flip to, rather than a rewrite of this test. Orthogonal to `ONNX_GENAI_CPU_DECODE_AFFINITY`, which chooses *which* CPUs the pool may use; this ranks a set already chosen. Both policies are permutations of their input, so no placement can widen or shrink what a cgroup allows. * Where spread is what is wanted, it is asserted explicitly and by name, in `an_explicit_spread_policy_places_one_worker_per_physical_core`. Mutations, because an assertion nothing can falsify is the thing being removed here, not the thing being added: * compact default -- `a_compact_policy_shares_cores_and_still_passes_every_policy_neutral_check` re-runs every policy-neutral claim the sweep makes against a compact pool. If any fails, the sweep is still encoding spread somewhere and #1802 is still blocked. Measured on a 4-core/8-thread cpuset: `realized=shared-core`, `placement=0`, `honest=1`, `pinned=1`. * explicit spread -- `the_spread_assertion_rejects_a_compact_placement` shows the one-per-core predicate can fail on this host today, so the explicit-spread test is not passing because `realized` is pinned to a constant somewhere. * the syscall-stub pin (`a_pin_that_reports_success_without_the_syscall_fails_the_realized_check`) and the dropped/altered assignment (`a_pool_that_misreports_its_placement_is_caught_end_to_end`) already existed and are re-run against the new assertions unchanged. * the selector itself: `compact_and_spread_are_permutations_that_differ_on_an_smt_host` fails if the knob parses and changes nothing, which is #1792's shape one level down. Unsupported platforms still skip, but only on compile-time target properties (`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. Refs #1805, #1802, #1792, #1729 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2034 +/- ##
==========================================
+ Coverage 80.32% 80.69% +0.37%
==========================================
Files 426 429 +3
Lines 204769 215097 +10328
Branches 204769 215097 +10328
==========================================
+ Hits 164483 173580 +9097
- Misses 34665 35746 +1081
- Partials 5621 5771 +150
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…e machine Review finding on #2034: the compact tests gated their layout assertion on `topology.has_smt()`, which is a property of the whole machine's sibling map, while whether `compact` can actually double up is a property of the *allowed set*. `taskset -c 24,26,28,30` on this SMT host leaves no sibling pair inside the cpuset, so compact and spread are the same layout there -- and the old gate demanded `shared-core` anyway. A false failure on a legitimate host, and the cheap way out of one is to weaken the assertion. Replaced with an exact prediction derived from the cpuset the child actually measured. `node_shards_with` orders the whole allowed set through `order_pin_targets_for` and then caps only the worker *count* (`reserve_single_group_headroom` returns a count; it does not remove CPUs from the list), and the pool pins worker `i` to `cpus[i % len]` -- so the first `workers` entries of that ordering are the CPUs the workers get. This is stronger than the assertion it replaces, not weaker: it asserts the exact code, so a compact pool that shares the *wrong* cores now fails too. It is not circular -- the prediction comes from the ordering policy, `realized` comes from each worker reading the mask the kernel is enforcing for it. `parent_cpuset_matching` asserts the child measured the same cpuset shape the parent sees before any prediction is trusted, so a divergence announces itself rather than silently grading another machine. The discrimination half (compact must not land in spread's layout) now runs only where the two policies actually predict different layouts, and says out loud when it does not. Verified both ways on this 16-core/32-thread host: * `taskset -c 24-31` (full sibling pairs): compact realizes `shared-core`, spread predicts `one-per-core`, discrimination half runs, all three tests pass. * `taskset -c 24,26,28,30` (no sibling pair): both policies predict `one-per-core`, the discrimination half and the mutation each print why they did not run, all three tests pass. The previous gate would have failed here. Also from the same review, both minor: * The five `decode_spmd` tests that assert the spread ordering now call `order_pin_targets_for(.., Spread)` rather than the wrapper, so they name the policy they assert instead of inheriting whatever `ONNX_GENAI_CPU_DECODE_PLACEMENT` happens to be set to in the test environment. * `order_pin_targets_for`'s "always a permutation" doc now states the distinct-input precondition (a repeated CPU de-duplicates, under both policies and before this PR too; the callers are cpusets and cannot contain one), and `explicit_affinity_shards`'s comment no longer says "the same spread the default path uses" now that the default path reads a selector. Refs #1805, #1802, #1792, #1729 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Second review pass on #2034 (Pris). Two findings that would actually bite, plus three sharpenings. **A check that could not fail.** The sweep asserted `matches!(placement_policy, "spread" | "compact")`. The child prints `CorePlacement::as_str()`, which is total over the enum, so that field is *always* one of those two and the assertion was decoration presented as a load-bearing check. It now asserts equality with the shipped default, which is a real claim: this arm removes `DECODE_PLACEMENT_ENV` from the child, so it fails if an ambient value leaks in or `from_env` reads the wrong variable, and it must be updated deliberately when #1802 flips the default. Verified by running the sweep with `ONNX_GENAI_CPU_DECODE_PLACEMENT=compact` set in the parent: the child still reports `spread`, so the removal is real. **A check that failed for the wrong reason.** `parent_cpuset_matching` hard-asserted that the child's cpuset shape matched the parent's. Both read `sched_getaffinity` at different instants, so a cgroup resize or a CPU hot-unplug between them panicked a healthy run -- an environmental event presented as a defect, and the cheapest resolution to that red is to delete the check. It is now a skip with both cpusets printed. It also compares the *set* rather than its cardinality, which is what the docstring always claimed: the child now reports its cpuset (`cpulist=`), because equal-sized but differently populated sets predict different layouts. **Coverage restored.** The removed one-per-core assertions ran at every benchmarked width; the replacement ran at one probe width, leaving a pool-side pinning defect that only appears past 8 workers unguarded -- placement wrong, honesty green, nothing red. `an_explicit_spread_policy_places_one_worker_per_physical_core` now sweeps `BENCHMARKED_DECODE_WIDTHS` intersected with the host's core budget, with a `checked > 0` guard so a sweep in which every width skipped is a failure rather than a pass. **`predicted_placement_code`'s degenerate branch returned `one-per-core`** -- the value that makes one caller's assertion pass and the other's mutation skip. Failure value equal to passing value, in the middle of a change about exactly that. It returns `UNPREDICTABLE_PLACEMENT` now, the compact test asserts it did not get it, and the mutation proceeds only on a positive `shared-core` prediction rather than skipping on `one-per-core`. **`CorePlacement::from_env` was untestable and untested.** Its `OnceLock` means whichever value the first caller in the binary sees is the only one any test could observe. The decision half is split out as `from_value` and unit-tested (`an_unparseable_placement_falls_back_without_going_silent`), and a child test covers the two ways this knob can be inert that a parser test cannot reach -- reading the wrong variable name, and swallowing the diagnostic: `a_misspelled_placement_falls_back_loudly_and_still_builds_a_pool` asserts the child falls back to the default, still builds a pool, and names both the variable and the rejected value on stderr. Disclosed rather than fixed, because the backstop already exists: an inert `compact` selector makes the two child-side discrimination checks *skip*, not fail, since both are derived from `order_pin_targets_for`. What catches that is the deterministic unit test `compact_and_spread_are_permutations_that_differ_on_an_smt_host`, which runs on a synthetic SMT topology on every host. Local, bounded to 8 CPUs: fmt, clippy `-D warnings`, aarch64 cross-clippy, and 1729 lib tests green. Re-verified on both `taskset -c 24-31` (full sibling pairs, discrimination half runs) and `taskset -c 24,26,28,30` (no sibling pair, both halves skip with printed reasons). Refs #1805, #1802, #1792, #1729 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
marked this pull request as ready for review
August 25, 2026 03:01
…lacement-honesty # Conflicts: # crates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs
Opus review of the merge resolution asked for this explicitly. The assertion is safe today because every placement policy orders pin targets as a permutation of the allowed CPUs, so on a leader-only cpuset compact and spread are indistinguishable -- the mask forces one-per-core, the policy does not choose it. Say so, and name the condition that would end it (an oversubscribing shared-core default), so the next reader gates the arm on the policy rather than deleting a width assertion that is policy-neutral. 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>
…lacement-honesty # Conflicts: # crates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs
This was referenced Aug 25, 2026
…lacement-honesty # Conflicts: # crates/onnx-runtime-ep-cpu/src/kernels/matmul_nbits.rs
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.
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_requestsasserted two different things under one name: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.rsmodule 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 (comment5384470084).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_checkwas 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|compactanddecode_affinity::CorePlacement.order_pin_targetskeeps its signature and delegates toorder_pin_targets_for(.., CorePlacement::from_env()), so all four production call sites read one policy and cannot drift apart.Spread). This PR does not flip anything; it makes CPU EP: #1729 made the spread placement the unconditional default, against a recorded correction that quiet-host-only policies are not valid defaults #1802's flip a one-line default change with a named alternative to flip 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 ortasksetallows.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.#1802cannot invalidate it: it may change what the default is, it cannot change whatspreadmeans.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.
a_compact_policy_shares_cores_and_still_passes_every_policy_neutral_checkworkers=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.the_spread_assertion_rejects_a_compact_placementrealizedis a constant somewhere.a_pin_that_reports_success_without_the_syscall_fails_the_realized_check(pre-existing,STUB_PIN_SYSCALL)a_pool_that_misreports_its_placement_is_caught_end_to_end(pre-existing,ONNX_GENAI_TEST_PLACEMENT_DISHONEST)compact_and_spread_are_permutations_that_differ_on_an_smt_hostPlus
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, andwithout_a_topology_every_policy_is_the_identity.pin_targets_are_ordered_one_per_core_then_siblingsnow 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 shapemainalready fixed for Linux topology detection viarequire_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,30is four cores with no sibling pair, so a machine-levelhas_smt()gate would have demanded a shared core the cpuset cannot produce and false-failed. The prediction is exact becausenode_shards_withorders the whole allowed set throughorder_pin_targets_forand caps only the worker count, so the firstworkersentries 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,realizedcomes from kernel affinity masks. Verified on both cpusets — on24-31the discrimination halves run, on24,26,28,30both 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 withmain)--all-targets -p onnx-runtime-ep-cpu✅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 thedecode_spmdcall sites nameSpreadexplicitly instead of reading the ambient policy.Pris (tester) found five things, all fixed rather than deferred:
matches!(placement_policy, "spread" | "compact")could never fail — decoration, not a check. It now asserts the policy equalsCorePlacement::default().as_str(), a claim that can fail.parent_cpuset_matchinghard-asserted, so a mid-test cgroup resize or CPU hot-unplug would red the test on an environmental event. It now returnsOptionand skips with both cpusets printed.BENCHMARKED_DECODE_WIDTHS ∩ [2, cores]plus the probe width, with achecked > 0anti-vacuity guard.predicted_placement_code's degenerate branch returned the passing value — a false oracle one level down. It now returns anUNPREDICTABLE_PLACEMENTsentinel 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
compactselector were ever inert, both child-side discrimination checks would skip rather than fail, because both derive fromorder_pin_targets_for. The backstop is the deterministic synthetic-topology unit testcompact_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
CorePlacement::from_env()isOnceLock-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 callorder_pin_targets_forwith the policy named.DecodeAffinity::Compactmeans single NUMA node — a different axis fromCorePlacement::Compact(SMT siblings before next core). Same word, orthogonal knobs; that is why this is a new enum rather than a newDecodeAffinityvariant.Refs #1805, #1802, #1792, #1729
Also in this PR: the rest of the Windows red
I found
mainred 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
expectonset_current_thread_affinity. That function is#[cfg(target_os = "linux")]; every other target gets a documented no-op returningErr. On Windows the guards above it all pass —allowed_cpus()answers viawindows_imp,require_host_for_placement()succeeds — so the child reached theexpectand died. It merged with both Windows lanes alreadyFAILURE. macOS was green only becauseallowed_cpus()returnsNonethere 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_cpusreturns whether it narrowed, and states a reason on every declining path;narrowed=1/0, and past the platform gate the parent asserts it unconditionally;AFFINITY_MASKING_SUPPORTEDindecode_affinity.rsrather than two copies ofcfg!(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.narrowedis reported rather than inferred fromallowed == coresbecause 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 == coresonly differs on an SMT host. The mutation below fires on a cpuset of[24, 26, 28, 30], whereallowed == cores == 4on the un-narrowed set and every assertion in the arm passes while testing nothing.Mutations, each simulating a fact this host cannot execute
false, masking worksset_current_thread_affinity succeeded on a target where AFFINITY_MASKING_SUPPORTED is falsetruerestrict the child to leader CPUs: … a failure rather than an absent capabilityfalse— Windows exactlySKIP …: process-wide CPU affinity masking is implemented only on Linux→okthe child must have narrowed itself … narrowed_to_leaders: false, allowed: 4, cores: 4The last row is the one #2078 cannot catch, and the one the extra field exists for.