Fix for Deepgram async websocket #486
Conversation
WalkthroughThe changes in this pull request involve modifications to the WebSocket handling in the Changes
Assessment against linked issues
Possibly related PRs
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 (
|
|
Hi @xzys thanks for the PR! We'll need to update the version of the websocket dependency in the relevant files (requirements.txt, setup.py, and pyproject.toml) in addition to making the change you've already made. If you want to make that change, the rest of our team will review this. Otherwise we'll work off your changes. |
xzys
force-pushed
the
fix-async-websocket-headers
branch
from
b376970 to
6921dd2
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 and nitpick comments (3)
setup.py (1)
31-31: Consider adding an upper bound version constraint.While the current change fixes the immediate issue, having no upper bound on the websockets version could lead to compatibility issues if future versions introduce breaking changes.
Consider using a more specific version range:
- "websockets>=14.0", + "websockets>=14.0,<15.0",pyproject.toml (1)
Remove Poetry configuration and maintain Setuptools as the sole build system
The codebase is actively using Setuptools as its build system, evidenced by:
- An actively maintained setup.py with complete package configuration
- README.md documentation using pip-based installation instructions
- requirements.txt following pip/setuptools conventions
- No active usage of Poetry in the codebase
The commented-out Poetry configuration in pyproject.toml is obsolete and should be removed to prevent confusion. The active Setuptools configuration in pyproject.toml aligns with the rest of the build infrastructure.
🔗 Analysis chain
Line range hint
1-54: Clarify build system configuration strategyThe file contains both Setuptools and Poetry configurations, with Poetry sections partially commented out. This dual configuration could lead to confusion about which build system should be used.
Let's check if this is part of a broader migration:
Consider:
- If migrating to Setuptools, remove commented Poetry configuration
- If keeping Poetry, restore its configuration and remove Setuptools
- Document the intended build system in README.md
Let me gather more information about the build system configuration by checking the content of setup.py and requirements.txt, and also look for any migration-related information in README.md.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other build configuration files echo "Checking for related build files..." fd -t f "setup\.py|poetry\.lock|requirements\.txt" # Check if other files reference poetry echo -e "\nChecking for poetry references..." rg -i "poetry" --type yaml --type tomlLength of output: 585
Script:
#!/bin/bash # Check setup.py content echo "=== setup.py content ===" cat setup.py echo -e "\n=== requirements.txt content ===" cat requirements.txt echo -e "\n=== Checking README.md for build system info ===" rg -i "(\bpoetry\b|\bsetuptools\b|\bbuild\b|\binstall\b)" README.mdLength of output: 2810
deepgram/clients/common/v1/abstract_async_websocket.py (1)
138-141: Consider adding a version compatibility note in docstringSince this change introduces a dependency on websockets ≥14.0, it would be helpful to document this requirement in the class or method docstring.
Add a note like:
""" Starts the WebSocket connection for live transcription. + + Note: Requires websockets library version 14.0 or higher. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
deepgram/clients/common/v1/abstract_async_websocket.py(1 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)setup.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🔇 Additional comments (4)
setup.py (1)
31-31: LGTM! Version update addresses the websockets compatibility issue.
The update to websockets>=14.0 correctly addresses issue #483 by allowing the use of the additional_headers parameter instead of the deprecated extra_headers.
Let's verify the dependency consistency and check for security advisories:
✅ Verification successful
The previous search didn't find the files, so let's try with fd. We need to verify the dependency consistency across all requirement files before making a final decision. The security check showed no critical/high vulnerabilities affecting version 14.0 or later, which is good, but we still need to confirm the dependency specifications are consistent across the project.
LGTM on the websockets upgrade, with consistent dependency specifications
The dependency update to websockets>=14.0 is safe and properly synchronized across the project:
requirements.txtspecifieswebsockets==14.*pyproject.toml(poetry) specifieswebsockets = "^14.1"- No critical/high security vulnerabilities affect version 14.0 or later
- The version constraints are consistent and maintain compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dependency consistency across files and check for security advisories
# Check dependency specifications in other files
echo "Checking dependency files for consistency..."
rg -g "requirements*.txt|pyproject.toml" "websockets"
# Check for security advisories
echo "Checking for security advisories..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "websockets", severities: [CRITICAL, HIGH]) {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1208
Script:
#!/bin/bash
# Let's check for dependency files in a different way and also look for any test requirements
fd -e txt -e toml "requirements|pyproject" --exec cat {} \; 2>/dev/null
Length of output: 2243
pyproject.toml (2)
34-34: LGTM: Websockets version update aligns with PR objectives
The update to websockets = "^14.1" correctly addresses issue #483 by supporting version 14.0+ where the additional_headers parameter is required.
Line range hint 18-18: Update version placeholder in Poetry configuration
The version is currently set to a placeholder 3.X.Y with a comment requesting update. This should be replaced with the actual version number.
Let's check the current version:
✅ Verification successful
Version placeholder should be updated to 0.0.0
The version in pyproject.toml is currently set to a placeholder 3.X.Y, but based on the codebase analysis, it should be updated to 0.0.0 to match the version defined in deepgram/__init__.py. This ensures consistency across the package.
- Update the version in
pyproject.tomlfromversion = "3.X.Y"toversion = "0.0.0"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for version references
echo "Checking version in deepgram module..."
rg -A 1 "__version__"
echo -e "\nChecking version in other config files..."
rg -A 1 "version\s*=" --type=toml --type=python
Length of output: 3455
Script:
#!/bin/bash
# Check pyproject.toml content
echo "Checking pyproject.toml content..."
cat pyproject.toml
Length of output: 1590
deepgram/clients/common/v1/abstract_async_websocket.py (1)
139-139: LGTM! Parameter name updated correctly for websockets v14.0
The change from extra_headers to additional_headers aligns with the websockets library v14.0 API. This fixes the TypeError mentioned in issue #483.
Let's verify the websockets version constraints in the project configuration:
✅ Verification successful
Version constraints are properly updated for websockets v14.0
The version constraints are correctly specified across all configuration files:
- pyproject.toml:
websockets = "^14.1" - requirements.txt:
websockets==14.* - setup.py:
websockets>=14.0
These constraints ensure compatibility with the new additional_headers parameter, making the code change safe and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check websockets version constraints across project files
echo "Checking pyproject.toml..."
cat pyproject.toml 2>/dev/null | grep -A 5 "websockets"
echo -e "\nChecking requirements.txt..."
cat requirements.txt 2>/dev/null | grep "websockets"
echo -e "\nChecking setup.py..."
cat setup.py 2>/dev/null | grep -A 5 "websockets"
Length of output: 644
|
@jjmaldonis Added the changes to upgrade websockets to 14.1. Didn't add any additional tests, but existing tests are passing (except for some minor transcription errors, aka not picking up commas) |
|
@xzys We have a concern that this change will be breaking and we'll need to consider how to offer backwards compatibility for users still using older versions of the the Python websockets library. Ideally both cc @jjmaldonis |
Proposed changes
The Python websockets library seems to accept
additional_headersnotextra_headers:https://github.com/python-websockets/websockets/blob/main/src/websockets/asyncio/client.py#L79
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
New Features
Bug Fixes
websocketslibrary version to ensure compatibility and access to new features.Chores
pyproject.toml,requirements.txt,setup.py) to reflect the new dependency management approach.