ADR-339: Add Sample/Manifest/ManifestStore data model for ML pipeline - #252
Conversation
Sample/SampleWithPath/TextSample/AudioSample/SampleSpectrogram/SampleTokens dataclasses in ml/pipeline/core/sample.py. TextSample.__post_init__ computes content_hash = sha256(content) and sets name = content_hash, per the content-addressable identity decision in ml/_spec_OopPipeline.md. Wrapper-tier per the spec's component taxonomy; __post_init__ gets direct unit test coverage since it's genuine computed behavior. Also adds pytest as an explicit ml/requirements.txt dependency and minimal [tool.pytest.ini_options] config to ml/pyproject.toml -- needed for this task's tests to be reliably runnable outside this devcontainer's pre-provisioned /opt/ml-env venv.
Manifest[S] (ml/pipeline/core/manifest.py): a typed, in-memory sample collection with by_name/by_content_hash lookup, indexed at construction. Raises ValueError on a duplicate sample name -- a load-bearing correctness check per the spec, covered directly. ManifestStore (same file, per the spec's directory layout): JSON round-trip persistence, schema_version 1. write() raises ValueError on an empty manifest or mixed sample types. read() dispatches on a self-describing 'sample_type' field recorded by write(), so callers don't need to pass an expected type back in -- resolves an open question the task brief flagged as unspecified in the spec. Covered test-first for both TextSample and AudioSample round trips, multi-sample round trips, and the unknown-sample-type read failure. Both are Testable-tier per the spec's component taxonomy.
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed against ADR-339's exit criteria and ml/_spec_OopPipeline.md. All four exit-criteria checklist items are met: the Sample/SampleWithPath/TextSample/AudioSample/SampleSpectrogram/SampleTokens hierarchy in sample.py (with TextSample.__post_init__ correctly setting name = content_hash), Manifest[S] with by_name/by_content_hash lookups and duplicate-name ValueError, and ManifestStore's JSON schema-v1 round-trip with type-dispatch serialise/deserialise and ValueError on empty/mixed-type writes. Tests were written test-first and land in ml/test/pipeline/core/.
Verified independently: python3 -m pytest (21/21 pass) and mypy --strict pipeline test (clean across all 10 source files) both pass locally, and scripts/validate-ml-build.sh passes.
One non-blocking suggestion posted inline. Two process notes for awareness, not blocking this PR:
- No quality-gate script currently runs the new
ml/test/pytest suite (scripts/validate-ml-build.shonly runsmypy --strict;scripts/validate-tests.shonly runsdotnet test). This is called out as a known ambiguity in the task brief and isn't owned by any task in the spec's breakdown yet -- worth a follow-up work item so these new tests are actually enforced in CI, not just runnable locally. SampleSpectrogram/SampleTokensextendingSampleWithPath(vs.Sampledirectly) is explicitly flagged in the PR description as an inference from the spec rather than a stated requirement -- worth confirming before ADR-347/ADR-348 build on it, since correcting the hierarchy later would ripple through those tasks.
No Priority 1-4 blocking issues found. Approving.
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review — ADR-339
Re-reviewed PR #252 after the first-pass review. No commits landed since the prior review pass, so there are no newly modified files to scan for regressions.
Prior thread status:
ml/pipeline/core/manifest.py:51(content-hash collision onby_content_hash, last-one-wins) — the original comment explicitly framed this as a non-blocking suggestion out of scope for ADR-339's exit criteria (which only require duplicate-name checking), deferred to a deliberate decision before ADR-341 starts relying on content-hash lookups. Resolving this thread for ADR-339 sign-off on that basis; the recommendation to make and document a deliberate choice (raise/warn/accept-last-wins) still stands as a follow-up before ADR-341 begins.
Independent verification: python3 -m pytest (21/21 pass), mypy --strict pipeline test (clean), scripts/validate-ml-build.sh all pass. git diff --stat against the base branch matches the PR diff exactly.
No new Priority 1-4 issues found. All four exit-criteria checklist items from ml/_spec_OopPipeline.md's ADR-339 section remain met.
Signing off: approved.
PRs should not be limited to running build-and-test only when they target main or a feature branch. This is required for using stacked PRs.
2 participants
Work item
ADR-339: Build the unified
Sample/Manifest[S]/ManifestStoredata model inml/pipeline/core/— the sample dataclass hierarchy, typed manifest collection, and JSON round-trip persistence that every later ML pipeline stage (fromPhraseVariatorthroughModelEvaluator) will construct, index, and serialise through.Changes
ml/pipeline/core/sample.py(new) —Sample,SampleWithPath,TextSample,AudioSample,SampleSpectrogram,SampleTokensdataclasses.TextSample.__post_init__computescontent_hash = sha256(content)and setsname = content_hash.ml/pipeline/core/manifest.py(new) —Manifest[S](generic collection,by_name/by_content_hashlookups, raisesValueErroron duplicate names) andManifestStore(JSON schema-v1 round-trip, type-dispatch serialise/deserialise, raisesValueErroron empty/mixed-sample-type writes).ml/pipeline/core/__init__.py(new) — empty, matching the existingml/pipeline/__init__.pyconvention.ml/test/pipeline/__init__.py,ml/test/pipeline/core/__init__.py(new) — package inits for the new test tree.ml/test/pipeline/core/test_sample.py(new) — direct unit tests forTextSample.__post_init__.ml/test/pipeline/core/test_manifest.py(new) — unit tests forManifest[S].ml/test/pipeline/core/test_manifest_store.py(new) — unit tests forManifestStore.ml/requirements.txt(modified) — addedpytest==9.1.1pin (previously only present transitively in the devcontainer venv, not pinned).ml/pyproject.toml(modified) — added minimal[tool.pytest.ini_options]withtestpaths = ["test"].Design decisions
labellives onTextSample/AudioSampledirectly, not on the sharedSamplebase — cross-referencing the spec, it's only ever read off those two concrete types.parent_name/parent_content_hash/applied_valueslive onSampleWithPathrather than a new intermediate base class — every sample type needing lineage tracking is also exactly the set that writes a physical file.SampleSpectrogram/SampleTokensextendSampleWithPath(notSampledirectly), consistent with the spec's per-sample physical-file output model forModifierStagesubclasses. This is an inference from the spec, not an explicit statement — worth confirming before ADR-347/ADR-348 build on it.TextSample.name/content_hasharefield(init=False, default=""), computed in__post_init__, so callers only ever passcontent/label.ManifestStore.read()takes nosample_typeparameter —write()recordssample_type(class name) andschema_version: 1in the JSON document;read()dispatches via an internal type registry and raisesKeyErroron an unrecognizedsample_type.ManifestStoreis a stateless, no-arg class.pytestinto anyscripts/validate-*.sh— out of scope per the task's exit criteria; left as a known gap.ml/pipeline/core/_doc_core.md— deferred until ADR-340/ADR-341 complete thecoresubpackage, per the spec's own doc-timing convention.Testing completed
cd ml && python3 -m pytestmypy --strict pipeline test(run fromml/) reports no issues across all 10 source filesManifest[S]/ManifestStorewere driven through genuine red-green-refactor TDD by hand (confirmed each red failure was for the right reason before implementing, ran a refactor pass after each green)