Fix QNN E2E evaluation failures - #1247
Conversation
Parse wrapped winml build artifact paths so successful builds are not misreported as export failures, and classify QNN backend graph-finalization rejections as unsupported perf skips instead of relying on registry target blacklists. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Temporarily switch HF-style attention configs to eager only while torch.onnx.export runs, then restore the original attention implementation. This keeps transformers 5 exports compatible with QNN by avoiding SDPA mask guard graphs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move the eager-attention export helper into transformers_compat so transformers 5 export compatibility behavior stays in one module while preserving lazy import behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Treat Windows native access violation exits from winml build as non-fatal only when the completed ONNX artifact can be resolved from the build output. This preserves real build failures while allowing QNN eval jobs to continue after interpreter teardown crashes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Document the generic export compatibility policy design for EP/device-targeted export overrides and portable default target merging. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add a task-by-task implementation plan for the EP/device export compatibility policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Capture perf target explicitness before EP resolution so the implementation plan preserves portable export defaults. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <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>
Move export compatibility target derivation into build config generation so command paths no longer pass export-policy-specific target state. Preserve portable-default semantics separately from resolved runtime EP/device handles and round-trip resolved empty compatibility configs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
KayMKM
left a comment
There was a problem hiding this comment.
Reviewed the branch locally (24 files, +1144/-46), ran the new tests (234 passed), and empirically verified two of the findings below.
What's good
Single choke point (HTPExporter._convert_model_to_onnx is the only torch.onnx.export caller), data-driven JSON rules instead of hardcoded model logic, compatibility persisted into the config so it participates in cache keys, and solid serialization/validation coverage.
Findings
1. use_eager_attention_for_export restore is lossy (real bug)
src/winml/modelkit/transformers_compat.py:49
Transformers 5's _attn_implementation setter recurses into sub_configs (configuration_utils.py:401-417). Setting the parent config overwrites its children before the loop reaches them, so the children's original values are never recorded in restored. Verified against transformers 5.14.1 with a CLIP config:
before: sdpa flash_attention_2
inside: eager eager
after : sdpa sdpa # text_config silently downgraded, not restored
Suggested fix: snapshot every reachable config's value in a first pass, mutate in a second pass, then restore all of them. Alternatively use the public model.set_attn_implementation() (present in 5.14.1, modeling_utils.py:2176) instead of poking the private attribute.
2. Auto path no longer forwards the resolved target into build-config generation
src/winml/modelkit/models/auto.py:375-380, src/winml/modelkit/commands/build.py:1027-1038
When neither device nor ep is explicit, generate_hf_build_config now receives device="auto", ep=None. That makes _resolve_policy_target (config/build.py:324) re-resolve the target via auto_detect_device() + EP_SUPPORTED_DEVICES, while the runtime target is resolved via resolve_device() (session/ep_device.py:686), which iterates EP_DEVICE_SPECS in catalog priority order and never consults auto_detect_device().
Two different algorithms now decide the same thing, so build-time quant/compile policy can select a different device than the session actually runs on. The comment deleted in this PR ("Forwarding both concrete axes keeps analyzer/build output aligned with the target selected by the catalog-backed resolver") was guarding exactly this case.
Could you confirm the two resolvers cannot diverge, or thread the resolved runtime target through separately from the export-policy target? The export policy needs the request; quant/compile policy still needs the resolved target.
3. PR description is stale
Commit b782d681 ("chore: prune qnn export policy changes") reverted the E2E scripts, _native_ep_registration.py, analyze/analyzer.py, loader/hf.py, and optracing/qnn/profiler.py. Summary bullets 3 and 4 describe code that is no longer in the diff — worth trimming so reviewers and the changelog match reality.
4. The "portable default" is effectively "always eager"
QNN is in EP_DEVICE_SPECS, so resolve_export_compatibility(None) always resolves to eager. Net effect:
winml build -m X→ eager attentionwinml build -m X --device gpu --ep dml→ SDPA
Two different ONNX graphs (and different numerics) for what resolves to the same runtime target on a GPU machine. The same flip happens on eval's CPU fallback (eval/evaluate.py:343), which sets device="cpu"/ep="cpu", turning the request explicit and forcing a full re-export on the retry.
If eager is the portable answer for unspecified targets, is the explicit-target branch earning its complexity? Worth stating the intended contract explicitly.
5. Cache-key churn from {} vs. absent
Verified locally:
unresolved default : 6c13c2bc15df31cf
resolved-empty ({}) : cdb71a905c9bb3c7
Both produce byte-identical exports, but the is_resolved bookkeeping leaks into artifact identity because to_dict() emits "compatibility": {} for a resolved-but-empty config and omits the key otherwise. Consider tracking "policy already applied" out of band so it does not participate in the cache key.
6. Docs not updated
docs/reference/index.md:47-60 documents the export config fields; compatibility is missing from the table.
Nits
TransformersAttentionPolicy = Literal["eager"](export/policy.py:24) is defined but never used; the field is typedstr | None.EXPORT_COMPATIBILITY_RULES = load_export_compatibility_rules()runs at module scope, so importingexport/config.py(widely imported) does JSON file I/O, and a malformed resource becomes an import-time crash. A lazyfunctools.cacheaccessor would be safer.is_resolvedparticipates in__eq__, and the__bool__overload doubling as "policy already resolved" is subtle enough that an explicitis_resolvedcheck at the three call sites would read better._coerce_target/Sequence[object]is dead generality now that onlyExportPolicyTargetis ever passed (after4c5e8a06).- Vestigial assertions that are always true and test nothing:
assert "export_policy_targets" not in mock_gen.call_args.kwargs(tests/unit/commands/test_build.py,tests/unit/commands/test_perf_module.py) andassert "export_target_was_explicit" not in received(tests/unit/commands/test_perf_cli.py) — those kwargs no longer exist anywhere in the codebase.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use the resolved runtime EP/device for quantization and compile policy while preserving the original request/default target for export compatibility policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1. Finding 2 is still live on the
|
Split build CLI runtime targets from export policy targets, restore nested attention configs in topology order, and narrow export compatibility targets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
One spot looks like it was missed by the same split. Composite sub-model branch still generates configs from the raw request
component_config = gen_cfg(
model,
task=component_task,
trust_remote_code=trust_remote_code,
device=device,
precision=precision,
ep=ep_value,
...
)Every other Before this PR that was harmless, because Suggested fix: component_config = gen_cfg(
model,
task=component_task,
trust_remote_code=trust_remote_code,
device=runtime_device,
precision=precision,
ep=runtime_ep_value,
export_policy_target=(request_device, request_ep_value),
...
)A composite regression test asserting the component configs see the resolved runtime target would keep this from drifting again. |
Use resolved runtime targets for composite component config generation while preserving the original request for export compatibility policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Validation
uv run pytest tests\unit\config\test_build.py tests\unit\export\test_export_policy.py tests\unit\export\test_config_validation.py tests\unit\export\test_htp_exporter_attention_compat.py tests\unit\models\auto\test_auto_onnx.py tests\unit\models\auto\test_composite_model_type_routing.py tests\unit\models\winml\test_composite_from_pretrained.py tests\unit\commands\test_build.py tests\unit\commands\test_export.py tests\unit\commands\test_perf_cli.py tests\unit\commands\test_perf_module.py tests\unit\test_uv_lock.py -q— 548 passeduv run --with ruff ruff check ...— passeduv run --with ruff ruff format --check ...— passed for Python changesuv run --with mypy --with types-colorama mypy -p winml.modelkit— passeduv lock --check— passedgit diff --check— passedscripts\e2e_eval\run_eval.py --eval-type perf— passed:openai/clip-vit-base-patch32,feature-extraction, QNN GPUopenai/clip-vit-base-patch32,zero-shot-image-classification, QNN GPUgoogle-bert/bert-base-multilingual-cased,feature-extraction, QNN GPUgoogle-bert/bert-base-multilingual-cased,fill-mask, QNN GPUgoogle-bert/bert-base-multilingual-cased,masked-lm, QNN GPUgoogle-bert/bert-base-multilingual-cased,fill-mask, QNN NPUgoogle/flan-t5-base, QNN GPU