Conversation
4a8673d to
6f02a13
Compare
6f02a13 to
ff717ab
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Looks Good
- Clean implementation of configurable ElevenLabs API and websocket URLs
- New
_import_elevenlabs_environment()helper avoids circular imports and keeps lazy-loading pattern - Graceful fallback to default client when
base_url/wss_urlare not both provided - Same pattern applied consistently to both sync (
_generate_elevenlabs) and streaming (stream_tts_to_speaker) paths - Comprehensive regression tests with mocked environment verify correct URL passing
- No security concerns — custom URL support is a standard feature for enterprise proxy/infrastructure setups
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean, well-scoped change with good test coverage. No security concerns.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real parity gap: current main constructs the ElevenLabs client without URL configuration in both the sync path (tools/tts_tool.py:1003-1005) and streaming path (tools/tts_tool.py:2797-2799), while STT already resolves stt.elevenlabs.base_url (tools/transcription_tools.py:1579-1585).
Problems
- The new
if base_url and wss_urlbranches (tools/tts_tool.py:1008,:2808) ignore a configuredbase_urlunlesswss_urlis also set. This does not provide base-URL-only support. pyproject.toml:172pinselevenlabs==1.59.0; that SDK acceptsElevenLabs(base_url=...)directly. Use an explicit environment only for a separately configured WebSocket URL.gh pr diff 66311 --name-onlyshows no test changes, despite altering both construction paths.
Suggested changes
- Handle
base_urlindependently in both paths, withwss_urlas the opt-in case forElevenLabsEnvironment. - Add regression coverage for sync and streaming construction, including base-only configuration.
- Document the new user-facing config keys in the TTS configuration references.
Automated hermes-sweeper review.
| @@ -1001,7 +1008,11 @@ def _generate_elevenlabs(text: str, output_path: str, tts_config: Dict[str, Any] | |||
| output_format = "mp3_44100_128" | |||
There was a problem hiding this comment.
base_url alone is ignored here, so the advertised tts.elevenlabs.base_url configuration still routes to the default endpoint unless users also supply wss_url. The pinned ElevenLabs 1.59.0 client accepts base_url directly; handle that case independently, reserving ElevenLabsEnvironment for an explicitly distinct WSS URL.
Salvaged from PR #66311 (@moeadham), rebased onto the current streaming registry. tts.elevenlabs.base_url (+ optional wss_url, derived from base_url when omitted) routes both the sync ElevenLabs path and the chunked ElevenLabsStreamer through an ElevenLabsEnvironment, matching the STT side's ELEVENLABS_STT_BASE_URL/config override pattern.
…g tests Class-level sweep following the ElevenLabs salvage (#66311): every cloud TTS provider section now honors tts.<provider>.base_url. xAI, MiniMax, Gemini, OpenAI and DeepInfra already did; Mistral (SDK server_url) was the remaining gap. Adds per-provider config tests locking in the ElevenLabs environment plumbing and the Mistral server_url passthrough.
|
Merged into main via consolidated salvage PR #73512 (merge Your contribution is credited to you in git history. Thank you! Closing this PR as merged-via-salvage. |
Salvaged from PR NousResearch#66311 (@moeadham), rebased onto the current streaming registry. tts.elevenlabs.base_url (+ optional wss_url, derived from base_url when omitted) routes both the sync ElevenLabs path and the chunked ElevenLabsStreamer through an ElevenLabsEnvironment, matching the STT side's ELEVENLABS_STT_BASE_URL/config override pattern.
…g tests Class-level sweep following the ElevenLabs salvage (NousResearch#66311): every cloud TTS provider section now honors tts.<provider>.base_url. xAI, MiniMax, Gemini, OpenAI and DeepInfra already did; Mistral (SDK server_url) was the remaining gap. Adds per-provider config tests locking in the ElevenLabs environment plumbing and the Mistral server_url passthrough.
Salvaged from PR NousResearch#66311 (@moeadham), rebased onto the current streaming registry. tts.elevenlabs.base_url (+ optional wss_url, derived from base_url when omitted) routes both the sync ElevenLabs path and the chunked ElevenLabsStreamer through an ElevenLabsEnvironment, matching the STT side's ELEVENLABS_STT_BASE_URL/config override pattern.
…g tests Class-level sweep following the ElevenLabs salvage (NousResearch#66311): every cloud TTS provider section now honors tts.<provider>.base_url. xAI, MiniMax, Gemini, OpenAI and DeepInfra already did; Mistral (SDK server_url) was the remaining gap. Adds per-provider config tests locking in the ElevenLabs environment plumbing and the Mistral server_url passthrough.
Salvaged from PR NousResearch#66311 (@moeadham), rebased onto the current streaming registry. tts.elevenlabs.base_url (+ optional wss_url, derived from base_url when omitted) routes both the sync ElevenLabs path and the chunked ElevenLabsStreamer through an ElevenLabsEnvironment, matching the STT side's ELEVENLABS_STT_BASE_URL/config override pattern.
…g tests Class-level sweep following the ElevenLabs salvage (NousResearch#66311): every cloud TTS provider section now honors tts.<provider>.base_url. xAI, MiniMax, Gemini, OpenAI and DeepInfra already did; Mistral (SDK server_url) was the remaining gap. Adds per-provider config tests locking in the ElevenLabs environment plumbing and the Mistral server_url passthrough.
Certain elevenlabs environments require non-default URLs. This is already supported on STT, but for some reason was omitted on TTS.
This minimal change reads an optional
base_urlandwss_urlfrom the tts.elevenlabs config block, mirroring how the OpenAI TTS provider reads tts.openai.base_url. When unset, the ElevenLabs client is constructed exactly as before (no behavior change). When set, it is passed through environment=ElevenLabsEnvironment(...), since the ElevenLabs SDK does not accept a base_url kwarg directly.Applied to both construction sites: the sync _generate_elevenlabs handler and the streaming stream_tts_to_speaker path.
What does this PR do?
Fixes an omission where elevenlabs base_url is only supported on STT but not TTS.
Type of Change
Changes Made
How to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs