refactor: introduce WinMLManifest dataclass as single source of truth for build_manifest.json

[xieofxie]

Summary

Introduces a WinMLManifest dataclass as the single source of truth for the build_manifest.json schema. This is a pure refactor — no new behavior, no filename changes.

Changes

New: WinMLManifest / ManifestStage dataclasses (utils/manifest.py)

• Central schema for build_manifest.json — all producers construct a WinMLManifest and call .save()
• to_dict() drops None fields for compact JSON; from_dict() is forward-compatible (unknown keys preserved in extras at both top-level and stage-level)
• _sanitize_value() converts Path objects to strings and numpy scalars to native Python types, preserving numeric precision
• find(), load(), manifest_path_for() helpers for consumers to adopt incrementally

Build producers updated to use WinMLManifest

• build/hf.py: raw dict → WinMLManifest + ManifestStage
• build/onnx.py: same refactor

Tests

• 16 tests in tests/unit/utils/test_manifest.py (round-trip, None-omission, forward-compat extras at top and stage level, I/O, find, Path sanitization, numpy scalar sanitization)

Cleanup

• Removed dead _cleanup_partial_composite function from export.py (defined but never called)

[DingmaomaoBJTU]

Solid, well-structured refactor — the WinMLManifest/ManifestStage dataclasses are clean, well-documented, forward-compatible (extras), and have a dedicated test_manifest.py. A few issues though, most notably the rename is incomplete.

🔴 Bugs / correctness

1. Missed rename in utils/cli.py — the rename is not complete

classify_model_input (src/winml/modelkit/utils/cli.py:91-92) still globs the old name:

manifest_files = list(path.glob("build_manifest.json")) + list(path.glob("*_build_manifest.json"))

Producers now write winml_manifest.json, so a folder produced by the new winml build/export yields is_winml_cli_folder=False. It's currently latent (no command reads is_winml_cli_folder/folder_has_onnx — only tests/unit/utils/test_cli.py does), so the paired tests (~L447-474) still pass while validating stale behavior. This contradicts the PR's "all references updated (15 files)" claim. Please update the globs and those tests.

2. Dead code: _cleanup_partial_composite (commands/export.py:67)

Defined but never called. The composite-failure path (export.py:595) deliberately calls _warn_partial_composite, and its docstring states "we do NOT delete anything." So this helper is unreachable — either wire it in intentionally or remove it.

🟠 Coverage / design

3. New "export writes manifest" behavior is untested

commands/export.py now writes winml_manifest.json (single) and <stem>_winml_manifest.json (composite), but tests/unit/commands/test_export.py has no manifest assertions. The hf/onnx manifest-writing paths are tested; export's new path isn't. Please add coverage for both the single and composite-prefixed cases.

4. No fallback for pre-existing build_manifest.json caches

inference/engine.py, inspect/resolver.py, and serve/app.py now look only for winml_manifest.json (no fallback). Combined with #1, the codebase splits: cli.py recognizes only the old name while everything else recognizes only the new name — so neither pre-existing caches nor new outputs are recognized consistently across all components. If caches are considered disposable/pre-release this may be acceptable, but it deserves an explicit decision (or a one-time fallback read of the old filename).

🟡 Nits

• save() uses json.dumps(..., default=str) (utils/manifest.py), which the old writer didn't. If export_stats/analyze_details ever contain numpy scalars, numeric metrics silently serialize as strings ("10" rather than 10). Consider sanitizing the payload instead of a blanket default=str.
• Forward-compat asymmetry: top-level unknown keys survive in extras, but unknown keys inside a stage are dropped by from_dict (filtered against ManifestStage.__dataclass_fields__). Minor, but the "forward-compatible" guarantee doesn't hold for stage entries.
• Import convention: producers use function-level from ..utils.manifest import ... (reaching into the submodule) even though both symbols are exported by utils/__init__.py; per the repo convention prefer from ..utils import ManifestStage, WinMLManifest.

[DingmaomaoBJTU]

Nice refactor — WinMLManifest as a single source of truth is a clear improvement and the tests are thorough. Two things worth a look inline before merge.

[xieofxie]

need to rethink about how to detect if the model is generated by winml cli

[DingmaomaoBJTU]

Review — WinMLManifest refactor

Overall a clean, well-tested refactor: the naming split (build_manifest.json for build, export_manifest.json for export), the export-side manifest generation, and the composite prefix logic all look correct, and the earlier threads (numpy sanitization, filename revert, backward-compat glob) appear addressed.

Main structural feedback — the "single source of truth" is only half-wired: the write side goes through WinMLManifest.save(), but the read side (find()/load()/from_dict()) has no production callers and consumers still hand-parse JSON with duplicated filename literals. Details inline; nothing here is blocking.

🤖 Reviewed with GitHub Copilot CLI

[DingmaomaoBJTU]

Follow-up review — update since d2221e56

The two new commits cleanly address my earlier feedback:

• Export manifest generation removed → PR is now a focused, pure refactor. This also drops the build/export manifest pooling + mtime-merge ambiguity I flagged, and reverts the contextlib.suppress(OSError) unlink from my third note.
• Consumers wired to WinMLManifest + MANIFEST_FILENAME (resolver, serve, engine._resolve_model_id_from_dir), so the read API is no longer test-only dead code and the filename literal is centralized.

One verified item remains — a robustness / backward-compat regression: WinMLManifest.load()/find() raise an uncaught TypeError on a valid-JSON manifest missing a required field, which is exactly the shape of pre-upgrade hf build_manifest.json files (no source key). Consumers don''t catch it, so inspect/serve crash instead of degrading. Details inline; the fix is ~1 line. Holding approval on that one point.

🤖 Reviewed with GitHub Copilot CLI

[DingmaomaoBJTU]

Nice refactor — centralizing the schema in WinMLManifest reads well, and the test coverage (round-trip, None-omission, forward-compat extras, sanitization, find) is solid. A few things worth a look before merge:

• Robustness: the two direct WinMLManifest.load() callers (resolver.py, serve/app.py) catch a narrower exception set than _try_load, so a valid-JSON-but-non-object manifest can raise an uncaught ValueError and crash them. I confirmed this locally.
• Behavioral drift in a "pure refactor": serve now returns to_dict() instead of the raw file, which reshapes the response and forces schema_version: 1; and from_dict silently downgrades a future-versioned manifest on re-serialise.
• Single-source-of-truth gap: the new manifest_path_for() helper has no production callers — the producers still build the path themselves.

Rest are minor style/scope notes inline. Leaving as COMMENT — not blocking.

[DingmaomaoBJTU]

Thanks for the quick turnaround — all the points from my previous review are addressed, and I verified the behavior locally:

• Both direct WinMLManifest.load() callers now catch ValueError, so a valid-JSON-but-non-object manifest degrades gracefully instead of crashing resolve_cache / the serve endpoint.
• schema_version is now a preserved dataclass field — confirmed a v2 manifest round-trips as 2 (no more silent downgrade), unversioned files still default to 1, and top-level/stage extras are preserved.
• numpy import hoisted to module level, ..utils imports moved to module scope in both build producers, unused manifest_path_for dropped, importorskip swapped for a plain import, and the unrelated test_export.py churn reverted.

Clean, well-tested refactor. LGTM 👍