ADR-226: Audio I/O protocols and DelayAugmentor stage - #230
Conversation
- ml/pipeline/io/: new AudioReader/AudioWriter Protocol classes with
LibrosaAudioReader (mono conversion via librosa.load) and
SoundfileAudioWriter (PCM_16 WAV write)
- ml/pipeline/speech/delay_stage.py: DelayAugmentor(ModifierStage)
with independent prefix/suffix should_vary checks; both keys always
in applied_values (0.0 when not applied) for hash stability;
_derive_id format: "{id}_pre{ms}_suf{ms}"
- ml/pipeline/stages/speech_04_add_delays.py: thin DVC entry-point
- ml/pipeline/stages/params.py: AddDelaysParams dataclass + load()
- ml/params.yaml: add stages.add_delays section (6 float keys)
- ml/requirements.txt: add librosa>=0.10
- Tests: 28 delay stage tests, 9 audio I/O tests, 5 new params tests
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Review — ADR-226: Audio I/O protocols and DelayAugmentor stage
All exit criteria are met: AudioReader/AudioWriter protocols and default implementations are present, DelayAugmentor implements the correct ID formula and independent should_vary checks, both keys are always stored in applied_values, speech_04_add_delays.py entry-point is correct, unit test coverage is thorough, and params.yaml / PipelineParams are updated with the six float keys.
Two issues require changes before merge.
Priority 1 — Blocking I/O in async methods (2 locations in audio_io.py)
LibrosaAudioReader.read() and SoundfileAudioWriter.write() declare async def but call synchronous blocking I/O directly (librosa.load() and sf.write()). This blocks the event loop for the entire duration of each file operation. CONTRIBUTING.md: "no blocking calls on async code paths." Both methods must offload to loop.run_in_executor(None, ...). See inline comments for suggested fix.
Priority 4 — Documentation not updated (2 doc gaps)
ml/pipeline/io/is a new subsystem with no_doc_*.mdfile. Thespeech/package sets the pattern with_doc_speech.md; a_doc_io.mdis expected.ml/pipeline/speech/_doc_speech.mdstill listsdelay_stage.pyas a "planned, not yet implemented" stage and the Stages table does not includeDelayAugmentor. Both should be updated in this PR since the implementation is complete.
Style (non-blocking)
In test_audio_io.py, import asyncio is placed inside individual test methods rather than at the module top level. This is unconventional and slightly obscures the module's dependencies — move to the top-level imports.
Both async methods called synchronous disk I/O directly (librosa.load and sf.write), blocking the event loop for the full duration of each file operation. Offload each to a thread pool executor via run_in_executor so the event loop remains free during I/O, matching the async contract.
import asyncio was placed inside each individual test method. Move it to the top-level imports per standard Python convention.
New subsystems require a _doc_*.md per CONTRIBUTING.md. Documents the AudioReader/AudioWriter Protocol contracts, default implementations (LibrosaAudioReader, SoundfileAudioWriter), algorithm choices, thread pool offload rationale, and deferred import rationale.
delay_stage.py / DelayAugmentor was listed as a planned (not yet implemented) stage. It is now implemented; add it to the Stages table and remove it from the planned list.
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review — ADR-226 audio I/O protocols and DelayAugmentor.
All three prior review threads have been resolved:
-
LibrosaAudioReader blocking I/O (PRRT_kwDOQYcoLM6KUbEY): Fixed in c7ecfe6.
librosa.load()is now offloaded vialoop.run_in_executor(None, lambda: ...). Resolved. -
SoundfileAudioWriter blocking I/O (PRRT_kwDOQYcoLM6KUbK7): Fixed in c7ecfe6 alongside the reader fix.
sf.write()is now offloaded to thread pool. Resolved. -
Missing
_doc_io.mdand outdated_doc_speech.md(PRRT_kwDOQYcoLM6KUblh): Fixed in baa20af and 59e7a7e._doc_io.mdcovers Protocol contracts, default implementations, algorithm choice, thread pool rationale, and deferred import rationale._doc_speech.mdnow listsDelayAugmentorin the Stages table and removes it from the planned list. Resolved.
Scan of modified files (audio_io.py, test_audio_io.py, _doc_io.md, _doc_speech.md) found no new Priority 1–4 issues.
All exit criteria from the task brief are met. All quality gates (build, unit tests, E2E tests) pass per the validation log. Ready for human review.
…med type AudioReader.read() now returns AudioData(samples, sample_rate) instead of a raw tuple. This improves call-site readability and makes intent explicit at the protocol boundary. LibrosaAudioReader updated to construct AudioData; tests updated to access result.samples and result.sample_rate instead of unpacking.
…precision; omit zero-delay ID parts - DelayAugmentor constructor now accepts a single AddDelaysParams instead of six individual float arguments. The entry-point passes params.add_delays directly. - MinMaxFilter precision reduced from 6 to 1 (100ms resolution is sufficient for audio delay augmentation; sub-millisecond granularity is unnecessary). - _derive_id now omits _pre and _suf segments when the respective delay is 0.0, producing cleaner IDs (e.g. "TV_ON_Jenny_r77" instead of "TV_ON_Jenny_r77_pre0_suf0"). - Tests updated: _make_stage accepts AddDelaysParams; TestDeriveId updated for new format; TestGenerateOutput/TestAppliedValues use precision=1-valid delay values (0.1s).
… defaults for precision=1 Renamed speech_04_add_delays.py to speech_02_add_delays.py because only speech_01 exists; the delay stage is the second speech pipeline stage, not the fourth. Updated params.yaml default delay range from 0.02-0.15s to 0.1-0.5s so that the defaults are valid at MinMaxFilter precision=1 (100ms resolution). The previous defaults (20ms, 150ms) would round to 0.0 and 0.1 at precision=1, making the effective range degenerate.
Document AudioData dataclass in the Protocols table and update the AudioReader contract description to reflect the new return type.
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review — all prior threads addressed, no new issues found.
Prior threads (all resolved):
- PRRT_kwDOQYcoLM6KXDi- (AudioData type): Fixed in 9b458a2.
AudioDatadataclass added;AudioReader.read()returnsAudioData; all callers updated. Resolved. - PRRT_kwDOQYcoLM6KXFpQ (rename speech_04 to speech_02): Fixed in 3123dd0.
speech_02_add_delays.pyin place; old file deleted. Resolved. - PRRT_kwDOQYcoLM6KXJAb (pass AddDelaysParams, not six floats): Fixed in e2445a0. Constructor takes
params: AddDelaysParams; entry-point passesparams.add_delays. Resolved. - PRRT_kwDOQYcoLM6KXKgS / KXKn9 (precision=1 on both MinMaxFilter instances): Fixed in e2445a0. Both filters use
precision=1. Resolved. - PRRT_kwDOQYcoLM6KXLvw (fewer digits in ID suffixes): Intent addressed — with
precision=1, delays are multiples of 100ms so IDs are naturally shorter (e.g._pre100). Resolved. - PRRT_kwDOQYcoLM6KXL_M (omit pre/suf when zero): Fixed in e2445a0.
_derive_idappends segments conditionally; zero-delay samples get bare input ID. Resolved.
Scan of modified files (commits 9b458a2, e2445a0, 3123dd0, 2189f09):
Files reviewed: audio_io.py, delay_stage.py, speech_02_add_delays.py, params.yaml, test_audio_io.py, test_delay_stage.py, _doc_io.md.
No new Priority 1–4 issues found.
SoundfileAudioWriter.write() was still taking data/sample_rate as separate arguments rather than AudioData, inconsistent with the AudioWriter Protocol definition (line 44) that was updated in the previous fix iteration. Updated SoundfileAudioWriter.write() to accept audio: AudioData and unpack audio.samples / audio.sample_rate internally. Updated delay_stage._generate_output() to pass AudioData to the writer. Updated _RecordingAudioWriter stub and all test call sites in test_audio_io.py and test_delay_stage.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The AudioWriter contract description still showed the old data/sample_rate signature. Updated to reflect the current audio: AudioData parameter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3 participants
Work item: ADR-226 — Implement the
ml/pipeline/io/package (AudioReader and AudioWriter protocols with default librosa/soundfile implementations) and theDelayAugmentorstage that reads/writes WAV files and prepends/appends silence, plus its DVC entry-point.Changes
ml/pipeline/io/__init__.py— new empty package init for theiosub-packageml/pipeline/io/audio_io.py—AudioReaderandAudioWriterProtocol classes;LibrosaAudioReader(useslibrosa.load(..., mono=True)for stereo-to-mono conversion);SoundfileAudioWriter(PCM_16 WAV write via soundfile)ml/pipeline/speech/delay_stage.py—DelayAugmentor(ModifierStage[AudioSample, AudioSample])with injectedAudioReader,AudioWriter, andinput_dir; implements_get_applied_values(independentshould_varyper key; 0.0 stored when not applied),_derive_id(f"{input.id}_pre{int(prefix*1000)}_suf{int(suffix*1000)}"),_generate_output(prepend/append zero-filled silence; preserve transcript)ml/pipeline/stages/speech_04_add_delays.py— thin DVC entry-point; constructsDelayAugmentorwithLibrosaAudioReader/SoundfileAudioWriter; reads delay params fromPipelineParams; callsasyncio.run(stage.transform(...))ml/pipeline/stages/params.py— addedAddDelaysParamsdataclass (6 float fields); addedadd_delays: AddDelaysParamsfield toPipelineParams; extendedload()to readstages.add_delays.*ml/params.yaml— addedstages.add_delays:section with 6 float keys and defaults (0.333 vary probability, 0.02–0.15 s range for prefix and suffix)ml/requirements.txt— addedlibrosa>=0.10ml/test/pipeline/io/__init__.py— new empty test package initml/test/pipeline/io/test_audio_io.py— 9 unit tests forLibrosaAudioReader(stereo→mono, float32 dtype, correct sample rate) andSoundfileAudioWriter(creates file, readable WAV, preserves sample count)ml/test/pipeline/speech/test_delay_stage.py— 28 unit tests coveringTestAppliedValues,TestDeriveId,TestGenerateOutput, andTestSkipPathml/test/pipeline/stages/test_params.py— addedadd_delayssection to fixture; addedTestAddDelaysParamsLoadclass (5 tests)Design decisions
input_diradded toDelayAugmentorconstructor: The brief said "no constructor-time audio dependencies beyond the reader and writer" — interpreted as audio service dependencies, not directory paths.input_diris required to resolveAudioSample.path(bare filename) to an absolute path for reading. Without it the stage cannot function in production.MinMaxFilterprecision=6 for second-scale delays: Defaultprecision=0would round 0.02–0.15 s values to 0 (integer scale), making the filter degenerate.precision=6gives microsecond granularity appropriate for audio timing.librosa.load(..., mono=True): The spec specifies the outcome (1-D mono float32), not the algorithm.librosa.load(..., mono=True)is idiomatic and simpler thandata.mean(axis=1); consistency withEdgeTtsProviderwas not required by the spec.