feat(config): add dynamic export controls to config command#1106
feat(config): add dynamic export controls to config command#1106xieofxie wants to merge 8 commits into
Conversation
Mirror build/perf by adding --input-specs, --export-config, and --dynamic-axes to `winml config`. Overrides are applied via the shared merge_export_overrides helper so --input-specs patches auto-resolved input tensors by name (preserving unlisted inputs and their dtype/value_range) and symbolic dims re-derive dynamic axes. Wired into the single, --module list, and composite-pipeline paths; rejected for pre-exported ONNX inputs and configs with export=null. Docs and config CLI tests updated.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
A few notes on the new export controls — mainly the composite-pipeline fan-out and test coverage. Otherwise nice work bringing this to parity with build/perf.
- Reject --input-specs/--export-config/--dynamic-axes for composite (multi-component) models instead of fanning the same overrides across sub-components with distinct inputs, which could append bogus inputs. - Move the pre-exported-ONNX rejection ahead of loading/validating the override JSON so the error names the ONNX input (not a downstream "Invalid export configuration") and skips needless file I/O. - Replace mock-based tests with integration coverage that lets merge_export_overrides run and asserts on to_dict(): by-name patching, unlisted-input preservation, symbolic-dim -> dynamic_axes derivation, export=null rejection, the --module list path, composite rejection, and ONNX rejection ordering. - Document the composite rejection in config.md.
…lxie/dynamic_config
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Reviewed the export-controls addition to winml config. Overall this is clean and mirrors winml build faithfully — the shared merge_export_overrides / _patch_input_tensors by-name patching, the export=null guard, and the up-front ONNX rejection (with its ordering test) are all solid, and the integration-style tests that let merge_export_overrides actually run are a nice improvement over pure mocks. No correctness bugs found. A few non-blocking notes below: two minor UX/consistency points and one trivial nit.
…ejection Address PR #1106 review comments: - --module now rejects --input-specs/--export-config/--dynamic-axes up front instead of fanning them onto every submodule config (would append bogus inputs on submodules with different input names). - Reject the ONNX / --module / composite paths on raw flag presence before loading the override JSON, so the path-specific error wins over a downstream "Invalid export configuration" (mirrors the ONNX guard). - Move load_export_overrides below composite resolution; the single HF config path is now the only consumer. - Simplify Path(input_specs).name to input_specs.name (already Path|None). - Docs + tests updated: --module rejection, composite-precedes-JSON test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Thanks for the follow-up — this cleanly resolves all three earlier notes: --module and composite now both reject on raw flag presence before load_export_overrides, so the path-specific error wins over Invalid export configuration; load_export_overrides is scoped to the single-HF path (verified export_overrides is still defined before _merge_export_overrides on that branch); the Path(...).name nit is gone; and the docs read accurately. One new issue from the test refactor — see inline.
The previous refactor fused two tests: the export=null assertion body was left orphaned at the tail of test_composite_rejection_precedes_json_validation, so test_merge_export_overrides_rejects_export_null no longer existed as a discrete test. Split it back into its own method. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…hang build calls resolve_composite_components before dispatching, which resolves the model config from HuggingFace Hub (AutoConfig.from_pretrained) over the network. The build CLI plumbing tests only mocked _run_single_build, so this detection call stayed live: repeated metadata requests get throttled and huggingface_hub's retry backoff sleeps, making a later unrelated test appear to hang mid-run. Add an autouse fixture that defaults resolve_composite_components to None (non-composite), matching the existing mock_task_model_compatibility_validator pattern that keeps these tests off HF resolution paths. Composite-specific tests already override it with their own patch, which nests inside and wins. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
Approving. All review feedback is addressed:
--input-specs/--export-config/--dynamic-axesare rejected up front (on raw flag presence, before loading the JSON) for the ONNX,--module, and composite paths, so the path-specific error wins over a downstreamInvalid export configuration;load_export_overridesis scoped to the single-HF path and_merge_export_overridesstill applies there.ae412b3csplits the accidentally-fused test back out —test_merge_export_overrides_rejects_export_nullis a discrete method again and the composite test ends cleanly.d7aeaa8a(build-test autouse stub forresolve_composite_components) is a sound, well-targeted flake fix that matches the existing autouse pattern; composite-specific tests still override it.
Docs read accurately. Nice work. (Note: I reviewed by reading the diff/branch; the suite couldn't be run on my ARM64 host, so a green CI run is worth a final glance before merge.)
2 participants
Summary
Adds the dynamic export controls (
--input-specs,--export-config,--dynamic-axes) towinml config, bringing it to parity withwinml build/winml perf/winml export. This is a follow-up to #1083, which introduced these controls for build/perf and the sharedcli_utilsoption/loader helpers.What changed
commands/config.pyinput_specs_option,export_config_option,dynamic_axes_option) and their params.cli_utils.load_export_overrides(validates early, doesn't clobber auto-detected fields)._merge_export_overrideshelper applies them via the sharedmerge_export_overrides, so--input-specspatches the auto-resolvedinput_tensorsby name (unlisted inputs and theirdtype/value_rangepreserved) and symbolic dims re-derive dynamic axes throughWinMLExportConfig.__post_init__.--modulelist, and composite-pipeline paths.export=null(clearUsageError).docs/commands/config.md: three new flag rows, an example, and a pitfall note.tests/unit/commands/test_config_cli.py:TestConfigExportControls— help visibility, override forwarding/parsing, no-op when absent, and ONNX rejection.Verification
ruff check+ruff formatclean.tests/unit/commands/test_config_cli.py(55) andtest_config_composite_resolution.py+test_config_value_priority.py(63) pass.