Feat/sam3 onnx hub support (issue #324)#582
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds support for Hub-hosted pre-exported ONNX inputs (<org>/<repo>/<path>.onnx) to enable SAM 3 consumption via Scenario D, and fixes pre-quantized (QDQ + QOperator) detection/routing so already-quantized models skip ORT optimization/quantization stages that can crash on unsupported integer kernels.
Changes:
- Introduces
loader/onnx_hub.pyto recognize and download Hub-hosted ONNX files (plus best-effort.onnx_datasidecars), and wires resolution into multiple entry points (CLI + inference/model loading). - Expands
is_quantized_onnxto detect QOperator quantized models and adds shared quant-op constants. - Updates build pipelines to honor “pre-quantized” routing (skip optimize/quantize) and adds regression/unit/integration tests for SAM 3 and Hub-ONNX refs.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/onnx/test_detection.py | New tests for quantized (QDQ + QOperator) and compiled ONNX detection. |
| tests/unit/loader/test_onnx_hub.py | New unit tests for Hub-ONNX path detection/splitting/downloading behavior. |
| tests/unit/inference/test_engine.py | Ensures Hub-ONNX refs are resolved before inference routing logic. |
| tests/unit/commands/test_perf_cli.py | Verifies winml perf resolves Hub-ONNX refs before ONNX path validation. |
| tests/unit/commands/test_hub_onnx_ref.py | New CLI tests for wmk config/wmk build with Hub-hosted ONNX refs. |
| tests/unit/commands/test_eval.py | Adds Hub-ONNX resolution coverage in eval model-path resolution. |
| tests/unit/build/test_onnx.py | Regression tests ensuring pre-quantized ONNX skips optimize/quantize stages. |
| tests/unit/build/test_hf.py | Regression tests ensuring pre-quantized exported ONNX skips optimize/quantize stages. |
| tests/integration/test_sam3_e2e.py | End-to-end integration tests for SAM3 decoder + encoder via Hub-hosted ONNX artifacts. |
| src/winml/modelkit/onnx/detection.py | Updates quantized detection to use unified QDQ+QOperator op set. |
| src/winml/modelkit/models/auto.py | Resolves Hub-ONNX refs before fast-path ONNX/HF routing in from_pretrained. |
| src/winml/modelkit/loader/onnx_hub.py | New implementation for Hub-hosted ONNX ref resolution via hf_hub_download. |
| src/winml/modelkit/loader/init.py | Re-exports Hub-ONNX helper APIs. |
| src/winml/modelkit/inference/engine.py | Normalizes Hub-ONNX refs to local paths in load and load_schema_only. |
| src/winml/modelkit/data/hub_models.json | Adds SAM3 encoder/decoder catalog entries using Hub-ONNX refs. |
| src/winml/modelkit/compiler/utils.py | Adds QOperator + union quantization op-type constants. |
| src/winml/modelkit/compiler/init.py | Exposes new quantization op-type constants from compiler package. |
| src/winml/modelkit/commands/perf.py | Resolves Hub-ONNX refs prior to ONNX path checks. |
| src/winml/modelkit/commands/inspect.py | Treats Hub-ONNX refs as ONNX inputs for consistent “not supported” messaging. |
| src/winml/modelkit/commands/eval.py | Resolves Hub-ONNX refs before validating ONNX file existence. |
| src/winml/modelkit/commands/config.py | Resolves Hub-ONNX refs before dispatching config generation path. |
| src/winml/modelkit/commands/build.py | Resolves Hub-ONNX refs and adds skip-optimize plumbing for ONNX pipeline. |
| src/winml/modelkit/build/onnx.py | Ensures pre-quantized models skip ORT optimize and don’t crash on QOperator ops. |
| src/winml/modelkit/build/hf.py | Same as above for HF-exported ONNX artifacts that are already quantized. |
| src/winml/modelkit/build/common.py | Adds skip_optimize to the shared optimize/analyze loop. |
| README.md | Documents Hub-hosted ONNX input form and supported commands. |
Comments suppressed due to low confidence (1)
src/winml/modelkit/commands/build.py:1162
--no-optimizesetsextra_kwargs["skip_optimize"], but_build_onnx_pipeline()never reads it. As a result, the CLI flag has no effect unlessis_quantized_onnx()happens to detect the model as pre-quantized. Consider consumingextra_kwargs.pop("skip_optimize", False)and combining it withis_pre_quantized(and settingmax_iters=0when skipping) so users can force skipping the optimize/autoconf stage for problematic ONNX inputs.
from ..onnx import copy_onnx_model, is_quantized_onnx
max_iters: int = extra_kwargs.pop("hack_max_optim_iterations", 3)
# ── Validate + setup ─────────────────────────────────────────
if not onnx_path.exists():
raise FileNotFoundError(f"ONNX file not found: {onnx_path}")
try:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
(supersedes the earlier review — consolidated with design feedback)
Code-level issues
See inline comments below.
One additional nit not inlined (existing code, not touched by this PR): _run_quantize_stage at line 952 of commands/build.py still says "(QDQ nodes already present)". Since this PR broadens detection to cover QOperator, the message should read "(QDQ or QOperator nodes already present)" for consistency with the log messages updated in build/onnx.py and build/hf.py.
Design feedback
Three structural concerns — the bug fixes (QOperator detection, skip_optimize) are solid, but the way the Hub-ONNX input form is introduced creates maintenance surface that should be addressed before or shortly after merge.
1. "Hub-hosted ONNX" is not a distinct input type — it is a download step
org/repo/path/file.onnx -> hf_hub_download() -> /local/cache/file.onnx
After the download, this is just a local .onnx file. The downstream pipeline does not (and should not) care that it once lived on the Hub. Yet the PR threads detection logic (is_hf_onnx_path / maybe_resolve_hf_onnx_path) through 7+ command entry points, giving it the status of a persistent input type. This inflates both the concept count and the code surface.
A cleaner model is: resolve once at a single entry point, return a local path, downstream is unaware.
2. Model input identification needs a single resolver — is_hub_model already exists
hub_utils.py already has is_hub_model() with comprehensive local-path rejection (checks Path.exists(), ./, ../, ~/, Windows drive letters). The new is_hf_onnx_path reimplements only Path.exists(), missing the other cases.
The codebase now has three parallel detection mechanisms with no shared logic:
| Input form | Detector | Local-path rejection |
|---|---|---|
HF model ID (org/model) |
is_hub_model() |
Full (exists, ./, ../, ~/, Win drive) |
| Local ONNX file | scattered path.suffix == ".onnx" checks |
N/A |
Hub-hosted ONNX (org/repo/path.onnx) |
is_hf_onnx_path() (new) |
Partial (only Path.exists()) |
Suggested direction: a single resolve_model_input() that classifies and resolves in one place, reusing is_hub_model's rejection logic. Each command calls it once; adding a fourth input form means changing one function, not 7+.
def resolve_model_input(value: str) -> ModelInput:
# 1. Local-path rejection (reuse is_hub_model logic)
# 2. If org/repo/path.onnx -> download -> return as local_onnx
# 3. If org/model -> return as hf_id
# 4. If local .onnx exists -> return as local_onnx
# No persistent "hub_onnx" type needed3. Pre-quantized ONNX handling should be decided once, not scattered across three pipelines
The detect-and-skip logic lives independently in three places with inconsistent behavior:
| Location | Detects via | Skips optimize | Skips quantize |
|---|---|---|---|
build/onnx.py |
is_quantized_onnx() |
skip_optimize=True |
Explicit |
build/hf.py |
is_quantized_onnx() |
skip_optimize=True |
Explicit |
commands/build.py |
is_quantized_onnx() |
skip_optimize=True |
Relies on user config having quant: null |
Plus _run_quantize_stage has its own is_quantized_onnx() guard (line 951) — a fourth detection point.
The CLI pipeline is the odd one out: if a user provides a config with quant settings for a pre-quantized model, it will attempt to re-quantize. The library functions protect against this; the CLI does not.
Suggested direction: detect once at pipeline entry, stamp the result onto WinMLBuildConfig (e.g. config.skip_optimize = True; config.quant = None), and have all downstream stages read config. This also eliminates redundant is_quantized_onnx calls on the same model.
4. No discovery mechanism for Hub ONNX files
The current UX requires the user to already know the exact file path inside a Hub repo (onnx/vision_encoder_int8.onnx), the right variant (fp32 vs int8, encoder vs decoder), and the task. Compare with the HF model ID flow where architecture, task, and export are all auto-detected.
The only discovery path is the static hub_models.json catalog, which is manually curated. If this is an intentional V1 scope limitation, it should be documented as such. Otherwise, consider accepting a repo ID (onnx-community/sam3-tracker-ONNX) and listing available .onnx files, or reading repo metadata to auto-configure task and roles.
Questions on validation and evaluationThe bug fixes are well-tested (synthetic ONNX models, mock-based CLI tests, regression assertions) — nice work on the silent-skip audit and the QOperator coverage. A few questions on the SAM 3 model validation side — would be great if you could share any additional results you have. What the PR verifies (solid foundation)
These are all great and necessary. A few additional data points would help reviewers feel confident about the end-to-end quality: Would love to see
Task inconsistency between catalog and testsSmall thing I noticed — the encoder's task differs between
The encoder only produces image embeddings, so SuggestionIf some of the above are blocked (NPU availability, no |
SAM 3 (facebook/sam3) requires transformers>=5, but optimum-onnx pins
transformers<4.58, so the standard HF + Optimum export route SAM 2 uses
is blocked. This change wires SAM 3 in through the existing pre-exported
ONNX (Scenario D) pipeline by recognizing path-style Hub references
('org/repo/path/to/file.onnx') and downloading the file once via
huggingface_hub.
Changes:
- New src/winml/modelkit/loader/onnx_hub.py: is_hf_onnx_path,
resolve_hf_onnx_path. Mirrors the is_xxx/resolve_xxx pair pattern
used by is_compiled_onnx/is_quantized_onnx.
- Wire into wmk config, wmk build, and WinMLAutoModel.from_pretrained
with the same 2-line 'if is_hf_onnx_path(x): x = str(resolve_hf_onnx_path(x))'
pattern.
- Add 2 sam3_tracker entries to hub_models.json so 'wmk hub --model-type
sam3_tracker' lists them.
- Tests: 12 unit tests for the resolver, 2 CLI plumbing tests, and 3
end-to-end integration tests (slow/network/integration).
The existing build_onnx_model pipeline runs unchanged on the resolved
local path: the int8 ONNX is auto-detected as quantized via
is_quantized_onnx, the quantization stage is skipped, and the artifact
flows through Optimize -> Analyze<->Optimize -> Compile -> Finalize.
consumer. Also fix two latent bugs in the build pipeline that any QOperator-quantized model would have hit. Background ---------- Issue #324 asks for SAM 2-style native HuggingFace export support for ``facebook/sam3`` (Sam3*IOConfig, Sam3ModelPatcher, etc.). That path is blocked by an upstream constraint: ``optimum-onnx`` pins ``transformers<4.58``, but ``facebook/sam3`` requires ``transformers>=5`` (the ``Sam3Model`` class only exists there). Resolving the pin would need either an upstream optimum-onnx PR or vendoring SAM 3 patcher code that bypasses optimum entirely. Instead, this PR introduces a generic "Hub-hosted ONNX file" input form and lets SAM 3 ride on the existing pre-exported-ONNX (Scenario D) pipeline that already worked for any local ``.onnx`` file. The infrastructure is reusable for any future model with similar version constraints (Whisper / Phi / RWKV / etc. all ship pre-exported ONNX repos on the Hub today). What's added ------------ 1. Hub-hosted ONNX URI resolver - ``loader/onnx_hub.py``: ``is_hf_onnx_path()``, ``resolve_hf_onnx_path()``, ``maybe_resolve_hf_onnx_path()`` - Recognizes inputs of the form ``<org>/<repo>/<path/to/file>.onnx``, downloads via ``huggingface_hub.hf_hub_download``, returns the local cache path. Falls through unchanged for HF model IDs / local paths / ``None``. - Best-effort ``.onnx_data`` sidecar fetch for >2 GiB models. ``EntryNotFoundError`` is expected (inlined weights); ``OSError`` surfaces as a WARNING (disk/permission/network problems should not be silently dropped — the model would later fail to load with a confusing error). 2. CLI wiring (every command that accepts a model identifier) - ``wmk config`` / ``wmk build``: resolve at the top of the command - ``wmk inspect``: friendly "ONNX inspection not yet supported" error for Hub-ONNX refs (matches local .onnx UX) - ``wmk run`` / ``wmk serve``: ``InferenceEngine.load()`` and ``load_schema_only()`` resolve before routing - ``wmk perf``: resolve before the ``Path(model_id).suffix == '.onnx' and exists()`` check (otherwise Hub refs are mistaken for missing local files and rejected with FileNotFoundError) - ``wmk eval``: ``_resolve_model_path`` resolves before the local existence check - ``WinMLAutoModel.from_pretrained``: resolves before HF/ONNX dispatch - Stage-tool commands (``analyze``/``optimize``/``quantize``/ ``compile``/``export``) intentionally NOT wired — they take ``click.Path(exists=True)`` and operate on local files only. 3. SAM 3 catalog entries (``data/hub_models.json``) - Two entries for ``onnx-community/sam3-tracker-ONNX``: the vision encoder and the prompt-encoder + mask-decoder. Note: was already present in the base branch — this PR does not modify it. 4. Integration tests (``tests/integration/test_sam3_e2e.py``) - 4 decoder tests + 2 encoder tests, marked ``@slow @network @integration`` - Asserts: Hub URI resolves, quantization detected, build produces ``model.onnx``, autoconf produces an ``optimization_config``, and for the encoder: pre-quantized round-trip preserves the ``ConvInteger`` / ``MatMulInteger`` ops byte-identically. - Skips narrowed to ``HfHubHTTPError`` / ``OSError`` only — real bugs in the build/analyze pipeline will surface as test failures rather than green skips. Bugs fixed (would affect any QOperator-quantized model, not just SAM 3) --------------------------------------------------------------------- A. ``is_quantized_onnx`` only detected QDQ format (``QuantizeLinear`` / ``DequantizeLinear``). The SAM 3 vision encoder uses ``QuantFormat.QOperator`` (no QDQ pairs, just integer ops: ``ConvInteger``, ``MatMulInteger``, ``QLinear*``). Previously misclassified as not quantized → routed through the optimize + quantize stages → tried to re-quantize an already-int8 model. Fix: ``compiler/utils.py`` adds ``QOPERATOR_OP_TYPES`` and ``QUANTIZATION_OP_TYPES = QDQ ∪ QOperator``. ``onnx/detection.py`` uses the union. B. The ``is_pre_quantized`` branches in ``build_onnx_model``, ``build_hf_model``, and the CLI's ``_build_onnx_pipeline`` logged "skipping optimize" but still invoked ``optimize_onnx`` → ``ort_graph`` → loaded the model into an ORT session. For QOperator models on hosts without a CPU ``ConvInteger`` kernel (e.g. ``onnxruntime-windowsml`` 1.23.x), this crashes the build stage with ``NOT_IMPLEMENTED``. Fix: ``build/common.py::run_optimize_analyze_loop`` gains a real ``skip_optimize: bool`` knob that bypasses ``optimize_onnx`` and the autoconf re-optim loop, just copying the input as the "optimized" artifact. All three pre-quantized branches now pass ``skip_optimize=True``. The downstream behavior (skip quantize + skip compile when configured) is unchanged. Verification ------------ - ``onnx.checker.check_model(full_check=True)`` passes on built artifacts - Built decoder produces NUMERICALLY IDENTICAL outputs to input decoder (``max|built - input| = 0.0`` across all 3 outputs) — pre-quantized round-trip is a true pass-through, not just structurally similar - Encoder runtime feasibility on CPU is identical to input encoder (both fail on CPU because of upstream ORT ``ConvInteger`` kernel gap; encoder requires NPU EP — unchanged from input) - Decoder real inference produces sane SAM-shaped outputs: ``iou_scores ∈ [0, 1]``, ``pred_masks`` logits span both signs, ``object_score_logits`` non-degenerate Test count ---------- - 4518 unit tests pass (+12 new regression tests across: ``test_onnx_hub.py``, ``test_detection.py``, ``test_eval.py``, ``test_perf_cli.py``, ``test_engine.py``) - 6 integration tests pass (live HF download, ~30s) - Ruff check + format clean on all 24 changed files Silent-skip audit (per SAM 2 review feedback) --------------------------------------------- Removed ``except Exception: pytest.skip(...)`` patterns from SAM 3 integration tests — they were swallowing real bugs (including the ``ConvInteger`` regression fixed in this PR). All skips now narrowed to ``HfHubHTTPError`` / ``OSError`` (network) or specific runtime exceptions; ``RuntimeError`` from ``build_onnx_model`` and ``analyze_onnx`` now fails loudly. Removed unnecessary ``pytest.importorskip("huggingface_hub")`` (it's a hard transitive dep). Sidecar download ``OSError`` now logs WARNING instead of DEBUG. Known limitations (not addressed in this PR) -------------------------------------------- - SAM 3 encoder requires NPU EP (QNN / OpenVINO / VitisAI) because ``onnxruntime-windowsml`` ships no CPU kernel for ``ConvInteger(10)``. This is true for both the input and built artifact — our build preserves runtime behavior exactly. Decoder uses ``MatMulInteger`` and runs on either CPU or NPU. - Catalog entries for SAM 3 have ``quantization: null`` so ``wmk perf`` falls back to default random-input shapes that violate the SAM 3 decoder's internal reshape constraints. Populating ``quantization.input_tensors`` with proper shape hints (the pattern every other catalog entry follows) is the recommended fix; out of scope for this PR.
df2f38a to
6e47c07
Compare
6e47c07 to
4464ce6
Compare
4464ce6 to
5bf6fb7
Compare
…on evaluator Follow-up to the SAM 3 / Hub-hosted ONNX commits, addressing inline review feedback from @DingmaomaoBJTU, Copilot review, and CodeQL. Refactor: single model-input classifier and resolver ---------------------------------------------------- New module ``winml.modelkit.utils.model_input`` is the single entry point for classifying a ``-m/--model`` value (one of ``hub_onnx``, ``hf_id``, ``local_onnx``, ``build_dir``, ``invalid``) and resolving it (download for ``hub_onnx``, pass-through otherwise). Replaces the previous scattered detection across ``loader/onnx_hub`` (``is_hf_onnx_path``, ``maybe_resolve_hf_onnx_path``), ``utils/cli`` (``is_onnx_file_path``), and ad-hoc ``Path(value).suffix == ".onnx"`` checks in commands. Callers updated: ``commands/build``, ``config``, ``eval``, ``inspect``, ``perf``; ``inference/engine`` (``load`` and ``load_schema_only``); ``models/auto.WinMLAutoModel.from_pretrained``. Dead code removed ----------------- * ``resolve_model_input(...).local_path or str(model_path)`` -- the ``or`` branch was unreachable; ``local_path`` is set for every resolvable input. * Case-sensitive ``.onnx`` check in ``loader/onnx_hub`` (now consistent with case-insensitive ``.lower()`` check in callers). Build pipeline -------------- * ``ensure_pre_quantized_stamped`` is now the single defensive detection point for pre-quantized models in library entry points; the unified CLI path stamps the config up front, library entry points only run detection when needed. * ``run_optimize_analyze_loop`` enforces ``max_optim_iterations = 0`` whenever ``skip_optimize=True``; ORT lacks kernels for ``ConvInteger`` / ``MatMulInteger`` on host EPs, so re-optimize would crash for the same reason the initial optimize is skipped. * Skip-log message updated to ``"QDQ or QOperator nodes present"`` to match ``is_quantized_onnx`` accepting both quantization formats. * ``compiler/utils.QUANTIZATION_OP_TYPES`` extended with ``DynamicQuantizeLinear`` / ``DynamicQuantizeMatMul``; exported via ``__all__`` to satisfy CodeQL unused-global warning. Mask-generation evaluator (response to "would love to see eval") ---------------------------------------------------------------- * ``winml.modelkit.eval.mask_generation_evaluator`` drives encoder + decoder ORT sessions for SAM-family promptable mask generation. Profile-dispatch design supports SAM 3 (1008 input, mean=std=0.5, direct resize) and SAM 2.1 (1024 input, ImageNet mean/std, longest-side-pad). Verified preprocessing is byte-correct against ``onnx-community/sam3-tracker-ONNX/preprocessor_config.json``. * ``winml.modelkit.datasets.mask_generation`` -- ``MaskGenerationDataset`` wraps ``mattmdjaga/human_parsing_dataset`` with binary foreground/background masks. * ``winml.modelkit.eval.metrics.binary_segmentation`` -- mIoU + Dice on binary masks. * Composite SAM 3 entry in ``models_with_acc.json``. Reference scripts ----------------- * ``scripts/sam3_reference_check.py`` -- spot-check against the published reference ``iou_scores`` from the model card. * ``scripts/mask_generation_eval.py`` -- generic harness for any SAM-family ONNX pair with ``--preset`` (sam2 / sam3) and ``--dataset`` (human_parsing / coco). * ``scripts/sam3_smoke_eval.py`` -- back-compat shim that delegates to the generic harness with the SAM 3 preset. Test cleanups (CodeQL + Copilot) -------------------------------- * ``tests/integration/test_sam3_e2e.py`` fixtures: explicit ``raise`` after ``pytest.skip`` to make control flow obvious. * ``tests/unit/onnx/test_detection.py``: consolidate ``import onnx`` / ``from onnx import ...`` into a single import form. * ``tests/unit/core/test_onnx_utils.py``: expected keys updated for new ``input_symbolic_shapes`` field in ``get_io_config`` output. Other ----- * ``.gitignore``: ignore stray ``<UUID>.data`` ORT external-data sidecars at repo root, ``quantized_info.csv`` Quark side-effect file, and ``scripts/_*.py`` / ``scripts/_*.json`` private debug scripts.
5bf6fb7 to
a0e9c7c
Compare
Thanks for the detailed review and for calling this out. This has been addressed in the latest push, but via a small flow refactor rather than just a string edit: _run_quantize_stage no longer carries the old “(QDQ nodes already present)” wording path. Pre-quantized detection is now decided earlier and stamped on config (skip_optimize=True, quant=None), so _run_quantize_stage exits on config.quant is None for both QDQ and QOperator cases. For consistency, the stage-level logs in build/onnx.py and build/hf.py now use “pre-quantized model” wording (covering QDQ + QOperator). If you prefer an explicit QDQ/QOperator mention in the commands/build.py skip path as well, I can add that as a follow-up log line too. |
Thanks for the detailed consolidated feedback. This is very helpful. On the quantize-stage wording nit, I agree on the consistency goal. In the current branch, the old skip-message path you referenced is no longer the active behavior. The quantize stage now exits based on config state (quant is None) for pre-quantized inputs, and the pre-quantized wording is handled in the build pipelines. See src/winml/modelkit/commands/build.py, src/winml/modelkit/build/onnx.py, and src/winml/modelkit/build/hf.py. On design point 1 (Hub-hosted ONNX as download step): I agree with what you suggested. The current implementation is now centered on single-point normalization and resolution, where entry points resolve to local paths and downstream pipeline stages operate on local ONNX paths. See src/winml/modelkit/utils/cli.py and src/winml/modelkit/utils/model_input.py. |
This is addressed in the latest revision by introducing a single resolver path and routing command entry points through it. We now use src/winml/modelkit/utils/model_input.py as the classification and resolution center, and src/winml/modelkit/utils/cli.py normalize_model_arg delegates to resolve_model_input so callers do not have to implement their own Hub-vs-local detection. Also, local-path rejection is no longer limited to Path.exists-style checks; model_input reuses shared local-path logic via _is_local_path from src/winml/modelkit/utils/hub_utils.py, which covers the ./, ../, ~/, and Windows-drive cases you listed. On the hub_onnx type: agreed on the principle that downstream should operate on local ONNX after resolution. In the current shape, hub_onnx is an internal classification state used before resolution; after resolve_model_input, downstream paths consume local_path (for example src/winml/modelkit/inference/engine.py and src/winml/modelkit/models/auto.py). |
This is addressed in the latest revision. We now decide pre-quantized status once and stamp it onto config, then downstream stages read config state instead of re-detecting: A shared stamping helper sets skip_optimize true and clears quant when the input is pre-quantized (or when skip_optimize is forced). |
You are right that the current Hub ONNX UX is still artifact-path driven: users must provide a fully qualified file path, and we do not yet support repo-only input or automatic task and role inference from repo metadata. So the limitation you described is real. What this PR does improve is the failure and guidance path: when a Hub ONNX path is incorrect, we now surface available ONNX files from that repo in the error hint, so users can recover without guessing blindly. That is a usability improvement, but it is not full discovery. Below is the status as of now:
I will track the full discovery work as follow-up so this does not remain an implicit V1 constraint. |
I ran an informal offline mask-generation eval locally and can share a baseline: Model: onnx-community/sam3-tracker-ONNX (int8 encoder and decoder) mIoU: 0.9494 |
Thanks for pointing this. This support has been added in this PR. The eval path now includes a dedicated mask-generation evaluator and binary-segmentation metrics: Task routing for mask-generation is wired in src/winml/modelkit/eval/evaluate.py. |
Yes, I do have preliminary perf numbers, but they were collected via manual ORT benchmarking scripts rather than wmk perf because of the current shape-hints limitation you mentioned. What I measured so far: Scope: SAM 3 encoder ONNX (pixel_values shape [1,3,1008,1008]), not full end-to-end tracker pipeline. |
I validated against the published onnx-community/sam3-tracker-ONNX reference example (same image and prompt) and confirmed the pipeline is producing close outputs, but I did not run a direct facebook/sam3 PyTorch-to-ONNX parity comparison in this change set. For the reference spot-check, the IoU scores were: Reference: [0.931315, 0.037516, 0.512856] |
Thanks for raising these, In the current branch I have addressed the following:
|
This PR’s functional pieces are done and passing (Hub-ONNX resolution, pre-quantized QDQ/QOperator handling, and test coverage), but I can’t say we have stable end-to-end NPU perf yet in this environment. When I reproduced the suggested flow (winml.modelkit sys + winml.modelkit perf -m microsoft/resnet-50 --device npu --monitor --verbose), sys detected AMD NPU hardware, but perf failed because EP setup did not complete (MIGraphXExecutionProvider download/install failed, resulting EPs were CPU + DML only, then ValueError: Device 'npu' requested but no compatible EP is available). I do have script-based measurements from a different path: SAM3 encoder run (VitisAI + CPU fallback) showed min 32172 ms, p50 33406 ms, mean 33621.8 ms, max 35109 ms; CPU baseline for the same encoder was min 21734 ms, p50 23532 ms, mean 25097.0 ms, max 34219 ms; and ResNet50 script run (VitisAI + CPU fallback) averaged 35.87 ms (min 33.08 ms). We have preliminary mixed-provider numbers from custom scripts, but not a clean reproducible winml perf NPU benchmark in this setup; given that, I propose we merge this PR for correctness and move full end-to-end NPU validation/benchmarking to a dedicated follow-up PR. |
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
PR Review: SAM3 ONNX Hub Support (#324)
This PR is well-structured with a clean ModelInput classifier/resolver design, robust sidecar handling, and excellent error enrichment for missing files. Several issues need attention before merge.
See inline comments for details.
All SAM3-specific scripts have been moved to scripts/models/sam3/, including: mask_generation_eval.py |
--shape-config is now allowed for ONNX files and is applied during dummy input generation, which lets SAM3 ONNX benchmarks override dynamic dims that the positional defaults can’t satisfy. |
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Re-review: SAM3 ONNX Hub Support (Issue #324)
This re-review checks whether previously-raised issues were addressed in code, and surfaces additional findings. All 6 previously-raised issues remain unresolved in code (author replied in comments but made no corresponding commits). New issues are also noted below.
Issues not fixed since initial review
hub_models.json:supported_eps: {}is still empty for both SAM3 entries (catalog EP filtering broken)hub_models.json:model_idstill uses full ONNX path instead oforg/reposchemaonnx_hub.py: hint URL is still hardcoded to/tree/mainregardless ofrevisioneval.py: Hub download failures in_resolve_model_pathare still unhandledmask_generation_evaluator.py: bare dict key access on hardcoded embedding names is still presentmask_generation_evaluator.py: textprompt_modestill silently fails per-sample
New findings
perf.py: Same unhandled exception pattern aseval.pyfornormalize_model_argbuild.py(_build_onnx_pipeline): readsconfig.skip_optimizebut never callsensure_pre_quantized_stamped, allowing double-quantization when-c config.jsonwithquant != Noneis passed with a pre-quantized ONNX model
|
Summary of merge-readiness findings and fixes from this review:
Validation performed locally: registry JSON parses, ruff passes on the changed Python files, |
## Summary - Add QNN NPU recipe configs for `facebook/dinov2-small` and `facebook/dinov2-base` using W8A16, opset 21, and `bias_softmax_fusion`. - Add a QNN NPU recipe config for `microsoft/swinv2-tiny-patch4-window16-256` using fp16/no quantization, opset 21, and `matmul_transpose_fusion`. - Place QNN recipes under model-specific `qnn/npu/` folders while keeping existing config basenames aligned with the other recipes. - Add regression coverage that parses each new recipe through `WinMLBuildConfig.from_dict` and checks the expected opset, optimization flag, task, and quantization shape. ## Performance gains | Model | QNN NPU recipe | Perf gain | Evidence note | |---|---|---:|---| | `facebook/dinov2-small` | W8A16 + opset 21 + `bias_softmax_fusion` | +38.6% total; +14.1% incremental over opset21-only | Direct auto-config finding. | | `facebook/dinov2-base` | W8A16 + opset 21 + `bias_softmax_fusion` | +24-31% DINOv2-family gain | Conservative family-level recipe; no base-specific incremental `bias_softmax_fusion` claim. | | `microsoft/swinv2-tiny-patch4-window16-256` | fp16/no quantization + opset 21 + `matmul_transpose_fusion` | +12% | Direct auto-config finding. | ## Notes - Scope is intentionally limited to the three conservative auto-config findings. - This PR does not add ResNet, SegFormer, or Swin-large recipes. ## Validation - `uv run --no-project --with ruff ruff check tests\unit\recipes\test_qnn_recipes.py` - `PYTHONPATH=src uv run --no-project --with pytest --with pytest-cov --with pytest-timeout --with onnxruntime --with onnx --with pydantic --with rich --with click --with tqdm pytest tests\unit\recipes\test_qnn_recipes.py -q -o filterwarnings= -W "ignore:Could not mock _is_ep_available_locally:UserWarning"` --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Extend the E2E Test pipeline's AMD stage with two new EP/device pairs: - migraphx_gpu -> ep=migraphx (MIGraphXExecutionProvider), device=gpu - trtrtx_gpu -> ep=nv_tensorrt_rtx (NvTensorRTRTXExecutionProvider), device=gpu Add both to the e2e-test-jobs catalog and to the amdPairs default so the perf phase runs them on the NPU-AMD agent. The pytest e2e suites already parametrize these EPs and self-skip via require_ep(), so no test changes are needed.
…inputs (#1063) ## Summary This PR fixes a pattern case serialization bug where input_constraints could carry a dtype that no longer matches the actual input tensors used for model generation and EP run checks. ## Root Cause In OpInputGenerator.iter(), input_constraints was serialized from optional_input_comb before type_var application. For InputValueConstraint(np.ndarray), to_dict() persisted the original array dtype (for example float32), while runtime inputs were generated from get_value(type_annotation=...) after type vars were applied (for example float16). This dtype drift can make persisted cases inconsistent with runtime behavior and lead to mismatches such as: - analyze(single-node): supported - ep_checker.run(pattern-level): false ## Fix - Keep generating applied_input_comb from type-var-applied annotations. - Rebuild input_constraints using applied values for InputValueConstraint: - InputValueConstraint(applied_input_comb[k]).to_dict() - Keep existing serialization behavior for InputShapeConstraint and non-constraint values. - Add inline comments to document why this is required. ## Impact - check_results[].input_constraints now stays consistent with actual generated model/input dtypes. - Improves deterministic replay/signature consistency for persisted cases. - Removes one root-cause category in ov_cpu mismatch investigation. ## Changed File - src/winml/modelkit/pattern/op_input_gen/op_input_gen.py Fixes #1062
This PR contains 1. Fix bugs found while running evaluation for depth-estimation and keypoint-detection. The issues causes evaluation errors during inference. 2. Run the evaluation and record the baseline cache. The baseline cache aligns with published or community shared metric values
## Summary Unifies the two e2e evaluation scripts (`run_eval.py` and the example-only `run_example_tests.py`) into a single recipe-driven runner: build a model, run `winml perf`, and — when perf passes — run `winml eval`, writing one unified `eval_result.json` per (model, task, precision). Also adds two `--clean-cache` disk-hygiene fixes so long matrix runs don't fill the disk. ## Recipe-driven runner - Add `utils/recipes.py`: data-driven discovery of `examples/recipes/<slug>` configs (fp16/w8a16/w8a8), single and composite (encoder/decoder, image/text-encoder). - `run_eval.py` expands the registry into one job per recipe precision variant; builds via `winml build -c`, falling back to `winml config` when a model has no recipe. Perf-gated accuracy reuses the existing both-mode gating. - Accuracy phase now only runs `winml eval` and records the full metrics + dataset; delta/verdict grading vs the PyTorch baseline is left to the report site. Inline baseline removed; offline `--update-baseline` maintains the baseline cache. - Remove runner-side report generation (`generate_report.py`, `analyze_results.py`); slim `reporter.py`/`accuracy.py` (keep `compute_delta` + thresholds for the site). Drop the in-pipeline `generate_report` step. ## Disk hygiene under `--clean-cache` - **Per-job build artifacts:** remove a job's `export/optimized/model.onnx` (+ `.data`/`.bin`) once its `eval_result.json` is written, keeping only the JSON the report site reads — peak disk stays at ~one job. - **Stray ORT/QNN scratch models:** sweep temporary models ORT/QNN leak into the working directory (UUID-named `<uuid>.onnx`/`.data`, QNN EP-context dumps `<uuid>.onnx_<EP>.bin`, and `sym_shape_infer_temp.onnx`). Matches only UUID-prefixed or the fixed temp name so real files are never touched; runs at startup and after each job. ## Notes - Rebased onto latest `main`; the prerequisite build-only EP-matrix work landed separately in #845, so this branch carries only the 3 commits for this change.
Adds SAM 3 support and introduces a generic Hub-hosted ONNX input form (
<org>/<repo>/<path>.onnx) that downloads pre-exported ONNX files from HuggingFace viahuggingface_hub. SAM 3 is the first consumer.The standard SAM 2-style export route is blocked:
optimum-onnxpinstransformers<4.58, butfacebook/sam3requirestransformers>=5. This PR uses the pre-exportedonnx-community/sam3-tracker-ONNXartifacts via the existing Scenario D pipeline.Changes
loader/onnx_hub.py)wmk config,wmk build,wmk inspect,wmk run,wmk serve,wmk perf,wmk evalis_quantized_onnxto detect QOperator format (ConvInteger,MatMulInteger,QLinear*) — was QDQ-onlyis_pre_quantizedbuild branch to truly skip the optimize stage (previously crashed on QOperator models)Tests
Limitations
ConvIntegerkernel inonnxruntime-windowsml); decoder runs on CPU + NPU.