fix(dub): async-ify _pitch_preserving_stretch (Greptile P1 from #133) - #152
Conversation
_pitch_preserving_stretch ran a blocking subprocess.run() inside the `_stream` async generator (on the event loop). Each ffmpeg atempo call is ~50-100 ms, so on a multi-segment time_stretch dub job it froze health checks, status SSE, and every other concurrent request for seconds. Convert to asyncio.create_subprocess_exec + await communicate() (same pattern as run_proc_streaming_stderr); await the call site in _stream. Drop the now-unused `import subprocess`. Tests: async coroutine + target-length + no-op cases (real ffmpeg). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR converts the ChangesAsync ffmpeg pitch stretch
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| import asyncio | ||
|
|
||
| import numpy as np |
|
| Filename | Overview |
|---|---|
| backend/api/routers/dub_generate.py | Converts _pitch_preserving_stretch to async using asyncio.create_subprocess_exec + proc.communicate(); single call site correctly updated with await; import subprocess removed. Implementation is correct and matches the established run_proc_streaming_stderr pattern. |
| tests/test_pitch_stretch_async.py | New test file placed correctly in tests/ (conftest.py adds backend/ to sys.path). Covers the async contract assertion and the real-ffmpeg path. Noop test's shape check is slightly under-specified (shape[-1] only; see comment). |
Sequence Diagram
sequenceDiagram
participant EL as Event Loop
participant SG as _stream (async gen)
participant PPS as _pitch_preserving_stretch (async)
participant FF as ffmpeg subprocess
Note over EL,FF: Before this PR — blocking path
EL->>SG: iterate segment
SG->>PPS: subprocess.run(...) [BLOCKS event loop ~50-100ms]
PPS-->>SG: bytes result
Note over EL: health-checks / SSE / concurrent requests stall
Note over EL,FF: After this PR — non-blocking path
EL->>SG: iterate segment
SG->>PPS: await _pitch_preserving_stretch(...)
PPS->>FF: asyncio.create_subprocess_exec(ffmpeg ...)
PPS->>FF: "await proc.communicate(input=pcm_bytes)"
FF-->>PPS: (stdout, stderr)
PPS-->>SG: stretched tensor
Note over EL: other coroutines run freely while ffmpeg executes
Reviews (1): Last reviewed commit: "fix(dub): async-ify _pitch_preserving_st..." | Re-trigger Greptile
| out = asyncio.run(_pitch_preserving_stretch(wav, sr, sr)) | ||
| assert out.shape[-1] == sr |
There was a problem hiding this comment.
The noop test only asserts on the last dimension, so a hypothetical regression that changed the number of channels (e.g., accidentally returning a 1-D tensor) would silently pass. Asserting the full shape keeps the contract tight.
| out = asyncio.run(_pitch_preserving_stretch(wav, sr, sr)) | |
| assert out.shape[-1] == sr | |
| out = asyncio.run(_pitch_preserving_stretch(wav, sr, sr)) | |
| assert out.shape == (1, sr) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Addresses the Greptile P1 flagged on #133.
Problem
_pitch_preserving_stretchran a blockingsubprocess.run()inside the_streamasync generator (directly on the event loop, not in an executor). Each ffmpegatempoinvocation is ~50-100 ms, so a 100-segmenttime_stretchdub job froze the event loop for seconds — health-check polls, status SSE streams, and every concurrent API request stalled.Fix
_pitch_preserving_stretch→async def, usingasyncio.create_subprocess_exec+await proc.communicate(input=…)(mirrorsrun_proc_streaming_stderrindub_pipeline.py).awaitthe single call site in_stream.import subprocess.Behavior is otherwise identical (same ffmpeg command, same pad/trim, same RuntimeError-on-failure contract).
Tests
tests/test_pitch_stretch_async.py— asserts it returns a coroutine (can't block the loop), hits the target length via real ffmpeg, and the no-op (already-target-length) path. 15 pass with the existing timing-strategy suite.No default-behavior change, no version bump.
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Performance Improvements
Tests