Repository navigation
cpu: report requested vs realized decode width so a t=N row can be verified - #1764
Conversation
… be verified
A published `t=N` decode row is a label, not a measurement. Nothing outside
`decode_spmd.rs` could check whether a requested width was the width that
actually ran, while at least three paths silently reduce it:
- `reserve_single_group_headroom` (single group, fully subscribed),
- `reserve_split_headroom` (same, per NUMA shard),
- the single-CPU cpuset branch of `build_from_env`, which declines the pool
and drops decode to the flat path entirely.
All three report only through `report_spmd_fallback`, which is
`tracing::debug!` / `NXRT_CALIB_DEBUG`-gated, so in a default benchmark run
they are invisible. A sweep across `ONNX_GENAI_CPU_DECODE_THREADS` therefore
prints a flat line at the bottom end that reads exactly like "this kernel does
not scale".
Adds `decode_width() -> DecodeWidth { requested, realized, path }`:
- Non-forcing. It reads `POOLS.get()`, not `pools()`, so asking cannot build
the pool as a side effect and cannot change which path the process takes.
- `is_as_requested()` is false unless *both* widths are known and equal, so a
harness that asserts too early fails rather than passing vacuously.
- `requested` is the *unclamped* `decode_thread_budget()`, not the width
`build_from_env` receives. The resolver already clamps to
`available_parallelism`, so recording its output would compare the reduced
width against itself and report every reduction as satisfied. This was a
real defect in the first draft of this change, caught by the mutation
below, and it is the whole point of the type.
Also sets the path label on three early returns that previously left it
`"unresolved"` -- the `=0` opt-out, the `Adaptive` + explicit-affinity branch,
and `total == 0`. Each is a decision that decode is flat, and leaving them
unresolved is the same silent-mislabel class this change exists to remove.
`active_decode_worker_count()` is not sufficient for this: it returns the Rayon
width when no pool is active, so it cannot distinguish "flat path, 16 Rayon
threads" from "SPMD pool, 16 workers", and it reports no requested width at
all. `pools()` is consumed externally but only as a boolean.
Tests. The end-to-end arm now goes through the production `pools()` entry
rather than constructing a pool beside it, so it checks the read a harness will
assert on instead of a parallel construction that could agree while production
disagrees. A new arm requests 8 lanes inside a 2-CPU cpuset -- the reduction
made deterministic -- and pins only that the read never claims the request was
honoured when it was not, leaving the realized width a policy detail. It skips
rather than fails where no process-wide affinity mask exists (non-Linux).
Verified by mutation, each killed by this suite and surviving without it:
- realized parroting requested,
- requested recorded post-clamp (the defect above),
- `is_as_requested` true when the width is unresolved.
Refs #1749.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#1765) `Mobius metadata packages` currently **cannot pass for anyone**, on any PR, regardless of content. ``` git fetch ... +refs/heads/justinchuby/catalogue-diffusion-builds*:... The process '/usr/bin/git' failed with exit code 1 (x3, then error) ``` `.github/workflows/mobius-producer-conformance.yml:28` checks out `onnxruntime/mobius` at `justinchuby/catalogue-diffusion-builds`. `git ls-remote` confirms that branch no longer exists. The job triggers on every `pull_request`, so it is a blanket false red — it is the **only** failure on #1747, and one of the reds on #1764 and #1729. ## Why now The step's own comment stated its exit condition: > `# Stacked with mobius#538 until its image/video workflow producer lands.` **mobius#538 merged today at 16:53 UTC** ("Support compact SD and complete video diffusion workflows") and its branch was deleted on merge. That is why this went red today rather than degrading gradually. The stacking condition is satisfied, so the pin comes off. ## Verified, not assumed I did not just swap the ref and call it fixed — pointing at a branch that lacks the producer would only trade a fetch failure for a script failure. - **mobius#538's merge commit `b9c90433` compares `identical` to `mobius/main`** — `ahead_by: 0`, `behind_by: 0`. So following `main` today fetches byte-equivalent content to what the deleted branch carried. This is a ref change, not a content change. - **All three paths the job consumes exist at `mobius/main`:** `tests/generate_onnx_genai_validation_packages.py`, `tests/compare_onnx_genai_validation_packages.py`, and the `tests/fixtures/onnx_genai_workflows` fixture directory. - YAML parses; the resolved ref is `main`. The real verification is this PR's own run of the job, since the failure is entirely in CI and cannot be reproduced locally. If it does not go green here, the diagnosis was incomplete and I would rather find that out on this PR than on someone else's. ## Tradeoff, stated rather than hidden `ref: main` floats. An upstream commit can redden this repo with no local change — the same class of externally-caused red I am removing here. A SHA pin would eliminate that, at the cost of the job no longer conforming against the *current* producer, which is the entire purpose of a producer-conformance check. Tracking `main` is the intent of the job, and its failure mode is an actionable red rather than a permanent one. If the upstream churn turns out to be noisy in practice, pinning-with-a-bump-process is the fallback, but that should be driven by observed churn rather than pre-emptively. ## Not addressed This does not touch the other reds on #1764 (inherited `E0599` from the `RegisteredMemoryContext::device` breakage, fixed on main by #1762 — those need a rebase) or #1729 (six genuine reds across Windows, macOS, and CUDA). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
…s realized Four fixes from adversarial review of the previous commit. `REQUESTED_WIDTH` is now latched in `pools()` rather than in `build_from_env`. `build_from_env` is `pub`, and latching there let a direct call record a requested width while leaving `POOLS` unbuilt -- so a later `pools()` could build a different width and `decode_width()` would report a request that no realized pool matched. That is the "t=N label is a lie" failure this read exists to catch, reintroduced through the front door. Latching where the pool is realized makes the two impossible to desynchronize. The child spawn now clears `ONNX_GENAI_CPU_DECODE_SCHEDULE`. `Command` inherits the parent environment, and the tests assert an exact path label, so an ambient `steal` relabelled the path `"work-stealing-pool"` and failed the assertion for a reason unrelated to decode width. Confirmed load-bearing: under `--features mlas` with the variable set, the arm fails without this change (`requested=Some(2) realized=Some(1) path=work-stealing-pool`) and passes with it. `DECODE_AFFINITY_ENV` was already cleared for the same reason; this closes the gap next to it. Two doc corrections. The `REQUESTED_WIDTH` comment claimed `None` under default policy and claimed the code did not use `threads`, but the fallback means a default run latches the resolved default and `is_as_requested()` is meaningful rather than trivially false -- which is the intent, so the doc was wrong, not the code. Also removed a stream-of-consciousness aside that read as an unfinished edit. Re-verified: all three mutants still killed (realized parroting requested, requested recorded post-clamp, `is_as_requested` true when unresolved). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…b/decode-width-report
Adversarial review (Opus) — verdictNo blocking issues. Four SHOULD-FIX, one NIT. All five addressed in The review independently traced the host-variation paths that could have made the new reduced-width arm vacuous — confinement landing on 1 CPU falling through to the flat path, Fixed1. 2. — and passes with it. (Worth noting for its own sake: 3 + 4. Two doc comments that disagreed with the code (SHOULD-FIX ×2). Both concerned the default-policy case. The 5. Stream-of-consciousness aside in the Confirmed sound, no change needed
Re-validation after the fixesAll three mutants still killed (realized parroting requested; requested recorded post-clamp; Latest |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1764 +/- ##
==========================================
+ Coverage 80.67% 80.71% +0.04%
==========================================
Files 411 411
Lines 198661 198803 +142
Branches 198661 198803 +142
==========================================
+ Hits 160267 160467 +200
+ Misses 32972 32912 -60
- Partials 5422 5424 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…d verdict Adversarial review caught a claim in the new doc that my own measurement in this branch had already refuted: `reserve_single_group_headroom` does not reduce the realized width. It reduces the *spawned thread* count, but it only runs in the single-group case, which is exactly where `dispatcher_owns_a_shard` (`shards.len() == 1 && shards[0].workers < requested`) is true, so `total_workers` adds the lane straight back. Measured: a 2-lane budget on a 2-CPU cpuset spawns one thread and realizes two lanes, `as_requested`. The genuine net reducers are three, not four: the pre-clamp to `available_parallelism`, `reserve_split_headroom` (NUMA-split only, and uncompensated because the dispatcher shard is single-group-only), and the single-CPU-cpuset fallback. Also corrects the same list in the shipped `DecodeWidth` doc from #1764, which had the mirror-image error: it named both headroom reservations, omitted the pre-clamp, and claimed all three report through `report_spmd_fallback`. Only the cpuset fallback does -- neither headroom function logs anything at all (call sites are :569, :1619, :1642, and neither :1749 nor :1763 is among them). Splits `WIDTH-MISMATCH` into `WIDTH-MISMATCH` (both widths known, unequal -- the row's label is wrong) and `WIDTH-UNRESOLVED` (never resolved -- no decode reached the pool). A matrix script wants to treat those differently. Verified by execution, all three verdicts: t=4 plain -> requested=4 realized=4 spmd-pool as_requested t=8 under taskset -c 0,2 -> requested=8 realized=2 spmd-pool WIDTH-MISMATCH t=1 plain -> requested=1 realized=1 flat as_requested The mismatch arm is the pre-clamp reducer caught in the act. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #1763.
A published
t=Ndecode row is a label, not a measurement. Nothing outsidedecode_spmd.rscould check whether the requested width was the width that actually ran, while at least three paths silently reduce it —reserve_single_group_headroom,reserve_split_headroom, and the single-CPU cpuset branch ofbuild_from_envthat drops decode to the flat path. All three report only throughreport_spmd_fallback, which istracing::debug!/NXRT_CALIB_DEBUG-gated and therefore invisible in a default benchmark run.Found by roy while chasing an unrelated flat
t=1vst=2timing.What this adds
Three properties are load-bearing rather than incidental:
It cannot build the pool.
decode_width()readsPOOLS.get(), notpools(). A diagnostic that forces the thing it measures would change which path the process takes and when — the read has to be inert.It cannot pass vacuously.
is_as_requested()is false unless both widths areSomeand equal. A harness that asserts before the first decode step fails, rather than quietly reporting success against an unresolved state.requestedis the unclamped request. This one was a genuine defect in my first draft, and it is worth being explicit about because it is the exact bug the type exists to prevent.resolve_persistent_decode_threads_with_overridealready clamps toavailable_parallelism(), so recording the widthbuild_from_envreceives would compare the reduced width against itself — a request for 8 lanes inside a 2-CPU cpuset would reportrequested=2, realized=2, satisfied. It now recordsdecode_thread_budget(), which is what the user actually asked for. The mutation suite below is what caught it; the first version of these tests passed happily with the bug in place.Also fixed
Three early returns previously left
DECODE_PATH_LABELat"unresolved"when decode was definitively flat — the=0opt-out, theAdaptive+ explicit-affinity branch, andtotal == 0. Each is a decision, not an unresolved state, and leaving them unlabelled is the same silent-mislabel class this PR removes.Why not the reads that already exist
active_decode_worker_count()returns the Rayon width when no pool is active, so it cannot tell "flat path, 16 Rayon threads" from "SPMD pool, 16 workers", and reports no requested width.decode_path_label()carries the path but not the width.pools()is consumed externally, but only as a boolean.Tests
The existing end-to-end arm now goes through the production
pools()entry instead of callingbuild_from_envbeside it. That matters: the previous form built a local pool, so it verified a parallel construction that could agree while production disagreed.A new arm requests 8 lanes inside a 2-CPU cpuset — the reduction made deterministic — and asserts only that the read never claims the request was honoured when it was not. The realized width is deliberately not pinned; that is a policy detail, and pinning it would make this test a change-detector for tuning decisions it has no opinion about. It skips rather than fails where no process-wide affinity mask exists (non-Linux).
Verified by mutation. Each mutant is killed by this suite and survives without it:
realizedparrotsrequestedrequestedrecorded post-clamp (the defect above)is_as_requested()true when the width is unresolvedObserved on this host:
requested=8in a 2-CPU cpuset →allowed=2 realized=Some(2) path=spmd-pool,is_as_requested() == false. Unconfined budgets 2/4/8 →requested == realized == budget,path=spmd-pool, withbudget-1spawned threads plus the dispatcher's shard (the #1746 invariant).Not in scope
No behaviour change to dispatch, sizing, or affinity policy — this PR only makes the existing outcome observable. No performance claim; nothing here is on a decode hot path.
@roy offered to take the harness-side assertion once this lands, which is the natural split.
Validation
cargo test -p onnx-runtime-ep-cpu --lib— 1628 passed, 0 failed (1624 on main + 4 new)offline-linuxpackage set — exit 0, 233 suites okcargo fmt --all --check— cleancargo clippy -p onnx-runtime-ep-cpu --all-targets -D warnings— clean, also under--features mlasand--no-default-features--target aarch64-unknown-linux-gnuclippy--all-targets— cleanRust qualityscripts — pass