Fix behavior when missing Pip #6020
Conversation
Updated UI prompt to include Pip in the modules to be installed when missing, and updated installer logic to actually do that.
|
All contributors have signed the CLA ✍️ ✅ |
|
E2E Tests 🚀 |
|
I have read the CLA Document and I hereby sign the CLA |
Revised l10n call to conform better to the localization API
Mocked up environment to ensure that Pip gets installed when `ModuleInstallFlags.installPipIfRequired` is set
|
Added a unit test to check the business logic of installing Pip; do we need an e2e test? |
|
P.S., a screencap of the new behavior: |
| ModuleInstallFlags.installPipIfRequired, | ||
| ); | ||
| expect(result).to.equal(InstallerResponse.Installed, 'Should be Installed'); | ||
| }); |
| let message; | ||
| if (!hasPip) { | ||
| message = vscode.l10n.t( | ||
| 'To enable Python support, Positron needs to {0} the packages <code>{1}</code> and <code>{2}</code> for the active interpreter {3} at: <code>{4}</code>.', |
There was a problem hiding this comment.
Could you change the header of the popup too? Right now it says Install Python package "ipykernel"? even if the body asks to install pip too.
There was a problem hiding this comment.
Ah - good catch! I'll check out implementing this too. 😄
There was a problem hiding this comment.
Does this verbiage sound good?
There was a problem hiding this comment.
Is pip a Python package itself? 🤔
There was a problem hiding this comment.
Yes! A package which exposes a console script.
In this case, it is being installed by the ensurepip builtin module.
Also removes an unnecessary Positron overlay comment
There was a problem hiding this comment.
This is looking great and working well for me!
With no pip or ipykernel (note that we should update the header as pointed out by @austin3dickey):
With no ipykernel only:
We mostly, sort of support uv right now and certainly want to do better for uv users as we move forward; what happens if someone is using uv? In term of us detecting if pip is there or not? See https://docs.astral.sh/uv/pip/compatibility/
And then same question for conda, which do expect to work in Positron well. I believe that currently for conda environments, the call to this.install() uses conda not pip.
|
The CI failures definitely look unrelated, and sadly we are having some MASSIVE CI PROBLEMS right now as runners moved to Ubuntu 24. Let's not merge this until we get an all clear from QA folks on the situation on |
FWIW I set up my local test by doing |
|
UV behavior: As Austin said, no Pip, but it gets installed per the new behavior. Is this what we want (vs. Conda behavior: I created a conda environment with just Python (+ required dependencies). I got no prompt to install |
I do not think we should install pip into uv environments, no. I don't know if we can install ipykernel into uv easily right now (does it exist as one of the installer types already?) but solving that is probably out of scope for this PR. Let's think about this PR as installing pip when needed for non-conda, non-uv environments. |
When selecting an interpreter missing ipykernel, the UI prompt now states that both Pip and ipykernel must be installed.
ProductInstallerlogic was modified to pass flags to enable Pip to be installed automatically. Additional flags were set inProductInstallerto enable the Pip install to respect thepython.installModulesInTerminalsetting.Addresses #5505
Release Notes
New Features
Bug Fixes
QA Notes