Skip to content

Detach C++ node tests from ONNX on-disk data/node artifacts (onnx#7959) + retiring equivalence proof + corpus-collapse tripwire - #29504

Merged
titaiwangms merged 16 commits into
mainfrom
node-test-detach-from-onnx
Jul 16, 2026
Merged

Detach C++ node tests from ONNX on-disk data/node artifacts (onnx#7959) + retiring equivalence proof + corpus-collapse tripwire#29504
titaiwangms merged 16 commits into
mainfrom
node-test-detach-from-onnx

Conversation

@titaiwangms

@titaiwangms titaiwangms commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Motivation

ONNX onnx/onnx#7959 removes the on-disk onnx/backend/test/data/node/ corpus (targeted for ONNX 1.23), replacing it with on-the-fly in-memory generation. ORT's C++ onnx_test_runner reads that corpus from disk — after the deletion it would load 0 node tests and silently exit 0 (green CI on a corpus that no longer exists). This PR detaches ORT from the deleted artifacts ahead of the bump.

What this does (3 pieces)

  1. Detach — a build-time materializer (tools/python/materialize_onnx_node_tests.py) regenerates the node corpus to disk from ONNX's surviving generator (collect_testcases), so the C++ runner keeps reading from disk unchanged. EP-agnostic (CPU/CUDA/QNN share one materialized tree).
  2. Equivalence proof — a retiring test (onnxruntime/test/python/onnx_node_test_equivalence_test.py + tools/python/compare_node_test_corpora.py) proves the materialized corpus is byte-identical to the original (modulo a documented ULP band); it auto-skips once the ONNX on-disk oracle disappears post-Add transformers tool test to pipeline #7959.
  3. Cause-agnostic tripwire — build-time --min-cases gate (FATALs the build on every MATERIALIZE=ON leg if generation < floor) + a runtime -m floor on the CPU node ctest, turning silent-green-on-empty into a loud red.

Key decisions

  • numpy: cmake configure = HARD FATAL on off-pin numpy (build-env reproducibility across CI legs); Python materializer = SOFT WARN (standalone advisory). Different layers, different questions. The onnx pin stays HARD on both sides.
  • QNN legs: repointed to the materialized tree + pinned onnx/numpy installed pre-build. Node-dir runs carry a low -m 1 collapse sentinel (NOT the 1500 build floor): -e qnn legitimately reduces the collected set to ~1529, so a 1500 runtime floor would be a false-red timebomb on the next opset bump — the strict count is enforced at build time via --min-cases, while -m 1 (zero false-red risk) catches a per-leg MATERIALIZE=OFF that still runs the node dir. The android leg's single-case cp -r source is repointed to the materialized tree (the old data/node source vanishes post-Add transformers tool test to pipeline #7959).

Testing

  • Runtime tripwire empirically verified: empty/truncated corpus → runner FATAL (nonzero); full → exit 0; default (no -m) unchanged.
  • Equivalence: 1799-case byte-probe (dir-set match; byte-identical modulo the documented ULP band).
  • cmake configure verified to generate cleanly on latest main (ep_context + node-test regions coexist, no collisions).
  • [needs-run] on a pinned-numpy CI box: the MATERIALIZE=ON inner path (actual materialization + the two add_test bodies) — this dev box is off-pin so the numpy gate FATALs by design.

Relates to: onnx/onnx#7959


Cross-consumer viability note (re: onnx/onnx#7959)

While detaching, we assessed all of ORT's node-test consumers. The in-memory generator approach behind #7959 works cleanly for single-version consumers (C++, C#, docs), but has a gap for consumers needing a historical multi-opset matrix:

Net: workable for single-version consumers; the multi-version case would need a small per-version serialization utility or a documented migration note upstream.

titaiwangms and others added 5 commits July 2, 2026 22:28
…ence test

Detach ORT's node-test coverage from the on-disk ONNX corpus
(onnx/backend/test/data/node) that ONNX PR #7959 deletes, by regenerating it
from ONNX's in-memory Python generators instead.

- tools/python/materialize_onnx_node_tests.py: build-time generator that calls
  ONNX's collect_testcases and writes the node-test corpus (model.onnx +
  test_data_set_*/*.pb) into the build tree, with onnx/numpy version guards and
  a byte-equivalence comparator against the on-disk oracle.
- tools/python/compare_node_test_corpora.py: standalone oracle-vs-materialized
  equivalence comparator, reusable outside cmake.
- onnxruntime/test/python/onnx_node_test_equivalence_test.py: retiring unittest
  that materializes to a tmpdir and byte-compares against the on-disk oracle,
  auto-skipping once the pin advances past #7959.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…runner

Add a -m/--min_cases floor to onnxruntime/test/onnx/main.cc: after loading the
test corpus, if the collected case count is below the floor, exit non-zero. This
catches a silently empty/partial/truncated materialized node corpus at the
consumption point (onnx_test_runner skips non-existent dirs and would otherwise
exit green with near-zero coverage). Names ONNX #7959 in the failure message.

Mirror the same floor in onnxruntime/test/python/onnx_backend_test_series.py
(MIN_NODE_CASES) so the Python-driven node series has the equivalent guard.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ts into cmake

Add the onnxruntime_MATERIALIZE_ONNX_NODE_TESTS option (cmake/CMakeLists.txt),
ON by default when building unit tests (non-reduced, non-minimal builds).

In cmake/onnxruntime_unittests.cmake:
- Fail the configure step loudly with an actionable message if onnx/numpy are
  missing (never silently skip materialization).
- Add the onnx_node_tests_materialized build target that runs the materializer
  with --min-cases (build-time GENERATION floor).
- Register the onnx_test_node_materialized ctest (onnx_test_runner -m, the
  CONSUMPTION-point floor) and the auto-retiring onnx_node_tests_equivalence
  ctest (gated on the on-disk oracle existing at configure time, so it stops
  registering cleanly once the pin advances past #7959).
- Single-source both floors via ORT_ONNX_NODE_MIN_CASES.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lized corpus

Repoint the QNN CI node-test consumers from the on-disk ONNX corpus
(cmake/external/onnx/.../data/node) to the build-tree materialized copy
(onnx_node_tests/node), and install onnx==1.22.0 + numpy (python-conditional
pins matching the CMake gate's assertion) before the build so the host-side
materializer can run.

Covers linux-qnn, win-qnn-arm64, windows_qnn_x64, and the android-arm64 QNN
cross-compile pipeline (whose single-case copy source is repointed too). No -m
floor is added to the QNN legs: they build in-place on the same tree the
build-time --min-cases gate already guards, so the global corpus-collapse
regression is caught PR-wide by that gate plus the CPU node ctest -m backstop.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…set-bump checklist

Document in the onnx-opset-bump-checklist skill that ONNX PR #7959 deletes the
on-disk node-test corpus, that ORT now materializes it from ONNX's Python
generators at build time, and how the build-time --min-cases gate plus the
consumption-point -m tripwire guard against a silent coverage collapse when the
ONNX pin advances past #7959.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread onnxruntime/test/python/onnx_node_test_equivalence_test.py Fixed
Comment thread tools/python/materialize_onnx_node_tests.py Dismissed
Comment thread tools/python/materialize_onnx_node_tests.py Dismissed
…visioned legs

The materialize option defaulted ON on every unit-test build, but only the QNN
legs install onnx, so the configure-time HARD-FATAL gate broke every other
unit-test CI leg with `ModuleNotFoundError: No module named 'onnx'`.

Flip the cmake_dependent_option default to OFF (explicit opt-in) and keep the
HARD-FATAL gate unchanged so it still fires loudly when the option is explicitly
set ON but onnx/numpy are missing or off-pin. With the default OFF, non-opted-in
legs revert to exactly their pre-PR behavior (node tests read from the
FetchContent'd ONNX source tree, which still ships data/node on the pinned
version) -- a zero-regression no-op today.

Enable materialization explicitly via
`--cmake_extra_defines onnxruntime_MATERIALIZE_ONNX_NODE_TESTS=ON` on the legs
that consume the materialized corpus and are provisioned with the pinned
onnx==1.22.0 + numpy at CONFIGURE-time:
- the 3 QNN legs (linux-qnn, win-qnn-arm64, windows_qnn_x64), which pip-install
  the pins in a dedicated step before build;
- the android-arm64 QNN cross-compile leg, whose single-case `cp` reads from the
  materialized tree (it also pip-installs the pins pre-build);
- the Linux CPU release leg build-linux-x64-release-py314, whose configure
  interpreter is /opt/python/cp314-cp314 -- cp314 is one of the interpreters that
  the manylinux install_deps.sh provisions with requirements.txt (onnx==1.22.0 +
  numpy==2.4.2 for py>=3.11, matching the gate's pins), and the leg runs tests,
  so the equivalence + materialized-node ctests actually execute in upstream CI.
  (The cp310 build-linux-x64-release leg is deliberately NOT opted in: cp310 is
  not in install_deps.sh's provisioned interpreter list, so its configure python
  would fail the onnx gate.)

Also correct a now-stale comment in onnxruntime_unittests.cmake that still said
the option defaults ON.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prepares ONNX Runtime’s C++ ONNX node-test coverage for ONNX PR onnx/onnx#7959, which removes the on-disk onnx/backend/test/data/node/ corpus. It introduces a build-time materialization step to regenerate the node-test artifacts to disk (so onnx_test_runner can keep consuming a directory), adds an equivalence oracle that auto-retires when the upstream oracle disappears, and adds “silent-green” collapse tripwires (build-time and run-time) across CMake, C++ runner, and Python test discovery.

Changes:

  • Add Python tooling to materialize ONNX node tests from ONNX generators and (optionally) byte-compare vs the pre-#7959 on-disk oracle.
  • Add CMake option + build/test wiring to generate the node corpus into the build tree, run it via onnx_test_runner, and (while available) run an equivalence ctest.
  • Add “minimum discovered cases” tripwires to prevent silent pass on empty corpora; update QNN CI legs and Linux CI to consume the materialized tree.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tools/python/materialize_onnx_node_tests.py New materializer + oracle comparator core for node-test corpus regeneration and equivalence classification.
tools/python/compare_node_test_corpora.py Build-wired wrapper to compare an already-materialized corpus vs the on-disk ONNX oracle.
tools/ci_build/github/azure-pipelines/win-qnn-arm64-ci-pipeline.yml Installs pinned onnx/numpy, enables materialization, repoints QNN node-test run to materialized tree with -m 1 sentinel.
tools/ci_build/github/azure-pipelines/linux-qnn-ci-pipeline.yml Same as Windows QNN: install deps, enable materialization, repoint node-test run + sentinel floor.
tools/ci_build/github/azure-pipelines/android-arm64-v8a-QNN-crosscompile-ci-pipeline.yml Enables materialization on host; repoints the single copied node test to materialized tree.
onnxruntime/test/python/onnx_node_test_equivalence_test.py New retiring Python unittest to materialize into a tmpdir and compare vs oracle when present.
onnxruntime/test/python/onnx_backend_test_series.py Adds a Python discovery-count floor to fail loud if node tests disappear from disk.
onnxruntime/test/onnx/main.cc Adds -m minimum-tests option to onnx_test_runner to prevent silent-green on empty dirs.
cmake/onnxruntime_unittests.cmake Adds onnxruntime_MATERIALIZE_ONNX_NODE_TESTS wiring: configure-time dependency gating, build-time materialization target, ctests, and oracle retirement.
cmake/CMakeLists.txt Introduces the new opt-in dependent CMake option for node-test materialization.
.github/workflows/windows_qnn_x64.yml Installs pinned onnx/numpy, enables materialization, repoints node-test run to materialized tree + -m 1 sentinel.
.github/workflows/linux_ci.yml Enables materialization on a Linux CPU CI leg to exercise the build/test wiring.
.agents/skills/onnx-opset-bump-checklist/SKILL.md Documents the onnx#7959 “silent-green” landmine and mitigation guidance in the opset bump checklist.

Comment thread tools/python/materialize_onnx_node_tests.py
Comment thread tools/python/materialize_onnx_node_tests.py Outdated
Comment thread cmake/onnxruntime_unittests.cmake
@titaiwangms

Copy link
Copy Markdown
Contributor Author

Review synthesis (multi-model review team)

Strong, well-engineered PR. An end-to-end run of the materializer confirmed 1799 cases, byte-identical to the on-disk oracle, the 6 inverse-trig ULP cases reproduced exactly, and the writer is a faithful verbatim transcription of ONNX's cmd_tools.py::generate_data. One CI-breaking bug should be fixed before merge.

🔴 Critical (verified against ONNX source — blocks merge)

onnxruntime/test/python/onnx_backend_test_series.py — the new MIN_NODE_CASES gate fires on every full run.
ONNX's Runner calls _add_model_test(rt, "Node") with a capital "N" (onnx/backend/test/runner/__init__.py:82), but the override checks if kind == "node" (lowercase). _node_case_count therefore stays 0, so 0 < 1500 raises RuntimeError("discovered only 0 …") on every full backend-series run.
Fix: check model_test.kind == "node" (the TestCase.kind attribute is lowercase), or compare case-insensitively.

🟠 Major

  1. Unwired consumers still read the on-disk data/node corpus — the PR repoints the QNN/CPU legs but misses:
  2. Post-retirement schema-drift is caught by nothing. Once the equivalence oracle retires (post-Add transformers tool test to pipeline #7959), only the version-string gate + the count floor remain — neither detects a wheel whose compiled schema registry bakes a wrong opset (string matches, count unchanged). Acceptable given CI pip-pin discipline, but worth stating explicitly, or adding a cheap since_version cross-check anchor.

🟡 Minor

  • min_cases checked before the kind == "node" filter (tools/python/materialize_onnx_node_tests.py) — the floor validates the collected count, not the written node count. Benign for 1.22.0 (all 1799 are node), but violates the guard's own invariant; check len(written).
  • CMake STREQUAL vs Python _release_base — CMake requires an exact onnx version-string match and FATALs before the script runs, while the Python side intentionally accepts rcN/.dev of the same release base. Inconsistent policy on opset-bump / pre-release workflows.
  • The 1500 floor is triple-duplicated — CMake single-sources its own 3 consumers from ORT_ONNX_NODE_MIN_CASES, but the two Python MIN_NODE_CASES literals are independent, coupled only by a comment. "Move in lockstep" is a convention, not a mechanism.
  • _max_ulp_diff float16/bfloat16 branch — the uint16 sign-remap is dead code (values are always ≥0), and bfloat16 never reaches it (np.issubdtype(ml_dtypes.bfloat16, np.floating) is False → classified Class B first). Safe direction, but the docstring claiming bfloat16 is ULP-handled is misleading.
  • Stale-corpus risk on incremental buildsadd_custom_command DEPENDS only on the script + cmake/deps.txt, not the onnx wheel; reinstalling a different wheel without touching deps.txt leaves .stamp fresh and reuses the old corpus. Low impact in clean-build CI.

Nits

  • ctest DEPENDS uses a target name (not a test name → no real runtime ordering; benign since the target is ALL).
  • Dead _onnx_url_ver variable (CMAKE_MATCH_1 is used directly).
  • C++ FATAL message cites "gotcha (p)" without the file path — add .agents/skills/onnx-opset-bump-checklist/SKILL.md.
  • Duplicated "do not rename" comments on the input/output loops.
  • Hardcoded onnx==1.22.0 / numpy pins in the ymls are drift-prone vs cmake/deps.txt + requirements.txt (verified they agree today).

Recommendation: fix the Critical ("Node" capitalization) — a one-liner that unblocks the Python leg — and address the Major unwired-consumer gaps (at least C# + the docs 404), since they undercut the "detach ahead of the bump" goal. The rest are polish/hardening.

🤖 Generated with multi-model review (readability / code / critical / deep / integration).

titaiwangms and others added 5 commits July 3, 2026 00:31
…N/android legs (f2py perms)

The android + linux-qnn legs run on bare non-root Ubuntu agents using the
system python3 (no venv). A plain 'pip install onnx==1.22.0 numpy' tries to
write /usr/local/bin/f2py -> [Errno 13] Permission denied. --user installs into
~/.local for the same agent user; the configure-time 'import onnx' (build.py)
runs as that same user with the same python3, so it resolves from ~/.local.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s floor

onnx 1.22.0's Runner.__init__ dispatches the node corpus to _add_model_test
with the capitalized category label "Node", not the lowercase load kind. Our
MIN_NODE_CASES twin counted 'if kind == "node"', which never matched, so
_node_case_count was always 0 and the floor false-fired on every full
onnx_backend_test_series.py run (reds every CPU full-backend leg). Compare
case-insensitively. Verified against onnx 1.22.0: healthy corpus tallies 1799
(>= 1500, satisfied); a genuinely empty corpus still tallies 0 (< 1500, floor
still fires) -- the guard is corrected, not disabled.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lowercase block-local temps

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… + image_decoder equivalence class + narrow bare-except

Agent-signed-off: Architect (dae3a478) [claude-opus-4.8 via copilot]
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… doc comments + register pin sites & remaining 1.23 consumers in bump checklist

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@titaiwangms

Copy link
Copy Markdown
Contributor Author

Re-review of the update

Verified the pushed fixes against source (and empirically for the numerical one). The blocking Critical is resolved and the core fixes are correct.

✅ Verified fixed

  • 🔴 Critical — "Node" capitalization: now kind.lower() == "node" (onnx_backend_test_series.py:83). ONNX's Runner passes the capitalized display category "Node", so the old == "node" kept the counter at 0 and would have raised RuntimeError("discovered only 0") on every full backend-series run. Fixed.
  • 🟡 min_cases counts written node cases: floor moved after the kind != "node" filter → if len(written) < min_cases (materialize_onnx_node_tests.py:279).
  • 🟡 CMake onnx gate is now RC/.dev-aware: regex-strips to X.Y.Z base, mirroring the Python _release_base (onnxruntime_unittests.cmake:1585) — no longer stricter than the materializer.
  • 🟡 ULP signed-zero / overflow: reworked to an unsigned sign-magnitude fold in Python-int (object) domain. Empirically confirmed: ±0.0 → 0 ULP (fp32 & fp64), adjacent → 1 ULP, neg/pos crossing → 2 ULP, bfloat16 → out-of-band sentinel 5 (>4 ⇒ forced Class B). Correct.
  • Bonus follow-through (TestCase.cc): the new CPU onnx_test_node_materialized ctest now runs node tests on the CPU EP (previously QNN-only), so convinteger_with_padding + dft_(i)rfft* are correctly hoisted into the common GetBrokenTests initializer, mirroring the Python filter exclusions. Sound.

⚠️ Detach completeness — 3 non-C++ consumers still read the on-disk data/node (for a follow-up)

The C++ detach (the PR's stated scope) is complete and well-guarded. These remaining on-disk consumers were not migrated and leave the same silent-green landmine on other language legs. All are latent (fire only once the onnx pin/submodule/master actually passes #7959), so not blockers for this PR — flagging for a follow-up:

Consumer Fires when Impact
docs/python/conf.py:124 onnx master merges #7959 (independent of ORT's pin) URL hardcodes master; urllib.request.urlretrieve on a 404 raises HTTPError and is not wrapped in try/except → Sphinx docs build hard-fails. Fires soonest — recommend pinning the URL to a fixed tag even in this PR.
csharp/test/Microsoft.ML.OnnxRuntime.EndToEndTests/runtest.sh:25 cmake/external/onnx submodule advances past #7959 macOS NuGet EndToEnd leg only (if IsMacOS); the data/node symlink goes dangling → no opset* dir → silent-green, zero C# model tests.
js/scripts/prepare-onnx-node-tests.ts:53 a post-#7959 opset is added to TEST_DATA_OPSET_VERSIONS Extracts nothing from the archive → silent JS node-test coverage loss.

Remaining minors (non-blocking)

  • Post-retirement, opset fidelity rests entirely on pip-pin discipline (version string + count floor; no executable schema-drift check once the equivalence oracle retires). Worth a one-line note.
  • The 1500 floor is still triple-defined (CMake single-sources its own 3 consumers, but the two Python MIN_NODE_CASES literals are coupled only by comment).

Net: good to go for the C++ objective once the above follow-ups are tracked. Nice work on the fixes.

🤖 Multi-model review re-pass.

titaiwangms and others added 2 commits July 6, 2026 16:55
…, parity with filters.jsonc), not missing IRFFT kernel

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…master #7959 node-data deletion)

Agent-signed-off: Architect (dae3a478) [claude-opus-4.8 via copilot]
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@titaiwangms

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Dispositions:

  • docs/python/conf.py — fixed in this PR: pinned the test_sigmoid model URL from onnx master to v1.22.0 (matching deps.txt), so the docs build stays green regardless of when Add transformers tool test to pipeline #7959 lands on onnx master (it fetches GitHub master directly, independent of our submodule pin).
  • csharp EndToEndTests runtest.sh — analogous to the C++ detachment (single pinned opset, reuses the materializer); handling it in a stacked follow-up PR so it lands before the onnx->1.23 submodule bump, keeping this PR focused on the C++ objective.
  • js prepare-onnx-node-tests.ts — deferred by design: it consumes 15 historical frozen opset archives from immutable rel-* branches, which Add transformers tool test to pipeline #7959 does not rewrite, so it stays green indefinitely; it only breaks if a post-Add transformers tool test to pipeline #7959 opset is manually added, which needs a different multi-version mechanism.
  • Post-retirement opset fidelity rests on the pinned onnx version + the min-count tripwire once the equivalence oracle auto-retires.

Cross-consumer note for the #7959 viability question: the in-memory generator approach works cleanly for single-version consumers (C++, csharp, docs), but not for consumers needing a historical multi-opset matrix (our JS tests span opset 7-21), since onnx's generator is single-version and can't regenerate old opsets from a new onnx.

@titaiwangms

Copy link
Copy Markdown
Contributor Author

Review team re-review (round 2, current state)

Ran the full 5-reviewer team (readability / code / critical / deep / integration) against the latest state. All round-1 fixes confirmed resolved:

  • kind.lower() == "node" — verified against ONNX runner/__init__.py:82 (ONNX passes capitalized "Node"), so the counter now actually increments. This is a real correctness fix, not cosmetic.
  • min_cases now derived from len(written) (post kind-filter).
  • _max_ulp_diff is a correct ULP metric (matches the canonical Dawson formulation up to the additive signmask constant; ±0→0, adjacent→1, cross-sign→2).
  • ✅ CMake RC/.dev regex effectively mirrors _release_base.
  • TestCase.cc broken-list move drops no coverage — the node corpus was never exercised on non-QNN EPs before; CPU now runs it via the new ctest, so the global skip is required and mirrors the jsonc exclusions.

Still-open / new findings (none are merge-blockers)

🟠 Major — ULP classifier accepts finite↔∞ and ∞↔NaN as "Class A" (materialize_onnx_node_tests.py::_classify_tensor_diff).
Verified: max-finite fp32 vs +inf classifies as ('A', '1 ULP, max|d|=inf'). Raw-bit adjacency makes max-finite↔+inf = 1 ULP and +inf↔NaN(0x7f800001) = 1 ULP, so a structural finite→∞ (or ∞→NaN) output drift is silently blessed by the equivalence oracle despite max|d|=inf. Low real-world likelihood (retiring test, requires an actual finite/inf drift), but a valid robustness hole.
Cheap fix: in the Class-A branch, reject when np.isfinite(a) != np.isfinite(b), the NaN class differs, or max_abs is non-finite.

🟡 Minor — signed-zero false-RED. A pure +0/-0 diff hits np.array_equal(+0,-0) == True and returns Class B ("proto encoding drift") before ULP can collapse it, contradicting the ULP signed-zero doc claim.

🟡 Minor — stale IRFFT reason in onnx_backend_test_series_filters.jsonc:383-385. It still says ORT "does not implement IRFFT … outputs complex last dim=2", which is provably false per core/providers/cpu/signal/dft.cc:452-459 (IRFFT sets last dim=1). TestCase.cc was corrected and now claims "parity with jsonc" — but the jsonc twin still carries the old wrong reason. Please sync it.

🟡 Minor — two more residual on-disk data/node consumers not covered by this PR:

  • .agents/skills/onnx-opset-bump-checklist/SKILL.md:471 — the checklist's own verification gauntlet runs onnx_test_runner -e cpu …/_deps/onnx-src/…/data/node, which breaks post-Add transformers tool test to pipeline #7959 (ironically added by this same PR).
  • docs/python/examples/plot_load_and_predict.py:16blob/main/…/test_sigmoid link → 404 after retirement.

Pin/repoint both to the materialized dir or a tagged ONNX ref.

🟡 Minor — 1500 floor is now quadruple-defined (a 4th copy landed in onnx_node_test_equivalence_test.py), lockstep enforced only by comment with no cross-reference. ctest DEPENDS onnx_node_tests_materialized uses a build-target name in a CTest test-DEPENDS slot (benign since the target is ALL, but no real runtime ordering).

🔵 Nits — the kind.lower() rationale comment lives in __init__ rather than at the call site; the FATAL floor message omits the SKILL.md path.

Bottom line

No merge-blocker. The one item worth fixing here is the ULP finite/∞/NaN classifier gap (cheap, valid), plus the jsonc IRFFT reason-sync and the two extra doc/skill consumers.

@gramalingam

Copy link
Copy Markdown
Contributor

Is the utility for materializing the node test cases worth upstreaming to onnx repo? (separately, not to block this PR.)

gramalingam
gramalingam previously approved these changes Jul 6, 2026
…(F1/F2/F3/F4a)

F1 (MAJOR): _classify_tensor_diff mis-blessed finite<->inf and inf<->NaN
output drift as Class A because the raw-bit ULP metric makes max-finite/inf
and inf/adjacent-NaN 1 ULP apart. Reject any finite/inf/NaN structural
mismatch (or non-finite max|d|) as Class B before the Class-A ULP band.

F2 (MINOR): document that a pure +0.0/-0.0 flip is intentionally caught as
byte/proto Class-B drift by the array_equal gate, and that _max_ulp_diff's
signed-zero merge only prevents a co-occurring signed zero from inflating a
genuine mixed-tensor skew -- resolves the docstring/behavior contradiction.

F4a (MINOR): pin plot_load_and_predict.py test_sigmoid gallery link from
onnx blob/main to blob/v1.22.0 (survives onnx#7959 node-data deletion),
mirroring the conf.py pin.

F3 (MINOR): correct the DFT rfft/irfft skip *reason* in
onnx_backend_test_series_filters.jsonc -- the old text ('ORT does not
implement IRFFT, outputs complex last dim=2') is false per dft.cc:458-459
(IRFFT sets last dim=1). Real cause is platform-dependent near-zero FFT
roundoff exceeding the tight abs tol; mirrors TestCase.cc. Skip retained.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread .agents/skills/onnx-opset-bump-checklist/SKILL.md
…e r3537835567

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread cmake/onnxruntime_unittests.cmake

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the node-test detach + materializer + tripwire. Overall this is a careful, well-documented change and the design (materialize from ONNX's surviving Python generators, keep the C++ runner reading from disk, add a cause-agnostic min-count tripwire, auto-retire the equivalence oracle post-#7959) is sound. The earlier automated-review findings look addressed on this head: the per-dataset arity guard in _write_case, the overflow-proof unsigned sign-magnitude fold in _max_ulp_diff, and the RC/.dev-aware release-base comparison in the cmake onnx gate are all present.

Two coupled points on the new CPU node ctest are worth confirming before merge (both left inline):

  1. New CPU full-corpus coverage / broken-test parity. onnx_test_node_materialized runs the entire data/node corpus on the CPU EP for the first time. onnx_test_runner hard-fails on any case that fails on CPU but isn't in the C++ GetBrokenTests list, and this PR mirrors only convinteger + the 4 dft_* cases, while the Python onnx_backend_test_series_filters.jsonc excludes a much larger set.

  2. -m floor vs. skipped-case count. The -m 1500 floor is checked against tests.size(), which excludes broken/disabled/training-domain cases, so the real headroom is smaller than the ~1799 -> 1500 reasoning implies, and the C++ and Python floors measure different populations.

The MATERIALIZE=ON py314 leg runs mode: test, so both would surface as CI-red rather than silently — but the PR description's own [needs-run] note says the MATERIALIZE=ON inner path (materialization + the two add_test bodies) hasn't executed on a pinned box yet. Running that leg's ctests once on-pin would de-risk the merge.

Minor / non-blocking: the numpy pin (2.2.6/2.4.2) is now duplicated across cmake, the 4 CI yaml files, and requirements.txt; the SKILL.md documents the lockstep requirement, but it remains three manual sync points.

Comment thread cmake/onnxruntime_unittests.cmake
Comment thread cmake/onnxruntime_unittests.cmake
…tionally-different populations (tianleiwu P2)

The C++ -m floor checks tests.size() (post-skip collected count) while the
Python MIN_NODE_CASES counts the pre-exclude population. Same 1500 literal,
different populations by design; ~1794 vs 1500 today is huge margin and any
future over-growth of the CPU skip set fails loud (never silent). Comment-only,
no logic or floor-value change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@titaiwangms
titaiwangms requested a review from tianleiwu July 9, 2026 17:19
@titaiwangms

Copy link
Copy Markdown
Contributor Author

cc @tianleiwu

@tianleiwu tianleiwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the current head (381ccd6). This is a thorough, well-documented change and my three earlier threads are now addressed — resolved them:

  • python 3.10 literal: agreed it's a find_package lower bound, not a hard version, so 3.10 legs aren't blocked.
  • CPU onnx_test_node_materialized broken-test parity: confirmed green on-pin (run 28893847416); future CPU-failing node tests surface as loud CI-red on the same leg.
  • ORT_ONNX_NODE_MIN_CASES floor: the tightened comment now documents the two intentionally-different populations and the safe failure direction.

The materializer/compare core, the retiring equivalence guards (Python unittest + compare_node_test_corpora.py ctest), the configure-time onnx/numpy gates, and the -m runtime tripwire all look correct and internally consistent. One minor non-blocking nitpick left inline. LGTM once CI is green.

Comment thread cmake/onnxruntime_unittests.cmake
Comment thread docs/python/conf.py
Comment thread tools/ci_build/github/azure-pipelines/linux-qnn-ci-pipeline.yml
Comment thread tools/ci_build/github/azure-pipelines/win-qnn-arm64-ci-pipeline.yml
@titaiwangms
titaiwangms enabled auto-merge (squash) July 16, 2026 20:00
@titaiwangms

Copy link
Copy Markdown
Contributor Author

I will address comments with another PR.

@titaiwangms
titaiwangms merged commit c4f76f6 into main Jul 16, 2026
88 checks passed
@titaiwangms
titaiwangms deleted the node-test-detach-from-onnx branch July 16, 2026 20:01
titaiwangms pushed a commit that referenced this pull request Jul 16, 2026
… for QNN materialize deps, cmake ctest comment (#29504 follow-up)

Follow-up to merged #29504 addressing tianleiwu's non-blocking post-merge
review comments.

N1 (cmake): Drop the two no-op `set_tests_properties(... DEPENDS
onnx_node_tests_materialized)` calls. That name is an add_custom_target(... ALL),
not a registered test, so a ctest test-level DEPENDS on it is a silent no-op.
Replace each with a comment documenting the real ordering guarantee: the ALL
target materializes the corpus during the normal build (before ctest), and the
-m / --min-cases tripwire fails loud on a missing/partial corpus.

N2 (conf.py): Replace the doc-build-time urlretrieve of
onnx/backend/test/data/node/test_sigmoid/model.onnx with an in-memory
onnx.helper Sigmoid graph (input "x" -> output "y", float32 [3,4,5]) saved to
the same destination. Removes the network dependency and the latent 404 once
onnx/onnx#7959 deletes that on-disk corpus; keeps the doc build hermetic.

N3-5 (requirements file): Add a dedicated minimal
requirements-materialize-onnx-node-tests.txt (onnx + numpy only, with the hybrid
numpy env markers: 2.2.6 for python_version<'3.11', 2.4.2 for >='3.11'). Repoint
the four QNN/Android CI legs from inline pip pins to `pip install -r` (preserving
--user on the linux + android legs; the two Windows legs omit it). Placed at the
repo root so Dependabot (pip directory: "/") can track the onnx/numpy upgrades
without dragging the heavy docker requirements.txt onto the QNN legs. Pins stay
locked in lockstep with the CMake gate, which HARD-FATALs on installed!=pinned.
Update the onnx-opset-bump-checklist SKILL.md gotcha-p to reflect the new
requirements file (reversing its prior "inline by design" note) and add it to the
numpy pin sync-site list.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
titaiwangms added a commit that referenced this pull request Jul 27, 2026
…track QNN materialize deps (#29577)

Follow-up to the merged #29504 (ONNX node-test detachment). This PR
consolidates **all** post-#29504 follow-up work into a single change
set. Changes C/D/E address @tianleiwu's non-blocking post-merge review
comments; A/B were the original scope of this PR. None change the
behavior of the shipped feature — this is hardening + hygiene + doc
detachment.

### (A) C# — loud-fail guard on the macOS node-test symlink
`csharp/test/Microsoft.ML.OnnxRuntime.EndToEndTests/runtest.sh`
symlinked the ONNX on-disk node-test corpus. Once the onnx pin advances
past onnx/onnx#7959 (which deletes that corpus) the symlink target
vanishes and the suite would silently pass with zero cases. Added a
loud-fail guard: enforce a discovered-case floor (1000) and keep the
immutable v1.13.1 pin, so corpus absence becomes a red instead of a
silent-green.

### (B) JS — post-#7959 opset caveat in `prepare-onnx-node-tests.ts`
Documented that once onnx ships without the on-disk node-test data
(onnx/onnx#7959), post-#7959 opsets cannot use the `rel-*` archive
download path — a caveat for the next opset bump.

### (C = N1) cmake — drop no-op ctest `DEPENDS` + document real ordering
`cmake/onnxruntime_unittests.cmake`: the two `set_tests_properties(...
PROPERTIES DEPENDS onnx_node_tests_materialized)` calls named an
`add_custom_target(... ALL)`, not a registered test, so a CTest
test-level `DEPENDS` on it is a silent no-op. Removed both and
documented the real ordering guarantee: the `ALL` target materializes
the corpus during the build (before ctest), and the `-m` / `--min-cases`
tripwire fails loud on a missing/partial corpus.

### (D = N2) conf.py — hermetic in-memory doc model
`docs/python/conf.py` urlretrieved `.../node/test_sigmoid/model.onnx`
from GitHub at doc-build time — a latent 404 once onnx/onnx#7959 deletes
that path. Replaced with an in-memory `onnx.helper` single-node Sigmoid
graph (input `x` → output `y`, float32 `[3,4,5]`) written to the same
destination. Keeps the doc build hermetic and drop-in compatible.

### (E = N3-5) requirements file for QNN/Android materialize deps +
SKILL.md
The four QNN/Android CI legs used inline `pip install onnx==...
"numpy==...; ..."` pins (invisible to Dependabot) to feed the
`onnxruntime_MATERIALIZE_ONNX_NODE_TESTS` gate. Added a dedicated
minimal `requirements-materialize-onnx-node-tests.txt` (onnx + numpy
only, with hybrid numpy env markers `2.2.6` for `python_version<'3.11'`,
`2.4.2` for `>='3.11'`) and repointed all four legs to `pip install -r`
(preserving `--user` on linux + android; the two Windows legs omit it).
Placed at the repo root because `dependabot.yml` pins pip to `directory:
"/"` (non-recursive). Pins stay locked in lockstep with the CMake gate,
which HARD-FATALs on `installed != pinned`. Updated the
`onnx-opset-bump-checklist` SKILL.md gotcha-p to reflect the new file
(reversing its prior "inline by design" note) and added it to the numpy
pin sync-site list.

---
Consolidates the standalone #29746 (now closed) per author request.
Triple-review passed on both change sets independently. Merge after
#29504 (already merged), which this references.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants