Repository navigation
chore: remove dead code in inference.VAD - #2600
detail-app[bot] wants to merge 1 commit into
Conversation
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| /** Update one or more knobs at runtime, propagating to live streams. */ | ||
| updateOptions(opts: Partial<VADOptions>): void { | ||
| this._opts = { ...this._opts, ...opts }; | ||
| for (const ref of this.#streams) { | ||
| const stream = ref.deref(); | ||
| if (stream === undefined) { | ||
| this.#streams.delete(ref); | ||
| continue; | ||
| } | ||
| stream.updateOptions(opts); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Live VAD option updates crash
Existing JavaScript callers of VAD.updateOptions now get a TypeError because the exported method has been removed. Active streams can no longer receive runtime threshold or buffer changes.
Learn more
The inference VAD is exported through inference. Its former public updateOptions method updated both the VAD instance and the streams it had already created. Removing it breaks existing runtime callers and eliminates live updates to streams; keeping just a method that changes _opts would leave active streams on stale settings.
Example: An existing JavaScript service calls vad.updateOptions({ minSilenceDuration: 800 }) during a call. Before this change, its active stream begins using 800 ms; now that call throws a TypeError.
Recommended fix: Retain VAD.updateOptions and its live-stream propagation, including the stream buffer resizing when padding or maximum speech changes, or introduce a compatibility path before removing the public method.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified no callers: rg "vad\.updateOptions|VAD\.updateOptions" (excluding *.test.ts) returns nothing across the repo. The silero plugin's analogous updateOptions (plugins/silero/src/vad.ts:67) is only invoked by its own internal stream propagation at line 70, with no external non-test callers — matching this PR's premise. Restoring the method would reverse the PR's stated dead-code-removal intent, so I'm not making that change. Whether removing an exported member warrants a version bump for external consumers is a maintainer versioning-policy call, not something the repo's code can settle.
| get model(): string { | ||
| return this._model; | ||
| } | ||
|
|
||
| get provider(): string { | ||
| return 'livekit-local-inference'; | ||
| } |
There was a problem hiding this comment.
🟡 VAD model and provider metadata disappear
Existing JavaScript callers reading VAD.model or VAD.provider now receive undefined. Integrations using these public getters lose the configured model and provider labels.
Learn more
The inference VAD is exported through inference, and both public getters previously returned stable model identity. Removing them changes observable runtime behavior even though no internal callers read them. This is separate from the base VAD's label, which identifies the implementation rather than supplying the old provider and model fields.
Example: A deployed JavaScript agent records vad.model and vad.provider as silero and livekit-local-inference. After upgrading, both values are undefined and its model attribution disappears.
Recommended fix: Preserve the getters if this is not an explicitly managed breaking API change, or provide a compatible metadata accessor and migration plan for downstream consumers.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified no readers: rg "vad\.(model|provider)\b|VAD\.(model|provider)" returns nothing across the repo. These getters lived only on the concrete inference.VAD (agents/src/inference/vad.ts), not on the abstract VAD contract in agents/src/vad.ts (which only requires label and stream()), so nothing typed against the base VAD read them. Restoring them would reverse the PR's stated dead-code-removal intent, so I'm not making that change. Whether removing exported members warrants a version bump for external consumers is a maintainer versioning-policy call.
|
Re: #2600 (review) — addressed both flagged issues on their threads (the |
1 similar comment
|
Re: #2600 (review) — addressed both flagged issues on their threads (the |
8875631 to
1b3de78
Compare
1 participant
Description
Removed unused getters, the
updateOptionscascade, and the#streamsregistry from the bundled inference VAD, plus the
vad.test.tsfilewhich only exercised
updateOptionspropagation.Changes Made
VAD.modelandVAD.providergetters plus the backingprotected _modelfield and its constructor assignment — these had nocallers anywhere in the repo (production or otherwise).
VAD.updateOptions,InferenceVADStream.updateOptions, andthe
#streamsWeakRefset used to fan option updates to live streams.These had no production callers; only
vad.test.tsinvoked them.vad.test.tsentirely, since it was solely dedicated toexercising the now-removed
updateOptionspropagation.VAD.stream()to drop the now-unused#streams.addand thestale snapshot comment.
Pre-Review Checklist
Testing
restaurant_agent.tsandrealtime_agent.tswork properly (for major changes)Additional Notes
Context:
model/providergetters were invoked nowhere in the repo.updateOptionsand its stream counterpart were test-only (tests don'tcount as production consumers);
AudioRecognition/AgentActivityreplacethe whole VAD instance rather than mutate options. The silero plugin's
analogous
updateOptionsis similarly test-only, confirming the parallelconvention no longer has live callers.
Tooling: Detail: Dead Code
History
Dead Code PRs can be configured here.