fix: use python -m pip instead of pip --python for PipPackageManager#8840
fix: use python -m pip instead of pip --python for PipPackageManager#8840dmadisetti merged 1 commit intomainfrom
Conversation
On macOS with pip-installed Python (e.g. python.org installer),
only 'pip3' is available in PATH, not 'pip'. This caused
PipPackageManager.is_manager_installed() to return False because
shutil.which('pip') could not find it.
Changes:
- PipPackageManager.is_manager_installed(): primary check via
'python -m pip --version', fallback to which('pip')
- PipPackageManager.install_command(): use 'python -m pip' instead of
'pip --python python_exe'
- PipPackageManager.uninstall(): same
- PipPackageManager.list_packages(): same
- Add unit tests for fallback logic and error handling
Fixes #8775
Cherry-picked from PR #8779 (commits 6953b4a, db75b74)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR updates Marimo’s PipPackageManager to invoke pip via python -m pip (rather than relying on a pip executable on PATH), addressing macOS setups where only pip3 is available and dependency detection previously failed.
Changes:
- Switch pip install/uninstall/list commands to
python_exe -m pip .... - Override
PipPackageManager.is_manager_installed()to primarily validate pip viapython_exe -m pip --version, with a PATH-based fallback. - Update/add tests to reflect the new command format and the new installation-check behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| marimo/_runtime/packages/pypi_package_manager.py | Moves pip invocation to python -m pip and adds a custom pip availability check. |
| tests/_runtime/packages/test_pypi_package_manager.py | Updates expected pip command arguments and adds tests for the new is_manager_installed() logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except (subprocess.TimeoutExpired, FileNotFoundError, OSError): | ||
| pass | ||
| # Fallback: check if pip is on PATH (handles cases where pip is | ||
| # available but python -m pip is not desired/needed) | ||
| if DependencyManager.which(self.name): | ||
| return True |
There was a problem hiding this comment.
is_manager_installed() falls back to DependencyManager.which("pip") even when subprocess.run([self._python_exe, "-m", "pip", "--version"]) raises (e.g. FileNotFoundError / PermissionError). If pip exists on PATH but self._python_exe is invalid/unexecutable, this will incorrectly return True and later pip operations will fail (and may raise from subprocess.run in _run_sync). Consider treating these exceptions as a hard failure (return False and optionally log a targeted error about python_exe), and only using the PATH fallback when the python invocation ran successfully but returned a non-zero exit code (or remove the PATH fallback entirely since pip commands now always use self._python_exe -m pip).
| # Primary check: use the same invocation method we actually use | ||
| # (python -m pip) rather than relying on PATH pip, which could be | ||
| # a different Python's pip than self._python_exe | ||
| try: | ||
| proc = subprocess.run( | ||
| [self._python_exe, "-m", "pip", "--version"], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
| if proc.returncode == 0: | ||
| return True |
There was a problem hiding this comment.
is_manager_installed() now spawns an extra python -m pip --version subprocess on every check (and then most call sites immediately run another pip command). This adds measurable overhead compared to the previous shutil.which()-style check, especially when listing/installing packages repeatedly. Consider caching the result for the lifetime of the manager instance, or restructuring so the first real pip command is attempted directly and errors are handled there (avoiding a dedicated --version probe).
| @patch("subprocess.run") | ||
| @patch("marimo._dependencies.dependencies.DependencyManager.which") | ||
| def test_pip_is_manager_installed_permission_error( | ||
| mock_which: MagicMock, mock_run: MagicMock | ||
| ): | ||
| """python -m pip raises OSError (e.g. PermissionError) → returns False.""" | ||
| mock_run.side_effect = PermissionError("python not executable") | ||
| mock_which.return_value = False | ||
| mgr = PipPackageManager() | ||
| assert mgr.is_manager_installed() is False | ||
|
|
||
|
|
||
| @patch("subprocess.run") | ||
| @patch("marimo._dependencies.dependencies.DependencyManager.which") | ||
| def test_pip_is_manager_installed_both_fail( | ||
| mock_which: MagicMock, mock_run: MagicMock | ||
| ): | ||
| """Both python -m pip and which("pip") fail → returns False.""" | ||
| mock_run.return_value = MagicMock(returncode=1) | ||
| mock_which.return_value = False | ||
| mgr = PipPackageManager() | ||
| assert mgr.is_manager_installed() is False |
There was a problem hiding this comment.
There’s no test covering the case where the primary python -m pip --version probe raises (e.g. FileNotFoundError/PermissionError) while DependencyManager.which("pip") returns True. Given pip commands now always use self._python_exe -m pip, the expected behavior in that scenario is likely False to avoid later crashes when running commands with an invalid python_exe. Adding a regression test here would help lock in the intended semantics.
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.21.2-dev49 |
Fixes #8775
Cherry picked from #8776 #8779 by @KJyang-0114