Skip to content

ADR-342: Add AudioReader/AudioWriter I/O seam - #255

Draft
jodavis-claude wants to merge 2 commits into
dev/claude/ADR-339-sample-manifest-data-modelfrom
dev/claude/ADR-342-audio-io-seam
Draft

ADR-342: Add AudioReader/AudioWriter I/O seam#255
jodavis-claude wants to merge 2 commits into
dev/claude/ADR-339-sample-manifest-data-modelfrom
dev/claude/ADR-342-audio-io-seam

Conversation

@jodavis-claude

Copy link
Copy Markdown
Collaborator

Work item

ADR-342: Build the injectable audio I/O seam (AudioReader/AudioWriter protocols plus their librosa/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 the io subpackage.
  • ml/pipeline/io/audio_io.py — new. Defines AudioData (dataclass: samples: npt.NDArray[np.float32], sample_rate: int), AudioReader/AudioWriter (Protocol classes with async def read/write), and LibrosaAudioReader/SoundfileAudioWriter (thin call-throughs offloaded to a thread pool via asyncio.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 stdlib unittest.mock.
  • ml/requirements.txt — added librosa==1.0.0 and soundfile==0.14.0.

Design decisions

  • read/write are async def, matching the async transform() pipeline that will consume this seam; blocking librosa.load/soundfile.write calls are offloaded via asyncio.to_thread.
  • LibrosaAudioReader.read passes sr=None to librosa.load to preserve the file's native sample rate instead of resampling to librosa's 22050 Hz default.
  • Used Protocol (not ABC) for the seam, per the project convention that Protocol is used for injectable interfaces with no shared implementation.
  • Did not create ml/pipeline/io/_doc_io.md — deferred pending explicit confirmation since the spec doesn't explicitly require it at this point.
  • Used stdlib unittest.mock for mocking (no mocking library precedent existed yet in ml/test/); no pytest-asyncio dependency added.

Testing completed

  • Confirmed tests red (ModuleNotFoundError) before implementation, then green after (7/7 new tests passing).
  • mypy --strict clean over ml/pipeline and ml/test.
  • Full pytest suite (28 tests) passes with no regressions.

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.
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown

Test Results

401 tests  ±0   401 ✅ ±0   2m 3s ⏱️ +2s
  5 suites ±0     0 💤 ±0 
  5 files   ±0     0 ❌ ±0 

Results for commit 4e1c5ab. ± Comparison against base commit 04a7121.

♻️ This comment has been updated with latest results.

@jodavis-claude jodavis-claude left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread ml/pipeline/io/audio_io.py
Addresses PR #255 review comment: this task completes the io subpackage's entire planned content (audio_io.py), so per the spec's per-subpackage doc convention and CLAUDE.md Quality Gate #4, the subpackage doc is added now rather than deferred.

@jodavis-claude jodavis-claude left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread ml/pipeline/io/_doc_io.md

- 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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants