Fix behavior when missing Pip #6020

samclark2015 · 2025-01-16T15:39:05Z

When selecting an interpreter missing ipykernel, the UI prompt now states that both Pip and ipykernel must be installed. ProductInstaller logic was modified to pass flags to enable Pip to be installed automatically. Additional flags were set in ProductInstaller to enable the Pip install to respect the python.installModulesInTerminal setting.

Addresses #5505

Release Notes

New Features

N/A

Bug Fixes

N/A

QA Notes

Updated UI prompt to include Pip in the modules to be installed when missing, and updated installer logic to actually do that.

github-actions · 2025-01-16T15:39:23Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

github-actions · 2025-01-16T15:39:24Z

E2E Tests 🚀
This PR will run tests tagged with: @:critical

^readme ^{valid tags}

samclark2015 · 2025-01-16T15:40:59Z

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

samclark2015 · 2025-01-16T21:51:06Z

Added a unit test to check the business logic of installing Pip; do we need an e2e test?

samclark2015 · 2025-01-16T21:54:47Z

P.S., a screencap of the new behavior:
https://github.com/user-attachments/assets/7c98e872-a40a-4480-870d-7ef9f1573727

austin3dickey

nice, I tried it out and it seems to work well! (Can't tell if the test failures are related to this PR or not)

austin3dickey · 2025-01-17T16:17:48Z

extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts

+            ModuleInstallFlags.installPipIfRequired,
+        );
+        expect(result).to.equal(InstallerResponse.Installed, 'Should be Installed');
+    });


extensions/positron-python/src/client/positron/session.ts

Also removes an unnecessary Positron overlay comment

juliasilge

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.

isabelizimm

it doesn't look like the tests failures are related to these changes since it looks like they're failing in setup (?). it might be worth trying to kick them off again. nice tests and the change looks reasonable to me!

juliasilge · 2025-01-17T17:37:26Z

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 main.

austin3dickey · 2025-01-17T19:19:22Z

what happens if someone is using uv?

FWIW I set up my local test by doing uv venv in the positron-python directory and picking the venv that was created there, since I think uv venvs don't come with pip by default.

samclark2015 · 2025-01-17T20:08:11Z

UV behavior: As Austin said, no Pip, but it gets installed per the new behavior. Is this what we want (vs. ipykernel getting installed directly by uv)?

Conda behavior: I created a conda environment with just Python (+ required dependencies). I got no prompt to install ipykernel when activating it in Positron. No ipykernel in the conda list output. Do we know if Conda has some integrated version of ipykernel?

juliasilge · 2025-01-17T20:34:22Z

UV behavior: As Austin said, no Pip, but it gets installed per the new behavior. Is this what we want (vs. ipykernel getting installed directly by uv)?

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.

juliasilge · 2025-01-17T20:37:24Z

For conda, for learning mainly for you @samclark2015, can you step through with the debugger and see what is happening around here for conda:

positron/extensions/positron-python/src/client/positron/session.ts

Lines 242 to 246 in 1093ae4

    
           const hasCompatibleKernel = await installer.isProductVersionCompatible( 
        
               Product.ipykernel, 
        
               IPYKERNEL_VERSION, 
        
               interpreter, 
        
           );

i.e. is the infrastructure already there checking if the conda env has appropriate ipykernel?

samclark2015 · 2025-01-17T21:26:02Z

The infrastructure does check for for ipykernel in conda environments; it was being bypassed due to ipykernel being installed in my user directory (~/.local/lib/python3.12/site-packages/ipykernel, this is Pip's default behavior when it's standard site-packages directory is read-only). 🤔 After ipykernel removing from there, the Positron installer behaves as expected for conda (i.e., detects no ipykernel in the conda env, installs it with conda).

Re UV: there is no infrastructure to install via UV, nor to even detect if it is a UV environment. Currently, it only detects Conda, Pipenv, and Poetry environments. If none of those match, it's declared a Pip environment and subsequently uses Pip to install the package.

Some questions:

Should we prevent Pip from installing ipykernel in this user directory?
Do we need to implement infrastructure for detecting a UV environment? (Perhaps out of scope for this PR)

juliasilge · 2025-01-17T21:36:36Z

For uv, we've got a couple of other issues tracking better support:

The only thing I would advocate we do in this PR is if possible not install pip into uv (because I think that will be very weird/bad), but I am open to discussion on that, especially if we don't even have tools to see if we are in an uv env yet. If so, let's open a followup issue to address this. Our uv users will be able to workaround by installing ipykernel when setting up their environments (they have to do this now anyway, i.e. #3093).

juliasilge · 2025-01-17T21:37:18Z

Should we prevent Pip from installing ipykernel in this user directory?

I am going to look to @austin3dickey and @isabelizimm for answers on this.

samclark2015 · 2025-01-17T22:14:25Z

The only thing I would advocate we do in this PR is if possible not install pip into uv (because I think that will be very weird/bad), but I am open to discussion on that, especially if we don't even have tools to see if we are in an uv env yet.

Doesn't look like there are tools to tell if it's UV; it's labeled as EnvironmentType.Venv. Perhaps we can discuss more in next week's sync?

austin3dickey · 2025-01-21T15:22:48Z

The only thing I would advocate we do in this PR is if possible not install pip into uv

I don't think installing the pip package into a uv environment would be that bad; in fact their docs recognize that many projects have an undeclared dependency on pip and recommend installing pip if that's the case.

isabelizimm · 2025-01-21T15:34:30Z

Are the conda problems happening because ipykernel is being installed in ~/.local instead of the conda environment, or that ipykernel was previously installed and we are looking in the wrong spot for pip/ipykernel? I think either of those two things could be addressed without blocking installation at a certain path. Historically, we have not prevented people from installing ipykernel anywhere (including in system or global Pythons 😬).

…-pip

samclark2015 · 2025-01-21T18:00:52Z

ipykernel was previously installed and we are looking in the wrong spot for pip/ipykernel

Discussed, but for posterity, this is the case. It was previously installed in the user-local site-packages directory when selecting a system interpreter in a previous run.

When in a clean state, the installer uses conda to install ipykernel within a conda environment.

isabelizimm · 2025-01-21T18:10:01Z

extensions/positron-python/src/test/common/installer/productInstaller.unit.test.ts

+    IModuleInstaller,
+    InterpreterUri,
+    ModuleInstallFlags,
+} from '../../../client/common/installer/types';


Small nit: can you pull these out into their own line? with // --- Start/End Positron ---

See: https://connect.posit.it/positron-wiki/overlay-strategy.html#a-note-on-imports

i.e. this? Will the lint check yell about importing the package multiple times?

You should be able to // eslint-disable-next-line no-duplicate-imports right above that line to ignore the yelling 👀

Sorry - should have looked at the link more closely. 😅 Two notes:

Looks like the annotation is now import/no-duplicates... the no-duplicate-imports has no impact so it seems and errors with Definition for rule ... was not found.

With either annotation, the original import is still flagged. The first one at least satisfies ESLint for the added import.

Thoughts? Should the whole import block in the referenced commit just be wrapped with Positron markers?

Oooh that is new! I don't have strong opinions here, but others might. (@seeM maybe?)

~~I wonder if our advice is now outdated because of the change in imports in the recent upstream merge.~~

No wait actually, did it ever work in the Python extension?

Here are some files to see how we are managing this:

https://github.com/posit-dev/positron/blob/main/extensions/positron-python/src/test/common/variables/environmentVariablesProvider.unit.test.ts

https://github.com/posit-dev/positron/blob/2c9fe8c8e486836e3864a0cf3af0f2dbb1f23b67/extensions/positron-python/src/test/common/variables/envVarsProvider.multiroot.test.ts

Looks like we need... both?

…-pip

juliasilge

Still working great for pyenv, conda, venvs after the latest changes:

Fix behavior when missing Pip

e0fd89c

Updated UI prompt to include Pip in the modules to be installed when missing, and updated installer logic to actually do that.

samclark2015 linked an issue Jan 16, 2025 that may be closed by this pull request

Python environments that don't have 'pip' or 'ipykernel' cannot be used #5505

Closed

samclark2015 added 2 commits January 16, 2025 15:38

Update l10n for prompt to install Pip

e3abcf1

Revised l10n call to conform better to the localization API

Add unit tests for installing Pip with the DataScienceInstaller

a77962c

Mocked up environment to ensure that Pip gets installed when `ModuleInstallFlags.installPipIfRequired` is set

samclark2015 removed a link to an issue Jan 16, 2025

Python environments that don't have 'pip' or 'ipykernel' cannot be used #5505

Closed

Fix issue with flipped logic in session Pip check

1093ae4

samclark2015 marked this pull request as ready for review January 16, 2025 22:03

samclark2015 requested review from juliasilge, isabelizimm and seeM January 16, 2025 22:03

austin3dickey reviewed Jan 17, 2025

View reviewed changes

Updates prompt title in ProductInstaller when installing Pip

ce88b51

Also removes an unnecessary Positron overlay comment

juliasilge reviewed Jan 17, 2025

View reviewed changes

isabelizimm reviewed Jan 17, 2025

View reviewed changes

Merge branch 'main' into bugfix/prompt-install-pip

3442995

juliasilge mentioned this pull request Jan 18, 2025

Using uv leads to a failed ipykernel installation #3093

Open

Merge remote-tracking branch 'origin/main' into bugfix/prompt-install…

e3d1c97

…-pip

isabelizimm previously approved these changes Jan 21, 2025

View reviewed changes

samclark2015 added 2 commits January 22, 2025 09:08

Merge remote-tracking branch 'origin/main' into bugfix/prompt-install…

035e589

…-pip

Adds Positron import markers where necessary

62c8627

samclark2015 dismissed isabelizimm’s stale review via 62c8627 January 22, 2025 15:10

juliasilge approved these changes Jan 22, 2025

View reviewed changes

samclark2015 merged commit 775a468 into main Jan 22, 2025
27 checks passed

samclark2015 deleted the bugfix/prompt-install-pip branch January 22, 2025 17:36

github-actions bot locked and limited conversation to collaborators Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix behavior when missing Pip #6020

Fix behavior when missing Pip #6020

samclark2015 commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

samclark2015 commented Jan 16, 2025

samclark2015 commented Jan 16, 2025

samclark2015 commented Jan 16, 2025

austin3dickey left a comment

austin3dickey Jan 17, 2025

juliasilge left a comment

isabelizimm left a comment

juliasilge commented Jan 17, 2025

austin3dickey commented Jan 17, 2025

samclark2015 commented Jan 17, 2025

juliasilge commented Jan 17, 2025

juliasilge commented Jan 17, 2025

samclark2015 commented Jan 17, 2025

juliasilge commented Jan 17, 2025

juliasilge commented Jan 17, 2025

samclark2015 commented Jan 17, 2025 •

edited

Loading

austin3dickey commented Jan 21, 2025

isabelizimm commented Jan 21, 2025

samclark2015 commented Jan 21, 2025

isabelizimm Jan 21, 2025

samclark2015 Jan 21, 2025

isabelizimm Jan 21, 2025

samclark2015 Jan 21, 2025

isabelizimm Jan 21, 2025

juliasilge Jan 21, 2025 •

edited

Loading

juliasilge Jan 21, 2025

juliasilge left a comment

Fix behavior when missing Pip #6020

Fix behavior when missing Pip #6020

Conversation

samclark2015 commented Jan 16, 2025 • edited Loading

Release Notes

New Features

Bug Fixes

QA Notes

github-actions bot commented Jan 16, 2025 • edited Loading

github-actions bot commented Jan 16, 2025 • edited Loading

samclark2015 commented Jan 16, 2025

samclark2015 commented Jan 16, 2025

samclark2015 commented Jan 16, 2025

austin3dickey left a comment

Choose a reason for hiding this comment

austin3dickey Jan 17, 2025

Choose a reason for hiding this comment

juliasilge left a comment

Choose a reason for hiding this comment

isabelizimm left a comment

Choose a reason for hiding this comment

juliasilge commented Jan 17, 2025

austin3dickey commented Jan 17, 2025

samclark2015 commented Jan 17, 2025

juliasilge commented Jan 17, 2025

juliasilge commented Jan 17, 2025

samclark2015 commented Jan 17, 2025

juliasilge commented Jan 17, 2025

juliasilge commented Jan 17, 2025

samclark2015 commented Jan 17, 2025 • edited Loading

austin3dickey commented Jan 21, 2025

isabelizimm commented Jan 21, 2025

samclark2015 commented Jan 21, 2025

isabelizimm Jan 21, 2025

Choose a reason for hiding this comment

samclark2015 Jan 21, 2025

Choose a reason for hiding this comment

isabelizimm Jan 21, 2025

Choose a reason for hiding this comment

samclark2015 Jan 21, 2025

Choose a reason for hiding this comment

isabelizimm Jan 21, 2025

Choose a reason for hiding this comment

juliasilge Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

juliasilge Jan 21, 2025

Choose a reason for hiding this comment

juliasilge left a comment

Choose a reason for hiding this comment

samclark2015 commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited

Loading

samclark2015 commented Jan 17, 2025 •

edited

Loading

juliasilge Jan 21, 2025 •

edited

Loading