Port ipykernel install fix to release (#13975)

* Fix installing ipykernel into interpreters for raw kernel (#13959) * update news Co-authored-by: Ian Huff <ianhuff@CEIDCCEVIPSVC01.redmond.corp.microsoft.com>
microsoft · Sep 17, 2020 · 8c44490 · 8c44490
1 parent 411aae9
commit 8c44490
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 44 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -57,6 +57,8 @@
    ([#13612](https://github.com/Microsoft/vscode-python/issues/13612))
 1. Fix the behavior of the 'python.showStartPage' setting.
    ([#13706](https://github.com/Microsoft/vscode-python/issues/13706))
+1. Correctly install ipykernel when launching from an interpreter.
+   ([#13956](https://github.com/Microsoft/vscode-python/issues/13956))
 
 ### Code Health
 

diff --git a/package.nls.json b/package.nls.json
@@ -595,5 +595,6 @@
     "DataScience.interactiveWindowModeBannerTitle": "Do you want to open a new Python Interactive window for this file? [More Information](command:workbench.action.openSettings?%5B%22python.dataScience.interactiveWindowMode%22%5D).",
     "DataScience.interactiveWindowModeBannerSwitchYes": "Yes",
     "DataScience.interactiveWindowModeBannerSwitchAlways": "Always",
-    "DataScience.interactiveWindowModeBannerSwitchNo": "No"
+    "DataScience.interactiveWindowModeBannerSwitchNo": "No",
+    "DataScience.ipykernelNotInstalled": "IPyKernel not installed into interpreter {0}"
 }
diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts
@@ -1116,6 +1116,10 @@ export namespace DataScience {
     );
     export const connected = localize('DataScience.connected', 'Connected');
     export const disconnected = localize('DataScience.disconnected', 'Disconnected');
+    export const ipykernelNotInstalled = localize(
+        'DataScience.ipykernelNotInstalled',
+        'IPyKernel not installed into interpreter {0}'
+    );
 }
 
 export namespace StartPage {

diff --git a/src/client/datascience/jupyter/kernels/kernelSelector.ts b/src/client/datascience/jupyter/kernels/kernelSelector.ts
@@ -27,7 +27,8 @@ import {
     IJupyterSessionManagerFactory,
     IKernelDependencyService,
     INotebookMetadataLive,
-    INotebookProviderConnection
+    INotebookProviderConnection,
+    KernelInterpreterDependencyResponse
 } from '../../types';
 import { createDefaultKernelSpec, getDisplayNameOrNameOfKernelConnection } from './helpers';
 import { KernelSelectionProvider } from './kernelSelections';
@@ -504,6 +505,8 @@ export class KernelSelector implements IKernelSelectionUsage {
         if (!kernelSpec && !activeInterpreter) {
             return;
         } else if (!kernelSpec && activeInterpreter) {
+            await this.installDependenciesIntoInterpreter(activeInterpreter, ignoreDependencyCheck, cancelToken);
+
             // Return current interpreter.
             return {
                 kind: 'startUsingPythonInterpreter',
@@ -512,6 +515,11 @@ export class KernelSelector implements IKernelSelectionUsage {
         } else if (kernelSpec) {
             // Locate the interpreter that matches our kernelspec
             const interpreter = await this.kernelService.findMatchingInterpreter(kernelSpec, cancelToken);
+
+            if (interpreter) {
+                await this.installDependenciesIntoInterpreter(interpreter, ignoreDependencyCheck, cancelToken);
+            }
+
             return { kind: 'startUsingKernelSpec', kernelSpec, interpreter };
         }
     }
@@ -545,6 +553,25 @@ export class KernelSelector implements IKernelSelectionUsage {
         return { kernelSpec, interpreter, kind: 'startUsingPythonInterpreter' };
     }
 
+    // If we need to install our dependencies now (for non-native scenarios)
+    // then install ipykernel into the interpreter or throw error
+    private async installDependenciesIntoInterpreter(
+        interpreter: PythonEnvironment,
+        ignoreDependencyCheck?: boolean,
+        cancelToken?: CancellationToken
+    ) {
+        if (!ignoreDependencyCheck) {
+            if (
+                (await this.kernelDependencyService.installMissingDependencies(interpreter, cancelToken)) !==
+                KernelInterpreterDependencyResponse.ok
+            ) {
+                throw new Error(
+                    localize.DataScience.ipykernelNotInstalled().format(interpreter.displayName || interpreter.path)
+                );
+            }
+        }
+    }
+
     /**
      * 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
@@ -5,13 +5,12 @@
 import type { nbformat } from '@jupyterlab/coreutils';
 import { inject, injectable, named } from 'inversify';
 import * as path from 'path';
-import { CancellationToken, CancellationTokenSource } from 'vscode';
+import { CancellationToken } from 'vscode';
 import { IWorkspaceService } from '../../common/application/types';
-import { wrapCancellationTokens } from '../../common/cancellation';
 import { traceError, traceInfo } from '../../common/logger';
 import { IPlatformService } from '../../common/platform/types';
 import { IPythonExecutionFactory } from '../../common/process/types';
-import { IExtensionContext, IInstaller, InstallerResponse, IPathUtils, Product, Resource } from '../../common/types';
+import { IExtensionContext, IPathUtils, Resource } from '../../common/types';
 import { IEnvironmentVariablesProvider } from '../../common/variables/types';
 import { IInterpreterLocatorService, IInterpreterService, KNOWN_PATH_SERVICE } from '../../interpreter/contracts';
 import { captureTelemetry } from '../../telemetry';
@@ -20,7 +19,6 @@ import { Telemetry } from '../constants';
 import { defaultKernelSpecName } from '../jupyter/kernels/helpers';
 import { JupyterKernelSpec } from '../jupyter/kernels/jupyterKernelSpec';
 import { IDataScienceFileSystem, IJupyterKernelSpec } from '../types';
-import { getKernelInterpreter } from './helpers';
 import { IKernelFinder } from './types';
 // tslint:disable-next-line:no-require-imports no-var-requires
 const flatten = require('lodash/flatten') as typeof import('lodash/flatten');
@@ -56,7 +54,6 @@ export class KernelFinder implements IKernelFinder {
         @inject(IPlatformService) private platformService: IPlatformService,
         @inject(IDataScienceFileSystem) private fs: IDataScienceFileSystem,
         @inject(IPathUtils) private readonly pathUtils: IPathUtils,
-        @inject(IInstaller) private installer: IInstaller,
         @inject(IExtensionContext) private readonly context: IExtensionContext,
         @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
         @inject(IPythonExecutionFactory) private readonly exeFactory: IPythonExecutionFactory,
@@ -65,9 +62,7 @@ export class KernelFinder implements IKernelFinder {
     @captureTelemetry(Telemetry.KernelFinderPerf)
     public async findKernelSpec(
         resource: Resource,
-        kernelSpecMetadata?: nbformat.IKernelspecMetadata,
-        cancelToken?: CancellationToken,
-        ignoreDependencyCheck?: boolean
+        kernelSpecMetadata?: nbformat.IKernelspecMetadata
     ): Promise<IJupyterKernelSpec | undefined> {
         await this.readCache();
         let foundKernel: IJupyterKernelSpec | undefined;
@@ -108,8 +103,7 @@ export class KernelFinder implements IKernelFinder {
 
         this.writeCache().ignoreErrors();
 
-        // Verify that ipykernel is installed into the given kernelspec interpreter
-        return ignoreDependencyCheck || !foundKernel ? foundKernel : this.verifyIpyKernel(foundKernel, cancelToken);
+        return foundKernel;
     }
 
     // Search all our local file system locations for installed kernel specs and return them
@@ -318,30 +312,6 @@ export class KernelFinder implements IKernelFinder {
         return flatten(fullPathResults);
     }
 
-    // For the given kernelspec return back the kernelspec with ipykernel installed into it or error
-    private async verifyIpyKernel(
-        kernelSpec: IJupyterKernelSpec,
-        cancelToken?: CancellationToken
-    ): Promise<IJupyterKernelSpec> {
-        const interpreter = await getKernelInterpreter(kernelSpec, this.interpreterService);
-
-        if (await this.installer.isInstalled(Product.ipykernel, interpreter)) {
-            return kernelSpec;
-        } else {
-            const token = new CancellationTokenSource();
-            const response = await this.installer.promptToInstall(
-                Product.ipykernel,
-                interpreter,
-                wrapCancellationTokens(cancelToken, token.token)
-            );
-            if (response === InstallerResponse.Installed) {
-                return kernelSpec;
-            }
-        }
-
-        throw new Error(`IPyKernel not installed into interpreter ${interpreter.displayName}`);
-    }
-
     private async getKernelSpecFromActiveInterpreter(
         kernelName: string,
         resource: Resource

diff --git a/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts b/src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts
@@ -31,7 +31,7 @@ import {
     LiveKernelModel
 } from '../../../../client/datascience/jupyter/kernels/types';
 import { IKernelFinder } from '../../../../client/datascience/kernel-launcher/types';
-import { IJupyterSessionManager } from '../../../../client/datascience/types';
+import { IJupyterSessionManager, KernelInterpreterDependencyResponse } from '../../../../client/datascience/types';
 import { IInterpreterService } from '../../../../client/interpreter/contracts';
 import { InterpreterService } from '../../../../client/interpreter/interpreterService';
 import { EnvironmentType, PythonEnvironment } from '../../../../client/pythonEnvironments/info';
@@ -72,6 +72,9 @@ suite('DataScience - KernelSelector', () => {
         kernelSelectionProvider = mock(KernelSelectionProvider);
         appShell = mock(ApplicationShell);
         dependencyService = mock(KernelDependencyService);
+        when(dependencyService.installMissingDependencies(anything(), anything())).thenResolve(
+            KernelInterpreterDependencyResponse.ok
+        );
         interpreterService = mock(InterpreterService);
         kernelFinder = mock<IKernelFinder>();
         const jupyterSessionManagerFactory = mock(JupyterSessionManagerFactory);

diff --git a/src/test/datascience/kernelFinder.unit.test.ts b/src/test/datascience/kernelFinder.unit.test.ts
@@ -11,7 +11,7 @@ import { Uri } from 'vscode';
 import { IWorkspaceService } from '../../client/common/application/types';
 import { IPlatformService } from '../../client/common/platform/types';
 import { PythonExecutionFactory } from '../../client/common/process/pythonExecutionFactory';
-import { IExtensionContext, IInstaller, IPathUtils, Resource } from '../../client/common/types';
+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';
@@ -30,7 +30,6 @@ suite('Kernel Finder', () => {
     let pathUtils: typemoq.IMock<IPathUtils>;
     let context: typemoq.IMock<IExtensionContext>;
     let envVarsProvider: typemoq.IMock<IEnvironmentVariablesProvider>;
-    let installer: IInstaller;
     let workspaceService: IWorkspaceService;
     let kernelFinder: IKernelFinder;
     let activeInterpreter: PythonEnvironment;
@@ -83,9 +82,6 @@ suite('Kernel Finder', () => {
         context.setup((c) => c.globalStoragePath).returns(() => './');
         fileSystem = typemoq.Mock.ofType<IDataScienceFileSystem>();
 
-        installer = mock<IInstaller>();
-        when(installer.isInstalled(anything(), anything())).thenResolve(true);
-
         platformService = typemoq.Mock.ofType<IPlatformService>();
         platformService.setup((ps) => ps.isWindows).returns(() => true);
         platformService.setup((ps) => ps.isMac).returns(() => true);
@@ -325,7 +321,6 @@ suite('Kernel Finder', () => {
                 platformService.object,
                 fileSystem.object,
                 pathUtils.object,
-                instance(installer),
                 context.object,
                 instance(workspaceService),
                 instance(executionFactory),
@@ -408,7 +403,6 @@ suite('Kernel Finder', () => {
                 platformService.object,
                 fileSystem.object,
                 pathUtils.object,
-                instance(installer),
                 context.object,
                 instance(workspaceService),
                 instance(executionFactory),