From dbbfbfc3b1f8ea46afd4dacdee48fae8b619cb1e Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 2 Oct 2020 10:48:22 -0700 Subject: [PATCH] Select kernel based on metadata in notebook (#14217) * Fix picking kernel based on metadata --- news/2 Fixes/14213.md | 1 + .../datascience/jupyter/kernels/helpers.ts | 8 ++-- .../jupyter/kernels/kernelSelector.ts | 2 +- .../jupyter/kernels/kernelSwitcher.ts | 5 ++- .../kernel-launcher/kernelFinder.ts | 44 +++++++++---------- .../kernel-launcher/kernelProcess.ts | 5 +-- .../datascience/notebookStorage/baseModel.ts | 37 +++++++++------- .../kernels/kernelSelector.unit.test.ts | 4 -- .../datascience/kernelFinder.unit.test.ts | 20 --------- 9 files changed, 52 insertions(+), 74 deletions(-) create mode 100644 news/2 Fixes/14213.md diff --git a/news/2 Fixes/14213.md b/news/2 Fixes/14213.md new file mode 100644 index 000000000000..2e3a90f6bdd0 --- /dev/null +++ b/news/2 Fixes/14213.md @@ -0,0 +1 @@ +Use the kernel defined in the metadata of Notebook instead of using the default workspace interpreter. diff --git a/src/client/datascience/jupyter/kernels/helpers.ts b/src/client/datascience/jupyter/kernels/helpers.ts index cebda0e1e334..bcbe703e4d33 100644 --- a/src/client/datascience/jupyter/kernels/helpers.ts +++ b/src/client/datascience/jupyter/kernels/helpers.ts @@ -25,8 +25,6 @@ import { // Helper functions for dealing with kernels and kernelspecs -export const defaultKernelSpecName = 'python_defaultSpec_'; - // https://jupyter-client.readthedocs.io/en/stable/kernels.html const connectionFilePlaceholder = '{connection_file}'; @@ -136,13 +134,13 @@ export function getKernelConnectionLanguage(kernelConnection?: KernelConnectionM return model?.language || kernelSpec?.language; } // Create a default kernelspec with the given display name -export function createDefaultKernelSpec(displayName?: string): IJupyterKernelSpec { +export function createDefaultKernelSpec(interpreter?: PythonEnvironment): IJupyterKernelSpec { // This creates a default kernel spec. When launched, 'python' argument will map to using the interpreter // associated with the current resource for launching. const defaultSpec: Kernel.ISpecModel = { - name: defaultKernelSpecName + Date.now().toString(), + name: interpreter?.displayName || 'Python 3', language: 'python', - display_name: displayName || 'Python 3', + display_name: interpreter?.displayName || 'Python 3', metadata: {}, argv: ['python', '-m', 'ipykernel_launcher', '-f', connectionFilePlaceholder], env: {}, diff --git a/src/client/datascience/jupyter/kernels/kernelSelector.ts b/src/client/datascience/jupyter/kernels/kernelSelector.ts index e398ded46d5f..4e9a172ce817 100644 --- a/src/client/datascience/jupyter/kernels/kernelSelector.ts +++ b/src/client/datascience/jupyter/kernels/kernelSelector.ts @@ -549,7 +549,7 @@ export class KernelSelector implements IKernelSelectionUsage { // When switching to an interpreter in raw kernel mode then just create a default kernelspec for that interpreter to use private async useInterpreterAndDefaultKernel(interpreter: PythonEnvironment): Promise { - const kernelSpec = createDefaultKernelSpec(interpreter.displayName); + const kernelSpec = createDefaultKernelSpec(interpreter); return { kernelSpec, interpreter, kind: 'startUsingPythonInterpreter' }; } diff --git a/src/client/datascience/jupyter/kernels/kernelSwitcher.ts b/src/client/datascience/jupyter/kernels/kernelSwitcher.ts index 73b0613c4e92..bce7b401799e 100644 --- a/src/client/datascience/jupyter/kernels/kernelSwitcher.ts +++ b/src/client/datascience/jupyter/kernels/kernelSwitcher.ts @@ -90,7 +90,10 @@ export class KernelSwitcher { const kernelSpec = kernelConnectionMetadataHasKernelSpec(kernelConnection) ? kernelConnection : undefined; const kernelName = kernelSpec?.kernelSpec?.name || kernelModel?.kernelModel?.name; // One of them is bound to be non-empty. - const displayName = kernelModel?.kernelModel?.display_name || kernelName || ''; + const displayName = + kernelConnection.kind === 'startUsingPythonInterpreter' + ? kernelConnection.interpreter.displayName || kernelConnection.interpreter.path + : kernelModel?.kernelModel?.display_name || kernelName || ''; const options: ProgressOptions = { location: ProgressLocation.Notification, cancellable: false, diff --git a/src/client/datascience/kernel-launcher/kernelFinder.ts b/src/client/datascience/kernel-launcher/kernelFinder.ts index 5315c09caf05..69e34934a0a2 100644 --- a/src/client/datascience/kernel-launcher/kernelFinder.ts +++ b/src/client/datascience/kernel-launcher/kernelFinder.ts @@ -16,7 +16,6 @@ import { IInterpreterLocatorService, IInterpreterService, KNOWN_PATH_SERVICE } f import { captureTelemetry } from '../../telemetry'; import { getRealPath } from '../common'; import { Telemetry } from '../constants'; -import { defaultKernelSpecName } from '../jupyter/kernels/helpers'; import { JupyterKernelSpec } from '../jupyter/kernels/jupyterKernelSpec'; import { IDataScienceFileSystem, IJupyterKernelSpec } from '../types'; import { IKernelFinder } from './types'; @@ -70,35 +69,32 @@ export class KernelFinder implements IKernelFinder { const kernelName = kernelSpecMetadata?.name; if (kernelSpecMetadata && kernelName) { - // For a non default kernelspec search for it - if (!kernelName.includes(defaultKernelSpecName)) { - let kernelSpec = await this.searchCache(kernelName); + let kernelSpec = await this.searchCache(kernelName); - if (kernelSpec) { - return kernelSpec; - } + if (kernelSpec) { + return kernelSpec; + } - // Check in active interpreter first - kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource); + // Check in active interpreter first + kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource); - if (kernelSpec) { - this.writeCache().ignoreErrors(); - return kernelSpec; - } - - const diskSearch = this.findDiskPath(kernelName); - const interpreterSearch = this.getInterpreterPaths(resource).then((interpreterPaths) => { - return this.findInterpreterPath(interpreterPaths, kernelName); - }); + if (kernelSpec) { + this.writeCache().ignoreErrors(); + return kernelSpec; + } - let result = await Promise.race([diskSearch, interpreterSearch]); - if (!result) { - const both = await Promise.all([diskSearch, interpreterSearch]); - result = both[0] ? both[0] : both[1]; - } + const diskSearch = this.findDiskPath(kernelName); + const interpreterSearch = this.getInterpreterPaths(resource).then((interpreterPaths) => { + return this.findInterpreterPath(interpreterPaths, kernelName); + }); - foundKernel = result; + let result = await Promise.race([diskSearch, interpreterSearch]); + if (!result) { + const both = await Promise.all([diskSearch, interpreterSearch]); + result = both[0] ? both[0] : both[1]; } + + foundKernel = result; } this.writeCache().ignoreErrors(); diff --git a/src/client/datascience/kernel-launcher/kernelProcess.ts b/src/client/datascience/kernel-launcher/kernelProcess.ts index dfeda3d60198..46b79635dc90 100644 --- a/src/client/datascience/kernel-launcher/kernelProcess.ts +++ b/src/client/datascience/kernel-launcher/kernelProcess.ts @@ -151,10 +151,7 @@ export class KernelProcess implements IKernelProcess { let kernelSpec = this._kernelConnectionMetadata.kernelSpec; // If there is no kernelspec & when launching a Python process, generate a dummy `kernelSpec` if (!kernelSpec && this._kernelConnectionMetadata.kind === 'startUsingPythonInterpreter') { - kernelSpec = createDefaultKernelSpec( - this._kernelConnectionMetadata.interpreter.displayName || - this._kernelConnectionMetadata.interpreter.path - ); + kernelSpec = createDefaultKernelSpec(this._kernelConnectionMetadata.interpreter); } // We always expect a kernel spec. if (!kernelSpec) { diff --git a/src/client/datascience/notebookStorage/baseModel.ts b/src/client/datascience/notebookStorage/baseModel.ts index 2a28e231352f..db62829a4d0e 100644 --- a/src/client/datascience/notebookStorage/baseModel.ts +++ b/src/client/datascience/notebookStorage/baseModel.ts @@ -56,7 +56,22 @@ export function updateNotebookMetadata( kernelConnection && kernelConnectionMetadataHasKernelModel(kernelConnection) ? kernelConnection.kernelModel : kernelConnection?.kernelSpec; - if (kernelSpecOrModel && !metadata.kernelspec) { + if (kernelConnection?.kind === 'startUsingPythonInterpreter') { + // Store interpreter name, we expect the kernel finder will find the corresponding interpreter based on this name. + const name = kernelConnection.interpreter.displayName || ''; + if (metadata.kernelspec?.name !== name || metadata.kernelspec?.display_name !== name) { + changed = true; + metadata.kernelspec = { + name, + display_name: name, + metadata: { + interpreter: { + hash: sha256().update(kernelConnection.interpreter.path).digest('hex') + } + } + }; + } + } else if (kernelSpecOrModel && !metadata.kernelspec) { // Add a new spec in this case metadata.kernelspec = { name: kernelSpecOrModel.name || kernelSpecOrModel.display_name || '', @@ -78,20 +93,12 @@ export function updateNotebookMetadata( metadata.kernelspec.display_name = displayName; kernelId = kernelSpecOrModel.id; } - } else if (kernelConnection?.kind === 'startUsingPythonInterpreter') { - // Store interpreter name, we expect the kernel finder will find the corresponding interpreter based on this name. - const name = kernelConnection.interpreter.displayName || kernelConnection.interpreter.path; - if (metadata.kernelspec?.name !== name || metadata.kernelspec?.display_name !== name) { - changed = true; - metadata.kernelspec = { - name, - display_name: name, - metadata: { - interpreter: { - hash: sha256().update(kernelConnection.interpreter.path).digest('hex') - } - } - }; + try { + // This is set only for when we select an interpreter. + // tslint:disable-next-line: no-any + delete (metadata.kernelspec as any).metadata; + } catch { + // Noop. } } return { changed, kernelId }; diff --git a/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts b/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts index 2b15157b520b..de2c3fddf0a5 100644 --- a/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts +++ b/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts @@ -19,7 +19,6 @@ import { Architecture } from '../../../../client/common/utils/platform'; import { StopWatch } from '../../../../client/common/utils/stopWatch'; import { JupyterSessionManager } from '../../../../client/datascience/jupyter/jupyterSessionManager'; import { JupyterSessionManagerFactory } from '../../../../client/datascience/jupyter/jupyterSessionManagerFactory'; -import { defaultKernelSpecName } from '../../../../client/datascience/jupyter/kernels/helpers'; import { KernelDependencyService } from '../../../../client/datascience/jupyter/kernels/kernelDependencyService'; import { KernelSelectionProvider } from '../../../../client/datascience/jupyter/kernels/kernelSelections'; import { KernelSelector } from '../../../../client/datascience/jupyter/kernels/kernelSelector'; @@ -453,9 +452,6 @@ suite('DataScience - KernelSelector', () => { assert.deepEqual(kernel?.interpreter, interpreter); expect((kernel as any)?.kernelSpec, 'Should have kernelspec').to.not.be.undefined; - expect((kernel as any)?.kernelSpec!.name, 'Spec should have default name').to.include( - defaultKernelSpecName - ); }); test('For a raw connection, if a kernel spec is selected return it with the interpreter', async () => { when(dependencyService.areDependenciesInstalled(interpreter, anything())).thenResolve(true); diff --git a/src/test/datascience/kernelFinder.unit.test.ts b/src/test/datascience/kernelFinder.unit.test.ts index bf37540a375c..8fc98be82c66 100644 --- a/src/test/datascience/kernelFinder.unit.test.ts +++ b/src/test/datascience/kernelFinder.unit.test.ts @@ -14,7 +14,6 @@ import { PythonExecutionFactory } from '../../client/common/process/pythonExecut import { IExtensionContext, IPathUtils, Resource } from '../../client/common/types'; import { Architecture } from '../../client/common/utils/platform'; import { IEnvironmentVariablesProvider } from '../../client/common/variables/types'; -import { defaultKernelSpecName } from '../../client/datascience/jupyter/kernels/helpers'; import { JupyterKernelSpec } from '../../client/datascience/jupyter/kernels/jupyterKernelSpec'; import { KernelFinder } from '../../client/datascience/kernel-launcher/kernelFinder'; import { IKernelFinder } from '../../client/datascience/kernel-launcher/types'; @@ -526,25 +525,6 @@ suite('Kernel Finder', () => { fileSystem.reset(); }); - test('Kernel metadata already has a default spec, and kernel spec not found, then return undefined', async () => { - setupFileSystem(); - fileSystem - .setup((fs) => fs.readLocalFile(typemoq.It.isAnyString())) - .returns((pathParam: string) => { - if (pathParam.includes(cacheFile)) { - return Promise.resolve('[]'); - } - return Promise.resolve('{}'); - }); - // get default kernel - const spec = await kernelFinder.findKernelSpec(resource, { - name: defaultKernelSpecName, - display_name: 'TargetDisplayName' - }); - assert.isUndefined(spec); - fileSystem.reset(); - }); - test('Look for KernelA with no cache, find KernelA and KenelB, then search for KernelB and find it in cache', async () => { setupFileSystem(); fileSystem