feat(e2e-eval): support pre-exported ONNX models#902
Conversation
CodeQL flagged 'onnx' being imported with both 'import onnx' and 'from onnx import ...' in the same module. Use attribute access (onnx.version_converter / onnx.helper / onnx.TensorProto) throughout instead of mixing import styles.
The Phase 2 onnx_file passthrough was dead: load_curated_entries dropped every key except hf_id/task/group/priority, so a rebuild that re-added or newly-created a curated onnx_file entry would lose it. Carry onnx_file through when present.
|
need to update this in favor of #582 |
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Reviewed the ONNX-eval enablement. The code changes themselves are small and mostly reasonable, but the PR description is significantly out of sync with what actually landed — worth reconciling before merge.
Description vs. diff mismatch
The "Changes" section describes functionality that isn't in this diff:
run_eval.pyis said to "resolve registry ONNX files before build/perf, preserve ONNX metadata through--hf-model, upgrade sub-minimum opsets, and resolve direct-ONNX build artifacts." The actualrun_eval.pydiff is formatting-only — there is noonnx_filehandling anywhere in the file.- The registry is said to carry "ONNX file metadata" /
onnx_file, but there is noonnx_filekey. The real mechanism encodes the ONNX path directly inhf_id(e.g.PaddlePaddle/PP-OCRv6_medium_det_onnx/inference.onnx) and leans on winml's existinghub_onnxresolution oforg/repo/path/file.onnxrefs. That's a cleaner design, but the description (and the earlierload_curated_entriespassthrough commit) describe the reverted approach.
It looks like the "revert and update" commit changed the approach. Please update the description so reviewers know the ONNX support rides on winml's existing nested-onnx resolution plus the registry/opset/cache tweaks here — as written it's misleading about where the behavior lives and what's actually tested.
models_all.json churn (FYI)
The ~1.6k-line diff there is almost entirely downloads/order refresh from --recheck-downloads (440/443 entries changed downloads, 315 reordered). Only the two PaddleOCR entries actually change identity (v5 → v6 _onnx). Verified: no entries added/dropped and no duplicate keys.
Inline notes below.
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Approving. The follow-up commit (7527b4c) cleanly addresses all three review threads:
- onnx_model.py — the analyzer opset floor is removed entirely, so the model records its opset without gating (a reasonable separation of concerns: analyze records, build enforces). Stale "upgrades sub-minimum opsets" wording dropped from the PR body.
- build.py — reverted to match
mainexactly, restoring the fail-fast on missingloader.task. Safe for the ONNX eval becauserun_eval.pypasses--taskfrom the registry entry, and the PaddleOCR entries now carry explicittaskmetadata. - models_curated.json — explicit
task: image-to-textadded, keeping curated source and the built registry consistent across rebuilds. The v5-server → v6-medium swap is confirmed intentional.
Verified locally against the PR head: test_onnx_model.py + test_build.py = 127 passed, and ruff check is clean on the changed files.
Minor non-blocking note: removing the opset floor outright (vs. lowering it) means genuinely ancient opsets now surface only later in optimize rather than at analysis time — acceptable given the layering.
Summary
Adds support for evaluating models that ship a pre-exported ONNX in the e2e_eval harness, including HF refs that point at a specific
.onnxfile under a repo. This supports models such as the PaddleOCR*_onnxrepos, where the native HF export path is blocked by the pinnedtransformers/optimum-onnxdependency stack.When a registry entry points at an ONNX file, the harness downloads that file from the HF repo and feeds the local ONNX path to
winml config,winml build, andwinml perf, bypassing HF-to-ONNX export.Changes
run_eval.py: resolves registry ONNX files before build/perf, preserves ONNX metadata through--hf-modeltask overrides, and resolves direct-ONNX build artifacts deterministically.build_registry.py: preserves ONNX metadata during registry rebuilds, adds--recheck-downloadsto refresh download counts before ordering, preserves existing download counts if a recheck lookup fails, and normalizes nested.onnxHF refs toorg/modelonly for HF API calls while keeping the full registryhf_id.Validation
uv run pytest tests/unit/eval/test_run_eval_script.pyuv run pytest tests/unit/commands/test_build.py -quv run pytest tests/unit/analyze/models/test_onnx_model.py -quv run pytest tests/unit/test_build_registry.py -q