[Plugin] Palabra TTS (+ voice cloning) - #627
Conversation
github-actions
Bot
added
dependencies
agents-core
plugins
config
docs
project-info
assets
labels
📝 WalkthroughWalkthroughAdded the Palabra plugin to the workspace and optional dependencies. Implemented persistent WebSocket streaming TTS with cancellation, retries, idle timeouts, and PCM output. Added REST-based voice cloning with validation, polling, quota management, and lifecycle operations. Added public exports, tests, runnable examples, project configuration, and documentation. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
plugins/palabra/vision_agents/plugins/palabra/__init__.py (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an explicit standard-library import.
Import
extend_pathfrompkgutilbefore the relative imports. Replace__import__("pkgutil")with the imported symbol.Proposed change
+from pkgutil import extend_path + from .tts import TTS, WS_URL_EU, WS_URL_US, PalabraTTSError from .voices import ClonedVoice, PalabraVoiceError, VoiceLimits, Voices # Re-export under the new namespace for convenience -__path__ = __import__("pkgutil").extend_path(__path__, __name__) +__path__ = extend_path(__path__, __name__)As per coding guidelines, imports must be at module scope and ordered standard library before local package imports.
Source: Coding guidelines
plugins/palabra/vision_agents/plugins/palabra/voices.py (1)
313-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBare
dictannotations do not meet the typing guideline.
_requestusesjson: Optional[dict]andparams: Optional[dict]and returnsdict._parse_voice(line 103) takesdata: dictand_upload_sample(line 274) takessample: dict. The guidelines require parameterized generics. Note thatclonealready buildspayload: dict[str, object]andlistalready buildsparams: dict[str, str | int], so the concrete types are known.Also annotate
__aexit__(line 343) rather than leaving*_untyped.Proposed change
async def _request( self, method: str, path: str, *, - json: Optional[dict] = None, - params: Optional[dict] = None, - ) -> dict: + json: Optional[dict[str, object]] = None, + params: Optional[dict[str, str | int]] = None, + ) -> dict[str, object]:-def _parse_voice(data: dict) -> ClonedVoice: +def _parse_voice(data: dict[str, object]) -> ClonedVoice:async def _upload_sample( - self, sample: dict, filename: str, data: bytes, mime_type: str + self, sample: dict[str, object], filename: str, data: bytes, mime_type: str ) -> None:- async def __aexit__(self, *_) -> None: + async def __aexit__( + self, + exc_type: Optional[type[BaseException]], + exc: Optional[BaseException], + tb: Optional[TracebackType], + ) -> None: await self.close()
_parse_voiceandlimitsthen need explicit narrowing (castor localstr(...)/int(...)conversion) to stay type-clean. If that churn is unwanted, a smallTypedDictper endpoint response is the cleaner alternative.As per coding guidelines: "Use type annotations everywhere. Modern syntax:
X | Yunions,dict[str, T]generics, fullCallablesignatures".Source: Coding guidelines
plugins/palabra/tests/test_voices.py (2)
89-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the type assertion.
assert isinstance(listed, list)restates the annotated return type oflist(). It tests no behavior. The second assertion already covers the useful property.Proposed change
listed = await voices.list(page_size=5) - assert isinstance(listed, list) assert all(voice.voice_id for voice in listed) + assert len(listed) <= 5As per path instructions: "Assert behavior and outputs, not initialization or call paths" and "Discourage verbosity and redundant tests".
Source: Path instructions
95-131: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign the cloned-TTS audio path to one streaming API shape.
pytest-timeoutis declared in the dev dependencies, sopytest.mark.timeout(300)is supported.Palabra.TTSalso supports bothsend_iter(line)andstream_audio(...), but use one iteration shape for this test.stream_audio(...)already returns an async iterator, so do not await it before iteration.plugins/palabra/example/clone_voice.py (1)
50-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated WAV writer.
write_sampleandspeakrepeat the same five-line WAV write plus the same duration calculation. One helper covers both.Proposed change
+def _write_wav(path: Path, audio: bytes, rate: int) -> float: + """Write mono 16-bit PCM to ``path`` and return its duration in seconds.""" + with wave.open(str(path), "wb") as wav: + wav.setnchannels(1) + wav.setsampwidth(2) + wav.setframerate(rate) + wav.writeframes(audio) + return len(audio) / 2 / rate + + async def write_sample(tts: palabra.TTS, path: Path) -> float: """Synthesize the passage into a WAV file and return its duration.""" audio = bytearray() for line in SAMPLE_SCRIPT: async for chunk in tts.send_iter(line): if chunk.data is not None: audio += chunk.data.samples.tobytes() - - with wave.open(str(path), "wb") as wav: - wav.setnchannels(1) - wav.setsampwidth(2) - wav.setframerate(tts.sample_rate) - wav.writeframes(audio) - return len(audio) / 2 / tts.sample_rate + return _write_wav(path, bytes(audio), tts.sample_rate)if chunk.data is not None: audio += chunk.data.samples.tobytes() rate = tts.sample_rate finally: await tts.close() - - with wave.open(str(path), "wb") as wav: - wav.setnchannels(1) - wav.setsampwidth(2) - wav.setframerate(rate) - wav.writeframes(audio) - return len(audio) / 2 / rate + return _write_wav(path, bytes(audio), rate)plugins/palabra/tests/test_tts.py (1)
36-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace private-state assertions with public behavior tests.
These tests depend on private fields. Refactoring serialization or connection storage can break them without changing public behavior.
plugins/palabra/tests/test_tts.py#L36-L63: verify configured synthesis through the public TTS API instead of parsing_init_message.plugins/palabra/tests/test_tts.py#L178-L188: verify successful consecutive utterances instead of comparing_websocketidentity.As per coding guidelines and path instructions, tests must assert behavior and outputs, not initialization or call paths.
Sources: Coding guidelines, Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c292ef97-130c-43f1-b99f-cefa4b366c57
⛔ Files ignored due to path filters (2)
plugins/palabra/example/palabra_smoke.wavis excluded by!**/*.wavuv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
CHANGELOG.mdagents-core/pyproject.tomlplugins/palabra/README.mdplugins/palabra/example/.env.exampleplugins/palabra/example/README.mdplugins/palabra/example/clone_voice.pyplugins/palabra/example/main.pyplugins/palabra/example/pyproject.tomlplugins/palabra/example/tts_smoke.pyplugins/palabra/py.typedplugins/palabra/pyproject.tomlplugins/palabra/tests/__init__.pyplugins/palabra/tests/test_tts.pyplugins/palabra/tests/test_voices.pyplugins/palabra/vision_agents/plugins/palabra/__init__.pyplugins/palabra/vision_agents/plugins/palabra/tts.pyplugins/palabra/vision_agents/plugins/palabra/voices.pypyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62b2ee99-719f-431c-8913-09380fa17bda
📒 Files selected for processing (5)
CHANGELOG.mdplugins/palabra/tests/test_tts.pyplugins/palabra/tests/test_voices.pyplugins/palabra/vision_agents/plugins/palabra/tts.pyplugins/palabra/vision_agents/plugins/palabra/voices.py
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/palabra/tests/test_tts.py
- CHANGELOG.md
- plugins/palabra/vision_agents/plugins/palabra/voices.py
- plugins/palabra/vision_agents/plugins/palabra/tts.py
| before = await voices.limits() | ||
| with pytest.raises(PalabraVoiceError): | ||
| await voices.clone("Rejected sample", sample, timeout=180, poll_interval=5) | ||
|
|
||
| assert (await voices.limits()).total == before.total |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Delete a voice if the rejection assertion fails.
If Palabra accepts the sample, voices.clone() returns a voice and pytest.raises fails. The test has no cleanup path for that voice. It can leave quota allocated and block later cloning tests.
Proposed fix
before = await voices.limits()
- with pytest.raises(PalabraVoiceError):
- await voices.clone("Rejected sample", sample, timeout=180, poll_interval=5)
+ created_voice: palabra.ClonedVoice | None = None
+ try:
+ with pytest.raises(PalabraVoiceError):
+ created_voice = await voices.clone(
+ "Rejected sample", sample, timeout=180, poll_interval=5
+ )
+ finally:
+ if created_voice is not None:
+ await voices.delete(created_voice.voice_id)
assert (await voices.limits()).total == before.total1 participant
Why
Changes
palabra-tts.mp4