Don't register kernel spec on raw kernel switch to interpreter (#11676)

IanMatthewHuff · web-flow · commit 641beceb538f · 2020-05-07T13:52:01.000-07:00
diff --git a/news/2 Fixes/11575.md b/news/2 Fixes/11575.md
@@ -0,0 +1 @@
+Don't register a kernelspec when switching to an interpreter in raw kernel mode.
diff --git a/news/2 Fixes/11672.md b/news/2 Fixes/11672.md
@@ -0,0 +1 @@
+For direct kernel connection, don't replace a notebook's metadata default kernelspec with a new kernelspec on startup.
diff --git a/src/client/datascience/jupyter/kernels/helpers.ts b/src/client/datascience/jupyter/kernels/helpers.ts
@@ -0,0 +1,34 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+'use strict';
+
+import type { Kernel } from '@jupyterlab/services';
+import { IJupyterKernelSpec } from '../../types';
+import { JupyterKernelSpec } from './jupyterKernelSpec';
+
+// 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}';
+
+// Find the index of the connection file placeholder in a kernelspec
+export function findIndexOfConnectionFile(kernelSpec: Readonly<IJupyterKernelSpec>): number {
+    return kernelSpec.argv.indexOf(connectionFilePlaceholder);
+}
+export function createDefaultKernelSpec(displayName?: string): 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(),
+        language: 'python',
+        display_name: displayName || 'Python 3',
+        metadata: {},
+        argv: ['python', '-m', 'ipykernel_launcher', '-f', connectionFilePlaceholder],
+        env: {},
+        resources: {}
+    };
+
+    return new JupyterKernelSpec(defaultSpec);
+}
diff --git a/src/client/datascience/jupyter/kernels/kernelSelector.ts b/src/client/datascience/jupyter/kernels/kernelSelector.ts
@@ -20,6 +20,7 @@ import { IKernelFinder } from '../../kernel-launcher/types';
 import { reportAction } from '../../progress/decorator';
 import { ReportableAction } from '../../progress/types';
 import { IJupyterKernelSpec, IJupyterSessionManager, IKernelDependencyService } from '../../types';
+import { createDefaultKernelSpec } from './helpers';
 import { KernelSelectionProvider } from './kernelSelections';
 import { KernelService } from './kernelService';
 import { IKernelSpecQuickPickItem, LiveKernelModel } from './types';
