Changes to listing kernel specs (#8880)

DonJayamanne · web-flow · commit 4be1c61e6d28 · 2019-12-03T12:38:13.000-08:00
* Support installing ipykernel as a product
diff --git a/src/client/common/types.ts b/src/client/common/types.ts
@@ -7,6 +7,7 @@ import { Request as RequestResult } from 'request';
 import { ConfigurationTarget, DiagnosticSeverity, Disposable, DocumentSymbolProvider, Event, Extension, ExtensionContext, OutputChannel, Uri, WorkspaceEdit } from 'vscode';
 import { CommandsWithoutArgs } from './application/commands';
 import { ExtensionChannels } from './insidersBuild/types';
+import { InterpreterUri } from './installer/types';
 import { EnvironmentVariables } from './variables/types';
 export const IOutputChannel = Symbol('IOutputChannel');
 export interface IOutputChannel extends OutputChannel { }
@@ -113,9 +114,9 @@ export enum ModuleNamePurpose {
 export const IInstaller = Symbol('IInstaller');
 
 export interface IInstaller {
-    promptToInstall(product: Product, resource?: Uri): Promise<InstallerResponse>;
-    install(product: Product, resource?: Uri): Promise<InstallerResponse>;
-    isInstalled(product: Product, resource?: Uri): Promise<boolean | undefined>;
+    promptToInstall(product: Product, resource?: InterpreterUri): Promise<InstallerResponse>;
+    install(product: Product, resource?: InterpreterUri): Promise<InstallerResponse>;
+    isInstalled(product: Product, resource?: InterpreterUri): Promise<boolean | undefined>;
     translateProductToModuleName(product: Product, purpose: ModuleNamePurpose): string;
 }
 
diff --git a/src/client/datascience/jupyter/jupyterSessionManager.ts b/src/client/datascience/jupyter/jupyterSessionManager.ts
@@ -86,7 +86,7 @@ export class JupyterSessionManager implements IJupyterSessionManager {
         return session;
     }
 
-    public async getActiveKernelSpecs(): Promise<IJupyterKernelSpec[]> {
+    public async getKernelSpecs(): Promise<IJupyterKernelSpec[]> {
         if (!this.connInfo || !this.sessionManager || !this.contentsManager) {
             throw new Error(localize.DataScience.sessionDisposed());
         }
diff --git a/src/client/datascience/jupyter/kernels/jupyterKernelSpec.ts b/src/client/datascience/jupyter/kernels/jupyterKernelSpec.ts
@@ -1,43 +1,43 @@
-// Copyright (c) Microsoft Corporation. All rights reserved.
-// Licensed under the MIT License.
-'use strict';
-import { Kernel } from '@jupyterlab/services';
-import * as fs from 'fs-extra';
-import * as path from 'path';
-
-import { noop } from '../../../common/utils/misc';
-import { PythonInterpreter } from '../../../interpreter/contracts';
-import { IJupyterKernelSpec } from '../../types';
-
-const IsGuidRegEx = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
-
-export class JupyterKernelSpec implements IJupyterKernelSpec {
-    public name: string;
-    public language: string;
-    public path: string;
-    public specFile: string | undefined;
-    public display_name: string;
-    // tslint:disable-next-line: no-any
-    public metadata?: Record<string, any> & { interpreter?: Partial<PythonInterpreter> };
-    constructor(specModel: Kernel.ISpecModel, file?: string) {
-        this.name = specModel.name;
-        this.language = specModel.language;
-        this.path = specModel.argv && specModel.argv.length > 0 ? specModel.argv[0] : '';
-        this.specFile = file;
-        this.display_name = specModel.display_name;
-        this.metadata = specModel.metadata;
-    }
-    public dispose = async () => {
-        if (this.specFile &&
-            IsGuidRegEx.test(path.basename(path.dirname(this.specFile)))) {
-            // There is more than one location for the spec file directory
-            // to be cleaned up. If one fails, the other likely deleted it already.
-            try {
-                await fs.remove(path.dirname(this.specFile));
-            } catch {
-                noop();
-            }
-            this.specFile = undefined;
-        }
-    }
-}
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+'use strict';
+import { Kernel } from '@jupyterlab/services';
+import * as fs from 'fs-extra';
+import * as path from 'path';
+
+import { noop } from '../../../common/utils/misc';
+import { PythonInterpreter } from '../../../interpreter/contracts';
+import { IJupyterKernelSpec } from '../../types';
+
+const IsGuidRegEx = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
+
+export class JupyterKernelSpec implements IJupyterKernelSpec {
+    public name: string;
+    public language: string;
+    public path: string;
+    public specFile: string | undefined;
+    public display_name: string;
+    // tslint:disable-next-line: no-any
+    public metadata?: Record<string, any> & { interpreter?: Partial<PythonInterpreter> };
+    constructor(specModel: Kernel.ISpecModel, file?: string) {
+        this.name = specModel.name;
+        this.language = specModel.language;
+        this.path = specModel.argv && specModel.argv.length > 0 ? specModel.argv[0] : '';
+        this.specFile = file;
+        this.display_name = specModel.display_name;
+        this.metadata = specModel.metadata;
+    }
+    public dispose = async () => {
+        if (this.specFile &&
+            IsGuidRegEx.test(path.basename(path.dirname(this.specFile)))) {
+            // There is more than one location for the spec file directory
+            // to be cleaned up. If one fails, the other likely deleted it already.
+            try {
+                await fs.remove(path.dirname(this.specFile));
+            } catch {
+                noop();
+            }
+            this.specFile = undefined;
+        }
+    }
+}
diff --git a/src/client/datascience/jupyter/kernels/kernelService.ts b/src/client/datascience/jupyter/kernels/kernelService.ts
@@ -79,7 +79,7 @@ export class KernelService {
      */
     public async findMatchingKernelSpec(
         interpreter: PythonInterpreter,
-        sessionManager: IJupyterSessionManager | undefined,
+        sessionManager?: IJupyterSessionManager | undefined,
         cancelToken?: CancellationToken
     ): Promise<IJupyterKernelSpec | undefined>;
     public async findMatchingKernelSpec(
@@ -118,7 +118,7 @@ export class KernelService {
             }
 
             // Now enumerate them again
-            const enumerator = sessionManager ? () => sessionManager.getActiveKernelSpecs() : () => this.enumerateSpecs(cancelToken);
+            const enumerator = sessionManager ? () => sessionManager.getKernelSpecs() : () => this.enumerateSpecs(cancelToken);
 
             // Then find our match
             return this.findSpecMatch(enumerator);
@@ -132,6 +132,7 @@ export class KernelService {
             }
         }
     }
+
     /**
      * Registers an interprter as a kernel.
      * The assumption is that `ipykernel` has been installed in the interpreter.
@@ -147,40 +148,64 @@ export class KernelService {
      */
     @captureTelemetry(Telemetry.RegisterInterpreterAsKernel, undefined, true)
     @traceDecorators.error('Failed to register an interpreter as a kernel')
-    public async registerKernel(interpreter: PythonInterpreter, cancelToken?: CancellationToken): Promise<IJupyterKernelSpec> {
-        if (!interpreter.displayName){
+    public async registerKernel(interpreter: PythonInterpreter, cancelToken?: CancellationToken): Promise<IJupyterKernelSpec | undefined> {
+        if (!interpreter.displayName) {
             throw new Error('Interpreter does not have a display name');
         }
         const ipykernelCommand = await this.commandFinder.findBestCommand(JupyterCommands.KernelCreateCommand, cancelToken);
-        if (ipykernelCommand.status === ModuleExistsStatus.NotFound || !ipykernelCommand.command){
+        if (ipykernelCommand.status === ModuleExistsStatus.NotFound || !ipykernelCommand.command) {
             throw new Error('Command not found to install the kernel');
         }
+        if (Cancellation.isCanceled(cancelToken)) {
+            return;
+        }
         const name = await this.generateKernelNameForIntepreter(interpreter);
         const output = await ipykernelCommand.command.exec(['install', '--user', '--name', name, '--display-name', interpreter.displayName], {
             throwOnStdErr: true,
             encoding: 'utf8',
             token: cancelToken
         });
-        const kernel = await this.findMatchingKernelSpec({display_name: interpreter.displayName, name}, undefined, cancelToken);
-        if (!kernel){
+        if (Cancellation.isCanceled(cancelToken)) {
+            return;
+        }
+
+        const kernel = await this.findMatchingKernelSpec({ display_name: interpreter.displayName, name }, undefined, cancelToken);
+        if (Cancellation.isCanceled(cancelToken)) {
+            return;
+        }
+        if (!kernel) {
             const error = `Kernel not created with the name ${name}, display_name ${interpreter.displayName}. Output is ${output.stdout}`;
             throw new Error(error);
         }
         if (!(kernel instanceof JupyterKernelSpec)) {
             const error = `Kernel not registered locally, created with the name ${name}, display_name ${interpreter.displayName}. Output is ${output.stdout}`;
             throw new Error(error);
         }
-        if (!kernel.specFile){
+        if (!kernel.specFile) {
             const error = `kernel.json not created with the name ${name}, display_name ${interpreter.displayName}. Output is ${output.stdout}`;
             throw new Error(error);
         }
         const specModel: ReadWrite<Kernel.ISpecModel> = JSON.parse(await this.fileSystem.readFile(kernel.specFile));
 
+        // Ensure we use a fully qualified path to the python interpreter in `argv`.
+        if (specModel.argv[0].toLowerCase() === 'conda') {
+            // If conda is the first word, its possible its a conda activation command.
+            traceInfo(`Spec argv[0], not updated as it is using conda.`);
+        } else {
+            traceInfo(`Spec argv[0] updated from '${specModel.argv[0]}' to '${interpreter.path}'`);
+            specModel.argv[0] = interpreter.path;
+        }
+
         // Get the activated environment variables (as a work around for `conda run` and similar).
         // This ensures the code runs within the context of an activated environment.
-        specModel.env = await this.activationHelper.getActivatedEnvironmentVariables(undefined, interpreter, true)
-                                // tslint:disable-next-line: no-any
-                                .catch(noop).then(env => (env || {}) as any);
+        specModel.env = await this.activationHelper
+            .getActivatedEnvironmentVariables(undefined, interpreter, true)
+            .catch(noop)
+            // tslint:disable-next-line: no-any
+            .then(env => (env || {}) as any);
+        if (Cancellation.isCanceled(cancelToken)) {
+            return;
+        }
 
         // Ensure we update the metadata to include interpreter stuff as well (we'll use this to search kernels that match an interpreter).
         // We'll need information such as interpreter type, display name, path, etc...
@@ -193,6 +218,7 @@ export class KernelService {
         await this.fileSystem.writeFile(kernel.specFile, JSON.stringify(specModel, undefined, 2));
         kernel.metadata = specModel.metadata;
 
+        traceInfo(`Kernel successfully registered for ${interpreter.path} with the name=${name} and spec can be found here ${kernel.specFile}`);
         return kernel;
     }
     /**
@@ -208,7 +234,7 @@ export class KernelService {
         return `${interpreter.displayName || ''}_${await this.fileSystem.getFileHash(interpreter.path)}`.replace(/[^A-Za-z0-9]/g, '');
     }
     private async getKernelSpecs(sessionManager?: IJupyterSessionManager, cancelToken?: CancellationToken): Promise<IJupyterKernelSpec[]> {
-        const enumerator = sessionManager ? sessionManager.getActiveKernelSpecs() : this.enumerateSpecs(cancelToken);
+        const enumerator = sessionManager ? sessionManager.getKernelSpecs() : this.enumerateSpecs(cancelToken);
         if (Cancellation.isCanceled(cancelToken)) {
             return [];
         }
@@ -402,21 +428,23 @@ export class KernelService {
             traceInfo('Parsing kernelspecs from jupyter');
             // This should give us back a key value pair we can parse
             const kernelSpecs = JSON.parse(output.stdout.trim()) as Record<string, { resource_dir: string; spec: Omit<Kernel.ISpecModel, 'name'> }>;
-            const specs = await Promise.all(Object.keys(kernelSpecs).map(async kernelName => {
-                const specFile = path.join(kernelSpecs[kernelName].resource_dir, 'kernel.json');
-                const spec = kernelSpecs[kernelName].spec;
-                // Add the missing name property.
-                const model = {
-                    ...spec,
-                    name: kernelName
-                };
-                // Check if the spec file exists.
-                if (await this.fileSystem.fileExists(specFile)){
-                    return new JupyterKernelSpec(model as Kernel.ISpecModel, specFile);
-                } else {
-                    return;
-                }
-            }));
+            const specs = await Promise.all(
+                Object.keys(kernelSpecs).map(async kernelName => {
+                    const specFile = path.join(kernelSpecs[kernelName].resource_dir, 'kernel.json');
+                    const spec = kernelSpecs[kernelName].spec;
+                    // Add the missing name property.
+                    const model = {
+                        ...spec,
+                        name: kernelName
+                    };
+                    // Check if the spec file exists.
+                    if (await this.fileSystem.fileExists(specFile)) {
+                        return new JupyterKernelSpec(model as Kernel.ISpecModel, specFile);
+                    } else {
+                        return;
+                    }
+                })
+            );
             return specs.filter(item => !!item).map(item => item as JupyterKernelSpec);
         } catch (ex) {
             traceError('Failed to list kernels', ex);
diff --git a/src/client/datascience/jupyter/liveshare/guestJupyterSessionManager.ts b/src/client/datascience/jupyter/liveshare/guestJupyterSessionManager.ts
@@ -17,7 +17,7 @@ export class GuestJupyterSessionManager implements IJupyterSessionManager {
         return this.realSessionManager.startNew(kernelSpec, cancelToken);
     }
 
-    public async getActiveKernelSpecs(): Promise<IJupyterKernelSpec[]> {
+    public async getKernelSpecs(): Promise<IJupyterKernelSpec[]> {
         // Don't return any kernel specs in guest mode. They're only needed for the host side
         return Promise.resolve([]);
     }
diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts
@@ -182,7 +182,7 @@ export interface IJupyterSessionManagerFactory {
 
 export interface IJupyterSessionManager extends IAsyncDisposable {
     startNew(kernelSpec: IJupyterKernelSpec | undefined, cancelToken?: CancellationToken): Promise<IJupyterSession>;
-    getActiveKernelSpecs(): Promise<IJupyterKernelSpec[]>;
+    getKernelSpecs(): Promise<IJupyterKernelSpec[]>;
     getConnInfo(): IConnection;
     getRunningKernels(): Promise<IJupyterKernel[]>;
 }
diff --git a/src/client/interpreter/configuration/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector.ts
@@ -42,7 +42,8 @@ export class InterpreterSelector implements IInterpreterSelector {
             // tslint:disable-next-line:no-non-null-assertion
             label: suggestion.displayName!,
             detail: `${cachedPrefix}${detail}`,
-            path: suggestion.path
+            path: suggestion.path,
+            interpreter: suggestion
         };
     }
 
diff --git a/src/client/interpreter/configuration/types.ts b/src/client/interpreter/configuration/types.ts
@@ -26,6 +26,13 @@ export interface IInterpreterSelector extends Disposable {
 
 export interface IInterpreterQuickPickItem extends QuickPickItem {
     path: string;
+    /**
+     * The interpreter related to this quickpick item.
+     *
+     * @type {PythonInterpreter}
+     * @memberof IInterpreterQuickPickItem
+     */
+    interpreter: PythonInterpreter;
 }
 
 export const IInterpreterComparer = Symbol('IInterpreterComparer');
diff --git a/src/test/configuration/interpreterSelector.unit.test.ts b/src/test/configuration/interpreterSelector.unit.test.ts
@@ -31,6 +31,8 @@ class InterpreterQuickPickItem implements IInterpreterQuickPickItem {
     public label: string;
     public description!: string;
     public detail?: string;
+    // tslint:disable-next-line: no-any
+    public interpreter = {} as any;
     constructor(l: string, p: string) {
         this.path = p;
         this.label = l;
@@ -136,7 +138,9 @@ suite('Interpreters - selector', () => {
         pythonSettings.setup(p => p.pythonPath).returns(() => 'python');
         const selectedItem: IInterpreterQuickPickItem = {
             description: '', detail: '', label: '',
-            path: 'This is the selected Python path'
+            path: 'This is the selected Python path',
+            // tslint:disable-next-line: no-any
+            interpreter: {} as any
         };
 
         workspace.setup(w => w.workspaceFolders).returns(() => undefined);
@@ -166,7 +170,9 @@ suite('Interpreters - selector', () => {
         pythonSettings.setup(p => p.pythonPath).returns(() => 'python');
         const selectedItem: IInterpreterQuickPickItem = {
             description: '', detail: '', label: '',
-            path: 'This is the selected Python path'
+            path: 'This is the selected Python path',
+            // tslint:disable-next-line: no-any
+            interpreter: {} as any
         };
 
         const folder = { name: 'one', uri: Uri.parse('one'), index: 0 };
@@ -197,7 +203,9 @@ suite('Interpreters - selector', () => {
         pythonSettings.setup(p => p.pythonPath).returns(() => 'python');
         const selectedItem: IInterpreterQuickPickItem = {
             description: '', detail: '', label: '',
-            path: 'This is the selected Python path'
+            path: 'This is the selected Python path',
+            // tslint:disable-next-line: no-any
+            interpreter: {} as any
         };
 
         const folder1 = { name: 'one', uri: Uri.parse('one'), index: 1 };
@@ -232,7 +240,9 @@ suite('Interpreters - selector', () => {
 
         const selectedItem: IInterpreterQuickPickItem = {
             description: '', detail: '', label: '',
-            path: 'This is the selected Python path'
+            path: 'This is the selected Python path',
+            // tslint:disable-next-line: no-any
+            interpreter: {} as any
         };
 
         const folder1 = { name: 'one', uri: Uri.parse('one'), index: 1 };
diff --git a/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts b/src/test/datascience/jupyter/kernels/kernelService.unit.test.ts
diff --git a/src/test/datascience/mockJupyterManager.ts b/src/test/datascience/mockJupyterManager.ts

Original file line number	Diff line number	Diff line change
`@@ -86,7 +86,7 @@ export class JupyterSessionManager implements IJupyterSessionManager {`
`86`	`86`	`return session;`
`87`	`87`	`}`
`88`	`88`
`89`		`- public async getActiveKernelSpecs(): Promise<IJupyterKernelSpec[]> {`
	`89`	`+ public async getKernelSpecs(): Promise<IJupyterKernelSpec[]> {`
`90`	`90`	`if (!this.connInfo \|\| !this.sessionManager \|\| !this.contentsManager) {`
`91`	`91`	`throw new Error(localize.DataScience.sessionDisposed());`
`92`	`92`	`}`
Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,7 @@ export class GuestJupyterSessionManager implements IJupyterSessionManager {`
`17`	`17`	`return this.realSessionManager.startNew(kernelSpec, cancelToken);`
`18`	`18`	`}`
`19`	`19`
`20`		`- public async getActiveKernelSpecs(): Promise<IJupyterKernelSpec[]> {`
	`20`	`+ public async getKernelSpecs(): Promise<IJupyterKernelSpec[]> {`
`21`	`21`	`// Don't return any kernel specs in guest mode. They're only needed for the host side`
`22`	`22`	`return Promise.resolve([]);`
`23`	`23`	`}`
Original file line number	Diff line number	Diff line change
`@@ -182,7 +182,7 @@ export interface IJupyterSessionManagerFactory {`
`182`	`182`
`183`	`183`	`export interface IJupyterSessionManager extends IAsyncDisposable {`
`184`	`184`	`startNew(kernelSpec: IJupyterKernelSpec \| undefined, cancelToken?: CancellationToken): Promise<IJupyterSession>;`
`185`		`- getActiveKernelSpecs(): Promise<IJupyterKernelSpec[]>;`
	`185`	`+ getKernelSpecs(): Promise<IJupyterKernelSpec[]>;`
`186`	`186`	`getConnInfo(): IConnection;`
`187`	`187`	`getRunningKernels(): Promise<IJupyterKernel[]>;`
`188`	`188`	`}`
Original file line number	Diff line number	Diff line change
`@@ -42,7 +42,8 @@ export class InterpreterSelector implements IInterpreterSelector {`
`42`	`42`	`// tslint:disable-next-line:no-non-null-assertion`
`43`	`43`	`label: suggestion.displayName!,`
`44`	`44`	detail: `${cachedPrefix}${detail}`,
`45`		`- path: suggestion.path`
	`45`	`+ path: suggestion.path,`
	`46`	`+ interpreter: suggestion`
`46`	`47`	`};`
`47`	`48`	`}`
`48`	`49`