Skip to content

fix: config validates device/EP combination without system check#780

Merged
xieofxie merged 8 commits into
mainfrom
hualxie/config_resolve
May 28, 2026
Merged

fix: config validates device/EP combination without system check#780
xieofxie merged 8 commits into
mainfrom
hualxie/config_resolve

Conversation

@xieofxie

Copy link
Copy Markdown
Contributor

Summary

  • Adds resolve_check_device_ep helper that validates a (device, EP) combination without requiring the device/EP to actually exist on the system. Closes config: should not use resolve_device #765.
  • commands/config.py and config/build.py now use resolve_check_device_ep instead of resolve_device so winml config no longer hard-fails on hosts where the requested EP isn't installed.
  • When device=auto or ep=None, the helper delegates to the existing resolve_device + resolve_eps flow (system-aware behavior preserved). When both device and ep are explicit, it only validates against the static EP_SUPPORTED_DEVICES mapping.
  • CLI cleanup: -m/--model, -c/--config, --device for the config command now use the shared cli_utils.*_option decorators.

Tests

  • New TestResolveCheckDeviceEp class in tests/unit/sysinfo/test_device.py covering both code paths (delegation and static-only) plus error cases (unknown EP, unsupported device, case-insensitivity).
  • Existing config-test mocks updated from resolve_device to resolve_check_device_ep (tests/unit/config/conftest.py, tests/unit/config/test_build.py, tests/unit/config/test_build_onnx.py, tests/unit/commands/test_config_cli.py) so the lazy import in config/build.py is intercepted.

🤖 Generated with Claude Code

@xieofxie xieofxie requested a review from a team as a code owner May 28, 2026 06:30
hualxie added 2 commits May 28, 2026 15:21
@DingmaomaoBJTU

Copy link
Copy Markdown
Collaborator

Code review

Found 1 issue:

  1. IndexError on _resolved_eps[0] when no EPs are installed -- In commands/config.py, the old code guarded EP display with if ep:, only printing when the user explicitly passed --ep. The new code unconditionally accesses _resolved_eps[0]. When device="auto" (default) and ep=None (not specified), resolve_check_device_ep delegates to resolve_eps(resolved_device), which returns [] on systems with no EPs registered (CI runners, fresh machines, builds without the Windows ML ORT extensions). The unconditional _resolved_eps[0] then raises IndexError: list index out of range, crashing winml config on the most common invocation pattern. The test mock always returns ["QNNExecutionProvider"], so this path is untested.

console.print(f" EP: [cyan]{_resolved_eps[0]}[/cyan]")
# Quant types — display exactly what config contains
if _quant:
console.print(

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

timenick

This comment was marked as outdated.

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

One nit on the new helper. The design intent is clear and well-documented in the PR description; only flagging this because the function returns the same tuple shape from two paths with different semantics.

Comment thread src/winml/modelkit/sysinfo/device.py
@xieofxie

Copy link
Copy Markdown
Contributor Author

Code review

Found 1 issue:

  1. IndexError on _resolved_eps[0] when no EPs are installed -- In commands/config.py, the old code guarded EP display with if ep:, only printing when the user explicitly passed --ep. The new code unconditionally accesses _resolved_eps[0]. When device="auto" (default) and ep=None (not specified), resolve_check_device_ep delegates to resolve_eps(resolved_device), which returns [] on systems with no EPs registered (CI runners, fresh machines, builds without the Windows ML ORT extensions). The unconditional _resolved_eps[0] then raises IndexError: list index out of range, crashing winml config on the most common invocation pattern. The test mock always returns ["QNNExecutionProvider"], so this path is untested.

console.print(f" EP: [cyan]{_resolved_eps[0]}[/cyan]")
# Quant types — display exactly what config contains
if _quant:
console.print(

🤖 Generated with Claude Code

  • If this code review was useful, please react with 👍. Otherwise, react with 👎.

for a resolved device, there should always be ep

@xieofxie xieofxie merged commit 0e1b16e into main May 28, 2026
9 checks passed
@xieofxie xieofxie deleted the hualxie/config_resolve branch May 28, 2026 09:46
DingmaomaoBJTU pushed a commit that referenced this pull request Jun 5, 2026
## Summary

- Adds `resolve_check_device_ep` helper that validates a (device, EP)
combination without requiring the device/EP to actually exist on the
system. Closes #765.
- `commands/config.py` and `config/build.py` now use
`resolve_check_device_ep` instead of `resolve_device` so `winml config`
no longer hard-fails on hosts where the requested EP isn't installed.
- When `device=auto` or `ep=None`, the helper delegates to the existing
`resolve_device` + `resolve_eps` flow (system-aware behavior preserved).
When both `device` and `ep` are explicit, it only validates against the
static `EP_SUPPORTED_DEVICES` mapping.
- CLI cleanup: `-m/--model`, `-c/--config`, `--device` for the config
command now use the shared `cli_utils.*_option` decorators.

## Tests

- New `TestResolveCheckDeviceEp` class in
`tests/unit/sysinfo/test_device.py` covering both code paths (delegation
and static-only) plus error cases (unknown EP, unsupported device,
case-insensitivity).
- Existing config-test mocks updated from `resolve_device` to
`resolve_check_device_ep` (`tests/unit/config/conftest.py`,
`tests/unit/config/test_build.py`,
`tests/unit/config/test_build_onnx.py`,
`tests/unit/commands/test_config_cli.py`) so the lazy import in
`config/build.py` is intercepted.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: hualxie <hualxie@microsoft.com>
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.

config: should not use resolve_device

3 participants