ENH: worktree creation now avoids nested venv#61
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdded a virtual environment activation guard to utils/setup_feature_worktree.py that aborts when a venv is active; regenerated API documentation entries for the setup script and removed LongitudinalRegistration experiment entries from the generated API map. Added API_MAP.md to docs/.gitignore. ChangesVirtual environment guard and API documentation update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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
🧹 Nitpick comments (1)
utils/setup_feature_worktree.py (1)
118-124: ⚡ Quick winAvoid
print()in new code; use logging calls.The new guard path adds direct
print()usage. Please switch these new lines to logger-based output to match repo standards.As per coding guidelines "Use logging module instead of print statements in Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/setup_feature_worktree.py` around lines 118 - 124, Replace the direct print() calls that report an active virtualenv with logger calls: use the module logger (e.g., logger.error / logger.info as appropriate) to emit the same messages (include the {location} value), keep the blank line and the PowerShell/cmd instructions as separate logger messages or one multi-line message, and then call sys.exit(1) as before; update the lines that print the environment notice and the deactivation instructions to use logger instead of print so output follows the repository logging convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/setup_feature_worktree.py`:
- Around line 121-124: The recovery hint currently hardcodes a Windows
deactivate path ("venv\\Scripts\\deactivate.bat") after the prints and before
sys.exit(1); update the three print lines so both PowerShell and cmd.exe advice
use the generic "deactivate" command (i.e., print "PowerShell : deactivate" and
"cmd.exe : deactivate") in utils/setup_feature_worktree.py where the script
currently prints the deactivate guidance and calls sys.exit(1).
---
Nitpick comments:
In `@utils/setup_feature_worktree.py`:
- Around line 118-124: Replace the direct print() calls that report an active
virtualenv with logger calls: use the module logger (e.g., logger.error /
logger.info as appropriate) to emit the same messages (include the {location}
value), keep the blank line and the PowerShell/cmd instructions as separate
logger messages or one multi-line message, and then call sys.exit(1) as before;
update the lines that print the environment notice and the deactivation
instructions to use logger instead of print so output follows the repository
logging convention.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8945d6d8-b59a-4431-8ddf-b8582ee7cba0
📒 Files selected for processing (2)
docs/API_MAP.mdutils/setup_feature_worktree.py
| print(" Deactivate it before running this script, then re-run.") | ||
| print(" PowerShell : deactivate") | ||
| print(" cmd.exe : venv\\Scripts\\deactivate.bat") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Use a generic deactivate command in the recovery hint.
cmd.exe guidance is hardcoded to venv\\Scripts\\deactivate.bat, which is wrong when the active env has a different name/path. Prefer deactivate for both shells.
Suggested fix
- print(" PowerShell : deactivate")
- print(" cmd.exe : venv\\Scripts\\deactivate.bat")
+ print(" PowerShell : deactivate")
+ print(" cmd.exe : deactivate")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/setup_feature_worktree.py` around lines 121 - 124, The recovery hint
currently hardcodes a Windows deactivate path ("venv\\Scripts\\deactivate.bat")
after the prints and before sys.exit(1); update the three print lines so both
PowerShell and cmd.exe advice use the generic "deactivate" command (i.e., print
"PowerShell : deactivate" and "cmd.exe : deactivate") in
utils/setup_feature_worktree.py where the script currently prints the deactivate
guidance and calls sys.exit(1).
There was a problem hiding this comment.
Pull request overview
This PR improves the Windows feature worktree setup utility by preventing dependency installation from being hijacked by an already-active outer virtual environment, and updates the generated API reference accordingly.
Changes:
- Add a prerequisite check that aborts worktree creation when an active venv is detected (via
VIRTUAL_ENVandsys.prefixvssys.base_prefix). - Update the API map to reflect the new public function and to remove some outdated entries.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| utils/setup_feature_worktree.py | Adds require_no_active_venv() and calls it from check_prerequisites() to avoid nested/outer-venv interference with uv pip install. |
| docs/API_MAP.md | Regenerates/updates the public API index to include the new helper and remove some entries. |
Comments suppressed due to low confidence (1)
docs/API_MAP.md:36
docs/API_MAP.mdstill lists modules underexperiments/LongitudinalRegistration/uniGradICON/..., but that directory does not exist in the repository checkout. Since this file is marked as generated, these stale entries suggest the API map was not fully regenerated (or the generator input root differs). Please re-runpy utils/generate_api_map.pyfrom the repo root (or adjust generation inputs) so the map only includes files that actually exist.
## experiments/LongitudinalRegistration/uniGradICON/scripts/prepare_l2r_datasets.py
- `def generate_oasis_json(data_dir, output_dir)` (line 31): Generate JSON for OASIS brain MRI (unpaired, with segmentations).
- `def generate_lungct_json(data_dir, output_dir)` (line 57): Generate JSON for LungCT (paired).
- `def generate_abdomenmrct_json(data_dir, output_dir)` (line 89): Generate JSON for AbdomenMRCT cross-modality dataset (unpaired, with segmentations).
- `def main()` (line 122)
## experiments/LongitudinalRegistration/uniGradICON/src/unigradicon/__init__.py
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print("[ERROR] A Python virtual environment is currently active:") | ||
| print(f" {location}") | ||
| print() | ||
| print(" Deactivate it before running this script, then re-run.") | ||
| print(" PowerShell : deactivate") | ||
| print(" cmd.exe : venv\\Scripts\\deactivate.bat") |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 32.58% 32.60% +0.01%
==========================================
Files 52 52
Lines 7156 7156
==========================================
+ Hits 2332 2333 +1
+ Misses 4824 4823 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Documentation