feat: opt-in coalescing of timed ASR calls - #391
Conversation
b6ba65b to
3a24a6c
Compare
QuentinFuxa
left a comment
There was a problem hiding this comment.
Thanks for measuring before proposing, and the premise checks out on my side too: with VAC on (the default), pacing comes from vac_chunk_size at audio_processor.py:79 and --min-chunk-size never throttles process_iter (its only other consumer, AlignAttConfig.segment_length, is never read). The gate being opt-in and the pure-function tests following the test_retention.py precedent are both right. But the deferral is not safe for the LocalAgreement policy, and both problems are silent data loss, so this needs changes before merge.
Blockers:
-
Trailing deferred audio is never transcribed at end of stream. The final chunks accrue less than
min_s, get inserted viainsert_audio_chunk, and skipprocess_iter; then SENTINEL arrives and_finish_transcription()runs with no deferral flush.OnlineASRProcessor.finish()(local_agreement/online_asr.py:399-408) runs no inference at all: it returnstranscript_buffer.buffer, which by construction excludes the deferred audio, and thebuffer_transcriptionfallback in_finish_transcriptionreads the same stale text. This fires on nearly every ended stream, because_flush_remaining_pcm(audio_processor.py:984-1000) always enqueues a final sub-second chunk right before SENTINEL, and withmin_sat 0.75-2.0 that chunk is deferred. So with localagreement (any backend, including REST/v1/audio/transcriptions), the last words of every session are silently dropped. The other processors are safe because theirfinish/start_silencerun real inference over pending audio; LocalAgreement's do not. -
Words spoken just before a long silence can no longer commit. The reset at
Silence.is_startingassumes "boundary flushes; nothing is stranded", but for LocalAgreement that is only half true:start_silence(online_asr.py:161-164) runsprocess_iteronce over the deferred audio, andHypothesisBuffer.flush(online_asr.py:59-86) only commits a token seen in two consecutive agreeing passes (confidence_validationis off by default). Deferred audio gets its first-ever pass insidestart_silence, so its tokens stay uncommitted, and when the silence ends at >= 5 s,end_silencecallsinit(offset=...)(online_asr.py:166-177), which discards them permanently. On main the tail is processed on arrival and again atstart_silence, so the two-pass agreement commits it; with the gate enabled that second pass no longer exists.
Both have the same fix, and it keeps your design: when deferred_audio_s > 0, drain it with a real (counted) process_iter before every boundary handler (Silence, ChangeSpeaker, SENTINEL) instead of just zeroing the counter. That restores main's behavior exactly at boundaries and only coalesces in steady state, which is where your savings come from anyway.
Asks:
-
--asr-coalesce-max-sis currently inert:resolve_coalesce_windowenforcesmax > min, so theprospective < max_sconjunct inshould_defer_inferenceis unreachable, and deferral already terminates onceprospective >= min_s, so held-back audio is inherently bounded bymin_splus one chunk. As shipped it is only an obscure on/off validator, with a trap: the defaultmax=1.0silently disables any--asr-coalesce-min-s >= 1.0, which includes three of the six rows in your own benchmark table. Either drop the flag (my preference: one knob, and the time-to-first-word bound genuinely ismin_s) or give it real semantics, and in any case fail or warn loudly when the pair resolves to disabled. -
Please add one TestHarness test of the actual deferral path (the helpers tests are fine but cannot catch either blocker): localagreement + coalescing on, feed real audio that ends mid-speech, assert the final words are present after
finish(); ideally a second one with a > 5 s silence asserting the pre-silence tail commits. Either would have caught both blockers. -
Scope the docs to the whisper-family backends: the qwen3 processors already coalesce internally (
_MIN_NEW_SECONDS = 1.0in the vllm processors,pending_sec/due_afterin qwen3-streaming), so the flag does approximately nothing there. Worth one sentence in docs/troubleshooting.md so people do not tune it against the wrong backend. For the longer term I keep in mind that a processor-level gate honoringis_lastcannot strand audio by construction, but I am fine landing this at the pipeline level once the boundaries drain.
Nits: fetch get_buffer() outside the lock and assign under it, matching the timeout branch at audio_processor.py:508-513; and the "boundary flushes; nothing is stranded" comment needs to go or be corrected either way.
process_iter() costs a full-length encoder pass regardless of how much new audio arrived, so short chunks re-encode mostly the same audio. Add a gate that defers inference until enough new audio has accrued, bounded by a ceiling so the added time-to-first-word stays finite. Disabled by default. --min-chunk-size does not reach this path when VAC is enabled, so there is currently no way to make this trade. The decision logic is two pure functions so it can be tested without loading a model.
Coalescing deferred inference past boundaries that reset the processor. On LocalAgreement that silently lost audio twice over: finish() runs no inference, so a deferred trailing chunk was never transcribed at all, and a token needs two agreeing passes to commit, so audio first seen inside start_silence() or new_speaker() was discarded by the init() that follows. Deferred audio now gets a real counted process_iter() before every boundary handler, giving it the first of the two passes. finish() only reports the hypothesis tail, so tokens committed by the sentinel drain are passed to _finish_transcription() explicitly. --asr-coalesce-max-s is removed: resolve_coalesce_window enforced max > min, which made the ceiling check unreachable, while its default silently disabled any min >= 1.0. Deferral is inherently bounded by min_s plus one chunk.
3a24a6c to
16dd925
Compare
|
Thanks for the review, and for the line numbers. They meant I could go straight to both of these instead of hunting for them. Both blockers reproduce. I instrumented the deferral state rather than reasoning about it, and the end-of-stream one shows up immediately: cut the feed mid-speech and Boundaries now drain. Deferred audio gets a real counted Two things I ran into:
Tests. Added, and they do fail on the pre-fix code with a diverged transcript tail. They nearly didn't, though. My first attempt fed at Docs, though vaguer than you asked for. I couldn't find Nits done: Rebased on main (v0.2.25). Full suite 117 passed, ruff clean. On the processor-level gate honouring |
Adds an opt-in gate that coalesces timed ASR calls. Off by default, so nothing
changes unless you ask for it.
Why
transcription_processor()runsprocess_iter()once per arriving chunk, andeach call costs a full-length encoder pass regardless of how much new audio it
contains. When chunks are short, most of that work re-encodes audio the previous
pass already saw.
There is currently no way to trade update cadence for that work.
--min-chunk-sizelooks like the knob but never reaches this path when VAD/VAC is on, which is the
default:
Measured rather than assumed:
--min-chunk-sizeat 0.1 (default), 0.5 and 1.0gives 40 ASR calls in all three cases.
Numbers
base+ faster-whisper, CPU, 3 samples fed at real time, 3 reps per setting.n_asr_callscountsprocess_iter()invocations and reproduces exactly acrossreps, so it is the figure to trust here rather than the milliseconds.
--asr-coalesce-min-sThree things you would want to know before merging:
audio, so 0.5 barely fires while 0.75 coalesces nearly every pair. The useful
value is tied to a deployment's chunk cadence, which is why this ships with no
tuned default.
0.0833 / 0.0667 / 0.0833 / 0.0667 across those rows. That oscillation is noise
on a small word count, and I am not claiming an accuracy effect either way.
Only 3 samples, so treat the magnitudes as indicative, not general.
through them, which is the difference between these numbers and an earlier
revision of this PR. That is the price of not losing audio, and it is worth it.
Review notes
Three source files plus tests and a
docs/troubleshooting.mdentry.Deferred audio is drained with a real counted
process_iter()before everyboundary handler (
Silence,ChangeSpeaker, SENTINEL). This matters onLocalAgreement, whose
finish()runs no inference and whoseHypothesisBufferneeds two agreeing passes to commit: without the drain, a deferred tail is either
never transcribed or discarded by the
init()that follows a boundary. The drainsupplies the first pass so the handler's own pass is the second, matching the
behaviour when every chunk is inferred on arrival. Tokens the SENTINEL drain
commits are passed to
_finish_transcription()explicitly, sincefinish()onlyreports the hypothesis tail.
The threshold logic is two pure functions,
resolve_coalesce_min_sandshould_defer_inference, testable without loading a model, followingresolve_retention_secondsintest_retention.py. Deferral is bounded by thethreshold plus one chunk.
Tests:
tests/test_asr_coalescing.pycovers the threshold logic including thatthe shipped default resolves to disabled, and
tests/test_asr_coalescing_pipeline.pydrives the real pipeline for the two boundary cases. Both pipeline tests fail on
the pre-drain code with a diverged transcript tail. They feed at
speed=1.0andcut mid-speech deliberately: at
speed=0the whole file arrives as one chunk,nothing is deferred, and they pass vacuously.
Full suite 117 passed,
ruff check .clean, rebased on v0.2.25.Provenance
A machine was in the loop. This began as output from an automated optimisation
run (Artemis Discovery, the tool I work on at TurinTech) pointed at the streaming
policy, with ASR compute as the objective and the existing WER gate as the
constraint.
What it produced is not what is here. I reproduced every number against a clean
checkout and in doing so cut half its patch (a dedup at silence and speaker
boundaries that measured 40 calls, i.e. contributed nothing), dropped its
headline accuracy claim as noise, flipped the default from on to off, and
rewrote the logic to be testable. Worth naming the failure modes: it defaulted a
behaviour change to on, and reported an accuracy win a wider sweep does not
support. The boundary data-loss bugs found in review were in its output too, and
survived my own review of it.
CONTRIBUTING asks for significant changes to be discussed first, so happy to move
this to an issue if you would rather start there, or to reshape the flag.