Skip to content

Fix Qwen3 genai bundle component builds - #1248

Merged
DingmaomaoBJTU merged 5 commits into
mainfrom
dingmaomaobjtu-convert-qwen3-model
Jul 30, 2026
Merged

Fix Qwen3 genai bundle component builds#1248
DingmaomaoBJTU merged 5 commits into
mainfrom
dingmaomaobjtu-convert-qwen3-model

Conversation

@DingmaomaoBJTU

Copy link
Copy Markdown
Collaborator

Summary

  • Preserve quantization for raw graphs that intentionally skip ORT optimization.
  • Add a build-only path for HF/composite builds so genai bundle assembly consumes ONNX artifacts without creating runtime sessions.
  • Use the build-only component path in genai bundle orchestration and cover the Qwen3 regression cases.

Validation

  • uv run ruff check src\winml\modelkit\build\common.py src\winml\modelkit\commands\build.py src\winml\modelkit\config\build.py src\winml\modelkit\models\auto.py src\winml\modelkit\models\winml\composite_model.py src\winml\modelkit\models\winml\genai_bundle.py src\winml\modelkit\models\hf\qwen3\qwen_transformer_only.py tests\unit\build\test_hf.py tests\unit\build\test_onnx.py tests\unit\commands\test_build.py tests\unit\models\auto\test_from_pretrained_ep.py tests\unit\models\qwen3\test_qwen3_modeling.py tests\unit\models\winml\test_genai_bundle_orchestrator.py
  • uv run pytest tests\unit\build\test_hf.py tests\unit\build\test_onnx.py tests\unit\commands\test_build.py tests\unit\models\auto\test_from_pretrained_ep.py tests\unit\models\qwen3\test_qwen3_modeling.py tests\unit\models\winml\test_genai_bundle_orchestrator.py
  • x64 cmd full rebuild of Qwen/Qwen3-0.6B with --export-type optimized --ep qnn --device npu --rebuild exits 0 and produces matching bundle hashes.

@DingmaomaoBJTU
DingmaomaoBJTU requested a review from a team as a code owner July 29, 2026 06:27
@DingmaomaoBJTU
DingmaomaoBJTU force-pushed the dingmaomaobjtu-convert-qwen3-model branch 2 times, most recently from 8fc63fa to ceed825 Compare July 29, 2026 07:32
Preserve quantization when raw graphs skip ORT optimization, and make genai bundle assembly use private artifact builds so bundle export does not create runtime sessions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@DingmaomaoBJTU
DingmaomaoBJTU force-pushed the dingmaomaobjtu-convert-qwen3-model branch from ceed825 to e176488 Compare July 29, 2026 09:50
Comment thread src/winml/modelkit/build/hf.py Fixed

@xieofxie xieofxie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two correctness issues need to be addressed before merge: the Qwen skip-optimization setting is discarded during config assembly, and skip_optimize incorrectly suppresses explicit quantization.

Comment thread src/winml/modelkit/models/hf/qwen3/qwen_transformer_only.py
Comment thread src/winml/modelkit/commands/build.py Outdated
@KayMKM

KayMKM commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Heads-up on the src/winml/modelkit/ep_path.py part of this PR (the removal of
_release_winml_catalog and its atexit.register(...)). It is not mentioned in
the Summary and ep_path.py / test_winml_catalog_source.py are not in the
Validation lists, so flagging it in case it was incidental.

It does not stop the native catalog release from running at shutdown.

_get_catalog() is @functools.cached, so the catalog object stays strongly
referenced for the life of the process and is only released when the interpreter
clears module dicts during finalization. At that point:

windowsml/__init__.py:439  EpCatalog.__del__
windowsml/__init__.py:428  EpCatalog.close()
                           -> WinMLEpCatalogRelease   (the call that fails)

So removing the atexit hook only moves that call later into finalization,
where the process is further torn down.

Measured

Full tests/e2e/test_perf_e2e.py on the OV agent hardware, same machine, same
command, only the ep_path behaviour differing:

variant tests process exit code faulting module / code / offset
current main behaviour (atexit -> close) 35 passed, 15 skipped -1073741819 Microsoft.Windows.AI.MachineLearning.dll 2.0.300.41724 / 0xc0000005 / 0x26f5
this PR's ep_path change 35 passed, 15 skipped -1073741819 identical

Every test passes in both cases; the process then dies during shutdown, 1.0s
(baseline) / 1.4s (this PR) after the last test. Note the exit code reaches
Azure DevOps as PowerShell exited with code '1' after WerFault gets involved,
which makes it look like a test failure rather than a crash.

The fault site itself, disassembled at RVA 0x26f5:

+0x026f2  mov  rax, qword ptr [rdx]         ; load vtable   -- this one is fine
+0x026f5  mov  rax, qword ptr [rax + 0x10]  ; vtable[2] = IUnknown::Release   <-- AV
+0x026f9  jmp  <Release thunk>

A ComPtr-style release: the object's first qword still reads back, but the
vtable pointer taken from it is already invalid, i.e. the release runs after the
state/module that owns it is gone.

The same call also has a second failure mode: it can block forever instead of
faulting. Captured with py-spy --native on a CI process that had finished every
test and then sat there until the job timed out:

Py_FinalizeEx -> atexit -> EpCatalog.close()
  -> WinMLEpCatalogRelease -> WinMLAsyncClose -> SleepConditionVariableSRW

Which of the two you get depends on how far native teardown has progressed, so
neither removing one caller nor wrapping it in a timeout covers both.

Suggestion

If the intent is to stop the release at shutdown, clearing the handle is what
actually achieves it, because close() starts with if self._handle::

catalog._handle = None

That disarms atexit and __del__ in one place. The handle is process-scoped,
so the OS reclaims it at exit.

Also worth noting: test_get_catalog_does_not_register_atexit_cleanup now
asserts registered == [], which locks in "no cleanup hook" without providing a
replacement, so a later fix in this area would have to revisit that assertion.

Happy to share the raw logs / WER records if useful.

@DingmaomaoBJTU

Copy link
Copy Markdown
Collaborator Author

Handled in 749f024 by registering an atexit cleanup that only disarms the cached WinML catalog handle (catalog._handle = None) instead of calling close(). The lifecycle test now asserts the cleanup is registered, clears the handle, and does not invoke close(). I also ran the ep_path unit coverage and a live _get_catalog() exit smoke on this Windows machine.

@xieofxie xieofxie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed the latest head. The registered skip_optimize value now survives config assembly, quantization is based on actual ONNX graph state rather than skip_optimize, and regression tests cover both paths. No remaining issues found.

@DingmaomaoBJTU
DingmaomaoBJTU merged commit e5a1be8 into main Jul 30, 2026
9 checks passed
@DingmaomaoBJTU
DingmaomaoBJTU deleted the dingmaomaobjtu-convert-qwen3-model branch July 30, 2026 06:10
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.

4 participants