Reuse TTS synthesizer in API server#212
Conversation
📝 WalkthroughWalkthroughThe TTS API adds configurable synthesizer reuse, YAML and environment-based configuration, startup warming through a Litestar lifespan handler, and application-factory entrypoints for container and script launches. ChangesConfigurable TTS API startup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant lifespan
participant get_api_config
participant warm_tts
participant TTSFactories
Server->>lifespan: start Litestar application
lifespan->>get_api_config: read reuse_tts
get_api_config-->>lifespan: return configuration
lifespan->>warm_tts: initialize cached TTS resources when enabled
warm_tts->>TTSFactories: create synthesizer and converter
TTSFactories-->>warm_tts: return initialized resources
warm_tts-->>lifespan: return
lifespan-->>Server: serve requests
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
643ad8f to
0d2f5df
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/glados/api/app.py (1)
58-62: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider offloading
warm_tts()to a thread executor.
warm_tts()is a synchronous blocking call (ONNX session loading) inside an async context manager. While this runs during startup before any requests are served, it blocks the event loop and could delay ASGI lifespan completion, affecting orchestrator startup probes. Wrapping it inasyncio.to_thread()would keep the event loop responsive.This is low-priority since the block is one-time and startup-only.
♻️ Optional refactor
`@asynccontextmanager` async def lifespan(app: Litestar) -> AsyncIterator[None]: if get_api_config().reuse_tts: - warm_tts() + await asyncio.to_thread(warm_tts) yield🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/glados/api/app.py` around lines 58 - 62, Update the lifespan function to offload the synchronous warm_tts call to a worker thread using asyncio.to_thread, awaiting its completion before yield; add the required asyncio import while preserving the existing reuse_tts condition.tests/test_api_config.py (1)
48-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test for existing YAML file with missing expected key
The test suite covers missing files but not the case where a YAML file exists yet lacks the
"Api"top-level key. This is the exact edge case that triggers theKeyErroridentified inconfig.pylines 49-53. Adding a test here would verify the fix and prevent regression.🧪 Suggested test
def test_api_config_skips_missing_files() -> None: config = ApiConfig.from_yaml("/path/that/does/not/exist.yaml") assert config.reuse_tts is True + + +def test_api_config_skips_missing_key(tmp_path: Path) -> None: + config_file = tmp_path / "api_config.yaml" + config_file.write_text("Other:\n foo: bar\n", encoding="utf-8") + + config = ApiConfig.from_yaml(config_file) + + assert config.reuse_tts is True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_api_config.py` around lines 48 - 51, Add a test alongside test_api_config_skips_missing_files that creates or loads an existing YAML file without the top-level “Api” key, then asserts ApiConfig.from_yaml returns the expected default configuration, including reuse_tts being True. Ensure the test exercises the missing-key path rather than the nonexistent-file path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/glados/api/config.py`:
- Around line 49-53: Update the configuration traversal in the method containing
data, key_to_config, and config.update so each nested lookup safely handles
missing or non-dictionary YAML values by using an empty-dictionary fallback
instead of direct indexing. Preserve the existing config.update flow so missing
keys and empty YAML resolve through the model’s defaults without raising
KeyError or type errors.
---
Nitpick comments:
In `@src/glados/api/app.py`:
- Around line 58-62: Update the lifespan function to offload the synchronous
warm_tts call to a worker thread using asyncio.to_thread, awaiting its
completion before yield; add the required asyncio import while preserving the
existing reuse_tts condition.
In `@tests/test_api_config.py`:
- Around line 48-51: Add a test alongside test_api_config_skips_missing_files
that creates or loads an existing YAML file without the top-level “Api” key,
then asserts ApiConfig.from_yaml returns the expected default configuration,
including reuse_tts being True. Ensure the test exercises the missing-key path
rather than the nonexistent-file path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 411d0e2e-14ce-4c6c-a745-19fe0d0a02f8
📒 Files selected for processing (8)
DockerfileREADME.mdconfigs/api_config.yamlscripts/servesrc/glados/api/app.pysrc/glados/api/config.pysrc/glados/api/tts.pytests/test_api_config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/glados/api/tts.py
| data = data or {} | ||
| for key in key_to_config: | ||
| data = data[key] | ||
|
|
||
| config.update(data) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled KeyError when YAML file lacks the expected top-level key
data[key] on line 51 raises an unhandled KeyError if a YAML file exists but doesn't contain the expected key (e.g., "Api"). This also affects empty YAML files where yaml.safe_load returns None — data or {} produces {}, and the subsequent data["Api"] lookup crashes. The method docstring states "Missing files are skipped" but missing keys in existing files are not handled gracefully.
🛡️ Proposed fix: use `dict.get` with empty-dict fallback
data = data or {}
for key in key_to_config:
- data = data[key]
+ if not isinstance(data, dict):
+ data = {}
+ break
+ data = data.get(key, {})This skips missing keys (returning defaults via model_validate({})) and also guards against non-dict YAML values (e.g., a bare scalar or list).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data = data or {} | |
| for key in key_to_config: | |
| data = data[key] | |
| config.update(data) | |
| data = data or {} | |
| for key in key_to_config: | |
| if not isinstance(data, dict): | |
| data = {} | |
| break | |
| data = data.get(key, {}) | |
| config.update(data) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/glados/api/config.py` around lines 49 - 53, Update the configuration
traversal in the method containing data, key_to_config, and config.update so
each nested lookup safely handles missing or non-dictionary YAML values by using
an empty-dictionary fallback instead of direct indexing. Preserve the existing
config.update flow so missing keys and empty YAML resolve through the model’s
defaults without raising KeyError or type errors.
a4df594 to
8e83ebe
Compare
The OpenAI-compatible TTS API recreated SpeechSynthesizer and SpokenTextConverter on every request, reloading the ONNX session each time. Cache both at module scope and warm them during Litestar startup, matching the main Glados engine behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce ApiConfig (model-only subconfig) and configs/api_config.yaml with reuse_tts defaulting to true, plus GLADOS_API_REUSE_TTS env override. When enabled, cache SpeechSynthesizer and warm it at Litestar startup; when disabled, keep per-request ONNX loading. Update scripts/serve and Docker to use create_app. Co-authored-by: Cursor <cursoragent@cursor.com>
8e83ebe to
6ebf83d
Compare
1 participant
Summary
SpeechSynthesizerandSpokenTextConverterwhenreuse_ttsis enabled (default)ApiConfig+configs/api_config.yamlandGLADOS_API_REUSE_TTSenv overridereuse_tts: falserestores the previous per-request model loading behaviorThe main
Gladosengine already creates one synthesizer at startup and reuses it inTextToSpeechSynthesizer. The thin API wrapper did not, so each HTTP call reloaded the ONNX session before inference.Configuration
Test plan
pytest tests/test_api_config.py./scripts/serve/v1/audio/speechtwice withreuse_tts: true; second request should skip ONNX reloadreuse_tts: false; each request should reload the ONNX session