Repository navigation
test(bench): count the nodes on our EP instead of asking whether any are - #2250
Merged
Merged
Conversation
The dispatch benchmark validated EP assignment with
assert!(info.ops_on_our_ep().contains(&case.op))
which is a membership test. It answers "is at least one node of this type
ours", and every per-node figure in #1077 divides a total by the case's
nominal depth. A hundred-node chain of which we claimed three passes that
assert, and nothing in the output says so.
Replaced with a count: `ours.len() == case.expected_ep_nodes && theirs
.is_empty()`, refusing to report a ratio otherwise. `expected_ep_nodes` is
`depth` for a chain and 1 for the single-node cases.
The first run found one:
grid_identity_10_static: expected all 10 node(s) on this EP,
got 1 ours ["Identity"] and 0 elsewhere []
Ten Identity nodes go in and ORT collapses the redundant chain during
session build, so our EP sees one. The row is kept, with its expectation
set to the observed 1, as a pin on that folding behaviour -- deleting it
would throw away the only place the repo notices ORT does this.
Per-case audit of the whole grid, one process each:
16 of 17 exact, only grid_identity_10_static folded.
grid_relu_100_tiny and grid_relu_1000_tiny -- the cases behind every
published #1077 number -- matched exactly.
So no published figure was wrong; the denominator was simply never checked,
and now it is. The in-code per-node probe was already honest: `report_probe`
divides by `ops_on_our_ep().len()`, not by the case definition.
Mutation proofs, both under the host lock:
M2 expected_ep_nodes = depth + 1 -> healthy grid_relu_10_tiny FAILS,
listing all ten Relus. The count is compared, not merely non-zero.
M3 restore contains-only -> grid_identity_10_static PASSES
again. The old check could not see a folded chain; this one is
load-bearing rather than decorative.
Validation: fmt clean, clippy clean, grid set 17/17 rows, default bench set
44/44 rows, full onnx-runtime-ep-cpu-plugin suite green.
Refs #1077
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby
enabled auto-merge (squash)
August 27, 2026 02:19
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2250 +/- ##
==========================================
+ Coverage 80.71% 81.04% +0.33%
==========================================
Files 433 433
Lines 221629 221629
Branches 221629 221629
==========================================
+ Hits 178889 179623 +734
+ Misses 36815 36086 -729
+ Partials 5925 5920 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
What
The dispatch benchmark's EP-assignment check was a membership test:
It answers "is at least one node of this type ours". Every per-node figure in #1077 comes from running a chain of N nodes and dividing by N, so the question that actually matters is "are all N of them ours" — and
containscannot tell 100 from 1.This replaces it with a count:
expected_ep_nodesis a new field on the bench case struct:depthfor a chain,1for the single-node cases.What it found on its first run
Ten
Identitynodes go in; one arrives. ORT collapses the redundant chain during session build, so our EP never sees the other nine. (That is the observation. Which ORT pass does it is not checkable from this tree, and the comment no longer claims it.)The row is kept, with its expectation set to the observed
1, as a pin on that folding behaviour — deleting it would throw away the only place in the repo that notices ORT does this. Its comment says plainly that it must not be read as a depth-10 point.Was anything published wrong?
No — and that is a measured answer, not a hoped-for one. Per-case audit of the whole grid, one process per case so a mismatch could not mask the rest:
grid_identity_10_staticonlygrid_relu_100_tiny,grid_relu_1000_tinyThe two
relu_*_tinyrows are the cases behind every Ir/node number in the #1077 ledger, including the −8.3% in #2240. Their denominators are now verified rather than assumed.The in-code per-node probe was already honest:
report_probedivides byops_on_our_ep().len(), not by the case definition, with a comment saying why. The hazard was a human dividing a total by a nominal depth — which is exactly what the offline callgrind harness does withDEPTH=100.Mutation proofs
Both under the mechanical host lock (#1806).
expected_ep_nodes: depth + 1grid_relu_10_tinyFAILS, listing all tenRelus — the count is compared, not merely required non-zero. Incidentally proves that case really has 10 nodes on our EP.contains-only semanticsgrid_identity_10_staticPASSES again and prints a ratio — the old check could not see a folded chain. The new one is load-bearing, not decorative.An earlier M3 attempt excised the block and did not compile, so its run used a stale binary; that run proved nothing and was redone as a one-line semantic swap. Recording it because a mutation that silently runs the wrong binary is the same class of error this PR is about.
Validation
cargo fmt --checkcargo clippy --release --all-targetsNXRT_MM_BENCH=1, no grid)onnx-runtime-ep-cpu-pluginsuiteplugin_ort_e2e58 passed / 1 ignored)The default-set run is there because the new assert is stricter than the old one and gates that set too — the reviewer pointed out my first audit only covered the opt-in grid.
Review
Independent adversarial Opus review, read-only, no cargo (shared host under lock). Verdict APPROVE WITH NITS. It independently verified every construction site's node count against the generated ONNX TextFormat, that no path reports a ratio without passing the assert, and that
theirs.is_empty()cannot fire spuriously underdisable_cpu_ep_fallback=1. Two findings, both addressed:SF-2 (the default-set gap) is closed by the run in the table above.
Scope
Test-only. No production file changes, no behaviour change, no runtime cost — the bench is
#[ignore]d and the assert runs once per case at session build.The same membership-vs-count vacuity exists in
assert_ops_assigned_to_our_ep, used by ~20 conformance tests. Deliberately not touched here: those are correctness tests where partial assignment is sometimes legitimate, and converting them needs its own audit. Noted as a follow-up candidate.Refs #1077