feat: support cartesia enable words and add metrics for some tts#2121
Conversation
2466aaa to
b067f53
Compare
|
test |
|
full review pending - ignore this placeholder |
|
Code Review - Part 1 of 2 (Critical and High severity) CRITICAL
cartesia==2.0.15 was removed, but aiohttp (which the entire new implementation depends on) was not added. The extension will fail to import after this change. HIGH
In _process_audio_data, the end-of-audio signal (audio_data is None) is acted on unconditionally without checking request_id against self.current_request_id. Audio data chunks for old requests are correctly skipped, but the None sentinel is not guarded. When cancel_tts() is called, old_client.stop() puts (None, old_request_id, 0) into pcm_queue. That sentinel can later drain through _run_with_restart into the new client's consumer loop and trigger a spurious early completion of the new request. A request_id guard is needed here.
_build_request_payload extracts known structural keys from params but does not strip api_key. The remaining params loop then forwards api_key in every request payload. The key is already used for authentication via the URL query string; sending it in message bodies is redundant and adds unnecessary exposure. Strip it from the payload per the AGENTS.md pattern.
base_url was added under params in property.json, but it is a top-level CartesiaTTSConfig field, not a Cartesia API parameter. As a result, self.config.base_url stays empty string (falling back to the hardcoded default correctly), but base_url also ends up in self.config.params and gets forwarded to the Cartesia API in every request payload (see issue 3). Either remove it from property.json (the fallback default is already wss://api.cartesia.ai) or promote it to the top level of the JSON object. |
|
Code Review - Part 2 of 2 (Medium, Low/Nits, and Test feedback) MEDIUM
URL query parameters appear in proxy/load-balancer access logs, distributed tracing, and error reporting. The Cartesia WebSocket API also accepts an X-Api-Key header. Consider passing the key via aiohttp's headers parameter instead of the URL query string to reduce log exposure.
Putting bare None into typed queues so consumers raise TypeError on unpack is fragile. The exception will appear in logs as a confusing error, and any future refactoring of the consumer unpack will silently break shutdown. A clean sentinel tuple like (None, empty_string, 0) would let consumers detect shutdown without relying on an exception.
A new entry is written on every request but entries are never removed. Pop old entries when finishing a request.
audio_end_sent is initialised and reset to False in _reset_tts_request_info() but is never set to True anywhere in this diff. It appears to be a leftover from a refactor or a missing write path. LOW / NITS
The block handling done in chunk messages is commented out. Add a TODO explaining why if it is still under investigation, otherwise remove it before merging.
The docstring says First text start_ms == first audio frame timestamp but the implementation only enforces >= (returns False only when text starts before audio). Update the docstring to match the actual rule, or add a tolerance check in both directions if equality is truly intended.
The version was bumped from 0.7.0 to 0.8.1, skipping 0.8.0. Minor, but can be confusing in changelogs. Consider 0.8.0 or 1.0.0 given the scope of this rewrite.
cancel() clears TTFB tracking but not _last_word_end_ms[request_id]. The entry is only removed on a done message, which will not arrive for a cancelled context. A re-used request ID could acquire a spurious leading space word. TEST FEEDBACK
Overall the architecture change is an improvement and the test suite has been thoughtfully updated. The requirements.txt omission and the audio-end race condition are the most pressing items to address before merge. |
|
test |
|
deleting test |
|
PR Review: feat: support cartesia enable_words and add metrics for some TTS Overview This PR delivers two main features:
The overall direction is good. Below are the concerns found. Bugs 1. _base_start_ms and _all_samples are single-context - breaks under concurrent contexts cartesia_tts.py in _handle_ws_message has these fields shared across all context IDs. Cartesia WebSocket supports multiplexed contexts (different context_id values). If a second request arrives while the first is still streaming, _base_start_ms will be overwritten, corrupting timestamps. Fix: key these per context_id like _last_word_end_ms already does. 2. asyncio.create_task fire-and-forget without stored references In on_init and cancel_tts, asyncio.create_task calls for client.start(), _run_with_restart(_process_audio_data), and _run_with_restart(_process_transcription) are not stored. Python 3.10 docs warn that strong references are required to avoid GC. There is no way to cancel() them in on_stop. Task handles should be stored and cancelled in on_stop. 3. Incomplete request cleanup in error_callback error_callback calls send_tts_audio_end + finish_request only when has_received_text_input_end is True. When False, only send_tts_error is called with no finish_request. If the error fires mid-stream before text_input_end, the request is left hung. The base class likely expects finish_request for every started request. Code Quality 4. Misleading variable name _all_samples The formula stores byte_count / 2 * 1000, not an actual sample count. The downstream division by sample_rate gives milliseconds. A clearer name or comment would help. 5. Inconsistent character-metrics method In _process_transcription (cartesia), the call is metrics_add_input_characters. In google_tts, qwen3_tts, and rime_tts the call is metrics_add_output_characters. If these measure the same metric, the call sites should be consistent. 6. Callbacks as closures inside on_init error_callback, fatal_error_callback, and ttfb_metrics_callback are async closures inside on_init. Making them proper methods would be simpler and more testable. Test Issues 7. _validate_first_timestamp docstring/logic mismatch In test_subtitle_alignment.py, the docstring says equality (==) but the check only requires first_text_start >= first_audio_ts. The docstring and success message should be updated to say >=. 8. first_audio_frame_ts can be None at comparison self.first_audio_frame_ts is typed as int | None. _run_validations does not guarantee it is non-None after the audio_frames length guard. A simple assert after the guard would clarify the invariant and satisfy a type checker. Minor / Nits
Summary The metrics additions to google_tts, qwen3_tts, and rime_tts are clean and straightforward. The Cartesia rewrite is architecturally sound but has correctness issues worth addressing before merge: shared per-context timestamp state (1), dangling task references (2), and incomplete error cleanup (3). |
Code Review — PR #2121: Cartesia TTS full-duplex rewrite +
|
|
Removing placeholder - see full review below |
PR Review: feat: support cartesia enable words and add metrics for some ttsThis is a significant architectural improvement replacing the synchronous, SDK-based Cartesia client with a full-duplex BUGS 1. The accumulation in self._sample_count += float(len(audio_bytes)) / 2
cur_pcm_start_ms = int(self._base_start_ms + self._sample_count * 1000 / sample_rate)2.
if first_audio_ts is None:
return False, "No audio frames received"POTENTIAL ISSUES 3. async def _wait_for_client_available(self) -> bool:
while not self._is_stopped:
if self.client is not None:
return True
await asyncio.sleep(0.01)
return FalseIf the client is never recreated (e.g., due to a bug in 4. CPython may reuse memory addresses once an object is garbage-collected. If a 5.
6. Shared These fields are reset in the 7.
TEST CONCERNS 8. await asyncio.sleep(0.5)
ten_env.log_info("Running validations")This hardcoded wait is used to allow the last 9. A 1-second mismatch between text and audio duration would still pass the alignment test. For subtitle alignment a tighter threshold (e.g., 200-300 ms) would make the test more meaningful. 10. Unused All MINOR / STYLE 11.
12. Version jump from Minor, but worth aligning if there is a versioning convention. 13. Docstrings removed without replacement Several methods lost their docstrings ( STRENGTHS
Summary: The core architecture change is sound. Issues 1 (unit mismatch naming), 2 (None comparison crash in test), 6 (shared timestamp state across concurrent contexts), and 8 (fragile test sleep) are the most important to address before merging. |
PR Review: feat: support cartesia enable words and add metrics for some ttsThis is a substantial architectural refactor replacing the cartesia SDK with a direct aiohttp WebSocket implementation, moving to a full-duplex queue-based model. The overall design is solid and test coverage is good. Below are specific issues worth addressing. Bugs1. _all_samples stores scaled units, not actual sample count In cartesia_tts.py: This is mathematically correct (the x1000 cancels with /sample_rate), but the variable named _all_samples actually holds num_samples x 1000, not the sample count. This will confuse future readers. Consider renaming or restructuring to store the actual sample count and multiply by 1000 only at the use site. 2. base_url in params in property.json does not set config.base_url The new property.json adds base_url inside params. However config.base_url is a top-level field on CartesiaTTSConfig, not part of params. This entry is silently popped in _build_request_payload and never reaches _get_ws_url. As written it looks functional but does nothing. Move it to the top level or remove it. 3. _validate_first_timestamp docstring vs. implementation mismatch The docstring says "Rule 1: First text start_ms == first audio frame timestamp." But the check only fails when first_text_start < first_audio_ts. If first_text_start is significantly after first_audio_ts (misalignment in the other direction), the test still passes. Either fix the docstring or use an absolute tolerance check. Security4. Raw config dict (potentially containing API key) logged in update_configs If a caller passes a dict with api_key inside params, the key is logged in plaintext. A safe log already happens later via new_config.to_str(sensitive_handling=True). Remove the early raw log or redact sensitive fields before logging. Design Concerns5. _wait_for_client_available has no timeout If client recreation fails silently (an error path that does not set _is_stopped), this spins indefinitely blocking the incoming request. Add a deadline (e.g., 5 seconds) and return False on timeout. 6. Potential double tts_audio_end for the same request request_tts can send INTERRUPTED for an unfinished previous request via _handle_completed_request, while _process_audio_data may concurrently send REQUEST_END for that same request when the audio-end sentinel from the stopped queue arrives late. The last_audio_end_request_id guard helps, but it is read/written from two concurrent coroutines without synchronization. A comment (or a lock) documenting why this is safe would help. 7. _base_start_ms / _all_samples are single-context instance state These are reset on "done" but shared across all contexts. Since only one request is active at a time this works in practice, but a per-context map (like the existing _last_word_end_ms) would make the isolation explicit and more robust. Nits
Positives
|
f191e7e to
117f423
Compare
Code ReviewThis is a substantial and well-structured PR. The move from a request-response model to a full-duplex queue-based architecture is a clear improvement, and the new word-timestamp/subtitle alignment support is a nice addition. Below are my findings, ordered by severity. Bugs / Correctness1. Misleading variable name causes silent precision bug — The variable is named 2. Race between
3. If 4. Subtitle alignment test: rule 1 check is too lenient def _validate_first_timestamp(...):
"""Rule 1: First text start_ms == first audio frame timestamp."""
if first_text_start < first_audio_ts:
return False, ...
return True, "First text start_ms matches first audio frame timestamp"The docstring says == but the implementation accepts any value Similarly, the docstring for Test Coverage5. Removed audio-duration assertion in The old test verified that the reported Code Quality / Minor6. Polling loop instead of while not self._is_stopped:
if self.client is not None:
return True
if time.monotonic() >= deadline:
return False
await asyncio.sleep(0.01)A 10 ms polling interval for a 5-second window creates up to 500 unnecessary wake-ups. An 7. When 8. Version skip: 0.7.0 → 0.8.1 The manifest jumps from 9.
Summary
Overall this is clean, well-thought-out work. The full-duplex split, auto-reconnect loop with exponential backoff, config hot-reload, and the new subtitle alignment test infrastructure are all solid. Addressing points 2, 4, and 5 before merge would reduce the risk of subtle correctness issues in production. |


No description provided.