Skip to content

fix: migrate EPAlias to EPNameOrAlias for better alignment; fix infer_ihv_from_ep_name#1005

Merged
xieofxie merged 6 commits into
mainfrom
hualxie/literal_to_type
Jul 1, 2026
Merged

fix: migrate EPAlias to EPNameOrAlias for better alignment; fix infer_ihv_from_ep_name#1005
xieofxie merged 6 commits into
mainfrom
hualxie/literal_to_type

Conversation

@xieofxie

@xieofxie xieofxie commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Close #646 (migrate EPAlias to EPName where possible). EPNameOrAlias is the public-facing input type; EPName is the internal canonical form. This PR removes places where a bare alias was used where a canonical name belongs, and stops comparing against single alias literals.

Changes

  • analyze/utils/ep_utils.pyinfer_ihv_from_ep_name now does a direct, exact dict[EPName, IHVType] lookup covering every member of the EPName Literal. An unknown name now raises ValueError instead of silently defaulting to IHVType.MICROSOFT (the callers that matter already handle it: InformationEngine catches ValueError and falls back to loading all rules; the model validator catches it defensively). Also fixes a latent miss where CUDAExecutionProvider resolved to MICROSOFT instead of NVIDIA.
  • serve/schema.pyEpSwitchRequest.ep was the only ep field still typed bare EPAlias; every sibling is EPNameOrAlias and manager.switch_ep already accepts EPNameOrAlias. Widened it for consistency so it accepts both "qnn" and "QNNExecutionProvider".
  • compiler/utils.pyneeds_format_conversion compared ep == "qnn", a single alias literal. An EP can have multiple aliases (e.g. nv_tensorrt_rtx / nvtensorrtrtx), and the canonical name itself wouldn't match. Now normalizes via normalize_ep_name and compares against the canonical "QNNExecutionProvider"; parameter widened to EPNameOrAlias.

Notes

  • The compiler's EPConfig.provider deliberately remains alias-typed: every value it holds is an alias (provider="qnn", etc.), so retyping to EPName would require a value migration with serialization back-compat — out of scope here.
  • An audit of all ~90 EPNameOrAlias call sites confirmed every one either normalizes before a canonical sink or forwards to a callee that does — no other un-normalized canonical-sink usages were found.

Tests

  • Replaced the per-EP infer_ihv unit tests with one that verifies every EP_NAMES member resolves to an IHVType (enforcing map/Literal parity) plus an unknown-raises test.
  • Updated fixtures that relied on the old lenient behavior to use canonical EP names (ACEExecutionProviderVitisAIExecutionProvider; alias "openvino""OpenVINOExecutionProvider" in the validator tests).
  • Added test_qlinear_for_qnn_canonical_name covering the canonical-name case that previously failed in needs_format_conversion.

hualxie added 2 commits June 30, 2026 16:40
Compare against the canonical EPName instead of a single alias literal
(ep == "qnn"). An EP can have multiple aliases (e.g. nv_tensorrt_rtx /
nvtensorrtrtx) and the canonical name itself would not match, so the
alias-literal comparison was fragile. Normalize via normalize_ep_name
first and widen the parameter to EPNameOrAlias.
@xieofxie xieofxie requested a review from a team as a code owner June 30, 2026 09:09
hualxie and others added 2 commits June 30, 2026 17:27
The map covers every canonical EPName, so an unknown name now signals a
bug rather than silently defaulting to IHVType.MICROSOFT. InformationEngine
already catches ValueError (falls back to loading all rules) and the model
validator catches it defensively.

Replace the per-EP unit tests with one that verifies every EP_NAMES member
resolves to an IHVType (enforcing map/Literal parity) plus an unknown-raises
test. Update fixtures that relied on the old lenient behavior to use
canonical EP names.
@xieofxie xieofxie changed the title fix: migrate EPAlias to EPName where possible (#646) fix: migrate EPAlias to EPNameOrAlias where possible (#646) Jul 1, 2026
@xieofxie xieofxie changed the title fix: migrate EPAlias to EPNameOrAlias where possible (#646) fix: migrate EPAlias to EPNameOrAlias for better alignment; fix infer_ihv_from_ep_name Jul 1, 2026

@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.

Reviewed the EPAlias -> EPNameOrAlias migration and the infer_ihv_from_ep_name rework. Overall this is a clean, well-motivated change — the map/Literal-parity test is a nice touch, and catching CUDAExecutionProvider -> NVIDIA (was silently MICROSOFT) is a good latent-bug fix.

One concern about the new raise on unknown EPs and one un-audited caller (output_aggregator.build_ep_support) — left inline. Not necessarily blocking, but worth resolving for consistency with the other two call sites.

Comment thread src/winml/modelkit/analyze/utils/ep_utils.py Outdated
xieofxie added 2 commits July 1, 2026 16:07
Narrow inferred IHVType into a non-optional local before .value access, and annotate _model_type as str | None so the reordered composite-type branch type-checks.

@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.

Re-reviewed the two new commits (use IHVType.UNKNOWN instead of raise, resolve mypy union-attr and assignment narrowing).

My earlier concern is fully resolved. Switching infer_ihv_from_ep_name to return IHVType.UNKNOWN instead of raising makes inference total, so all three call sites — including the previously-unguarded output_aggregator.build_ep_support — are now safe. Verified end to end: UNKNOWN only flows into a debug log there; InformationEngine maps UNKNOWN -> None (loads all rules); the redundant try/except in BatchedConstMatMulValidator is correctly removed since UNKNOWN != INTEL. Tests updated accordingly and CI is green (lint/mypy + all test shards + CodeQL).

Two minor, non-blocking notes:

  1. rule_loader.load_runtime_rules(None) iterates for ihv in IHVType, which now includes UNKNOWN, producing one extra No rule files found for Unknown ... skipping warning on the load-all path. It's harmless and exactly mirrors the existing behavior for MICROSOFT (also absent from prefix_map), so no change needed — just flagging that adding an enum member has this ripple effect.

  2. models/auto.py is unrelated to the EP migration — it's a legit mypy narrowing fix (_model_type: str | None) bundled into the mypy commit, and it also defers AutoConfig.from_pretrained until there's no explicit model_type override (a small behavioral improvement). Fine to keep; noting it only because it's outside the PR's stated scope.

LGTM from my side.

@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.

Approving. My earlier concern about the unguarded output_aggregator.build_ep_support caller is fully resolved by making infer_ihv_from_ep_name total (returns IHVType.UNKNOWN instead of raising), which cleanly removes the inconsistency across all three call sites. Verified end to end, and CI is green (lint/mypy + all test shards + CodeQL). The two remaining items I noted (the harmless Unknown warning in rule_loader's load-all path, and the unrelated auto.py mypy fix) are non-blocking.

@xieofxie xieofxie merged commit dc7d1e3 into main Jul 1, 2026
13 of 14 checks passed
@xieofxie xieofxie deleted the hualxie/literal_to_type branch July 1, 2026 09:12
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.

migrate EPAlias to EPName where possible

2 participants