-
-
Notifications
You must be signed in to change notification settings - Fork 213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(tts): make tts server requests non-blocking, improve clean_for_speech #422
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #422 +/- ##
==========================================
- Coverage 70.49% 70.26% -0.23%
==========================================
Files 70 70
Lines 5876 5912 +36
==========================================
+ Hits 4142 4154 +12
- Misses 1734 1758 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 1b3f801 in 1 minute and 23 seconds
More details
- Looked at
271
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. gptme/tools/tts.py:241
- Draft comment:
Removing content within parentheses might strip important information. Consider refining this to only remove specific types of parenthetical content. - Reason this comment was not posted:
Confidence changes required:50%
Theclean_for_speech
function removes content within parentheses, which might remove important information. This could be problematic if the text contains necessary details within parentheses.
2. gptme/tools/tts.py:459
- Draft comment:
Consider handling cases whereclean
isFalse
to ensure text is still suitable for speech, or document the expected input format whenclean
isFalse
. - Reason this comment was not posted:
Confidence changes required:50%
Thespeak
function does not handle the case whereclean
is set toFalse
, and the text might contain unwanted content for speech.
Workflow ID: wflow_GVCNEIDkXLp7C9a7
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
clear_queue() | ||
log.info("Stopped TTS playback and cleared queue") | ||
with tts_request_queue.mutex: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearing tts_request_queue
using queue.mutex
is not thread-safe. Consider using a loop with get_nowait()
to safely clear the queue.
…eech to handle **emphasis**, emoji, and `(details)`
1b3f801
to
947b514
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on 947b514 in 5 minutes and 6 seconds
More details
- Looked at
301
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. gptme/tools/tts.py:241
- Draft comment:
Removing all content within parentheses might be too aggressive and could remove important information. Consider refining this regex to be more selective. - Reason this comment was not posted:
Confidence changes required:50%
Theclean_for_speech
function removes all content within parentheses, which might remove important information. This could be too aggressive for some use cases.
2. gptme/tools/tts.py:72
- Draft comment:
Directly accessingtts_request_queue.mutex
is not thread-safe and can lead to race conditions. Consider using a loop withget_nowait()
to safely clear the queue. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is incorrect. The Queue class in Python is designed to have its mutex accessed this way for atomic operations. The suggested alternative of using get_nowait() in a loop would actually be less thread-safe as it could miss items added between iterations. The mutex context manager ensures atomic access to clear the queue and notify waiters.
Could there be edge cases where accessing mutex directly could cause issues? Is there official documentation recommending against this pattern?
No, this is a well-established pattern. The Queue class's mutex is specifically designed to be used this way for atomic operations. The Python standard library itself uses similar patterns.
The comment should be deleted as it suggests a less thread-safe alternative to correct code.
3. gptme/tools/tts.py:370
- Draft comment:
Consider handling cases where the TTS server returns a non-200 status code without content to improve error logging clarity. - Reason this comment was not posted:
Confidence changes required:50%
Thetts_processor_thread_func
function does not handle the case where the TTS server returns a non-200 status code without content, which might lead to unclear error logging.
Workflow ID: wflow_4RkmmU8UZsOxCBbE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Make TTS server requests non-blocking and improve text cleaning for speech in
tts.py
.tts_request_queue
andtts_processor_thread
intts.py
.clean_for_speech()
to remove Italic markup, details in parentheses, and emojis.tts_processor_thread_func()
for processing TTS requests in the background.ensure_threads()
to manage both playback and TTS processor threads.speak()
to queue text chunks for TTS processing and handle blocking mode.stop()
to clear both audio and TTS request queues and stop the processor thread.This description was created by for 947b514. It will automatically update as commits are pushed.