Fix support for Python WebSocket >=14 while preserving backward compatibility #499
Conversation
WalkthroughThe pull request addresses compatibility issues with the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (
|
|
@jjmaldonis did you have any concerns on this approach? |
fviard
force-pushed
the
fix-websockets-14-retrocompat
branch
from
18032cd to
6ecbe4a
Compare
|
Re-pushed to fix the linter/typing issues. :-) |
fviard
force-pushed
the
fix-websockets-14-retrocompat
branch
from
6ecbe4a to
43c0619
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: 0
🔭 Outside diff range comments (1)
deepgram/clients/common/v1/abstract_sync_websocket.py (1)
Line range hint
1-1: Consider adding version compatibility handling to sync implementation.The async implementation includes version-specific handling for the WebSocket library's breaking changes, but similar changes are missing in the sync implementation. Consider adding the same version compatibility approach to maintain consistency.
Apply these changes to the sync implementation:
import websockets + +try: + # Websockets versions >= 13 + from websockets.sync.client import connect, ClientConnection + WS_ADDITIONAL_HEADERS_KEY = "additional_headers" +except ImportError: + # Backward compatibility with websockets versions 12 + from websockets.sync.client import ( + connect, + WebSocketClientProtocol as ClientConnection, + ) + WS_ADDITIONAL_HEADERS_KEY = "extra_headers"Then update the connection setup:
- self._socket = connect(url_with_params, additional_headers=combined_headers) + ws_connect_kwargs: Dict = { + WS_ADDITIONAL_HEADERS_KEY: combined_headers + } + self._socket = connect(url_with_params, **ws_connect_kwargs)
🧹 Nitpick comments (1)
deepgram/clients/common/v1/abstract_async_websocket.py (1)
151-158: Clean implementation of version-agnostic connection setup!The use of a dictionary with dynamic key names is an elegant solution. Consider adding a type annotation for
ping_intervalto improve type safety.ws_connect_kwargs: Dict = { - "ping_interval": PING_INTERVAL, + "ping_interval": PING_INTERVAL, # type: float WS_ADDITIONAL_HEADERS_KEY: combined_headers, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deepgram/clients/common/v1/abstract_async_websocket.py(5 hunks)deepgram/clients/common/v1/abstract_sync_websocket.py(1 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)setup.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- requirements.txt
- setup.py
- pyproject.toml
🔇 Additional comments (4)
deepgram/clients/common/v1/abstract_async_websocket.py (3)
13-26: Well-structured version compatibility handling!The implementation elegantly handles the breaking change in the WebSocket library by using a try-except block to support both the new
additional_headers(>=13) and legacyextra_headers(12) parameter names.
61-61: Type annotation correctly updated!The socket type has been properly updated to use
ClientConnection, which is aliased correctly for both new and old versions of the library.
190-190: Exception handling properly updated!The exception handling has been correctly updated to use the
websockets.exceptionsnamespace, maintaining consistency with the library's error handling patterns.deepgram/clients/common/v1/abstract_sync_websocket.py (1)
187-187: Exception handling consistently updated across sync and async implementations!The exception handling in the sync client has been properly aligned with the async client, using the correct
websockets.exceptionsnamespace.
…tibility Python websockets did a breaking change with version 14. "extra_headers" was renamed to "additionnal_headers".
fviard
force-pushed
the
fix-websockets-14-retrocompat
branch
from
43c0619 to
14ad591
Compare
|
Sorry for the multiple pushs, I have reworked the fix to be more elegant. It is now all good, including linter checks, and I have verified that it works correctly with all the websockets released versions (12, 13, 14) for sync and async clients. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deepgram/clients/common/v1/abstract_async_websocket.py (2)
151-158: LGTM! Clean connection parameter handling with room for improvement.The connection logic properly handles version-specific header keys while maintaining existing functionality.
Consider moving the magic number
20to use the existingPING_INTERVALconstant:ws_connect_kwargs: Dict = { - "ping_interval": PING_INTERVAL, + "ping_interval": PING_INTERVAL, # Using constant defined at line 43 WS_ADDITIONAL_HEADERS_KEY: combined_headers, }
190-190: LGTM! Exception handling properly updated.The exception handling correctly uses the new import path for
ConnectionClosed.Consider adding specific handling for
websockets.exceptions.InvalidStatusCodeto provide more detailed error messages for HTTP-related connection failures:except websockets.exceptions.ConnectionClosed as e: self._logger.error( "ConnectionClosed in AbstractAsyncWebSocketClient.start: %s", e ) self._logger.debug("AbstractAsyncWebSocketClient.start LEAVE") if self._config.options.get("termination_exception_connect", False): raise return False + except websockets.exceptions.InvalidStatusCode as e: + self._logger.error( + "HTTP error in AbstractAsyncWebSocketClient.start (status code %d): %s", + e.status_code, + e + ) + self._logger.debug("AbstractAsyncWebSocketClient.start LEAVE") + if self._config.options.get("termination_exception_connect", False): + raise + return False except websockets.exceptions.WebSocketException as e:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deepgram/clients/common/v1/abstract_async_websocket.py(5 hunks)deepgram/clients/common/v1/abstract_sync_websocket.py(1 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)setup.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- requirements.txt
- setup.py
- pyproject.toml
- deepgram/clients/common/v1/abstract_sync_websocket.py
🔇 Additional comments (2)
deepgram/clients/common/v1/abstract_async_websocket.py (2)
61-61: LGTM! Type annotation correctly updated.The type annotation for
_sockethas been properly updated to use the version-agnosticClientConnectiontype.
13-26: LGTM! Clean version-specific import handling.The implementation elegantly handles backward compatibility with websockets v12 while supporting v13+ through version-specific imports and dynamic header key selection.
Let's verify the websockets version compatibility:
✅ Verification successful
✓ Version compatibility verified
The implementation correctly handles both websockets v12 (using
legacy.client.WebSocketClientProtocol) and v13+ (usingasyncio.client.ClientConnection). Import paths and type hints are accurate for both versions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify websockets package version compatibility # Test: Check if the code works with both v12 and v13+ of websockets # Check installed version pip show websockets | grep "Version" # Check imports in different versions python3 -c " import websockets import sys v = websockets.__version__ print(f'Testing with websockets {v}') if int(v.split('.')[0]) >= 13: # Should succeed from websockets.asyncio.client import ClientConnection print('✓ Successfully imported ClientConnection from asyncio.client') else: # Should succeed from websockets.legacy.client import WebSocketClientProtocol print('✓ Successfully imported WebSocketClientProtocol from legacy.client') "Length of output: 696
Script:
#!/bin/bash # Test both websockets v12 and v13 echo "Testing websockets v12..." pip install 'websockets>=12.0,<13.0' python3 -c " from websockets.legacy.client import WebSocketClientProtocol print('✓ v12: Successfully imported WebSocketClientProtocol from legacy.client') " echo -e "\nTesting websockets v13..." pip install 'websockets>=13.0' python3 -c " from websockets.asyncio.client import ClientConnection print('✓ v13: Successfully imported ClientConnection from asyncio.client') "Length of output: 1638
|
Hi @jjmaldonis , |
|
I approved, and @jpvajda and team can merge + deploy depending on their timing. |
|
I'm testing this change now with hopes to get it in our upcoming |
|
Test Results are promising: @jjmaldonis / @naomi-lgbt let me know if this requires additional testing.
==================================================== short test summary info =====================================================
FAILED tests/daily_test/test_daily_async_listen_rest_url.py::test_daily_async_listen_rest_url[url0-options0-expected_output0] - AssertionError: Test ID: b00dc103a62ea2ccfc752ec0f646c7528ef5e729a9d7481d2a944253a9128ce2-29e7c8100617f70da4ae9da1921cb5071a0...
FAILED tests/daily_test/test_daily_async_listen_rest_url.py::test_daily_async_listen_rest_url[url1-options1-expected_output1] - AssertionError: Test ID: f3b6208a662156067a41bddd295a1a0a53ea34a268e27a8f1a9d7107aa99732f-29e7c8100617f70da4ae9da1921cb5071a0...
FAILED tests/daily_test/test_daily_async_listen_websocket.py::test_daily_async_listen_websocket[preamble-websocket.wav-options1-expected_output1] - AssertionError: Test ID: a6d1b12d5ce73a51a7b69ab156f0c98c72cdc1cfcf4a25f7b634c328cce4d760-42fc5ed98cabc1fa1a2f276301c27c46dd1...
FAILED tests/daily_test/test_daily_listen_rest_url.py::test_daily_listen_rest_url[url0-options0-expected_output0] - AssertionError: Test ID: b00dc103a62ea2ccfc752ec0f646c7528ef5e729a9d7481d2a944253a9128ce2-29e7c8100617f70da4ae9da1921cb5071a0...
FAILED tests/daily_test/test_daily_listen_rest_url.py::test_daily_listen_rest_url[url1-options1-expected_output1] - AssertionError: Test ID: f3b6208a662156067a41bddd295a1a0a53ea34a268e27a8f1a9d7107aa99732f-29e7c8100617f70da4ae9da1921cb5071a0...
FAILED tests/daily_test/test_daily_listen_websocket.py::test_daily_listen_websocket[preamble-websocket.wav-options1-expected_output1] - AssertionError: Test ID: a6d1b12d5ce73a51a7b69ab156f0c98c72cdc1cfcf4a25f7b634c328cce4d760-42fc5ed98cabc1fa1a2f276301c27c46dd1...
=============================== 6 failed, 10 passed, 19 deselected, 4 warnings in 73.09s (0:01:13) =============================== |
Proposed changes
Websockets did a breaking change with version 14:
The client async base implementation changed and the "extra_headers" parameter was renamed to "additional_headers".
(See issue #483)
A previous change solved the issue by pinning the websockets version to 12 or 13 (#487).
An alternative solution was proposed with the PR #486 but it was not satisfying as it was breaking the backward compatibility.
The purpose of the current PR is to support the new versions of python websockets by preserving support for the older 12 and 13 versions.
Fixes #483
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
Dependencies
websocketslibrary version constraint to allow versions 12.0 and higher across multiple configuration files.Compatibility
websocketslibrary.