From 9abad386779b01d71e7888680490a226f684843e Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 22 Feb 2022 09:26:34 +1100 Subject: [PATCH 1/7] Shell cmd should load executables from venv folder --- src/client/common/variables/environment.ts | 27 ++++++++++++----- src/client/common/variables/types.ts | 1 + .../kernel-launcher/kernelEnvVarsService.ts | 8 +++++ .../notebook/executionService.vscode.test.ts | 29 ++++++++++++++++++- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/src/client/common/variables/environment.ts b/src/client/common/variables/environment.ts index b7f8ae5a43b..7ebe449d835 100644 --- a/src/client/common/variables/environment.ts +++ b/src/client/common/variables/environment.ts @@ -46,19 +46,28 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService } public appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]) { - return this.appendPaths(vars, 'PYTHONPATH', ...pythonPaths); + return this.appendPaths(vars, 'PYTHONPATH', true, ...pythonPaths); } public appendPath(vars: EnvironmentVariables, ...paths: string[]) { - return this.appendPaths(vars, 'PATH', ...paths); + return this.appendPaths(vars, 'PATH', true, ...paths); } - private appendPaths(vars: EnvironmentVariables, variableName: 'PATH' | 'PYTHONPATH', ...pathsToAppend: string[]) { - const valueToAppend = pathsToAppend + public prependPath(vars: EnvironmentVariables, ...paths: string[]) { + return this.appendPaths(vars, 'PATH', false, ...paths); + } + + private appendPaths( + vars: EnvironmentVariables, + variableName: 'PATH' | 'PYTHONPATH', + append: boolean, + ...pathsToAppend: string[] + ) { + const valueToAppendOrPrepend = pathsToAppend .filter((item) => typeof item === 'string' && item.trim().length > 0) .map((item) => item.trim()) .join(path.delimiter); - if (valueToAppend.length === 0) { + if (valueToAppendOrPrepend.length === 0) { return vars; } @@ -69,9 +78,13 @@ export class EnvironmentVariablesService implements IEnvironmentVariablesService const variable = vars && matchingKey ? vars[matchingKey] : undefined; const setKey = matchingKey || variableName; if (variable && typeof variable === 'string' && variable.length > 0) { - vars[setKey] = variable + path.delimiter + valueToAppend; + if (append) { + vars[setKey] = variable + path.delimiter + valueToAppendOrPrepend; + } else { + vars[setKey] = valueToAppendOrPrepend + path.delimiter + variable; + } } else { - vars[setKey] = valueToAppend; + vars[setKey] = valueToAppendOrPrepend; } return vars; } diff --git a/src/client/common/variables/types.ts b/src/client/common/variables/types.ts index c42de5171dc..a8f357dc456 100644 --- a/src/client/common/variables/types.ts +++ b/src/client/common/variables/types.ts @@ -19,6 +19,7 @@ export interface IEnvironmentVariablesService { mergeVariables(source: EnvironmentVariables, target: EnvironmentVariables): void; appendPythonPath(vars: EnvironmentVariables, ...pythonPaths: string[]): void; appendPath(vars: EnvironmentVariables, ...paths: string[]): void; + prependPath(vars: EnvironmentVariables, ...paths: string[]): void; } /** diff --git a/src/client/datascience/kernel-launcher/kernelEnvVarsService.ts b/src/client/datascience/kernel-launcher/kernelEnvVarsService.ts index 9c53c3afdda..d3b2ca64225 100644 --- a/src/client/datascience/kernel-launcher/kernelEnvVarsService.ts +++ b/src/client/datascience/kernel-launcher/kernelEnvVarsService.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import { inject, injectable } from 'inversify'; +import * as path from 'path'; import { traceError, traceInfo } from '../../common/logger'; import { Resource } from '../../common/types'; import { noop } from '../../common/utils/misc'; @@ -98,6 +99,13 @@ export class KernelEnvironmentVariablesService { if (kernelEnv.PYTHONPATH) { this.envVarsService.appendPythonPath(mergedVars, kernelEnv.PYTHONPATH); } + // Ensure the python env folder is always at the top of the PATH, this way all executables from that env are used. + // This way shell commands such as `!pip`, `!python` end up pointing to the right executables. + // Also applies to `!java` where java could be an executable in the conda bin directory. + if (interpreter) { + this.envVarsService.prependPath(mergedVars, path.dirname(interpreter.path)); + } + // Ensure global site_packages are not in the path. // The global site_packages will be added to the path later. // For more details see here https://github.com/microsoft/vscode-jupyter/issues/8553#issuecomment-997144591 diff --git a/src/test/datascience/notebook/executionService.vscode.test.ts b/src/test/datascience/notebook/executionService.vscode.test.ts index 2ac35de6e47..2a18df307f9 100644 --- a/src/test/datascience/notebook/executionService.vscode.test.ts +++ b/src/test/datascience/notebook/executionService.vscode.test.ts @@ -39,7 +39,8 @@ import { workAroundVSCodeNotebookStartPages, waitForTextOutput, defaultNotebookTestTimeout, - waitForCellExecutionState + waitForCellExecutionState, + getCellOutputs } from './helper'; import { ProductNames } from '../../../client/common/installer/productNames'; import { openNotebook } from '../helpers'; @@ -51,6 +52,7 @@ import { } from '../../../client/datascience/notebook/helpers/helpers'; import { getDisplayPath } from '../../../client/common/platform/fs-paths'; import { IPYTHON_VERSION_CODE, IS_REMOTE_NATIVE_TEST } from '../../constants'; +import { areInterpreterPathsSame } from '../../../client/pythonEnvironments/info/interpreter'; // eslint-disable-next-line @typescript-eslint/no-var-requires, @typescript-eslint/no-require-imports const expectedPromptMessageSuffix = `requires ${ProductNames.get(Product.ipykernel)!} to be installed.`; @@ -376,6 +378,31 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { await waitForExecutionCompletedSuccessfully(cell); }); + test('Shell commands should give preference to executables in Python Environment', async () => { + await insertCodeCell('import sys', { index: 0 }); + await insertCodeCell('import os', { index: 1 }); + await insertCodeCell('print(os.getenv("PATH"))', { index: 2 }); + await insertCodeCell('print(sys.executable)', { index: 3 }); + const [, , cell3, cell4] = vscodeNotebook.activeNotebookEditor?.document.getCells()!; + + // Basically anything such as `!which python` and the like should point to the right executable. + // For that to work, the first directory in the PATH must be the Python environment. + + await Promise.all([ + runAllCellsInActiveNotebook(), + waitForExecutionCompletedSuccessfully(cell4), + waitForCondition(async () => getCellOutputs(cell4).length > 0, defaultNotebookTestTimeout, 'No output') + ]); + + const pathValue = getCellOutputs(cell3).split(path.delimiter); + const sysExecutable = getCellOutputs(cell4).trim().toLowerCase(); + + // First path in PATH must be the directory where executable is located. + assert.ok( + areInterpreterPathsSame(path.dirname(sysExecutable), pathValue[0].toLowerCase()), + `First entry in PATH (${pathValue[0]}) does not point to executable (${sysExecutable})` + ); + }); test('Testing streamed output', async () => { // Assume you are executing a cell that prints numbers 1-100. // When printing number 50, you click clear. From d4a722920524e228accd9a74fc1bb3e37ceec96c Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 22 Feb 2022 09:29:40 +1100 Subject: [PATCH 2/7] Misc --- .eslintrc.js | 1 - 1 file changed, 1 deletion(-) diff --git a/.eslintrc.js b/.eslintrc.js index dfc4f353c80..4dc5d8d0a4e 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -520,7 +520,6 @@ module.exports = { 'src/client/common/dotnet/services/linuxCompatibilityService.ts', 'src/client/common/dotnet/services/windowsCompatibilityService.ts', 'src/client/common/variables/serviceRegistry.ts', - 'src/client/common/variables/types.ts', 'src/client/common/variables/sysTypes.ts', 'src/client/common/variables/systemVariables.ts', 'src/client/common/nuget/azureBlobStoreNugetRepository.ts', From 56a08365e9332d71ce86d48034cc92ce6945c768 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 22 Feb 2022 09:39:29 +1100 Subject: [PATCH 3/7] Fix tests --- .../common/variables/kernelEnvVarsService.unit.test.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/test/common/variables/kernelEnvVarsService.unit.test.ts b/src/test/common/variables/kernelEnvVarsService.unit.test.ts index d53397df5ca..f3d9128381a 100644 --- a/src/test/common/variables/kernelEnvVarsService.unit.test.ts +++ b/src/test/common/variables/kernelEnvVarsService.unit.test.ts @@ -73,7 +73,7 @@ suite('Kernel Environment Variables Service', () => { const processPath = Object.keys(process.env).find((k) => k.toLowerCase() == 'path'); expect(processPath).to.not.be.undefined; expect(vars).to.not.be.undefined; - expect(vars![processPath!]).to.be.equal('foobar'); + expect(vars![processPath!]).to.be.equal(`${path.dirname(interpreter.path)}${path.delimiter}foobar`); }); test('Paths are merged', async () => { @@ -90,7 +90,9 @@ suite('Kernel Environment Variables Service', () => { const processPath = Object.keys(process.env).find((k) => k.toLowerCase() == 'path'); expect(processPath).to.not.be.undefined; expect(vars).to.not.be.undefined; - expect(vars![processPath!]).to.be.equal(`foobar${path.delimiter}foobaz`); + expect(vars![processPath!]).to.be.equal( + `${path.dirname(interpreter.path)}${path.delimiter}foobar${path.delimiter}foobaz` + ); }); test('KernelSpec interpreterPath used if interpreter is undefined', async () => { @@ -113,7 +115,9 @@ suite('Kernel Environment Variables Service', () => { const processPath = Object.keys(process.env).find((k) => k.toLowerCase() == 'path'); expect(processPath).to.not.be.undefined; expect(vars).to.not.be.undefined; - expect(vars![processPath!]).to.be.equal(`foobar${path.delimiter}foobaz`); + expect(vars![processPath!]).to.be.equal( + `${path.dirname(interpreter.path)}${path.delimiter}foobar${path.delimiter}foobaz` + ); }); }); }); From 7124d772256b6a6f5ed036f1812f7201ebe41b42 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 22 Feb 2022 11:33:13 +1100 Subject: [PATCH 4/7] Misc --- news/2 Fixes/9089.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 news/2 Fixes/9089.md diff --git a/news/2 Fixes/9089.md b/news/2 Fixes/9089.md new file mode 100644 index 00000000000..5c1e4226c64 --- /dev/null +++ b/news/2 Fixes/9089.md @@ -0,0 +1,2 @@ +When running `shell commands`, ensure the kernel first looks for executables in Python Environment associated with the Kernel. +E.g. commands such as `!pip` and `!python` will point to the `pip` and `python` executable associated with the kernel. From 4301448f617259836c61e9ae261257fde984e396 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 22 Feb 2022 12:42:39 +1100 Subject: [PATCH 5/7] Misc --- .../datascience/notebook/executionService.vscode.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/test/datascience/notebook/executionService.vscode.test.ts b/src/test/datascience/notebook/executionService.vscode.test.ts index 2a18df307f9..fca8cde2de8 100644 --- a/src/test/datascience/notebook/executionService.vscode.test.ts +++ b/src/test/datascience/notebook/executionService.vscode.test.ts @@ -378,7 +378,10 @@ suite('DataScience - VSCode Notebook - (Execution) (slow)', function () { await waitForExecutionCompletedSuccessfully(cell); }); - test('Shell commands should give preference to executables in Python Environment', async () => { + test('Shell commands should give preference to executables in Python Environment', async function () { + if (IS_REMOTE_NATIVE_TEST) { + return this.skip(); + } await insertCodeCell('import sys', { index: 0 }); await insertCodeCell('import os', { index: 1 }); await insertCodeCell('print(os.getenv("PATH"))', { index: 2 }); From c8fec014c7ea45a3c83451bea083d2008e441cef Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 22 Feb 2022 12:54:20 +1100 Subject: [PATCH 6/7] Fixes --- .../jupyter/kernels/jupyterKernelService.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/client/datascience/jupyter/kernels/jupyterKernelService.ts b/src/client/datascience/jupyter/kernels/jupyterKernelService.ts index 2d1a5cdeebc..4897d3ad580 100644 --- a/src/client/datascience/jupyter/kernels/jupyterKernelService.ts +++ b/src/client/datascience/jupyter/kernels/jupyterKernelService.ts @@ -15,6 +15,7 @@ import { IFileSystem } from '../../../common/platform/types'; import { ReadWrite, Resource } from '../../../common/types'; import { noop } from '../../../common/utils/misc'; +import { IEnvironmentVariablesService } from '../../../common/variables/types'; import { IEnvironmentActivationService } from '../../../interpreter/activation/types'; import { ignoreLogging, logValue } from '../../../logging/trace'; import { PythonEnvironment } from '../../../pythonEnvironments/info'; @@ -38,7 +39,8 @@ export class JupyterKernelService { @inject(IKernelDependencyService) private readonly kernelDependencyService: IKernelDependencyService, @inject(IFileSystem) private readonly fs: IFileSystem, @inject(IEnvironmentActivationService) private readonly activationHelper: IEnvironmentActivationService, - @inject(ILocalKernelFinder) private readonly kernelFinder: ILocalKernelFinder + @inject(ILocalKernelFinder) private readonly kernelFinder: ILocalKernelFinder, + @inject(IEnvironmentVariablesService) private readonly envVarsService: IEnvironmentVariablesService ) {} /** @@ -256,6 +258,12 @@ export class JupyterKernelService { .catch(noop) // eslint-disable-next-line @typescript-eslint/no-explicit-any .then((env) => (env || {}) as any); + // Ensure the python env folder is always at the top of the PATH, this way all executables from that env are used. + // This way shell commands such as `!pip`, `!python` end up pointing to the right executables. + // Also applies to `!java` where java could be an executable in the conda bin directory. + if (interpreter && specModel.env) { + this.envVarsService.prependPath(specModel.env as {}, path.dirname(interpreter.path)); + } // Ensure global site_packages are not in the path. // The global site_packages will be added to the path later. From e55d79e50146242005715647edeb011c9c735691 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 22 Feb 2022 13:04:00 +1100 Subject: [PATCH 7/7] Fix tests --- .../jupyter/kernels/jupyterKernelService.unit.test.ts | 4 +++- .../notebook/intellisense/gotodef.vscode.test.ts | 9 ++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/test/datascience/jupyter/kernels/jupyterKernelService.unit.test.ts b/src/test/datascience/jupyter/kernels/jupyterKernelService.unit.test.ts index 04cbc1b16a2..5b0f7c5c69f 100644 --- a/src/test/datascience/jupyter/kernels/jupyterKernelService.unit.test.ts +++ b/src/test/datascience/jupyter/kernels/jupyterKernelService.unit.test.ts @@ -20,6 +20,7 @@ import * as path from 'path'; import { arePathsSame } from '../../../common'; import { DisplayOptions } from '../../../../client/datascience/displayOptions'; import { CancellationTokenSource } from 'vscode'; +import { IEnvironmentVariablesService } from '../../../../client/common/variables/types'; // eslint-disable-next-line suite('DataScience - JupyterKernelService', () => { @@ -297,7 +298,8 @@ suite('DataScience - JupyterKernelService', () => { instance(kernelDependencyService), instance(fs), instance(appEnv), - instance(kernelFinder) + instance(kernelFinder), + instance(mock()) ); }); test('Dependencies checked on all kernels with interpreters', async () => { diff --git a/src/test/datascience/notebook/intellisense/gotodef.vscode.test.ts b/src/test/datascience/notebook/intellisense/gotodef.vscode.test.ts index c2944b33631..1d6f3ddd353 100644 --- a/src/test/datascience/notebook/intellisense/gotodef.vscode.test.ts +++ b/src/test/datascience/notebook/intellisense/gotodef.vscode.test.ts @@ -89,7 +89,14 @@ suite('DataScience - VSCode Intellisense Notebook and Interactive Goto Definitio ); // Verify we are in cell1 - assert.equal(vscode.window.activeTextEditor?.document, cell1.document, 'Text editor did not switch'); + await waitForCondition( + async () => { + assert.equal(vscode.window.activeTextEditor?.document, cell1.document, 'Text editor did not switch'); + return true; + }, + 5_000, + 'Text editor did not switch' + ); }); test('Import pandas and goto it', async () => {