Skip to content

fix(perf): support composite (dual-encoder) models in winml perf#866

Merged
xieofxie merged 20 commits into
mainfrom
hualxie/fix_siglip
Jun 16, 2026
Merged

fix(perf): support composite (dual-encoder) models in winml perf#866
xieofxie merged 20 commits into
mainfrom
hualxie/fix_siglip

Conversation

@xieofxie

@xieofxie xieofxie commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Problem

winml perf crashed on composite (dual-encoder) models such as SigLIP/CLIP:

winml perf --ep openvino --device cpu -m google/siglip-base-patch16-224 \
  --task zero-shot-image-classification
...
AttributeError: 'WinMLModelForZeroShotImageClassification' object has no attribute 'io_config'

PerfBenchmark assumed every model exposes a single io_config / _session. Composite
models have neither — they orchestrate multiple sub-models (e.g. an image encoder and a
text encoder), each with its own ONNX session. The failure is device-independent: the
(model_type, task) registry routes SigLIP to the composite class regardless of --device.

Fix

Benchmark each sub-model individually — the same way --module benchmarks each
submodule — instead of timing the aggregate forward() pass:

  • run() returns a single BenchmarkResult for single-session models, or a
    {sub_model_name: BenchmarkResult} mapping for composites.
  • _run_sub_models() iterates sub_models and benchmarks each through the standard
    single-session pipeline by spawning a child PerfBenchmark with the already-loaded
    sub-model. Every sub-model reuses the existing compile / input-generation / simple +
    monitored loops / result-collection code, so per-sub-model measurement semantics match
    single-model runs exactly (pure-ORT time inside session.perf()).
  • Each sub-model reports its own I/O, task, device, and EP — no aggregation.
  • report_composite_results() renders a per-sub-model summary table and writes one
    combined JSON nesting each sub-model's full BenchmarkResult.to_dict() under
    components (model_id, task, component_count, components).

This removes the previous aggregate machinery (_aggregate_io_config,
_describe_outputs, _probe_composite_outputs, _composite_run_iteration) and the
composite branches inside the benchmark loops.

_is_composite stays duck-typed on sub_models (rather than
isinstance(..., WinMLCompositeModel)) on purpose: an isinstance check needs a runtime
import of composite_model, which imports torch and would blow the winml perf --help
import budget (tests/cli/test_import_time.py). Keeping WinMLCompositeModel a
TYPE_CHECKING-only import also lets the unit tests use lightweight duck-typed fakes.

Result

Sub-model: image-encoder

Device:      cpu / OpenVINOExecutionProvider
Task:        image-feature-extraction
Inputs:      pixel_values         [1, 3, 224, 224]       float32
Outputs:     last_hidden_state    [1, 196, 768]
             pooler_output        [1, 768]


Sub-model: text-encoder

Device:      cpu / OpenVINOExecutionProvider
Task:        feature-extraction
Inputs:      input_ids            [1, 64]                int32
Outputs:     last_hidden_state    [1, 64, 768]
             pooler_output        [1, 768]


                                                    Per-Sub-Model Perf
┏━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━┓
┃ Sub-Model     ┃ Task                     ┃ Device                          ┃ Mean (ms) ┃ P90 (ms) ┃ Min (ms) ┃ Max (ms) ┃
┡━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━┩
│ image-encoder │ image-feature-extraction │ cpu / OpenVINOExecutionProvider │     81.14 │   104.97 │    63.49 │   124.89 │
│ text-encoder  │ feature-extraction       │ cpu / OpenVINOExecutionProvider │     39.19 │    51.89 │    24.99 │   109.08 │
└───────────────┴──────────────────────────┴─────────────────────────────────┴───────────┴──────────┴──────────┴──────────┘

Tests

tests/unit/commands/test_perf_composite.py is rewritten for the per-sub-model behavior:
each sub-model produces its own BenchmarkResult with its own I/O / task / device / EP,
every sub-session is compiled and run warmup + iterations times, and
report_composite_results emits the combined per-component JSON (and JSON-mode stdout).
Existing test_perf_cli.py and test_perf_optracing_cli.py still pass — no regression.

🤖 Generated with Claude Code

`winml perf` assumed every model exposes a single `io_config`/`_session`,
so composite models (CLIP/SigLIP zero-shot-image-classification) crashed
with `AttributeError: ... has no attribute io_config` during input
generation.

