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');
+    });


austin3dickey · 2025-01-17T17:08:13Z

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

+            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>.',


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.

Ah - good catch! I'll check out implementing this too. 😄

Does this verbiage sound good?

Is pip a Python package itself? 🤔

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

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?

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

Open

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

Open

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

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

austin3dickey Jan 17, 2025

samclark2015 Jan 17, 2025

samclark2015 Jan 17, 2025

juliasilge Jan 17, 2025

samclark2015 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

Fix behavior when missing Pip #6020

Are you sure you want to change the base?

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

austin3dickey Jan 17, 2025

Choose a reason for hiding this comment

samclark2015 Jan 17, 2025

Choose a reason for hiding this comment

samclark2015 Jan 17, 2025

Choose a reason for hiding this comment

juliasilge Jan 17, 2025

Choose a reason for hiding this comment

samclark2015 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

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