@@ -358,7 +359,7 @@ export class KernelSelector {
         // First use our kernel finder to locate a kernelspec on disk
         selection.kernelSpec = await this.kernelFinder.findKernelSpec(
             resource,
-            notebookMetadata?.kernelspec?.name,
+            notebookMetadata?.kernelspec,
             cancelToken
         );
 
@@ -388,7 +389,7 @@ export class KernelSelector {
             return {};
         }
         // Check if ipykernel is installed in this kernel.
-        if (selection.selection.interpreter) {
+        if (selection.selection.interpreter && type === 'jupyter') {
             sendTelemetryEvent(Telemetry.SwitchToInterpreterAsKernel);
             return this.useInterpreterAsKernel(
                 resource,
@@ -399,6 +400,8 @@ export class KernelSelector {
                 false,
                 cancelToken
             );
+        } else if (selection.selection.interpreter && type === 'raw') {
+            return this.useInterpreterAndDefaultKernel(selection.selection.interpreter);
         } else if (selection.selection.kernelModel) {
             sendTelemetryEvent(Telemetry.SwitchToExistingKernel, undefined, {
                 language: this.computeLanguage(selection.selection.kernelModel.language)
@@ -425,6 +428,13 @@ export class KernelSelector {
             return {};
         }
     }
+
+    // When switching to an interpreter in raw kernel mode then just create a default kernelspec for that interpreter to use
+    private async useInterpreterAndDefaultKernel(interpreter: PythonInterpreter): Promise<KernelSpecInterpreter> {
+        const kernelSpec = createDefaultKernelSpec(interpreter.displayName);
+        return { kernelSpec, interpreter };
+    }
+
     /**
      * Use the provided interpreter as a kernel.
      * If `displayNameOfKernelNotFound` is provided, then display a message indicating we're using the `current interpreter`.
diff --git a/src/client/datascience/kernel-launcher/kernelFinder.ts b/src/client/datascience/kernel-launcher/kernelFinder.ts
@@ -2,7 +2,7 @@
 // Licensed under the MIT License.
 'use strict';
 
-import { Kernel } from '@jupyterlab/services';
+import type { nbformat } from '@jupyterlab/coreutils';
 import { inject, injectable, named } from 'inversify';
 import * as path from 'path';
 import { CancellationToken, CancellationTokenSource } from 'vscode';
@@ -14,6 +14,7 @@ import { IExtensionContext, IInstaller, InstallerResponse, IPathUtils, Product,
 import { IInterpreterLocatorService, IInterpreterService, KNOWN_PATH_SERVICE } from '../../interpreter/contracts';
 import { captureTelemetry } from '../../telemetry';
 import { Telemetry } from '../constants';
+import { createDefaultKernelSpec, defaultKernelSpecName } from '../jupyter/kernels/helpers';
 import { JupyterKernelSpec } from '../jupyter/kernels/jupyterKernelSpec';
 import { IJupyterKernelSpec } from '../types';
 import { getKernelInterpreter } from './helpers';
@@ -27,14 +28,6 @@ const macJupyterPath = path.join('Library', 'Jupyter', 'kernels');
 const baseKernelPath = path.join('share', 'jupyter', 'kernels');
 
 const cacheFile = 'kernelSpecPathCache.json';
-const defaultSpecName = 'python_defaultSpec_';
-
-// https://jupyter-client.readthedocs.io/en/stable/kernels.html
-const connectionFilePlaceholder = '{connection_file}';
-
-export function findIndexOfConnectionFile(kernelSpec: Readonly<IJupyterKernelSpec>): number {
-    return kernelSpec.argv.indexOf(connectionFilePlaceholder);
-}
 
 // This class searches for a kernel that matches the given kernel name.
 // First it searches on a global persistent state, then on the installed python interpreters,
@@ -68,40 +61,49 @@ export class KernelFinder implements IKernelFinder {
     @captureTelemetry(Telemetry.KernelFinderPerf)
     public async findKernelSpec(
         resource: Resource,
-        kernelName?: string,
+        kernelSpecMetadata?: nbformat.IKernelspecMetadata,
         cancelToken?: CancellationToken
     ): Promise<IJupyterKernelSpec> {
         this.cache = await this.readCache();
         let foundKernel: IJupyterKernelSpec | undefined;
 
-        if (kernelName && !kernelName.includes(defaultSpecName)) {
-            let kernelSpec = await this.searchCache(kernelName);
+        const kernelName = kernelSpecMetadata?.name;
 
-            if (kernelSpec) {
-                return kernelSpec;
-            }
+        if (kernelSpecMetadata && kernelName) {
+            // For a non default kernelspec search for it
+            if (!kernelName.includes(defaultKernelSpecName)) {
+                let kernelSpec = await this.searchCache(kernelName);
 
-            // Check in active interpreter first
-            kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource);
+                if (kernelSpec) {
+                    return kernelSpec;
+                }
 
-            if (kernelSpec) {
-                this.writeCache(this.cache).ignoreErrors();
-                return kernelSpec;
-            }
+                // Check in active interpreter first
+                kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource);
 
-            const diskSearch = this.findDiskPath(kernelName);
-            const interpreterSearch = this.getInterpreterPaths(resource).then((interpreterPaths) => {
-                return this.findInterpreterPath(interpreterPaths, kernelName);
-            });
+                if (kernelSpec) {
+                    this.writeCache(this.cache).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);
+                });
+
+                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 ? result : await this.getDefaultKernelSpec(resource);
+                foundKernel = result ? result : await this.getDefaultKernelSpec(resource);
+            } else {
+                // For a previous default kernel spec, just use it again
+                foundKernel = this.reuseExistingDefaultSpec(kernelSpecMetadata);
+            }
         } else {
+            // If we don't have kernel metadata then just get a default spec to use
             foundKernel = await this.getDefaultKernelSpec(resource);
         }
 
@@ -132,6 +134,10 @@ export class KernelFinder implements IKernelFinder {
         return this.workspaceToKernels.get(workspaceFolderId)!;
     }
 
+    private reuseExistingDefaultSpec(kernelMetadata: nbformat.IKernelspecMetadata): IJupyterKernelSpec {
+        return createDefaultKernelSpec(kernelMetadata.display_name);
+    }
+
     private async findResourceKernelSpecs(resource: Resource): Promise<IJupyterKernelSpec[]> {
         const results: IJupyterKernelSpec[] = [];
 
@@ -184,7 +190,11 @@ export class KernelFinder implements IKernelFinder {
             traceError(`Failed to parse kernelspec ${specPath}`);
             return undefined;
         }
-        return new JupyterKernelSpec(kernelJson, specPath);
+        const kernelSpec: IJupyterKernelSpec = new JupyterKernelSpec(kernelJson, specPath);
+
+        // Some registered kernel specs do not have a name, in this case use the last part of the path
+        kernelSpec.name = kernelJson?.name || path.basename(path.dirname(specPath));
+        return kernelSpec;
     }
 
     // For the given resource, find atll the file paths for kernel specs that wewant to associate with this
@@ -319,18 +329,7 @@ export class KernelFinder implements IKernelFinder {
     private async getDefaultKernelSpec(resource: Resource): Promise<IJupyterKernelSpec> {
         const activeInterpreter = await this.interpreterService.getActiveInterpreter(resource);
 
-        // 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: defaultSpecName + Date.now().toString(),
-            language: 'python',
-            display_name: activeInterpreter?.displayName ? activeInterpreter.displayName : 'Python 3',
-            metadata: {},
-            argv: ['python', '-m', 'ipykernel_launcher', '-f', connectionFilePlaceholder],
-            env: {},
-            resources: {}
-        };
-        return new JupyterKernelSpec(defaultSpec);
+        return createDefaultKernelSpec(activeInterpreter?.displayName);
     }
 
     private async readCache(): Promise<string[]> {
@@ -369,12 +368,7 @@ export class KernelFinder implements IKernelFinder {
         });
 
         if (kernelJsonFile) {
-            const spec = await this.getKernelSpec(kernelJsonFile);
-
-            if (spec) {
-                spec.name = kernelName;
-                return spec;
-            }
+            return this.getKernelSpec(kernelJsonFile);
         }
 
         return undefined;
diff --git a/src/client/datascience/kernel-launcher/kernelProcess.ts b/src/client/datascience/kernel-launcher/kernelProcess.ts
@@ -14,8 +14,8 @@ import { noop, swallowExceptions } from '../../common/utils/misc';
 import { PythonInterpreter } from '../../interpreter/contracts';
 import { captureTelemetry } from '../../telemetry';
 import { Telemetry } from '../constants';
+import { findIndexOfConnectionFile } from '../jupyter/kernels/helpers';
 import { IJupyterKernelSpec } from '../types';
-import { findIndexOfConnectionFile } from './kernelFinder';
 import { PythonKernelLauncherDaemon } from './kernelLauncherDaemon';
 import { IKernelConnection, IKernelProcess, IPythonKernelDaemon, PythonKernelDiedError } from './types';
 
diff --git a/src/client/datascience/kernel-launcher/types.ts b/src/client/datascience/kernel-launcher/types.ts
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 'use strict';
 
+import type { nbformat } from '@jupyterlab/coreutils';
 import { SpawnOptions } from 'child_process';
 import { CancellationToken, Event } from 'vscode';
 import { InterpreterUri } from '../../common/installer/types';
@@ -46,7 +47,7 @@ export const IKernelFinder = Symbol('IKernelFinder');
 export interface IKernelFinder {
     findKernelSpec(
         interpreterUri: InterpreterUri,
-        kernelName?: string,
+        kernelSpecMetadata?: nbformat.IKernelspecMetadata,
         cancelToken?: CancellationToken
     ): Promise<IJupyterKernelSpec>;
     listKernelSpecs(resource: Resource): Promise<IJupyterKernelSpec[]>;
diff --git a/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts b/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts
@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 import { nbformat } from '@jupyterlab/coreutils';
-import { assert } from 'chai';
+import { assert, expect } from 'chai';
 import * as sinon from 'sinon';
 import { anything, capture, instance, mock, verify, when } from 'ts-mockito';
 import { CancellationToken } from 'vscode-jsonrpc';
@@ -15,6 +15,7 @@ import { noop } from '../../../../client/common/utils/misc';
 import { Architecture } from '../../../../client/common/utils/platform';
 import { StopWatch } from '../../../../client/common/utils/stopWatch';
 import { JupyterSessionManager } from '../../../../client/datascience/jupyter/jupyterSessionManager';
+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';
@@ -25,7 +26,7 @@ import { IJupyterKernelSpec, IJupyterSessionManager } from '../../../../client/d
 import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../../../client/interpreter/contracts';
 import { InterpreterService } from '../../../../client/interpreter/interpreterService';
 
-// tslint:disable: max-func-body-length
+// tslint:disable: max-func-body-length no-unused-expression
 
 suite('Data Science - KernelSelector', () => {
     let kernelSelectionProvider: KernelSelectionProvider;
@@ -504,6 +505,36 @@ suite('Data Science - KernelSelector', () => {
                 appShell.showInformationMessage(localize.DataScience.fallBackToRegisterAndUseActiveInterpeterAsKernel())
             ).never();
         });
+        test('For a raw connection, if an interpreter is selected return it along with a default kernelspec', async () => {
+            when(dependencyService.areDependenciesInstalled(interpreter, anything())).thenResolve(true);
+            when(
+                kernelSelectionProvider.getKernelSelectionsForLocalSession(anything(), 'raw', anything(), anything())
+            ).thenResolve([]);
+            when(appShell.showQuickPick(anything(), anything(), anything())).thenResolve({
+                selection: { interpreter, kernelSpec: undefined }
+                // tslint:disable-next-line: no-any
+            } as any);
+
+            const kernel = await kernelSelector.selectLocalKernel(undefined, 'raw', new StopWatch());
+
+            assert.isOk(kernel.interpreter === interpreter);
+            expect(kernel.kernelSpec, 'Should have kernelspec').to.not.be.undefined;
+            expect(kernel.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);
+            when(kernelService.findMatchingInterpreter(kernelSpec, anything())).thenResolve(interpreter);
+            when(
+                kernelSelectionProvider.getKernelSelectionsForLocalSession(anything(), 'raw', anything(), anything())
+            ).thenResolve([]);
+            when(appShell.showQuickPick(anything(), anything(), anything())).thenResolve({
+                selection: { interpreter: undefined, kernelSpec }
+                // tslint:disable-next-line: no-any
+            } as any);
+            const kernel = await kernelSelector.selectLocalKernel(undefined, 'raw', new StopWatch());
+            expect(kernel.kernelSpec).to.equal(kernelSpec);
+            expect(kernel.interpreter).to.equal(interpreter);
+        });
     });
     // tslint:disable-next-line: max-func-body-length
     suite('Get a kernel for local sessions', () => {
diff --git a/src/test/datascience/kernelFinder.unit.test.ts b/src/test/datascience/kernelFinder.unit.test.ts
diff --git a/src/test/datascience/mockKernelFinder.ts b/src/test/datascience/mockKernelFinder.ts

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Don't register a kernelspec when switching to an interpreter in raw kernel mode.`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+For direct kernel connection, don't replace a notebook's metadata default kernelspec with a new kernelspec on startup.`