ADR-341: Add ModifierStage/RandomizedModifierStage base classes (TDD) - #256
Conversation
Seedless two-case skip/regenerate + GC base (ModifierStage) and a seeded three-case skip/regen-with-stored-seed/new-sample extension (RandomizedModifierStage), per ml/_spec_OopPipeline.md. RandomizedModifierStage narrows T_out to AudioSample and gives its inherited _compute_applied_values/_generate_output hooks NotImplementedError stubs rather than overriding them with a wider signature, keeping every override mypy --strict clean with no [override] suppressions. Adds ml/pipeline/core/_doc_core.md now that this is the last task touching ml/pipeline/core/.
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed ADR-341 (ModifierStage/RandomizedModifierStage base classes). Implementation closely follows the spec's skip/regen/GC algorithm at both levels, mypy --strict passes cleanly (zero [override] suppressions), and all 87 tests pass including the required GC-scope regression test and the RandomizedModifierStage partial-update case. Two broken relative links found in the new _doc_core.md, plus one minor design note about synchronous manifest I/O inside async def transform(). See inline comments.
| present in the stage's own output directory, so it can never enumerate or delete anything from | ||
| a different stage's directory — even when a passthrough sample's `.path` points at an ancestor | ||
| stage's directory. Covered by a dedicated regression test in | ||
| [`test_modifier_stage.py`](../../../test/pipeline/core/test_modifier_stage.py). |
There was a problem hiding this comment.
Broken relative link. This file lives at ml/pipeline/core/_doc_core.md, so 3 levels up (../../../) lands at the repo root, not ml/. The actual test file is at ml/test/pipeline/core/test_modifier_stage.py, which is 2 levels up from here, not 3. ../../../test/pipeline/core/test_modifier_stage.py resolves to a non-existent <repo-root>/test/pipeline/core/test_modifier_stage.py. Should be ../../test/pipeline/core/test_modifier_stage.py. Verified: ls ml/pipeline/core/../../../test/pipeline/core/ fails; ls ml/pipeline/core/../../test/pipeline/core/ succeeds.
| ## Testability | ||
|
|
||
| Every class in this subpackage is Testable-tier and covered test-first in | ||
| [`ml/test/pipeline/core/`](../../../test/pipeline/core/), mirroring this directory's layout |
There was a problem hiding this comment.
Same broken-link issue as the other _doc_core.md comment: ../../../test/pipeline/core/ should be ../../test/pipeline/core/ (2 levels up from ml/pipeline/core/_doc_core.md to reach ml/, not 3).
|
|
||
| output_manifest = Manifest(outputs) | ||
| self._garbage_collect(output_manifest) | ||
| self._manifest_store.write(output_manifest, self._output_dir / "manifest.json") |
There was a problem hiding this comment.
Minor/non-blocking: ManifestStore.write/.read (and Path.exists() in _read_previous_by_parent_hash) run synchronously inside async def transform(), blocking the event loop while writing/reading manifest.json. This is the same class of concern the sibling io subpackage's _doc_io.md calls out explicitly ("async seam, offloaded to a thread pool ... must not block it on synchronous file I/O"). I'm not blocking on this since it's a pre-existing ManifestStore API from ADR-339 that this task's brief explicitly said to reuse as-is, and it only runs once per stage invocation (not per-sample, unlike future AudioReader/AudioWriter calls), so the practical impact here is negligible given transform() is the only task in its event loop. Worth a look if ModifierStageEntryPoint (ADR-345) ever needs to run multiple stages concurrently in one process.
2 participants
Seedless two-case skip/regenerate + GC base (ModifierStage) and a seeded three-case skip/regen-with-stored-seed/new-sample extension (RandomizedModifierStage), per ml/_spec_OopPipeline.md. RandomizedModifierStage narrows T_out to AudioSample and gives its inherited _compute_applied_values/_generate_output hooks NotImplementedError stubs rather than overriding them with a wider signature, keeping every override mypy --strict clean with no [override] suppressions. Adds ml/pipeline/core/_doc_core.md now that this is the last task touching ml/pipeline/core/.
Stack created with GitHub Stacks CLI • Give Feedback 💬