Skip to content

feat: Add export override options to winml eval command#1141

Open
xieofxie wants to merge 1 commit into
mainfrom
hualxie/dyn_to_eval
Open

feat: Add export override options to winml eval command#1141
xieofxie wants to merge 1 commit into
mainfrom
hualxie/dyn_to_eval

Conversation

@xieofxie

Copy link
Copy Markdown
Contributor

Summary

Brings winml eval to parity with winml build and winml perf by adding four export override options:

  • --shape-config — JSON shape overrides for the auto-generated HuggingFace export config
  • --input-specs — JSON input tensor specs merged into the HuggingFace export config (symbolic dims infer dynamic axes)
  • --export-config — JSON ONNX export config overrides (opset version, constant folding, etc.)
  • --dynamic-axes — JSON dynamic axes mapping for HuggingFace ONNX export

These options shape the ONNX export that eval generates when -m is a HuggingFace model ID. When -m is a pre-built .onnx file there is no export step, so the flags are ignored and the command prints a warning.

Changes

  • eval/config.py — added shape_config and export_overrides fields to WinMLEvaluationConfig, JSON-safe serialization of InputTensorSpec values, and to_dict/from_dict support.
  • eval/evaluate.py_load_model threads {"export": ...} overrides plus shape_config into WinMLAutoModel.from_pretrained on the HuggingFace build path; the from_onnx path is unchanged.
  • commands/eval.py — added the four option decorators/params and an _apply_export_overrides helper that parses the JSON and warns + skips for pre-built ONNX inputs.
  • docs/commands/eval.md — documented the four flags and added a pitfall note.
  • Tests — added TestEvalExportOverrides (10 cases) and updated the existing from_pretrained assertion for the new shape_config kwarg.

Verification

  • uv run pytest tests/unit/commands/test_eval.py tests/unit/eval/test_eval.py → 103 passed
  • uv run ruff check → clean
  • Doc flag/code alignment check → passing

Bring winml eval to parity with build/perf by adding --shape-config,
--input-specs, --export-config, and --dynamic-axes. These shape the
ONNX export when eval builds from a HuggingFace model ID and are
ignored (with a warning) for pre-built ONNX inputs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@xieofxie
xieofxie requested a review from a team as a code owner July 20, 2026 06:40
@xieofxie xieofxie changed the title Add export override options to winml eval command feat: Add export override options to winml eval command Jul 20, 2026
# Passing a dict rather than a WinMLBuildConfig avoids clobbering the
# auto-resolved export config with default fields. Mirrors winml build/perf.
build_override: Any = quant_override
if config.export_overrides:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Updated todo list

Here's a PR-ready comment:


Review: input option precedence

I checked the implementation against the intended precedence order:

CLI explicit > config-file explicit > CLI default > config-class default

The four new flags (--shape-config, --input-specs, --export-config, --dynamic-axes) bypass the standard collect_cli_overrides/merge_config machinery (they bind to shape_config_path/input_specs/export_config/dynamic_axes, which aren't WinMLEvaluationConfig field names) and are handled entirely by the new _apply_export_overrides.

✅ What's correct

  • CLI default > config-class default — the options default to None; _apply_export_overrides returns early when no flag is provided, so CLI defaults never clobber anything.
  • config-file explicit > CLI default — the config-file eval section can carry shape_config/export_overrides, and they survive when the CLI omits the flags. They then flow into from_pretrained(config={"export": ...})generate_hf_build_config, which routes them through merge_export_overrides (patch input_tensors by name, re-derive dynamic axes), correctly preserving the auto-generated/Optimum defaults tier.
  • CLI explicit > config-file explicit for shape_config — single-value override, fine.

❌ Precedence bug: export_overrides is replaced wholesale, not merged

In _apply_export_overrides:

export_overrides = cli_utils.load_export_overrides(
    export_config=export_config, input_specs=input_specs, dynamic_axes=dynamic_axes,
)
if export_overrides:
    cfg.export_overrides = export_overrides   # wholesale replace

load_export_overrides returns a sparse dict containing only the CLI-provided sub-keys, so assigning it discards any config-file export_overrides sub-keys the CLI didn't set.

Repro: config file sets export_overrides = {"opset_version": 17, "dynamic_axes": {...}}, then run with only --input-specs foo.json. Result: cfg.export_overrides = {"input_tensors": [...]} — the config-file opset_version and dynamic_axes are lost, even though the CLI never set them. Per the intended ordering, those config-file values should win (config-file explicit > CLI default).

This is also a parity gap with build, which deliberately avoids exactly this by calling merge_export_overrides(cfg, export_overrides) on the loaded config "instead of merge_config replacing the list wholesale".

Suggested fix — merge instead of replace:

if export_overrides:
    merged = dict(cfg.export_overrides or {})
    merged.update(export_overrides)
    cfg.export_overrides = merged

Related concerns (not strictly precedence)

  1. Config-file input_tensors will crash. Config-file export_overrides is merged as a raw dict via merge_config(cfg, eval_data) (not through from_dict/load_input_tensor_specs), so its input_tensors stay plain dicts. Downstream _patch_input_tensors accesses patch.name/patch.dtype, which raises AttributeError on a dict — so config-file-supplied input_tensors is effectively unusable.
  2. Pre-built ONNX path: the warn-and-skip only covers CLI flags. If the config file carries shape_config/export_overrides alongside a pre-built .onnx, they stay on cfg (ignored downstream, but with no warning) — minor inconsistency.

Overall the ordering is correct for shape_config and for the config-file/default tiers, but export_overrides needs field-level merging between the CLI and config-file layers to satisfy the intended precedence.

"Ignored for pre-built ONNX inputs."
),
)
@cli_utils.input_specs_option(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

An optional suggestion: as we are having more and more options, do you think they can be wrapped into a single "--additional-options" for all those optional command line inputs?

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