Skip to content

feat(export): normalize exported ONNX in-place via optimize_onnx#681

Merged
vortex-captain merged 6 commits into
mainfrom
reny/normalize_after_export
May 20, 2026
Merged

feat(export): normalize exported ONNX in-place via optimize_onnx#681
vortex-captain merged 6 commits into
mainfrom
reny/normalize_after_export

Conversation

@vortex-captain
Copy link
Copy Markdown
Contributor

Fixes #677.

Summary

  • Adds a post-export normalization step inside export_pytorch(), the single function used by every export path (the winml export CLI, build_hf_model, the winml build command, and direct Python API callers), so normalization runs automatically for all of them.
  • New helper _normalize_exported_model(output_path) -> bool in src/winml/modelkit/export/pytorch.py:
    • logs Normalizing model
    • runs optimize_onnx() into a fresh tempfile.mkdtemp()
    • on success, replaces the original .onnx (and .data sidecar, if any) via copy_onnx_model() — confirmed to overwrite both files correctly by new tests in tests/unit/onnx/test_external_data.py
    • on failure, logs a warning and leaves the original export untouched
    • cleans up the temp directory in a finally clause either way
    • records the outcome in stats["model_normalization_succeeded"]

Tests

  • tests/unit/export/test_pytorch_export.py — success path populates value_info; mocked-failure path leaves the model un-shape-inferenced and reports model_normalization_succeeded=False.
  • tests/unit/onnx/test_external_data.py — two new overwrite tests covering copy_onnx_model against a pre-existing dst with and without external data sidecars.

Yi Ren added 2 commits May 20, 2026 14:34
Adds a post-export normalization step inside `export_pytorch()` so that
every export path (the `winml export` CLI, `build_hf_model`, the
`winml build` command, and direct Python API callers) benefits from
graph normalization automatically.

The new helper `_normalize_exported_model()`:
- logs `Normalizing model`
- runs `optimize_onnx()` into a `tempfile.mkdtemp()` working directory
- on success, replaces the original `.onnx` + sidecar in-place via
  `copy_onnx_model()` (which overwrites both files correctly — confirmed
  by new overwrite tests in `tests/unit/onnx/test_external_data.py`)
- on failure, logs a warning and leaves the original export untouched
- removes the temp directory in a `finally` clause either way
- records the outcome in `stats["model_normalization_succeeded"]`

Tests:
- `tests/unit/export/test_pytorch_export.py`: success path populates
  `value_info`; mocked-failure path leaves the model un-shape-inferenced.
- `tests/unit/onnx/test_external_data.py`: `copy_onnx_model` overwrites a
  pre-existing dst correctly with and without external data sidecars.
@vortex-captain vortex-captain requested a review from a team as a code owner May 20, 2026 06:37
Copy link
Copy Markdown
Collaborator

@timenick timenick left a comment

Choose a reason for hiding this comment

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

Six findings — four substantive (perf, API contract, error handling, atomicity claim) and two test gaps. None are strict blockers, but #1 and #4 in particular are worth addressing before this lands as the default for every export path.

Comment thread src/winml/modelkit/export/pytorch.py
Comment thread src/winml/modelkit/export/pytorch.py
Comment thread src/winml/modelkit/export/pytorch.py
Comment thread src/winml/modelkit/export/pytorch.py Outdated
Comment thread tests/unit/export/test_pytorch_export.py
Comment thread tests/unit/export/test_pytorch_export.py
@DingmaomaoBJTU
Copy link
Copy Markdown
Collaborator

Code review

Found 2 issues:

  1. _normalize_exported_model is called outside the warnings.catch_warnings() block, leaking optimizer/shape-inference warnings to callers. A previous PR (Centralize ONNX export warning suppression in export_pytorch() #460) explicitly centralized all warning suppression into this block to prevent stderr noise; the new call undoes that.

)
stats["model_normalization_succeeded"] = _normalize_exported_model(output_path)
return stats

  1. Normalization is unconditional — there is no parameter (e.g. normalize: bool = True) to skip it. export_pytorch is the single shared entry point for all export paths, so callers that need the raw torch-exporter output (debugging, downstream pipelines, tests asserting exporter semantics) have no escape hatch.

with warnings.catch_warnings():
warnings.filterwarnings("ignore")
stats = exporter.export(
model=model,
output_path=str(output_path),
export_config=export_config,
model_name_or_path=model_name_or_path,
task=task,
**kwargs,
)
stats["model_normalization_succeeded"] = _normalize_exported_model(output_path)
return stats

🤖 Generated with Claude Code

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

Yi Ren added 3 commits May 20, 2026 15:26
… docstring

- export_pytorch: new `normalize: bool = True` kwarg. Callers who need the
  raw torch.onnx.export output can opt out without dropping the normalize
  step for everyone else.
- Stats key renamed `model_normalization_succeeded` (bool) ->
  `model_normalization_status` (str), values `"not_run"` /
  `"succeeded"` / `"failed"`.
- `_normalize_exported_model`: keep the broad `except`, but pass
  `exc_info=True` so the traceback lands in the warning log -- a future
  helper-refactor bug now shows up instead of silently flipping a flag.
- Soften the docstring: optimize_onnx failures leave the original
  untouched, but copy_onnx_model writes directly to the destination, so
  a copy failure can corrupt the dest. The previous docstring overclaimed.

Tests:
- Both existing tests updated to assert on the new status key.
- New `test_normalize_false_skips_normalization` verifies the kwarg
  short-circuits the helper and yields `"not_run"`.
Pass `dir=output_path.parent` to `tempfile.mkdtemp()` so the staging
directory lives on the same volume as the export. For multi-GB models
(the primary WinML target) this avoids a cross-volume data transfer in
`copy_onnx_model` and keeps large sidecars off the system drive's
`%TEMP%`. Addresses the perf review comment on #681.
Move the `_normalize_exported_model` call inside the
`warnings.catch_warnings()` block so the optimizer's shape-inference
and ORT warnings stay suppressed for callers. PR #460 centralized
warning suppression here; the new normalization call had escaped it
and was leaking warnings to stderr. Addresses DingmaomaoBJTU's first
review finding on #681.
@vortex-captain vortex-captain enabled auto-merge (squash) May 20, 2026 07:44
@vortex-captain vortex-captain merged commit 5b088fd into main May 20, 2026
9 checks passed
@vortex-captain vortex-captain deleted the reny/normalize_after_export branch May 20, 2026 07:50
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.

winml analyze in v0.0.3 reports all ops as Unknown (regression from v0.0.2 - see screencaps)

3 participants