Skip to content

feat: merge windowsml usage using official example#729

Merged
xieofxie merged 3 commits into
mainfrom
hualxie/merge_windowsml
May 26, 2026
Merged

feat: merge windowsml usage using official example#729
xieofxie merged 3 commits into
mainfrom
hualxie/merge_windowsml

Conversation

@xieofxie
Copy link
Copy Markdown
Contributor

@xieofxie xieofxie commented May 25, 2026

@xieofxie xieofxie requested a review from a team as a code owner May 25, 2026 08:56
Comment thread src/winml/modelkit/session/ep_registry.py
@timenick
Copy link
Copy Markdown
Collaborator

timenick commented May 26, 2026

Quick question on the switch to with EpCatalog() as catalog: — the old code at ep_registry.py:92-104 had a long workaround comment explaining that WinMLEpCatalogRelease (called from EpCatalog.close() / __del__) crashes with ACCESS_VIOLATION (0xC0000005) on some QNN NPU driver configurations — a Windows SEH exception that Python try/except cannot catch. The workaround was to null out self._catalog._handle = None so close() becomes a no-op.

The new with EpCatalog() as catalog: still invokes close() via __exit__, which puts that crash path back — just relocated from interpreter shutdown to the return of _load_ep_catalog.

Two questions:

  1. Has the underlying WinMLEpCatalogRelease issue been fixed upstream in windowsml? The original TODO said "Remove once windowsml fixes WinMLEpCatalogRelease to be safe during process teardown."
  2. If not, has winml perf --device npu been verified on a QNN NPU machine after this change to confirm the crash doesn't return?

Side note: this PR also incidentally fixes a bug introduced by #712 — the two independent singletons (WinML and WinMLEPRegistry) both call ort.register_execution_provider_library(...) but dedup separately, so after #712 winml perf --device npu emits a library is already registered under QNNExecutionProvider traceback (benign but alarming, swallowed by except Exception). Worth calling out in the PR title/description so it shows up in git log --grep later.

Generated with Claude Code

@xieofxie
Copy link
Copy Markdown
Contributor Author

Quick question on the switch to with EpCatalog() as catalog: — the old code at ep_registry.py:92-104 had a long workaround comment explaining that WinMLEpCatalogRelease (called from EpCatalog.close() / __del__) crashes with ACCESS_VIOLATION (0xC0000005) on some QNN NPU driver configurations — a Windows SEH exception that Python try/except cannot catch. The workaround was to null out self._catalog._handle = None so close() becomes a no-op.

The new with EpCatalog() as catalog: still invokes close() via __exit__, which puts that crash path back — just relocated from interpreter shutdown to the return of _load_ep_catalog.

Two questions:

  1. Has the underlying WinMLEpCatalogRelease issue been fixed upstream in windowsml? The original TODO said "Remove once windowsml fixes WinMLEpCatalogRelease to be safe during process teardown."
  2. If not, has winml perf --device npu been verified on a QNN NPU machine after this change to confirm the crash doesn't return?

Side note: this PR also incidentally fixes a bug introduced by #712 — the two independent singletons (WinML and WinMLEPRegistry) both call ort.register_execution_provider_library(...) but dedup separately, so after #712 winml perf --device npu emits a library is already registered under QNNExecutionProvider traceback (benign but alarming, swallowed by except Exception). Worth calling out in the PR title/description so it shows up in git log --grep later.

Generated with Claude Code

from my understanding, this is the official usage of windowsml lib https://pypi.org/project/windowsml/ I feel it should be well tested so that access violation should not exist

@xieofxie xieofxie merged commit 1f027a3 into main May 26, 2026
9 checks passed
@xieofxie xieofxie deleted the hualxie/merge_windowsml branch May 26, 2026 02:32
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.

2 participants