ADR-342: Add AudioReader/AudioWriter I/O seam - #255
Conversation
Protocol-based seam per the spec's per-concern I/O abstraction decision. LibrosaAudioReader/SoundfileAudioWriter offload librosa.load/soundfile.write to a thread pool via asyncio.to_thread and are test-first per the task brief's explicit override of the Wrapper tier's normal test exclusion.
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed against the ADR-342 task brief and ml/_spec_OopPipeline.md. Implementation is solid: AudioReader/AudioWriter are correctly modeled as Protocols, both concrete wrappers offload their blocking library call via asyncio.to_thread (verified with a wrapped-mock test, not just an argument check), exceptions propagate untouched, and sr=None is passed to preserve native sample rate. Ran mypy --strict pipeline test and pytest locally — both clean/green as claimed in the PR description (28/28 tests pass). Naming and test structure match the ADR-339 precedent (class Test<ClassName>:, test_<ClassName>_<Method>_<Scenario>_<ExpectedBehavior>).
One documentation gap flagged inline: this task completes the entire planned content of the io/ subpackage (per the spec's Directory Layout, audio_io.py + _doc_io.md are the only two files ever planned under io/), and the spec's own header says per-subpackage docs are created "as each subpackage's implementation is completed." ml/pipeline/io/_doc_io.md previously existed (added under the discarded ADR-226 attempt, deleted as part of the rebuild) — its absence now isn't a fresh ambiguity so much as a doc debt the rebuild owes back. Given CLAUDE.md's Quality Gate #4 ("Affected _doc_*.md files are updated"), please add it before merge, or get explicit sign-off that it's deliberately deferred.
No other Priority 1-3 issues found.
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review for ADR-342 (PR #255).
Prior thread (missing ml/pipeline/io/_doc_io.md) is resolved -- the doc was added in commit 4e1c5ab and accurately describes the seam's design (Protocol seam, async/thread-pool offload, native-sample-rate preservation, deferred imports) matching audio_io.py.
One new issue found in the modified file itself: a broken relative link to the test file (see inline comment). Requesting a fix before merge.
|
|
||
| - The protocol contract (call-through arguments, thread-pool offload, exception propagation, | ||
| native-sample-rate preservation) is unit-tested test-first in | ||
| [`ml/test/pipeline/io/test_audio_io.py`](../../../test/pipeline/io/test_audio_io.py) — an |
There was a problem hiding this comment.
Documentation (Priority 4): broken relative link. From ml/pipeline/io/_doc_io.md, ../../../test/pipeline/io/test_audio_io.py resolves to <repo-root>/test/pipeline/io/test_audio_io.py (three levels up escapes past ml/ entirely, landing in the unrelated C# test/ tree). The actual file is at ml/test/pipeline/io/test_audio_io.py, which is two levels up from ml/pipeline/io/, i.e. ../../test/pipeline/io/test_audio_io.py. Please fix the link.
Work item
ADR-342: Build the injectable audio I/O seam (
AudioReader/AudioWriterprotocols plus theirlibrosa/soundfile-backed implementations) used by every future stage that reads or writes WAV files in the OOP ML pipeline.Changes
ml/pipeline/io/__init__.py— new, empty package init for theiosubpackage.ml/pipeline/io/audio_io.py— new. DefinesAudioData(dataclass:samples: npt.NDArray[np.float32],sample_rate: int),AudioReader/AudioWriter(Protocolclasses withasync def read/write), andLibrosaAudioReader/SoundfileAudioWriter(thin call-throughs offloaded to a thread pool viaasyncio.to_thread).ml/test/pipeline/io/__init__.py— new, empty test package init.ml/test/pipeline/io/test_audio_io.py— new, test-first unit tests covering the protocol contract using stdlibunittest.mock.ml/requirements.txt— addedlibrosa==1.0.0andsoundfile==0.14.0.Design decisions
read/writeareasync def, matching the asynctransform()pipeline that will consume this seam; blockinglibrosa.load/soundfile.writecalls are offloaded viaasyncio.to_thread.LibrosaAudioReader.readpassessr=Nonetolibrosa.loadto preserve the file's native sample rate instead of resampling to librosa's 22050 Hz default.Protocol(notABC) for the seam, per the project convention thatProtocolis used for injectable interfaces with no shared implementation.ml/pipeline/io/_doc_io.md— deferred pending explicit confirmation since the spec doesn't explicitly require it at this point.unittest.mockfor mocking (no mocking library precedent existed yet inml/test/); nopytest-asynciodependency added.Testing completed
ModuleNotFoundError) before implementation, then green after (7/7 new tests passing).mypy --strictclean overml/pipelineandml/test.pytestsuite (28 tests) passes with no regressions.