Make `PerfBenchmark` composite-aware:
- `_aggregate_io_config()` unions the sub-models inputs (their union is
  exactly the composite forward() kwargs) for input generation/display.
- Time the full `forward()` pass via an external PerfStats; single-session
  models keep recording pure-ORT time inside session.perf(). The monitored
  loop is refactored to take a run-iteration callable so both paths share it.
- Device/EP/task are resolved from a representative sub-model.
- `_probe_composite_outputs()` runs one forward() and introspects the result
  so reported outputs are the composite task-level tensors (e.g.
  logits_per_image) rather than a deduped union of sub-model ONNX outputs.

Add tests/unit/commands/test_perf_composite.py covering aggregation, output
describing/probing, input generation, device/EP/task resolution, and the
full-forward timing path.
@xieofxie xieofxie requested a review from a team as a code owner June 10, 2026 09:21
Comment thread src/winml/modelkit/commands/perf.py Fixed

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall this PR is well-structured and the approach of duck-typing on sub_models (to avoid torch import at load time) is a reasonable trade-off with a clear explanation. A few issues worth addressing before merge.

Comment thread src/winml/modelkit/commands/perf.py Outdated
Comment thread src/winml/modelkit/commands/perf.py Outdated
Comment thread src/winml/modelkit/commands/perf.py
Comment thread src/winml/modelkit/commands/perf.py
Comment thread tests/unit/commands/test_perf_composite.py
hualxie added 4 commits June 16, 2026 11:33
- Tighten _is_composite duck-type check with isinstance(dict) guard
- Promote _sub_models to a property for accessor consistency
- Isolate per-sub-model failures: log failing sub-model name before re-raise
- Warn when --op-tracing is passed for composite models (silently skipped)
- Reference WinMLCompositeModel via a bare local annotation to satisfy CodeQL
- Add empty sub_models edge-case tests (run + report)
Replace the duck-typed sub_models check with a concrete
isinstance(..., WinMLCompositeModel) check, imported function-locally so
composite_model (and torch) is only imported when a model is benchmarked,
never at module load — keeping the winml perf --help import budget intact.
Test fake now subclasses WinMLCompositeModel so the isinstance check matches.
The _run helper in TestPerfModuleQuantCompileToggles still mocked the old
3-tuple return of resolve_loader_config; it now returns 4 values
(loader_config, hf_config, resolved_class, resolution) to match the current
signature, fixing the unpack error in the perf --module path.

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Follow-up review: 4 of 5 previous findings are fully resolved. One remaining concern on error isolation (see inline comments), and one new minor issue introduced via the merge.

Comment thread src/winml/modelkit/commands/perf.py

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

New info-level issue (not inlineable — modified line from merge context): perf.py:719 — _resolution variable is named with an underscore prefix, signalling ''intentionally unused'', yet occupies a named binding. Use plain _ as the fourth discard: parent_loader_cfg, _, _, _ = resolve_loader_config(...). If the TaskResolution value is needed later, rename it at that point without the leading underscore.

Re-raise as RuntimeError("Sub-model '<name>' failed: <exc>") from exc so
the CLI error itself names the failing sub-model, instead of a bare re-raise
that only surfaced the name in logs. Add a test asserting the RuntimeError
names the component and chains the original exception via __cause__.

@DingmaomaoBJTU DingmaomaoBJTU left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

4/5 previous findings are fully resolved. The two remaining items are non-blocking:

  • perf.py:374 (warning, partial): \logger.error\ now names the failing sub-model, which is good. The bare
    aise\ still surfaces the original exception without sub-model context at the CLI, but this is a UX improvement rather than a correctness issue — the log output is sufficient for diagnosis.
  • perf.py:719 (info): _resolution\ could be plain _\ since it's unused, but this is cosmetic.

Core logic is correct: sub-models are benchmarked independently, results are keyed per component, the duck-type check is properly tightened, the --op-tracing\ gap is closed, and the test coverage is solid. Approving.

@xieofxie xieofxie merged commit 26fcc88 into main Jun 16, 2026
9 checks passed
@xieofxie xieofxie deleted the hualxie/fix_siglip branch June 16, 2026 06:23
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.

3 participants