refactor: introduce WinMLManifest dataclass as single source of truth for build_manifest.json#1057
refactor: introduce WinMLManifest dataclass as single source of truth for build_manifest.json#1057xieofxie wants to merge 19 commits into
Conversation
…, add cleanup + tests
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
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()usesjson.dumps(..., default=str)(utils/manifest.py), which the old writer didn't. Ifexport_stats/analyze_detailsever contain numpy scalars, numeric metrics silently serialize as strings ("10"rather than10). Consider sanitizing the payload instead of a blanketdefault=str.- Forward-compat asymmetry: top-level unknown keys survive in
extras, but unknown keys inside a stage are dropped byfrom_dict(filtered againstManifestStage.__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 byutils/__init__.py; per the repo convention preferfrom ..utils import ManifestStage, WinMLManifest.
- Fix build_manifest.json -> winml_manifest.json rename missed in utils/cli.py - Update corresponding test assertions in test_cli.py - Remove dead _cleanup_partial_composite (defined but never called) - Fix import convention: use 'from ..utils import' not 'from ..utils.manifest import' - Replace blanket default=str with _sanitize_value to preserve numeric types - Add ManifestStage.extras for forward-compatible stage fields - Add tests: export manifest (single + composite), Path sanitization, stage extras
The _sanitize_value handles known types (Path -> str) explicitly to preserve numerics, but default=str remains as a fallback for unexpected types (e.g. MagicMock in tests) to avoid serialization crashes.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
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.
|
need to rethink about how to detect if the model is generated by winml cli |
… for export Revert winml_manifest.json rename back to build_manifest.json for build outputs. Export outputs now use export_manifest.json instead. - Split MANIFEST_FILENAME into BUILD/EXPORT constants - manifest_path_for() accepts filename parameter - find() discovers both build and export manifests - Consumers (engine, resolver, serve) updated for both patterns - All tests and docs updated
xieofxie
changed the title
Add np.generic -> .item() conversion so numpy floats/ints/bools in export_stats or analyze_details serialize as native Python types instead of falling through to default=str.
DingmaomaoBJTU
left a comment
DingmaomaoBJTU
left a comment
There was a problem hiding this comment.
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
Revert export.py to main (no manifest generation in export). Remove EXPORT_MANIFEST_FILENAME, export manifest tests, and all export_manifest references. Export manifest generation deferred to issue #1038.
xieofxie
changed the title
- inference/engine.py: import MANIFEST_FILENAME + WinMLManifest, replace string literals with constant, use WinMLManifest.find() in _resolve_model_id_from_dir - inspect/resolver.py: use MANIFEST_FILENAME glob + WinMLManifest.load() with typed ManifestStage attribute access - serve/app.py: use MANIFEST_FILENAME + WinMLManifest.load().to_dict() - build/hf.py, build/onnx.py: use MANIFEST_FILENAME constant for path No string literal 'build_manifest.json' remains in production source.
4 participants
Summary
Introduces a
WinMLManifestdataclass as the single source of truth for thebuild_manifest.jsonschema. This is a pure refactor — no new behavior, no filename changes.Changes
New:
WinMLManifest/ManifestStagedataclasses (utils/manifest.py)build_manifest.json— all producers construct aWinMLManifestand call.save()to_dict()dropsNonefields for compact JSON;from_dict()is forward-compatible (unknown keys preserved inextrasat both top-level and stage-level)_sanitize_value()convertsPathobjects to strings and numpy scalars to native Python types, preserving numeric precisionfind(),load(),manifest_path_for()helpers for consumers to adopt incrementallyBuild producers updated to use
WinMLManifestbuild/hf.py: raw dict →WinMLManifest+ManifestStagebuild/onnx.py: same refactorTests
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
_cleanup_partial_compositefunction fromexport.py(defined but never called)