fix: replace venv+pip with uv for biometrics module setup#384
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughMigrates per-module Python environment management from patched system stdlib + Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer Script
participant UV as uv (binary)
participant Module as Module Dir
participant Service as systemd Service
Installer->>UV: ensure uv binary (download if missing)
Installer->>Module: cd module && run `uv sync --frozen`
UV-->>Module: create `.venv/` and install locked deps
Installer->>Service: deploy service files (ExecStart → .venv/bin/python)
Service->>Module: execute `.venv/bin/python` at runtime
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
scripts/README.md (1)
64-77:⚠️ Potential issue | 🟡 MinorFlowchart references undefined
SkipBionode.Line 73 has
SkipBio --> SSHbut there's noSkipBionode defined in the flowchart. This appears to be a remnant from a previous version or a missing conditional path.Suggested fix: Remove the orphaned reference or add the missing node
ModService --> SSH{Interactive\nterminal?} - SkipBio --> SSH SSH -->|yes| SSHSetup[Optional SSH setup\nport 8822, keys only]Or if there should be a skip path:
+ UV -->|failed| SkipBio[Skip biometrics\nmodule install] ModService --> SSH{Interactive\nterminal?} SkipBio --> SSH🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/README.md` around lines 64 - 77, The flowchart contains an undefined node reference "SkipBio" (see the edge "SkipBio --> SSH"); either remove that orphaned edge or define the missing node and connect it into the flow (e.g., add a node like SkipBio[Skip biometrics modules] and ensure Modules or another branch points to SkipBio before it goes to SSH), updating the flow so "SkipBio" is declared before it is referenced and the graph is consistent with the SSH branch.
🧹 Nitpick comments (1)
scripts/install (1)
610-615: Optional: Use explicit path for clarityThe review's analysis is technically correct—since the script verifies
EUID -eq 0at line 142,$HOMEwill consistently resolve to/rootwhen the uv installation runs. The current code using$HOME/.local/binworks as intended.However, using an explicit path improves code clarity:
Suggested change
if ! command -v uv &>/dev/null; then echo "Installing uv..." curl -LsSf https://astral.sh/uv/install.sh | sh - export PATH="$HOME/.local/bin:$PATH" + export PATH="/root/.local/bin:$PATH" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install` around lines 610 - 615, The uv installer appends $HOME/.local/bin to PATH which works but is ambiguous in a root-only script; update the uv install block to use an explicit path (e.g., /root/.local/bin) instead of $HOME/.local/bin so the PATH modification is unambiguous; locate the uv installation conditional (the curl | sh installation and subsequent export PATH line) and replace the $HOME reference with the explicit /root path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/bin/sp-update`:
- Around line 231-233: The sp-update script currently skips running "uv sync"
when the uv binary is missing, which can leave modules without dependencies;
update the check around the uv usage (the block that tests `command -v uv` and
runs `(cd "$MODULES_DEST/$mod" && uv sync) || true`) to mirror scripts/install
behavior: if uv is absent, attempt to install or bootstrap uv using the same
install routine or command used in scripts/install (or at minimum emit a clear
warning before skipping), then proceed to run `uv sync` for each module under
MODULES_DEST/$mod; ensure the fix reuses the existing install/bootstrap logic to
avoid duplicating installation steps and preserves the fallback behavior on
failure.
---
Outside diff comments:
In `@scripts/README.md`:
- Around line 64-77: The flowchart contains an undefined node reference
"SkipBio" (see the edge "SkipBio --> SSH"); either remove that orphaned edge or
define the missing node and connect it into the flow (e.g., add a node like
SkipBio[Skip biometrics modules] and ensure Modules or another branch points to
SkipBio before it goes to SSH), updating the flow so "SkipBio" is declared
before it is referenced and the graph is consistent with the SSH branch.
---
Nitpick comments:
In `@scripts/install`:
- Around line 610-615: The uv installer appends $HOME/.local/bin to PATH which
works but is ambiguous in a root-only script; update the uv install block to use
an explicit path (e.g., /root/.local/bin) instead of $HOME/.local/bin so the
PATH modification is unambiguous; locate the uv installation conditional (the
curl | sh installation and subsequent export PATH line) and replace the $HOME
reference with the explicit /root path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 246d256f-9241-4d1a-aaa5-f3e6de69fe32
⛔ Files ignored due to path filters (4)
modules/calibrator/uv.lockis excluded by!**/*.lockmodules/environment-monitor/uv.lockis excluded by!**/*.lockmodules/piezo-processor/uv.lockis excluded by!**/*.lockmodules/sleep-detector/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
docs/adr/0017-uv-python-package-management.mdmodules/calibrator/pyproject.tomlmodules/calibrator/requirements.txtmodules/calibrator/sleepypod-calibrator.servicemodules/environment-monitor/pyproject.tomlmodules/environment-monitor/requirements.txtmodules/environment-monitor/sleepypod-environment-monitor.servicemodules/piezo-processor/prototype_v2.pymodules/piezo-processor/pyproject.tomlmodules/piezo-processor/requirements.txtmodules/piezo-processor/sleepypod-piezo-processor.servicemodules/sleep-detector/pyproject.tomlmodules/sleep-detector/requirements.txtmodules/sleep-detector/sleepypod-sleep-detector.servicescripts/README.mdscripts/bin/sp-updatescripts/installscripts/patch-python-stdlibscripts/setup-python-venv
💤 Files with no reviewable changes (6)
- modules/sleep-detector/requirements.txt
- modules/environment-monitor/requirements.txt
- modules/calibrator/requirements.txt
- modules/piezo-processor/requirements.txt
- scripts/setup-python-venv
- scripts/patch-python-stdlib
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install (1)
610-618: Curl pipe to shell may silently fail.If the
curlcommand fails (network issues, DNS failure, etc.),shreceives empty input and may exit successfully, leavinguvuninstalled without any error message. The subsequent check on line 617 will catch this, but the root cause won't be logged.Consider adding explicit error handling:
♻️ Proposed fix to improve error visibility
# Install uv (Rust-based Python package manager — bypasses broken ensurepip/pyexpat on Yocto) if ! command -v uv &>/dev/null; then echo "Installing uv..." - UV_INSTALL_DIR=/usr/local/bin INSTALLER_NO_MODIFY_PATH=1 \ - curl -LsSf https://astral.sh/uv/install.sh | sh + if ! UV_INSTALL_DIR=/usr/local/bin INSTALLER_NO_MODIFY_PATH=1 \ + curl -LsSf https://astral.sh/uv/install.sh | sh; then + echo "Warning: uv installer script failed" + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install` around lines 610 - 618, The curl | sh pipeline can fail silently; modify the uv install block to detect and log failures by enabling pipefail or by running curl into a temporary file and checking its exit status before invoking sh. Specifically, around the UV_INSTALL_DIR/INSTALLER_NO_MODIFY_PATH invocation of curl and sh, ensure you check curl's exit code (or use set -o pipefail) and, on failure, emit a clear error message including curl's status/output and skip attempting sh/uv installation; only run sh if curl succeeded so the subsequent command -v uv check reflects the real failure cause.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install`:
- Around line 610-618: The curl | sh pipeline can fail silently; modify the uv
install block to detect and log failures by enabling pipefail or by running curl
into a temporary file and checking its exit status before invoking sh.
Specifically, around the UV_INSTALL_DIR/INSTALLER_NO_MODIFY_PATH invocation of
curl and sh, ensure you check curl's exit code (or use set -o pipefail) and, on
failure, emit a clear error message including curl's status/output and skip
attempting sh/uv installation; only run sh if curl succeeded so the subsequent
command -v uv check reflects the real failure cause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e509f82-fdb7-4aa4-90d3-095cb30f6422
📒 Files selected for processing (3)
.gitignorescripts/bin/sp-updatescripts/install
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/bin/sp-update
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install (1)
610-614: Consider pinning the uv version for reproducible installs.The current approach installs the latest uv version each time. While this ensures users get the latest features and fixes, it could cause unexpected breakage if a future uv release introduces breaking changes to
uv syncbehavior.You can pin a specific version by changing the URL:
curl -LsSf https://astral.sh/uv/0.7.8/install.sh | ...This is optional — the trade-off is maintenance burden vs. reproducibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install` around lines 610 - 614, Change the curl install URL for the uv installation so it pins a specific release instead of pulling latest; locate the block that checks command -v uv and the curl invocation that pipes to sh (references: uv check, curl -LsSf https://astral.sh/uv/install.sh, UV_INSTALL_DIR and INSTALLER_NO_MODIFY_PATH env vars) and modify the URL to include a chosen version (e.g. https://astral.sh/uv/<VERSION>/install.sh) or use a UV_VERSION variable to make the version explicit for reproducible installs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install`:
- Around line 610-614: Change the curl install URL for the uv installation so it
pins a specific release instead of pulling latest; locate the block that checks
command -v uv and the curl invocation that pipes to sh (references: uv check,
curl -LsSf https://astral.sh/uv/install.sh, UV_INSTALL_DIR and
INSTALLER_NO_MODIFY_PATH env vars) and modify the URL to include a chosen
version (e.g. https://astral.sh/uv/<VERSION>/install.sh) or use a UV_VERSION
variable to make the version explicit for reproducible installs.
ng
changed the title
Pod 3 Yocto image is missing pyexpat (C extension), which breaks both ensurepip and get-pip.py — making venv+pip unusable. uv handles venv creation and package install entirely in Rust, sidestepping the broken stdlib completely. - Add pyproject.toml + uv.lock per biometrics module - Replace venv/pip install with `uv sync` in install and sp-update - Delete patch-python-stdlib and setup-python-venv scripts - Update service files: venv/ → .venv/ (uv default) - Add CI=true to pnpm install for curl|bash TTY fix - Add ADR 0017 documenting the decision Co-Authored-By: onemec <onemec@users.noreply.github.com>
- Install uv to /usr/local/bin so sp-update can find it - Add warning in sp-update if uv is missing - Use uv sync --frozen to fail loudly on stale lockfiles - Clean up orphaned venv/ dirs from pre-uv installs - Add .venv to .gitignore Co-Authored-By: onemec <onemec@users.noreply.github.com>
Env vars before curl in `VAR=x curl ... | sh` are visible to curl but not to the sh that executes the downloaded installer. Move to `curl ... | VAR=x sh` so the installer sees UV_INSTALL_DIR. Verified on Pod 5: uv installs to /usr/local/bin correctly.
ng
force-pushed
the
fix/380-uv-python-env
branch
from
e130278 to
d729acf
Compare
2 tasks
ng
added a commit
that referenced
this pull request
Apr 13, 2026
## Summary Promotes everything on \`dev\` since the last main release (82 commits, 143 files, +9239/-2699). ### Headlining features - **Schedule redesign (#303)** — read-only schedule view with explicit curve management, full-screen \`CurveEditor\` (day picker + bedtime/wake + temp range + presets), \`Left | Right | Both\` side selector, active-curve highlight, atomic \`batchUpdate\` writes, day-conflict resolution, sparkline cards. - **Mini feature flag (#420)** — \`ENABLE_MINI\` env var; PubNub moved to \`optionalDependencies\`; conditional Mini router import. - **Auto-off on no presence (#301)** — schedule respects bed presence. - **Auto-unblock internet during update check (#308)**. - **Schedule batchUpdate cap raised to 1000 (#424)** — fixes AI-curve apply-to-all-days rejection. ### Operational fixes - Pod 3 install path (#383, #384, #386, #392) - Yocto image Python venv (#336) - DAC socket / Avahi on device startup (#331) - Free-sleep/sleepypod switch persistence (#337) - Cross-machine standalone deploys (#308) - Temperature unit conversion (#333) ### Dependency updates ~20 renovate PRs across React 19.2.5, Next 16.2.3, vitest 4.1.4, tanstack/react-query 5.97.0, tRPC 11.16, lucide-react 1.x, etc. ### Misc - ADR 0017 (uv) compiled into deployment wiki - Snoo pentest methodology + recon plan - Git hooks + ESLint cleanup (#313) - CI hardening (#388) ## Test plan - [x] All unit tests pass on dev (606+ tests) - [x] Typecheck clean - [x] Build succeeds (standalone output) - [x] Deployed to Pod 4 at \`192.168.1.88\` and smoke-tested: - schedule on/off - create curve from preset - edit curve, change days, save - day-conflict reassign dialog - delete curve - side selector left/right/both - active-curve highlighting + next set point
|
🎉 This PR is included in version 1.6.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
venv+pipwith uv for biometrics module environment setup — fixes Pod 3 install failure where missingpyexpatC extension broke bothensurepipandget-pip.pypyproject.toml+uv.lockper module, deleterequirements.txtscripts/patch-python-stdlibandscripts/setup-python-venv(no longer needed)venv/to.venv/(uv default)CI=trueonpnpm installforcurl | bashTTY fixBased on the approach from #381 by @onemec.
Closes #380
Test plan
From your dev machine:
Or, if you already have a working install on the pod:
cd /home/dac/sleepypod-core git fetch origin fix/380-uv-python-env git checkout fix/380-uv-python-env bash scripts/install --local --no-sshPod 5 results (verified via fresh install)
/usr/local/bin/uv0.11.3 (musl-static).venvdirsvenv/dirsTested as a completely clean install (removed
/home/dac/sleepypod-coreand/opt/sleepypod/modulesfirst). All services started on first attempt.Still needs testing
sp-updatepath (uv already present, modules synced on update)