-
Notifications
You must be signed in to change notification settings - Fork 12
Fix codegeneration flow #183
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
Conversation
WalkthroughThis PR refactors dependency management and build configuration. It removes the git-based twirp package from the release workflow, consolidates dependency installation into a single uv sync command in the build script, adds PATH validation for Go binaries, updates aiohttp version constraints, and loosens the twirp version requirement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
35-58: Remove duplicate and conflicting dependencies in the webrtc block.The
webrtcoptional-dependencies list contains significant duplicates with conflicting constraints:
- Conflicting protobuf versions: Line 43 specifies
"protobuf>=4.25.1"while line 55 specifies"protobuf>=6.31.1"— pip will use the last one, silently ignoring the earlier constraint.- Duplicate numpy: Lines 37 (
"numpy>=2.2.6,<2.3") and 48 ("numpy>=2.2.6") with different version ranges.- Duplicate torch & torchaudio: Lines 38–39 and lines 50–51.
- Duplicate scipy: Lines 41 (
"scipy>=1.15.3,<1.16") and 52 ("scipy>=1.15.3").- Duplicate websockets & websocket-client: Lines 44–45 and 56–57.
This suggests incomplete refactoring or a merge conflict resolution error. Consolidate into a single, clean list with non-conflicting constraints.
Apply this diff to deduplicate and align constraints:
webrtc = [ "aiortc-getstream==1.13.0.post1", "numpy>=2.2.6,<2.3", "torch>=2.7.1", "torchaudio>=2.7.1", "soundfile>=0.13.1", "scipy>=1.15.3,<1.16", "twirp", "protobuf>=6.31.1", "websockets>=15.0.1,<16", "websocket-client>=1.8.0", "python-dateutil>=2.8.2", "structlog<24", "tenacity>=9.1.2", "aiohttp>=3.13.2,<4", "ping3>=4.0.8", ]Note: Resolved to use
protobuf>=6.31.1(the newer constraint) and tightened numpy/scipy/websockets to versions with upper bounds per the first occurrence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks)generate_webrtc.sh(2 hunks)pyproject.toml(2 hunks)
⏰ 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). (4)
- GitHub Check: Tests (3.11)
- GitHub Check: Tests (3.13)
- GitHub Check: Tests (3.10)
- GitHub Check: Tests (3.12)
🔇 Additional comments (4)
generate_webrtc.sh (3)
15-19:-fflag on submodule add ensures idempotency.Adding the
-fflag allows the script to safely re-run if the submodule already exists. LGTM.
36-40: Clearer protoc installation instructions.Updated guidance from referencing a script to standard
brew install protocis more user-friendly and portable. LGTM.
42-44: Consolidation of dependency installation is sound.The single
uv synccall with--all-extras --dev --all-packagesensures all dependencies are available for code generation. LGTM..github/workflows/release.yml (1)
36-39: Remove the verification request—no resolution risk exists.The
uv removecommand on line 38 targets the dev group, buttwirpis only declared in thewebrtcoptional-dependency (no version constraint). Theuv.lockfile confirms thattwirp 0.0.7resolves successfully from PyPI, not from a git source. This is defensive code to avoid releasing git-based dependencies; it poses no resolution failure risk since the PyPI package already resolves correctly.Likely an incorrect or invalid review comment.
Summary by CodeRabbit