fix(installer): Prioritize system site packages over server-libs #38
Conversation
Change sys.path.insert(0, ...) to sys.path.append(...) for server site-packages so user-installed packages in system site-packages take precedence over bundled server-libs packages. Previously, server-libs were inserted at the front of sys.path, causing bundled versions to override user-installed versions (e.g., streamlit).
📝 WalkthroughWalkthroughAdds a keyword-only boolean parameter Sequence Diagram(s)sequenceDiagram
participant Caller
participant Venv as VirtualEnvironment.import_package_bundle
participant OS
participant Sys
participant FS as Filesystem
Caller->>Venv: import_package_bundle(bundle_path, condition_env?, priority?)
alt both condition_env and priority provided
Venv-->>Caller: raise ValueError (mutually exclusive)
else
alt condition_env provided
Venv->>OS: getenv(condition_env)
OS-->>Venv: value
alt value == "true"
Venv->>Sys: insert bundle into sys.path (or ensure import)
Sys-->>Venv: updated/loaded
Venv->>FS: write .pth entry with conditional check
FS-->>Venv: pth written
Venv-->>Caller: success (conditional)
else value != "true"
Venv-->>Caller: skipped (no load)
end
else if priority == true
Venv->>Sys: sys.path.insert(0, bundle_path)
Sys-->>Venv: path updated
Venv->>FS: write .pth entry that inserts at position 0
FS-->>Venv: pth written
Venv-->>Caller: success (priority)
else
Venv->>FS: append bundle_path to .pth file
FS-->>Venv: pth written
Venv-->>Caller: success (default append)
end
end
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (.cursorrules)
Files:
🪛 Ruff (0.14.5)installer/module/virtual_environment.py102-105: Avoid specifying long messages outside the exception class (TRY003) ⏰ 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). (8)
🔇 Additional comments (4)
Comment |
|
📦 Python package built successfully!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
+ Coverage 72.75% 73.04% +0.28%
==========================================
Files 93 93
Lines 5143 5149 +6
Branches 752 754 +2
==========================================
+ Hits 3742 3761 +19
+ Misses 1157 1144 -13
Partials 244 244
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
🚀 Review App Deployment Started
|
Change sys.path.insert(0, ...) to sys.path.append(...) for server site-packages so user-installed packages in system site-packages take precedence over bundled server-libs packages. Previously, server-libs were inserted at the front of sys.path, causing bundled versions to override user-installed versions (e.g., streamlit).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 selected for processing (1)
installer/module/virtual_environment.py(1 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). (10)
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Test - Python 3.13
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.9
- GitHub Check: Test - Python 3.12
…libs Add priority parameter to import_package_bundle that uses sys.path.insert(0) to place packages at the front of sys.path. System site-packages now uses this priority flag to ensure user-installed packages are found before bundled server-libs packages.
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)
installer/module/virtual_environment.py (1)
99-110: Inconsistent with PR description: condition branch still usesinsert(0).The PR description states: "replacing sys.path.insert(0, ...) with sys.path.append(...)." However, the conditional-env branch (used for server-libs) still performs
sys.path.insert(0, ...)at line 103.The current implementation achieves the priority goal only because system site-packages is written to
.pthafter server-libs, so itsinsert(0)pushes server-libs back. This makes the behavior fragile and dependent on call order in__main__.py.If the intent is to deprioritize server-libs, apply this diff to use append instead:
if condition_env: # Write conditional import that checks environment variable pth_content = ( f"import os, sys; " - f"sys.path.insert(0, '{bundle_site_package_path}') " + f"sys.path.append('{bundle_site_package_path}') " f"if os.environ.get('{condition_env}', '').lower() == 'true' else None" )This makes system site-packages always take precedence over server-libs regardless of the order of
import_package_bundlecalls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 selected for processing (2)
installer/__main__.py(1 hunks)installer/module/virtual_environment.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
installer/module/virtual_environment.py
86-86: Boolean-typed positional argument in function definition
(FBT001)
86-86: Boolean default positional argument in function definition
(FBT002)
⏰ 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). (8)
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.9
- GitHub Check: Test - Python 3.11
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.12
🔇 Additional comments (2)
installer/module/virtual_environment.py (1)
111-117: LGTM—priority branch correctly inserts at sys.path[0].The logic for
priority=Trueis correct and aligns with giving system site-packages precedence.installer/__main__.py (1)
273-277: Call order dependency verified and functional.The .pth file executes sequentially: server registers first (conditional insert), then system registers with insert(0), which naturally places it ahead in sys.path. This achieves the priority goal as intended.
The implementation works correctly but remains call-order dependent. Your earlier suggestion about using
appendfor server-libs remains valid for making the design more robust, though it's not required for correctness.
Add unit tests for VirtualEnvironment.import_package_bundle covering: - Plain path appending to .pth file - Conditional import with env var check - Priority flag using sys.path.insert(0, ...) - Ordering of bundles matching __main__.py usage
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installer/module/virtual_environment.py (1)
99-107: Userepr/!rfor safety when embedding paths and env var names.If
bundle_site_package_pathorcondition_envever contain quotes, the generated.pthcode can break. Using!ravoids that:- pth_content = ( - f"import os, sys; " - f"sys.path.insert(0, '{bundle_site_package_path}') " - f"if os.environ.get('{condition_env}', '').lower() == 'true' else None" - ) + pth_content = ( + "import os, sys; " + f"sys.path.insert(0, {bundle_site_package_path!r}) " + f"if os.environ.get({condition_env!r}, '').lower() == 'true' else None" + ) @@ - pth_content = ( - f"import sys; sys.path.insert(0, '{bundle_site_package_path}')" - ) + pth_content = ( + f"import sys; sys.path.insert(0, {bundle_site_package_path!r})" + )This keeps behavior while hardening against odd but valid paths or env names.
Also applies to: 112-120
♻️ Duplicate comments (1)
installer/module/virtual_environment.py (1)
83-88: Makepriority(and optionallycondition_env) keyword‑only.To avoid ambiguous positional calls with a trailing bool, it’s better to force keywords here (Ruff FBT001/FBT002):
- def import_package_bundle( - self, - bundle_site_package_path: str, - condition_env: Optional[str] = None, - priority: bool = False, - ) -> None: + def import_package_bundle( + self, + bundle_site_package_path: str, + *, + condition_env: Optional[str] = None, + priority: bool = False, + ) -> None:Call sites would then always spell
priority=Trueexplicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 selected for processing (2)
installer/module/virtual_environment.py(2 hunks)tests/unit/test_virtual_environment.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
installer/module/virtual_environment.pytests/unit/test_virtual_environment.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name test files with
test_*.pypattern
Files:
tests/unit/test_virtual_environment.py
🧬 Code graph analysis (1)
tests/unit/test_virtual_environment.py (1)
installer/module/virtual_environment.py (3)
site_packages_path(144-146)import_package_bundle(83-125)path(139-141)
🪛 Ruff (0.14.5)
installer/module/virtual_environment.py
87-87: Boolean-typed positional argument in function definition
(FBT001)
87-87: Boolean default positional argument in function definition
(FBT002)
tests/unit/test_virtual_environment.py
34-34: Unnecessary mode argument
Remove mode argument
(UP015)
47-47: Unnecessary mode argument
Remove mode argument
(UP015)
61-61: Unnecessary mode argument
Remove mode argument
(UP015)
70-70: Probable insecure usage of temporary file or directory: "/tmp/python3.11/server-libs/lib/python3.11/site-packages"
(S108)
72-72: Probable insecure usage of temporary file or directory: "/tmp/python3.11/kernel-libs/lib/python3.11/site-packages"
(S108)
81-81: Unnecessary mode argument
Remove mode argument
(UP015)
⏰ 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). (9)
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Test - Python 3.9
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.13
- GitHub Check: Test - Python 3.10
🔇 Additional comments (1)
tests/unit/test_virtual_environment.py (1)
14-25: Good coverage of the three code paths and ordering behavior.The fixture and the four tests exercise plain, conditional, priority, and mixed-order scenarios, and the assertions on
.pthcontents match the intended behavior and PR objective.Also applies to: 27-91
- Add guard to raise ValueError when both condition_env and priority are specified (mutually exclusive parameters) - Update docstring to document the mutual exclusivity constraint - Refactor tests to use pathlib.Path instead of os.path/os.makedirs - Add test for mutually exclusive parameter validation
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
installer/module/virtual_environment.py (1)
111-115: Usereprfor paths embedded in.pthPython codeInterpolating
bundle_site_package_pathdirectly into quoted Python strings breaks with backslashes and quotes. The generated.pthfile contains invalid Python.Lines 111–115 and 123–125:
- pth_content = ( - f"import os, sys; " - f"sys.path.insert(0, '{bundle_site_package_path}') " - f"if os.environ.get('{condition_env}', '').lower() == 'true' else None" - ) + pth_content = ( + "import os, sys; " + f"sys.path.insert(0, {bundle_site_package_path!r}) " + f"if os.environ.get('{condition_env}', '').lower() == 'true' " + "else None" + ) - pth_content = ( - f"import sys; sys.path.insert(0, '{bundle_site_package_path}')" - ) + pth_content = ( + f"import sys; " + f"sys.path.insert(0, {bundle_site_package_path!r})" + )Using
repr()ensures Windows paths and paths containing quotes are properly escaped in the generated code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
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 selected for processing (2)
installer/module/virtual_environment.py(2 hunks)tests/unit/test_virtual_environment.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Write clean, readable Python code following PEP 8 style guide
Use type hints with Optional[T] for parameters that can be None (not T = None)
Use explicit type hints for function parameters and return values
Maximum line length: 88 characters (Black default)
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Use black for code formatting
Use isort for import sorting (black profile)
Use flake8 for linting
Use early returns to reduce nesting and extract common checks into variables for readability
Use snake_case for variable and function names
Use PascalCase for class names
Use snake_case for file names
Support Python versions 3.9, 3.10, 3.11, 3.12, and 3.13
**/*.py: Follow PEP 8 with Black formatting (line length: 88)
Use isort with Black profile for import sorting
Use type hints consistently
Use docstrings for all functions/classes
Use f-strings instead of .format() for string formatting
Use pathlib.Path for file path operations instead of os.path
Always useOptional[T]for parameters that can be None (notT = None)
Use explicit type hints for function parameters and return values
Use snake_case for files, functions, and variables
Use PascalCase for classes
Use appropriate exception types with context logging for error handling
Handle Jupyter/IPython specific exceptions properly
Use early returns to reduce nesting and extract common checks into variables for readability
Use dictionary unpacking for headers (e.g.,headers = {"Content-Type": "application/json", **auth_headers})
Use space-separated format for CLI arguments (e.g.,--port 8080)
Files:
installer/module/virtual_environment.pytests/unit/test_virtual_environment.py
**/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Name test files with
test_*.pypattern
Files:
tests/unit/test_virtual_environment.py
🧬 Code graph analysis (1)
tests/unit/test_virtual_environment.py (1)
installer/module/virtual_environment.py (2)
site_packages_path(153-155)import_package_bundle(83-134)
🪛 Ruff (0.14.5)
installer/module/virtual_environment.py
87-87: Boolean-typed positional argument in function definition
(FBT001)
87-87: Boolean default positional argument in function definition
(FBT002)
101-104: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/test_virtual_environment.py
67-67: Probable insecure usage of temporary file or directory: "/tmp/python3.11/server-libs/lib/python3.11/site-packages"
(S108)
69-69: Probable insecure usage of temporary file or directory: "/tmp/python3.11/kernel-libs/lib/python3.11/site-packages"
(S108)
⏰ 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). (8)
- GitHub Check: Build and push artifacts for Python 3.9
- GitHub Check: Build and push artifacts for Python 3.13
- GitHub Check: Build and push artifacts for Python 3.12
- GitHub Check: Build and push artifacts for Python 3.11
- GitHub Check: Build and push artifacts for Python 3.10
- GitHub Check: Test - Python 3.10
- GitHub Check: Test - Python 3.11
- GitHub Check: Test - Python 3.9
Add bare * before priority parameter to enforce keyword-only usage, satisfying Ruff FBT rules for boolean parameters.
…e-user-installed-packages-due-to
mfranczel
requested review from
FilipPyrek and
hc2p
and removed request for
FilipPyrek and
hc2p
| server_site_packages_path, condition_env="DEEPNOTE_INCLUDE_SERVER_PACKAGES" | ||
| ) | ||
| venv.import_package_bundle(system_site_packages_path) | ||
| venv.import_package_bundle(system_site_packages_path, priority=True) |
There was a problem hiding this comment.
The issue was with server site packages. Since they're dynamically inserted into sys path, they would always be prioritized.
mfranczel
deleted the
michal/blu-5203-server-site-packages-override-user-installed-packages-due-to
branch
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.