Follow-up to #29504: detach csharp/js consumers + harden doc build + track QNN materialize deps - #29577
Conversation
There was a problem hiding this comment.
Pull request overview
Adds safeguards and documentation to prevent silent-green outcomes when ONNX node test corpora disappear post-onnx/onnx#7959, covering remaining non-C++ consumers (C# NuGet macOS leg and JS multi-opset node-test prep).
Changes:
- C#: Add a fail-loud minimum-case tripwire after symlinking
onnx/backend/test/data/nodeintomodels/opset${N}to prevent “0 tests run” silent-green when the corpus is missing/truncated. - JS: Document that future opsets (post-onnx/onnx#7959 removal of
data/node) can’t be added via the existingrel-*archive download mechanism without a new materialization approach.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| csharp/test/Microsoft.ML.OnnxRuntime.EndToEndTests/runtest.sh | Adds a post-symlink corpus-size guard to fail loudly if node test data collapses (avoids silent-green). |
| js/scripts/prepare-onnx-node-tests.ts | Adds an inline note documenting the post-onnx/onnx#7959 constraint for extending opset coverage via rel-* archives. |
Review team review (full 5-reviewer team)Ran the full team (readability / code / critical / deep / integration). Clean and ready — no merge-blocker. Load-bearing facts independently verified: 1097 node cases @ v1.13.1 ( Findings🟠 Major (merge-order dependency, documented). Comments and error messages here cite ⚪ Non-finding (false positive). One reviewer flagged android-arm64 / linux-qnn ymls still pointing at the old 🟡 Minors:
❓ Open question (needs-run, non-blocking). Does xUnit silently pass a zero-row Bottom lineClean modulo respecting the merge order (land after #29504). Nothing here blocks. |
…reen if onnx pin bumps past #7959 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… 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>
366e3f7 to
d7d8b84
Compare
…rfaced by Optional Lint filter_mode:file Optional Lint's reviewdog runs with filter_mode:file, which pulls the whole changed runtest.sh into scope and surfaces 12 legacy INFO/style findings. Surgical, behavior-preserving fixes: - SC2086: double-quote single-path/scalar expansions (cd, [ ] test operands, csproj paths, -s repo arg). No intended word-splitting changed. - SC2181: convert the two single-command `$?` checks (dotnet restore, USE_CUDA dotnet test) to the direct `if ! cmd; then` form; error messages + exit codes preserved verbatim. The final shared check guards the last command of a two-branch if/else (USE_TENSORRT or the default run), so it keeps `$?` with a scoped `# shellcheck disable=SC2181` explaining why it can't be inlined. No test logic, dotnet args, paths, or the floor-1000 R1 guard were changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ch-followup # Conflicts: # .github/workflows/windows_qnn_x64.yml
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.shsymlinked 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.tsDocumented 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 orderingcmake/onnxruntime_unittests.cmake: the twoset_tests_properties(... PROPERTIES DEPENDS onnx_node_tests_materialized)calls named anadd_custom_target(... ALL), not a registered test, so a CTest test-levelDEPENDSon it is a silent no-op. Removed both and documented the real ordering guarantee: theALLtarget materializes the corpus during the build (before ctest), and the-m/--min-casestripwire fails loud on a missing/partial corpus.(D = N2) conf.py — hermetic in-memory doc model
docs/python/conf.pyurlretrieved.../node/test_sigmoid/model.onnxfrom GitHub at doc-build time — a latent 404 once onnx/onnx#7959 deletes that path. Replaced with an in-memoryonnx.helpersingle-node Sigmoid graph (inputx→ outputy, 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 theonnxruntime_MATERIALIZE_ONNX_NODE_TESTSgate. Added a dedicated minimalrequirements-materialize-onnx-node-tests.txt(onnx + numpy only, with hybrid numpy env markers2.2.6forpython_version<'3.11',2.4.2for>='3.11') and repointed all four legs topip install -r(preserving--useron linux + android; the two Windows legs omit it). Placed at the repo root becausedependabot.ymlpins pip todirectory: "/"(non-recursive). Pins stay locked in lockstep with the CMake gate, which HARD-FATALs oninstalled != pinned. Updated theonnx-opset-bump-checklistSKILL.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.