Skip to content

Commit

Permalink
Reduce usage of sysPrefix from old Python API (#15095)
Browse files Browse the repository at this point in the history
* Reduce usage of sysPrefix from old Python API

* oops
  • Loading branch information
DonJayamanne authored Jan 31, 2024
1 parent 0112043 commit 3fc4a02
Show file tree
Hide file tree
Showing 10 changed files with 161 additions and 21 deletions.
17 changes: 12 additions & 5 deletions src/kernels/errors/kernelErrorHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ import { IInterpreterService } from '../../platform/interpreter/contracts';
import { PackageNotInstalledWindowsLongPathNotEnabledError } from '../../platform/errors/packageNotInstalledWindowsLongPathNotEnabledError';
import { JupyterNotebookNotInstalled } from '../../platform/errors/jupyterNotebookNotInstalled';
import { fileToCommandArgument } from '../../platform/common/helpers';
import { getPythonEnvDisplayName } from '../../platform/interpreter/helpers';
import { getPythonEnvDisplayName, getSysPrefix } from '../../platform/interpreter/helpers';
import { JupyterServerCollection } from '../../api';
import { getJupyterDisplayName } from '../jupyter/connection/jupyterServerProviderRegistry';

Expand Down Expand Up @@ -177,12 +177,15 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
// its possible the kernel failed to start due to missing dependencies.
return getIPyKernelMissingErrorMessageForCell(error.kernelConnectionMetadata) || error.message;
} else if (error instanceof BaseKernelError || error instanceof WrappedKernelError) {
const files = await this.getFilesInWorkingDirectoryThatCouldPotentiallyOverridePythonModules(resource);
const [files, sysPrefix] = await Promise.all([
this.getFilesInWorkingDirectoryThatCouldPotentiallyOverridePythonModules(resource),
getSysPrefix(error.kernelConnectionMetadata.interpreter)
]);
const failureInfo = analyzeKernelErrors(
workspace.workspaceFolders || [],
error,
getDisplayNameOrNameOfKernelConnection(error.kernelConnectionMetadata),
error.kernelConnectionMetadata.interpreter?.sysPrefix,
sysPrefix,
files.map((f) => f.uri)
);
if (failureInfo) {
Expand Down Expand Up @@ -472,12 +475,16 @@ export abstract class DataScienceErrorHandler implements IDataScienceErrorHandle
tokenSource.dispose();
}
} else {
const files = await this.getFilesInWorkingDirectoryThatCouldPotentiallyOverridePythonModules(resource);
const [files, sysPrefix] = await Promise.all([
this.getFilesInWorkingDirectoryThatCouldPotentiallyOverridePythonModules(resource),
getSysPrefix(kernelConnection.interpreter)
]);

const failureInfo = analyzeKernelErrors(
workspace.workspaceFolders || [],
err,
getDisplayNameOrNameOfKernelConnection(kernelConnection),
kernelConnection.interpreter?.sysPrefix,
sysPrefix,
files.map((f) => f.uri)
);
this.sendKernelTelemetry(err, errorContext, resource, failureInfo?.reason);
Expand Down
16 changes: 16 additions & 0 deletions src/kernels/errors/kernelErrorHandler.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
/* eslint-disable @typescript-eslint/no-explicit-any */

import dedent from 'dedent';
import * as sinon from 'sinon';
import { assert } from 'chai';
import { anything, capture, deepEqual, instance, mock, verify, when } from 'ts-mockito';
import { Disposable, Uri, WorkspaceFolder } from 'vscode';
Expand Down Expand Up @@ -43,6 +44,8 @@ import { IInterpreterService } from '../../platform/interpreter/contracts';
import { JupyterServer, JupyterServerCollection, JupyterServerProvider } from '../../api';
import { mockedVSCodeNamespaces, resetVSCodeMocks } from '../../test/vscode-mock';
import { dispose } from '../../platform/common/utils/lifecycle';
import { PythonExtension } from '@vscode/python-extension';
import { resolvableInstance } from '../../test/datascience/helpers';

suite('Error Handler Unit Tests', () => {
let dataScienceErrorHandler: DataScienceErrorHandler;
Expand All @@ -62,6 +65,7 @@ suite('Error Handler Unit Tests', () => {
sysPrefix: ''
};
let disposables: IDisposable[] = [];
let environments: PythonExtension['environments'];
setup(() => {
resetVSCodeMocks();
disposables.push(new Disposable(() => resetVSCodeMocks()));
Expand Down Expand Up @@ -98,6 +102,15 @@ suite('Error Handler Unit Tests', () => {
when(mockedVSCodeNamespaces.window.showErrorMessage(anything(), anything(), anything())).thenResolve();
// reset(mockedVSCodeNamespaces.env);
when(mockedVSCodeNamespaces.env.openExternal(anything())).thenReturn(Promise.resolve(true));

const mockedApi = mock<PythonExtension>();
sinon.stub(PythonExtension, 'api').resolves(resolvableInstance(mockedApi));
disposables.push({ dispose: () => sinon.restore() });
environments = mock<PythonExtension['environments']>();
when(mockedApi.environments).thenReturn(instance(environments));
when(environments.resolveEnvironment(jupyterInterpreter.id)).thenResolve({
executable: { sysPrefix: '' }
} as any);
});
teardown(() => {
disposables = dispose(disposables);
Expand Down Expand Up @@ -174,6 +187,9 @@ suite('Error Handler Unit Tests', () => {
executable: ''
}
});
when(environments.resolveEnvironment(kernelConnection.interpreter.id)).thenResolve({
executable: { sysPrefix: 'Something else' }
} as any);
});
const stdErrorMessages = {
userOverridingRandomPyFile_Unix: dedent`
Expand Down
3 changes: 2 additions & 1 deletion src/kernels/execution/cellExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import { SessionDisposedError } from '../../platform/errors/sessionDisposedError
import { isKernelSessionDead } from '../kernel';
import { ICellExecution } from './types';
import { KernelError } from '../errors/kernelError';
import { getCachedSysPrefix } from '../../platform/interpreter/helpers';

/**
* Factory for CellExecution objects.
Expand Down Expand Up @@ -324,7 +325,7 @@ export class CellExecution implements ICellExecution, IDisposable {
workspace.workspaceFolders || [],
error,
getDisplayNameOrNameOfKernelConnection(this.kernelConnection),
this.kernelConnection.interpreter?.sysPrefix
getCachedSysPrefix(this.kernelConnection.interpreter)
);
errorMessage = failureInfo?.message;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { getTelemetrySafeHashedString } from '../../../platform/telemetry/helper
import { isKernelLaunchedViaLocalPythonIPyKernel, isLikelyAPythonExecutable } from '../../helpers.node';
import { LocalKnownPathKernelSpecFinder } from './localKnownPathKernelSpecFinder.node';
import { areObjectsWithUrisTheSame, noop } from '../../../platform/common/utils/misc';
import { getSysPrefix } from '../../../platform/interpreter/helpers';

export function localPythonKernelsCacheKey() {
const LocalPythonKernelsCacheKey = 'LOCAL_KERNEL_PYTHON_AND_RELATED_SPECS_CACHE_KEY_V_2023_3';
Expand All @@ -48,7 +49,12 @@ export async function findKernelSpecsInInterpreter(
emitter: EventEmitter<IJupyterKernelSpec>
): Promise<void> {
// Find all the possible places to look for this resource
const kernelSearchPath = Uri.file(path.join(interpreter.sysPrefix, baseKernelPath));
const sysPrefix = await getSysPrefix(interpreter);
if (!sysPrefix) {
traceWarning(`Failed to get sysPrefix for interpreter ${getDisplayPath(interpreter.id)}`);
return;
}
const kernelSearchPath = Uri.file(path.join(sysPrefix, baseKernelPath));
const rootSpecPaths = await jupyterPaths.getKernelSpecRootPaths(cancelToken);
if (cancelToken.isCancellationRequested) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

import { assert } from 'chai';
import * as sinon from 'sinon';
import * as path from '../../../platform/vscode-path/path';
import { anything, instance, mock, when } from 'ts-mockito';
import { CancellationTokenSource, EventEmitter, Uri } from 'vscode';
Expand All @@ -14,10 +15,11 @@ import { GlobalPythonKernelSpecFinder, findKernelSpecsInInterpreter } from './in
import { baseKernelPath, JupyterPaths } from './jupyterPaths.node';
import { LocalKernelSpecFinder } from './localKernelSpecFinderBase.node';
import { ITrustedKernelPaths } from './types';
import { uriEquals } from '../../../test/datascience/helpers';
import { resolvableInstance, uriEquals } from '../../../test/datascience/helpers';
import { IJupyterKernelSpec } from '../../types';
import { LocalKnownPathKernelSpecFinder } from './localKnownPathKernelSpecFinder.node';
import { mockedVSCodeNamespaces } from '../../../test/vscode-mock';
import { PythonExtension } from '@vscode/python-extension';

suite('Interpreter Kernel Spec Finder Helper', () => {
let helper: GlobalPythonKernelSpecFinder;
Expand All @@ -38,6 +40,7 @@ suite('Interpreter Kernel Spec Finder Helper', () => {
sysPrefix: 'home/global',
uri: Uri.joinPath(Uri.file('globalSys'), 'bin', 'python')
};
let environments: PythonExtension['environments'];
setup(() => {
jupyterPaths = mock<JupyterPaths>();
when(jupyterPaths.getKernelSpecRootPath()).thenResolve();
Expand All @@ -62,6 +65,20 @@ suite('Interpreter Kernel Spec Finder Helper', () => {
version: { major: 3, minor: 10, patch: 0, raw: '3.10.0' }
};
disposables.push(helper);
const mockedApi = mock<PythonExtension>();
sinon.stub(PythonExtension, 'api').resolves(resolvableInstance(mockedApi));
disposables.push({ dispose: () => sinon.restore() });
environments = mock<PythonExtension['environments']>();
when(mockedApi.environments).thenReturn(instance(environments));
when(environments.resolveEnvironment(venvInterpreter.id)).thenResolve({
executable: { sysPrefix: 'home/venvPython' }
} as any);
when(environments.resolveEnvironment(condaInterpreter.id)).thenResolve({
executable: { sysPrefix: 'home/conda' }
} as any);
when(environments.resolveEnvironment(globalInterpreter.id)).thenResolve({
executable: { sysPrefix: 'home/global' }
} as any);
});
teardown(() => (disposables = dispose(disposables)));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@ import { PythonEnvironment } from '../../../platform/pythonEnvironments/info';
import { noop } from '../../../platform/common/utils/misc';
import { createInterpreterKernelSpec, getKernelId } from '../../helpers';
import { deserializePythonEnvironment, serializePythonEnvironment } from '../../../platform/api/pythonApi';
import { uriEquals } from '../../../test/datascience/helpers';
import { resolvableInstance, uriEquals } from '../../../test/datascience/helpers';
import { traceInfo } from '../../../platform/logging';
import { sleep } from '../../../test/core';
import { localPythonKernelsCacheKey } from './interpreterKernelSpecFinderHelper.node';
import { mockedVSCodeNamespaces } from '../../../test/vscode-mock';
import { ResourceMap } from '../../../platform/common/utils/map';
import { PythonExtension } from '@vscode/python-extension';

suite(`Local Python and related kernels`, async () => {
let finder: LocalPythonAndRelatedNonPythonKernelSpecFinder;
Expand Down Expand Up @@ -230,6 +231,24 @@ suite(`Local Python and related kernels`, async () => {
});
disposables.push(new Disposable(() => loadKernelSpecStub.restore()));
traceInfo(`Start Test (completed) ${this.currentTest?.title}`);

const mockedApi = mock<PythonExtension>();
sinon.stub(PythonExtension, 'api').resolves(resolvableInstance(mockedApi));
disposables.push({ dispose: () => sinon.restore() });
const environments = mock<PythonExtension['environments']>();
when(mockedApi.environments).thenReturn(instance(environments));
when(environments.resolveEnvironment(pythonKernelSpec.id)).thenResolve({
executable: { sysPrefix: 'home/python' }
} as any);
when(environments.resolveEnvironment(condaInterpreter.id)).thenResolve({
executable: { sysPrefix: 'home/conda' }
} as any);
when(environments.resolveEnvironment(globalInterpreter.id)).thenResolve({
executable: { sysPrefix: 'home/global' }
} as any);
when(environments.resolveEnvironment(venvInterpreter.id)).thenResolve({
executable: { sysPrefix: 'home/venvPython' }
} as any);
});
teardown(async function () {
traceInfo(`Ended Test (completed) ${this.currentTest?.title}`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { injectable } from 'inversify';
import { Uri } from 'vscode';
import { IKernel } from '../../../../kernels/types';
import { INbExtensionsPathProvider } from '../types';
import { getSysPrefix } from '../../../../platform/interpreter/helpers';

/**
* Returns the path to the nbExtensions folder for a given kernel (node)
Expand All @@ -18,11 +19,11 @@ export class NbExtensionsPathProvider implements INbExtensionsPathProvider {
return Uri.parse(kernel.kernelConnectionMetadata.baseUrl);
}
case 'startUsingPythonInterpreter': {
return Uri.joinPath(
Uri.file(kernel.kernelConnectionMetadata.interpreter.sysPrefix),
'share',
'jupyter'
);
const sysPrefix = await getSysPrefix(kernel.kernelConnectionMetadata.interpreter);
if (!sysPrefix) {
return;
}
return Uri.joinPath(Uri.file(sysPrefix), 'share', 'jupyter');
}
default: {
// We haven't come across scenarios with non-python kernels that use widgets
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
/* eslint-disable @typescript-eslint/no-explicit-any */

import * as path from '../../../../platform/vscode-path/path';
import * as sinon from 'sinon';
import { assert } from 'chai';
import { when, mock, instance } from 'ts-mockito';
import { Uri } from 'vscode';
Expand All @@ -19,16 +20,20 @@ import {
import { INbExtensionsPathProvider } from '../types';
import { NbExtensionsPathProvider } from './nbExtensionsPathProvider.node';
import { NbExtensionsPathProvider as WebNbExtensionsPathProvider } from './nbExtensionsPathProvider.web';
import { PythonExtension } from '@vscode/python-extension';
import { resolvableInstance } from '../../../../test/datascience/helpers';
import { dispose } from '../../../../platform/common/utils/lifecycle';

[false, true].forEach((isWeb) => {
const localNonPythonKernelSpec = LocalKernelSpecConnectionMetadata.create({
id: '',
kernelSpec: mock<IJupyterKernelSpec>()
});
const localPythonKernelSpec = PythonKernelConnectionMetadata.create({
id: '',
id: 'localPythonKernelSpec',
kernelSpec: mock<IJupyterKernelSpec>(),
interpreter: {
id: 'interpreterId',
sysPrefix: __dirname
} as any
});
Expand All @@ -48,10 +53,23 @@ import { NbExtensionsPathProvider as WebNbExtensionsPathProvider } from './nbExt
suite(`NBExtension Path Provider for ${isWeb ? 'Web' : 'Node'}`, () => {
let provider: INbExtensionsPathProvider;
let kernel: IKernel;
let disposables: { dispose(): void }[] = [];
setup(() => {
kernel = mock<IKernel>();
provider = isWeb ? new WebNbExtensionsPathProvider() : new NbExtensionsPathProvider();
const mockedApi = mock<PythonExtension>();
sinon.stub(PythonExtension, 'api').resolves(resolvableInstance(mockedApi));
disposables.push({ dispose: () => sinon.restore() });
const environments = mock<PythonExtension['environments']>();
when(mockedApi.environments).thenReturn(instance(environments));
when(environments.resolveEnvironment(localPythonKernelSpec.interpreter.id)).thenResolve({
executable: { sysPrefix: __dirname }
} as any);
});
teardown(() => {
disposables = dispose(disposables);
});

test('Returns base url for local non-python kernelspec', async () => {
when(kernel.kernelConnectionMetadata).thenReturn(localNonPythonKernelSpec);
assert.isUndefined(await provider.getNbExtensionsParentPath(instance(kernel)));
Expand All @@ -62,10 +80,7 @@ import { NbExtensionsPathProvider as WebNbExtensionsPathProvider } from './nbExt
if (isWeb) {
assert.isUndefined(baseUrl);
} else {
assert.strictEqual(
baseUrl?.toString(),
Uri.file(path.join(localPythonKernelSpec.interpreter.sysPrefix, 'share', 'jupyter')).toString()
);
assert.strictEqual(baseUrl?.toString(), Uri.file(path.join(__dirname, 'share', 'jupyter')).toString());
}
});
test('Returns base url for remote kernelspec', async () => {
Expand Down
5 changes: 4 additions & 1 deletion src/platform/api/pythonApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import { PythonExtensionActicationFailedError } from '../errors/pythonExtActivat
import { PythonExtensionApiNotExportedError } from '../errors/pythonExtApiNotExportedError';
import { getOSType, OSType } from '../common/utils/platform';
import { SemVer } from 'semver';
import { getEnvironmentType } from '../interpreter/helpers';
import { getEnvironmentType, setPythonApi } from '../interpreter/helpers';
import { getWorkspaceFolderIdentifier } from '../common/application/workspace.base';

export function deserializePythonEnvironment(
Expand Down Expand Up @@ -250,6 +250,9 @@ export class OldPythonApiProvider implements IPythonApiProvider {
if (extension?.packageJSON?.version) {
this._pythonExtensionVersion = new SemVer(extension?.packageJSON?.version);
}
if (extension?.exports) {
setPythonApi(extension.exports);
}
return extension?.exports;
}

Expand Down
Loading

0 comments on commit 3fc4a02

Please sign in to comment.