Notebooks: Fix pip installation not working #14506
Conversation
|
@corivera what are your thoughts here? My concerns are that a potential pip upgrade in the future could potentially break us and leave our users broken. Maybe we should make this as a setting? |
|
I found this old issue #8204 (comment) that is somewhat related. @corivera you mentioned in your comment that we already reconfigure pip at the end of our python install process. Where in the code do we do that? Or was that changed at some point? |
|
I looked around in the install code and I couldn't find where we do a pip reinstall. We might have cleaned that up when we got rid of forced package reinstalls, since that could disrupt a user's locally installed pip packages |
|
We should also take a look at upgrading our provided python package. Those bits might be too out of date at this point. The latest version of pandas can't be installed on Python 3.6, for example. |
Yes please 😻 |
| @@ -166,6 +166,9 @@ export class JupyterServerInstallation implements IJupyterServerInstallation { | |||
| if (!pythonExists || forceInstall) { | |||
| await this.installPythonPackage(backgroundOperation, this._usingExistingPython, this._pythonInstallationPath, this.outputChannel); | |||
| } | |||
| // upgrade pip to make sure the right version is installed | |||
| let cmd = `"${this._pythonExecutable}" -m pip install --upgrade pip`; | |||
There was a problem hiding this comment.
Since this only affects the New Install option, should we skip this upgrade when _usingExistingPython is true? We wouldn't want to force upgrade a user's pip packages as a side effect of another install.
There was a problem hiding this comment.
Yeah that makes sense.
| if (!this._usingExistingPython) { | ||
| let versionCmd = `"${this._pythonExecutable}" -m pip --version`; | ||
| let versionCmdResult = await this.executeBufferedCommand(versionCmd); | ||
| let version = versionCmdResult.substring(4, 10); |
There was a problem hiding this comment.
Version numbers aren't guaranteed to all have the same length. If the version was something like "20.2" then by the 10th index you've already started pulling in other characters from the version message. Given a message that looks like this: "pip 20.2 from ...", substring(4,10) would give us "20.2 f". Then you can get the opposite problem if the version number is too long, like if it includes some kind of pre-release tag.
There was a problem hiding this comment.
Looking at the pip release history, we have version "21.0" which hits the too short issue, and "10.0.0b2" which hits the too long issue.
There was a problem hiding this comment.
You can get the installed list of packages using the getInstalledPipPackages() method, which has all the versions listed. Frustratingly, "pip show" doesn't have a json output format like "pip list", so you'd have to get the list of all the packages.
| if (!this._usingExistingPython) { | ||
| let versionCmd = `"${this._pythonExecutable}" -m pip --version`; | ||
| let versionCmdResult = await this.executeBufferedCommand(versionCmd); | ||
| let version = versionCmdResult.substring(4, 10); |
There was a problem hiding this comment.
Looking at the pip release history, we have version "21.0" which hits the too short issue, and "10.0.0b2" which hits the too long issue.
| if (!this._usingExistingPython) { | ||
| let versionCmd = `"${this._pythonExecutable}" -m pip --version`; | ||
| let versionCmdResult = await this.executeBufferedCommand(versionCmd); | ||
| let version = versionCmdResult.substring(4, 10); |
There was a problem hiding this comment.
You can get the installed list of packages using the getInstalledPipPackages() method, which has all the versions listed. Frustratingly, "pip show" doesn't have a json output format like "pip list", so you'd have to get the list of all the packages.
This PR fixes #14192