feat(export): support exporting sub-models from composite models#1031
Conversation
Add --submodel option to `winml export` for selecting a specific
sub-model from composite models (e.g., encoder, decoder).
When a composite model is detected:
- --submodel <name>: exports only that component as {stem}_{name}.onnx
- No --submodel: exports all sub-models automatically
- Non-composite models: unchanged single-model export
Extract _export_single_model() helper shared by both paths and add
_resolve_composite_model_components() for registry/auto-detection.
Resolves #1014
…ft/winml-cli into hualxie/export_submodule
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Took a look — small, focused feature and the tests cover the happy path plus the two error cases. A few things worth addressing before merge: a test-hermeticity concern (the new --submodel docstring example is auto-run and uniquely depends on live Hub resolution), an exception-type consistency nit, and a --submodel + --input-specs dead end. Details inline.
Also, the PR description mentions extracting _export_single_model() and adding _resolve_composite_model_components(), but neither is in the diff — the code uses the existing _run_component_export() / resolve_composite_components(). Worth updating the description so it matches what actually changed.
- --submodel help now notes output is written to {stem}_{name}.onnx
- --submodel validation raises BadParameter (param_hint) for consistent
usage errors/exit codes instead of ClickException
- allow --input-specs when --submodel narrows to a single sub-model;
reword the multi-sub-model rejection to point at --submodel
- patch resolve_composite_components in test_export_help_examples_run and
assert it is called, so the --submodel example no longer hits the Hub
- add test_submodel_allows_input_specs
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Re-reviewed the fix(export): address PR #1031 review comments commit (3bf05092). All four earlier points are resolved correctly:
- Test hermeticity —
resolve_composite_componentsis now patched intest_export_help_examples_run(with a.calledassertion), so the--submodelexample no longer depends on the live Hub. Nice bonus: this makes all the docstring examples hermetic, not just the composite one. - Exception type — both
--submodelvalidations now raiseclick.BadParameter(param_hint="--submodel"), consistent with the rest of the command (usage error + exit code 2). --submodel+--input-specsdead end — the specs are now allowed for a single selected sub-model (input_specs and submodel is Noneguard), and the multi-component message points at--submodelinstead of the--taskdead end. I checkedtest_composite_rejects_input_specsstill holds (single-component mock, no--submodel→ still rejected, message still contains "composite").- Help text — now documents the
{stem}_{name}.onnxoutput.
One follow-up inline (test-strength, non-blocking). Also very minor: the reworded rejection says "exporting multiple sub-models", but the guard fires for any composite without --submodel; it reads slightly off against the single-component mock in test_composite_rejects_input_specs (real composites are all ≥2, so it's only cosmetic). LGTM once the test point is considered.
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Approving. All four points from the earlier review are addressed correctly and I confirmed there is no regression (test_composite_rejects_input_specs still holds; changes are within the 100-char line limit). The one remaining inline note (strengthening test_submodel_allows_input_specs to assert the specs actually apply) is non-blocking and can be picked up as a follow-up.
Note: I could not execute the suite locally (win_arm64 has no torch wheel), so this is based on code-path analysis; please make sure CI is green before merging.
Add --submodel option to
winml exportfor selecting a specific sub-model from composite models (e.g., encoder, decoder).When a composite model is detected:
Extract _export_single_model() helper shared by both paths and add _resolve_composite_model_components() for registry/auto-detection.
Resolves #1014