Add threading.Thread Class Override #490
Conversation
WalkthroughThe pull request introduces a new parameter Changes
Sequence DiagramsequenceDiagram
participant Client as WebSocket Client
participant ThreadFactory as Thread Factory
Client->>ThreadFactory: Specify custom thread class (optional)
alt Default Thread
ThreadFactory-->>Client: Use threading.Thread
else Custom Thread
ThreadFactory-->>Client: Use provided custom thread class
end
Client->>ThreadFactory: Create threads for operations
ThreadFactory-->>Client: Return thread instances
The sequence diagram illustrates how the new 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
deepgram/clients/common/v1/abstract_sync_websocket.py (1)
159-159: Document the thread_cls parameter usageThe thread creation correctly uses the custom thread class. Consider adding documentation about this parameter in the class docstring.
Add to the class docstring:
""" Abstract class for using WebSockets. This class provides methods to establish a WebSocket connection generically for use in all WebSocket clients. + + Args: + config (DeepgramClientOptions): all the options for the client + endpoint (str): the endpoint to connect to + thread_cls (Type[threading.Thread]): optional thread class to use for creating threads, + defaults to threading.Thread. Useful for custom thread management like ContextVar support. """deepgram/clients/listen/v1/websocket/client.py (2)
65-65: Document the thread_cls parameter in ListenWebSocketClientThe thread class parameter implementation is correct. Consider updating the class docstring to document this parameter.
Add to the class docstring:
""" Client for interacting with Deepgram's live transcription services over WebSockets. This class provides methods to establish a WebSocket connection for live transcription and handle real-time transcription events. Args: config (DeepgramClientOptions): all the options for the client. + thread_cls (Type[threading.Thread]): optional thread class to use for creating threads, + defaults to threading.Thread. Useful for custom thread management like ContextVar support. """Also applies to: 83-83
169-169: LGTM: Flush thread uses custom thread classThe flush thread correctly uses the configured thread class. Consider adding tests to verify custom thread class behavior.
Would you like me to help create test cases for verifying custom thread class functionality? This could include:
- Basic thread class override verification
- ContextVar propagation tests
- Thread lifecycle tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepgram/clients/common/v1/abstract_sync_websocket.py(4 hunks)deepgram/clients/listen/v1/websocket/client.py(4 hunks)
🔇 Additional comments (4)
deepgram/clients/common/v1/abstract_sync_websocket.py (2)
7-7: LGTM: Type imports and thread class attribute
The changes correctly add type hinting support and class attribute for thread class customization.
Also applies to: 55-55
62-62: LGTM: Constructor changes for thread class customization
The thread class parameter is correctly implemented with proper typing and default value, maintaining backward compatibility.
Also applies to: 78-78
✅ Verification successful
Thread class customization is properly implemented across the inheritance chain
The verification confirms that:
AbstractSyncWebSocketClientis the base class with properthread_clsparameter implementation- Both concrete implementations (
ListenWebSocketClientandSpeakWSClient) inherit from it - The
thread_clsparameter is consistently used across the codebase for thread creation in:- Base class:
_listen_threadcreation ListenWebSocketClient:_keep_alive_threadand_flush_threadcreation
- Base class:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the inheritance chain to ensure no conflicts
ast-grep --pattern 'class $_ (AbstractSyncWebSocketClient) {
$$$
}'
Length of output: 71
Script:
#!/bin/bash
# Let's try a different approach to verify the inheritance chain
# First, let's find all Python files that might contain the class
fd '\.py$' --exec grep -l "AbstractSyncWebSocketClient" {}
# Then, let's examine the content of these files to understand the inheritance
fd '\.py$' --exec grep -A 5 "class.*AbstractSyncWebSocketClient" {}
Length of output: 677
Script:
#!/bin/bash
# Let's examine the concrete implementations to verify thread_cls usage
rg -A 5 "class.*\(" ./deepgram/clients/listen/v1/websocket/client.py
rg -A 5 "class.*\(" ./deepgram/clients/speak/v1/websocket/client.py
# Also check for any existing thread_cls usage
rg "thread_cls" ./deepgram/clients/
Length of output: 1692
deepgram/clients/listen/v1/websocket/client.py (2)
7-7: LGTM: Consistent thread class attribute implementation
The changes correctly mirror the parent class implementation.
Also applies to: 58-58
161-161: LGTM: Keep-alive thread uses custom thread class
The keep-alive thread correctly uses the configured thread class.
|
Hey @davidvonthenen, @DamienDeepgram - can you let me know what you think about this change? This would help us at Hippocratic as we have custom thread classes (mostly for logging) and would like to pass them in when we open socket connections using the SDK. |
|
Thanks for the PR. Can you share a little more context about the motivation for this PR?
Also it would help to add a working example for this that can be run which shows how it would work and add it to be part of the CI to make sure it does not break. |
|
|
Also @DamienDeepgram, I'm having trouble adding an example as I believe the existing examples in |
|
@gerrymeixiong Yes, the examples rely on the installed Deepgram module rather than the source code. This is evident from the import statement. This import statement pulls from the installed deepgram package rather than the local source code. from deepgram import (
DeepgramClient,
ClientOptionsFromEnv,
PrerecordedOptions,
) |
|
Do you have any suggestions on how to add an example? |
|
I'm debating if we actually need one, Most of our examples show how the API works and can be used for testing and validating features. It really depends on if this is consider a major feature of the SDK, or just a minor one. Though if you were too create one, you can probably borrow code from an existing one, and add in this threading override. Also if you can review the Code Rabbit Suggestions here, It would be good to add this addition information |
|
@gerrymeixiong Let's skip the example for now, and if you can address those comments by Code Rabbit this will move the PR along. |
Thanks @jpvajda. I've addressed Code Rabbit's comments. |
Proposed changes
This adds the ability to override the
threading.Threadclass used withinListenWebSocketClient.By default, the standard Python
threading.Threadclass will be used. There is an option to specify an override which sub-classesthreading.Thread.Custom thread classes may be used for session management and logging, such as when using
ContextVar.Types of changes
What types of changes does your code introduce to the community Python SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Summary by CodeRabbit