Repository navigation
fix(metadata): ignore capabilities the decode path never exercises - #1715
Merged
Merged
Conversation
A package built by current mobius failed to load with
Invalid inference metadata: ["bounded_state_recurrence", "emit_valid_length",
"linear_effects", "loop_induction_values", "nested_control_flow",
"serving_service_contract", "typed_emit", "workflow_ssa"]
on a bare decoder that runs correctly once those declarations are removed. The
model was fine; the loader was refusing over workflow features it would never
reach.
`validate` folded two different questions into one Vec<String>. A structural
defect means the document does not describe a runnable model and nobody can
proceed. An unsupported capability means the package asks for a runtime feature
this build lacks, which only matters if the caller would exercise it. Merged,
every caller had to treat both as fatal.
Split them: `validate_structure_and_capabilities` returns a `CapabilityReport`
with the two kinds apart, and the engine fails on structural defects while
logging unsupported capabilities at info and continuing. `validate` keeps its
all-or-nothing contract for callers that want it.
Verified against the package that motivated this: the mobius-emitted workflow
metadata now loads and decodes (7.233 ms/token) where it previously failed at
load.
Note: `recurrent_state::replace_state_rejects_a_sequence_axis` fails on
origin/main before this change and is untouched here.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
==========================================
+ Coverage 79.70% 80.04% +0.33%
==========================================
Files 408 408
Lines 192418 192432 +14
Branches 192418 192432 +14
==========================================
+ Hits 153370 154025 +655
+ Misses 33707 33055 -652
- Partials 5341 5352 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
) ## What `cargo fmt --all` output. Three files, 4 insertions, 6 deletions, all whitespace and line wrapping. No semantic change. ## Why #1715 (`d3688e7e0`) landed with these three files unformatted. That turns two **required** checks red on `main` itself: - `Rust quality` — fails on `cargo fmt --all --check` - `Fast (Linux x86_64)` — same check, same three diffs Because they fail on `main`, they also fail on every PR branched from it, so no PR can currently show a green required set. Confirmed by running the check against pristine `origin/main` locally and by reading both job logs, which cite exactly: ``` crates/onnx-genai-engine/src/engine/load.rs:1172 crates/onnx-genai-metadata/src/lib.rs:54 crates/onnx-genai-metadata/tests/metadata_fixtures.rs:1106 ``` ## Scope Split out of #1714 deliberately. That PR is CPU-EP attention work and owns none of these files; unblocking `main` should not have to wait on an unrelated review, and an unrelated review should not have to carry someone else's formatting fix. `cargo check -p onnx-genai-metadata -p onnx-genai-engine` passes. Note: `Mobius metadata packages` is **also** red on `main`, for an unrelated reason (`validation/generated/diffusion` and friends fail pipeline-spec validation: `workflow image output 'image' must declare value_range`, and an `unknown field 'access'`). That one is a real content/schema mismatch rather than formatting, it is outside my lane, and it is **not** addressed here — it needs whoever owns the metadata schema.
justinchuby
added a commit
that referenced
this pull request
Aug 22, 2026
…retracts the 0.436x headline) (#1722) ## Summary The int4 decode A/B was **dividing two numbers that were not the same statistic**, and the mismatch was concentrated at exactly `sessions = 1` — the configuration the whole "acc0 single-session gap" conclusion rests on. This unifies the two arms, writes the definition down where it cannot drift again, and **retracts** the 24-cell matrix, the `0.436x` qwen `t=16 s=1` headline, and the "the gap is concurrency-dependent" reading posted to #1679 / #1676. ## The four biases | | native (before) | ORT (before) | |---|---|---| | denominator | wall included thread spawn + 3 warmup steps | warmup ran before `t0` | | session start | no barrier; staggered spawn absorbed into `wall` | `threading.Barrier` | | over repetitions | single shot | `min` (s=1) / `max` (s≥2) — the luckiest run | | **statistic** | wall-clock aggregate at every `s` | **`1000/median_ms` at s=1, wall-clock aggregate at s≥2** | The last row is the one that manufactures a result: the baseline switched from a **best-case** statistic to a **realistic** one at `sessions = 2`. A baseline that does that is guaranteed to look strongest at `sessions = 1` — which is precisely the shape that was reported as "we lose at one session and win at two and four". Separately, at `tokens = 24` the native warmup-inside-the-clock defect charged 27 steps of work against 24 counted tokens: a flat ~11% handicap the ORT arm never paid at any session count. Both sides now use one definition — numerator `sessions * tokens`; denominator wall from **barrier release** to last join; warmup **outside** the clock; **median** over repetitions — and both print `spread_%`. ## What the number actually is `qwen t=16 s=1 acc=0`, published as **0.436x**, reads **0.70x** under one definition. Six independent runs per arm show it cannot honestly be quoted more precisely than a **range**: ``` native 190.9 195.6 197.8 200.3 201.5 220.6 unimodal, ±8% ORT 218.3 229.8 246.2 396.9 414.9 427.7 two clusters, 1.79x apart ``` **0.436x was never a measurable quantity.** It is `max`-over-reps of ORT's fast cluster over a single-shot native run carrying an 11% handicap. ## Two conclusions deliberately *not* drawn - **"ORT is bimodal, so the anomaly is in the baseline."** Not supported. The slow cluster's intra-run spreads were 67.9% / 4.7% / 12.0% against the fast cluster's 1.4% / 1.1% / 0.7%. Elevated spread confined to the slow mode is the signature of **external contention**; a genuinely bimodal implementation would be stable in *both* modes. - **"The kernel is issue-bound at ~14% of FMA peak."** Directionally supported and probably right, but contention only *depresses* the measurement, so it is a **lower bound** — and a lower bound cannot establish distance from a ceiling. Recorded as a hypothesis to prove on a quiet host. ## What does survive Both arms eat the same contention, so the **within-window** comparison is valid. Interleaved native / ORT / native, native A/A partner at **1.018**: | arm | tok/s | achieved GB/s (same 145.7 MB/token footprint) | |---|---|---| | native acc0 | 196.0 | **28.5** | | ORT | 279.5 | **40.7** | **ORT sustains 1.43x our bandwidth on the identical footprint in identical conditions** — it *demonstrates* the bandwidth was available. So the deficit is real, and it is neither the memory system nor the busy host. Also: the **MLP-starvation hypothesis is provisionally falsified** — aggregate bandwidth is flat at ~22–28 GB/s across `s = 1, 2, 4, 8` rather than rising. Consequence worth stating plainly: our absolute throughput is **flat in session count**, so the `s=2`/`s=4` "wins" were the baseline degrading, not the kernel scaling. No kernel change should be justified by them. ## Why the measurement environment gets its own section Mid-investigation the host was found running, concurrently: another agent's `cargo test`/`llvm-cov` on this crate (~2470% CPU), **another agent's run of this same benchmark binary** (~1275% CPU), and a stray `while :; do :; done`. Peak load average **31.25** on 16 physical cores. The same cell measured **197.2 tok/s** in one window and **22.8 tok/s** in another — an **8.6x** environmental swing. The trap: several contaminated runs reported intra-run `spread_%` **under 6%**. A tight spread means the contention was *steady*, not that the host was idle. Intra-run spread is not a contention detector. `acc0_gap_matrix.py` therefore refuses to start a cell while any other process exceeds 150% CPU, and marks the cell `UNTRUSTED` rather than silently proceeding — because "give up after a timeout and measure anyway" is exactly how the bad numbers got made. ## Changes - `int4_decode_loop_ab.rs` — barrier; warmup outside the clock; `PROBE_REPS` with median; `spread_%`; the definition as a table in the module docs. - `ort_matmulnbits_baseline.py` — deleted `run_one`/`steady_median` (dead code computing a *different* statistic); all session counts through `run_concurrent`; `max` → `median`; `spread_pct`. - `acc0_gap_matrix.py` (new) — matrix driver under the single definition, per-cell interleaved A/A, tok/s → achieved GB/s, quiet-host gate with competing-process detection. - `docs/performance/CPU_MATMUL_ASSIGNMENT.md` — §27. - Three formatting-only hunks outside `benches/` are `cargo fmt` output for code that landed unformatted on main in #1715; without them the repo's fmt gate cannot pass. **No semantic change** — flagging separately since it means main's fmt gate is not currently enforcing. ## Validation No shipped code changes (benches are not part of the library). `cargo fmt --all --check` clean; `cargo clippy --all-targets -p onnx-runtime-ep-cpu -- -D warnings` clean. Full 21-gate matrix running; will report before undrafting. ## Follow-ups (not in this PR) - Re-run the matrix on a quiet host and publish the real ratios. - Prove or kill the unpack/convert issue-cost hypothesis in `borrowed_int4_nblock4_avx2` with a mechanism-isolating ablation. - Dispatch-width evidence handed to the runtime owner (total CPU-seconds flat across widths; `sys` time up ~20x from `t<=2` to `t>=4`) rather than tuned around in the kernel. Refs #1676, #1679, #1712 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 22, 2026
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.
Closes the second item in #1713.
A package built by current mobius failed to load on a bare decoder:
The model was fine — it decodes correctly once those declarations are removed. The loader was refusing over workflow features this decode path never reaches.
The conflation
validatefolded two different questions into oneVec<String>:Merged into one list, every caller was forced to treat both as fatal. That is why eight capability names were presented as if they were validation errors.
Change
validate_structure_and_capabilitiesreturns aCapabilityReportkeeping the two apart. The engine (both load sites) fails on structural defects and logs unsupported capabilities at info, then continues:validatekeeps its all-or-nothing contract, so strict callers are unchanged.Validation
unsupported_capabilities_are_reported_apart_from_structural_defectsasserts a well-formed document yields no structural defects, lists only the capability the runtime actually lacks, and that the strictvalidatestill errors.cargo test -p onnx-genai-metadata --test metadata_fixtures— 23 passed, 0 failed.Unrelated pre-existing failure
recurrent_state::replace_state_rejects_a_sequence_axisfails onorigin/mainbefore this change (verified by stashing). It is untouched here and needs its own fix.