Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The review found unsafe pickle loading, incorrect dynamic-shape recompilation, and reporting reliability issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds weekly Jenkins coverage for ONNX Model Zoo accuracy and performance reporting.
Changes:
- Adds accuracy, performance, and baseline-report generation.
- Supports legacy NPZ datasets and archive metadata.
- Documents local model execution and precision handling.
Review used a single-pass workflow because subagent fan-out was unavailable. End-to-end Jenkins validation remains necessary.
File summaries
| File | Description |
|---|---|
tools/test_runner.py |
Adds metadata filtering, NPZ support, and stricter output checks. |
tools/model_zoo/onnx_zoo/zoo_check.py |
Runs accuracy checks and emits metrics/driver arguments. |
tools/model_zoo/onnx_zoo/test_models.sh |
Orchestrates accuracy and performance runs. |
tools/model_zoo/onnx_zoo/summarize.py |
Generates Markdown reports and baseline comparisons. |
tools/model_zoo/onnx_zoo/README.md |
Documents local execution and precision behavior. |
tools/model_zoo/onnx_zoo/Jenkinsfile |
Defines the weekly build, execution, and publication pipeline. |
Review details
Suppressed comments (1)
tools/test_runner.py:243
allow_pickle=Truepermits arbitrary Python code execution from a model archive. These archives are processed in a container with a writable workspace, and the pipeline later executessummarize.pyfrom that workspace while report credentials are bound, so a compromised archive can persist code into the credentialed stage. Use a non-pickled fixture format or verify archives against immutable trusted hashes before loading legacy pickles.
data = np.load(npz_path, allow_pickle=True, encoding='bytes')
- Files reviewed: 6/6 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Accuracy tolerance, skipped-result reporting, archive identification, and unsafe NPZ loading need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
tools/model_zoo/onnx_zoo/test_models.sh:74
- Log identity uses only the archive basename. When recursive or multiple input directories contain the same filename, later runs overwrite the earlier accuracy/performance logs and the summary silently omits one model. Derive a stable unique stem from the archive's relative path (or otherwise disambiguate collisions).
tools/model_zoo/onnx_zoo/test_models.sh:98 - The int8/QDQ detection is case-sensitive, while the summarizer classifies those names case-insensitively. An archive such as
model_QDQ.tar.gzis therefore run through both fp32 and fp16 paths, including an unintended fp16 quantization pass, despite later being reported as int8. Normalize the basename before matching.
tools/model_zoo/onnx_zoo/zoo_check.py:73 - The relative tolerance is scaled by
actual, even thoughgoldis the reference tensor (and the range-based absolute tolerance is also derived fromgold). This can understate the normalized error; for example, expected1, actual2, andrtol=0.5yields a fraction of 1 instead of 2. Scale by the reference values.
This issue also appears on line 104 of the same file.
tools/model_zoo/onnx_zoo/zoo_check.py:104
np.allclose(a, b)applies the relative tolerance tob. Passinggoldfirst therefore grades relative to the produced value rather than the expected value and can falsely pass cases such as expected1, actual2,rtol=0.5. Reverse the arguments to makegoldthe reference.
if not np.allclose(gold, actual, args.rtol, atol):
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
Motivation
Runs specified ONNX Model Zoo models on our CI through Jenkins.
Technical Details
Introduces a new Jenkinsfile that runs weekly along with scripts to support both perf and accuracy metrics.
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot ApplicableFollow the LLVM AI Tool Use Policy for contributions using AI.