Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR adds restart functionality to the service management CLI and documentation, removes support for the "offline" transcription provider in favor of explicit "parakeet" configuration, makes audio stream workers conditional based on provider environment variables, and refactors environment and provider validation in test setup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
…ace with PARAKEET_ASR_URL in Docker Compose and documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backends/advanced/start-workers.sh (1)
110-110: SetMIN_WORKERS=7to account for all RQ workers.The script starts 7 RQ workers total (6 general workers + 1 audio persistence worker), but
MIN_WORKERSis set to 6. This means if only the audio persistence worker dies, the health check won't trigger a restart since the count remains at 6. Additionally, the stream workers (Deepgram/Parakeet) are Redis Streams consumers, not RQ workers, and are not monitored by this health check—if they die, they won't be automatically restarted.
🧹 Nitpick comments (1)
backends/advanced/src/advanced_omi_backend/workers/audio_stream_parakeet_worker.py (1)
84-86: Consider usinglogging.exception()for cleaner exception logging.The error handling correctly logs the full stack trace with
exc_info=True. Per project learnings, you could simplify this slightly by usinglogger.exception()which automatically includes the stack trace without needing theexc_infoparameter.🔎 Apply this diff for more idiomatic exception logging:
except Exception as e: - logger.error(f"Worker error: {e}", exc_info=True) + logger.exception(f"Worker error: {e}") sys.exit(1)Based on learnings: prefer
logging.exception()inside except blocks to automatically log the full stack trace.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backends/advanced/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Docs/init-system.md(1 hunks)Docs/ports-and-access.md(2 hunks)backends/advanced/run-test.sh(1 hunks)backends/advanced/src/advanced_omi_backend/controllers/websocket_controller.py(1 hunks)backends/advanced/src/advanced_omi_backend/services/transcription/__init__.py(0 hunks)backends/advanced/src/advanced_omi_backend/services/transcription/parakeet_stream_consumer.py(1 hunks)backends/advanced/src/advanced_omi_backend/workers/audio_stream_parakeet_worker.py(1 hunks)backends/advanced/start-workers.sh(5 hunks)backends/advanced/tests/test_integration.py(3 hunks)quickstart.md(2 hunks)services.py(3 hunks)
💤 Files with no reviewable changes (1)
- backends/advanced/src/advanced_omi_backend/services/transcription/init.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Black formatter with 100-character line length for Python code
Use isort for import sorting in Python code
ALL imports must be at the top of the file after the docstring. NEVER import modules in the middle of functions or files. Use lazy imports sparingly and only when absolutely necessary for circular import issues.
Group imports in order: standard library, third-party, local imports
Always raise errors, never silently ignore. Use explicit error handling with proper exceptions rather than silent failures.
Do not add defensivehasattr()checks. Research and understand input/response or class structure instead.
Useuv run pythonoruv run python3instead of directpythonorpython3commands for Python execution
Files:
backends/advanced/src/advanced_omi_backend/services/transcription/parakeet_stream_consumer.pybackends/advanced/src/advanced_omi_backend/workers/audio_stream_parakeet_worker.pybackends/advanced/src/advanced_omi_backend/controllers/websocket_controller.pybackends/advanced/tests/test_integration.pyservices.py
**/{websocket,ws,audio,streaming}*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Wyoming protocol (JSONL + binary) for WebSocket communication with structured audio sessions
Files:
backends/advanced/src/advanced_omi_backend/workers/audio_stream_parakeet_worker.pybackends/advanced/src/advanced_omi_backend/controllers/websocket_controller.py
**/{conversation,audio,memory,processing}*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only create conversations when speech is detected, eliminating noise-only sessions. Store all audio sessions in
audio_chunkscollection, conversations only inconversationscollection when speech is detected.
Files:
backends/advanced/src/advanced_omi_backend/workers/audio_stream_parakeet_worker.py
🧠 Learnings (3)
📚 Learning: 2025-12-08T23:52:34.959Z
Learnt from: AnkushMalaker
Repo: chronicler-ai/chronicle PR: 178
File: backends/advanced/src/advanced_omi_backend/services/memory/providers/mycelia.py:218-223
Timestamp: 2025-12-08T23:52:34.959Z
Learning: In Python code (chronicle project), prefer logging.exception() inside except blocks to automatically log the full stack trace. When re-raising exceptions, always chain with 'raise ... from e' to preserve the original context; use 'raise ... from None' only if you explicitly want to suppress the context. This improves debuggability across Python files.
Applied to files:
backends/advanced/src/advanced_omi_backend/services/transcription/parakeet_stream_consumer.pybackends/advanced/src/advanced_omi_backend/workers/audio_stream_parakeet_worker.pybackends/advanced/src/advanced_omi_backend/controllers/websocket_controller.pybackends/advanced/tests/test_integration.pyservices.py
📚 Learning: 2025-12-16T14:24:28.301Z
Learnt from: CR
Repo: chronicler-ai/chronicle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T14:24:28.301Z
Learning: Applies to **/*.py : Use `uv run python` or `uv run python3` instead of direct `python` or `python3` commands for Python execution
Applied to files:
Docs/ports-and-access.mdDocs/init-system.md
📚 Learning: 2025-12-16T14:24:28.301Z
Learnt from: CR
Repo: chronicler-ai/chronicle PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T14:24:28.301Z
Learning: Applies to **/{websocket,ws,audio,streaming}*.py : Use Wyoming protocol (JSONL + binary) for WebSocket communication with structured audio sessions
Applied to files:
backends/advanced/src/advanced_omi_backend/controllers/websocket_controller.py
🧬 Code graph analysis (1)
backends/advanced/run-test.sh (3)
backends/advanced/setup-https.sh (3)
print_info(15-17)print_error(27-29)print_warning(23-25)run-test.sh (3)
print_info(17-19)print_error(29-31)print_warning(25-27)extras/speaker-recognition/run-test.sh (3)
print_info(34-36)print_error(46-48)print_warning(42-44)
🪛 markdownlint-cli2 (0.18.1)
Docs/ports-and-access.md
121-121: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.8)
backends/advanced/src/advanced_omi_backend/services/transcription/parakeet_stream_consumer.py
40-40: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: robot-tests
🔇 Additional comments (18)
Docs/ports-and-access.md (2)
104-121: LGTM! Clear documentation for restart functionality.The restart command documentation is well-structured and provides helpful guidance:
- Clear examples for restarting all services vs. specific services
- Useful distinction between
restart(for config changes) andstop+start --build(for code changes)- Convenience scripts align with the implementation in services.py
145-145: Good update to port conflict resolution guidance.The updated recommendation to use
restart --allor./restart.shprovides a more streamlined workflow for resolving port conflicts after configuration changes.backends/advanced/src/advanced_omi_backend/services/transcription/parakeet_stream_consumer.py (1)
38-40: Good simplification of provider configuration.Removing the
OFFLINE_ASR_TCP_URIfallback and requiring explicitPARAKEET_ASR_URLimproves clarity and aligns with the PR's goal to streamline provider selection. The updated error message is more specific and helpful.Note: The Ruff warning (TRY003) about message length is a minor style preference and can be safely ignored here - the message is short and clear.
backends/advanced/src/advanced_omi_backend/controllers/websocket_controller.py (1)
307-320: Excellent improvement to provider auto-detection logic.The updated logic is clear, explicit, and follows a sensible priority order:
- Explicit provider configuration takes precedence
- Auto-detection prefers Parakeet (local) if
PARAKEET_ASR_URLis set- Falls back to Deepgram if
DEEPGRAM_API_KEYis set- Raises a clear, actionable error if no provider is configured
The removal of "offline" provider support simplifies the codebase and the error message guides users on what environment variables are required.
Docs/init-system.md (1)
151-177: Well-documented restart functionality with helpful user guidance.The documentation additions effectively cover:
- Restart commands for all and specific services
- Convenience scripts for quick operations
- Critical distinction between
restart(config changes) andstop+start --build(code changes)- Concrete example of the rebuild workflow
This guidance will help prevent common user mistakes like using restart when they need to rebuild after code changes.
quickstart.md (2)
197-202: Good addition of convenience script option.Providing both the quick
./start.shscript and the fulluv runcommand gives users flexibility while maintaining transparency. The "recommended" label on Option 1 guides beginners appropriately.
291-305: Excellent troubleshooting expansion.The enhanced troubleshooting section provides practical guidance for common issues:
- General service management with restart commands and status checks
- Cloud service considerations (costs, model recommendations)
- Local service troubleshooting (startup issues, performance, resource constraints)
This comprehensive guidance will significantly improve the user experience when encountering problems.
services.py (2)
240-262: Well-implemented restart functionality.The
restart_servicesfunction correctly:
- Validates service names against the SERVICES registry
- Checks service configuration before attempting restart
- Uses Docker Compose's
restartcommand (preserves images, restarts containers)- Tracks and reports success/failure for each service
The implementation follows the same pattern as
start_servicesandstop_services, maintaining consistency across the codebase.
304-309: Clean CLI integration for restart command.The restart subcommand implementation is well-designed:
- Follows the same argument structure as start/stop commands (consistency)
--allflag correctly filters to configured services only- Service name validation prevents invalid inputs
- Appropriate error messages for missing arguments
The handling in
main()mirrors the start/stop logic, maintaining code consistency and making the codebase easier to understand and maintain.Also applies to: 357-372
backends/advanced/tests/test_integration.py (2)
112-112: Good rename for clarity.Renaming
TEST_AUDIO_PATH_OFFLINEtoTEST_AUDIO_PATH_PARAKEETimproves code clarity and aligns with the PR's goal to use explicit provider naming instead of the ambiguous "offline" term.
428-428: Consistent naming updates throughout test file.The updates to phase comments and audio file selection logic correctly reflect the transition from "offline" to "parakeet" naming. Test behavior remains unchanged - appropriate test files are selected based on the configured provider.
Also applies to: 719-719, 1442-1442
backends/advanced/run-test.sh (2)
42-60: Improved environment loading with clear priority.The updated environment loading logic is much better:
- Clear priority order: CI environment → .env.test → .env
- Informative messages for each source
- Helpful error guidance for both local development and CI setup
The CI detection (checking for
DEEPGRAM_API_KEY) works well for typical CI scenarios where cloud services are used.
62-109: Excellent provider-based API key validation.The new validation logic is a significant improvement:
- Only validates keys actually required for the configured providers
- Supports multiple provider combinations (e.g., Parakeet + OpenAI, Deepgram + Ollama)
- Clear reporting of configured providers and key status
- Graceful handling of unknown providers with warnings instead of hard failures
This makes the test script much more flexible and user-friendly for different deployment scenarios.
Minor observation: Mistral is included as a transcription provider option (lines 79-85), but I don't see Mistral transcription support in the codebase changes. This might be future-proofing, which is fine.
backends/advanced/start-workers.sh (4)
54-73: LGTM! Conditional worker startup is well-implemented.The conditional logic correctly starts stream workers only when their respective environment variables are set. Setting PID variables to empty strings when workers are skipped enables safe conditional checks throughout the rest of the script.
83-84: LGTM! Conditional logging is correct.The conditional echo statements properly display worker PIDs only when the workers were actually started.
124-126: LGTM! Health monitoring kill logic handles conditional workers correctly.The conditional kill statements for stream workers are properly implemented and won't cause errors when those workers weren't started.
151-152: LGTM! Shutdown and cleanup logic is consistent.The conditional termination logic for stream workers is correctly implemented in both the shutdown handler and final cleanup section, maintaining consistency with the health monitoring approach.
Also applies to: 185-186
backends/advanced/src/advanced_omi_backend/workers/audio_stream_parakeet_worker.py (1)
31-35: LGTM! Configuration correctly uses explicit PARAKEET_ASR_URL.The removal of the
OFFLINE_ASR_TCP_URIfallback aligns with the PR's goal of removing implicit "offline" provider support in favor of explicit Parakeet configuration. The early return when the URL is not set is appropriate, especially since the worker startup script now conditionally launches this worker based on the environment variable.
|
Actually maybe instead of a separate restart.sh, it would be easier to just have a param in start.sh --restart which just restarts? |
…ad of offline option in ChronicleSetup.
|
is merge-dev the same as merging main now? |
|
as long as robot tests run with this config lgtm |
thestumonkey
left a comment
thestumonkey
left a comment
There was a problem hiding this comment.
lgtm as long as robot tests pass
…rces. Removed unused Google API dependencies from uv.lock.
… pip install command
|
| Metric | Count |
|---|---|
| ✅ Passed | 90 |
| ❌ Failed | 1 |
| 📊 Total | 91 |
📊 View Reports
GitHub Pages (Live Reports):
Download Artifacts:
- robot-test-reports-html - HTML reports
- robot-test-results-xml - XML output
3 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.