Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix installing ipykernel into interpreters for raw kernel #13959

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/13956.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Correctly install ipykernel when launching from an interpreter.
3 changes: 2 additions & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
}
4 changes: 4 additions & 0 deletions src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
29 changes: 28 additions & 1 deletion src/client/datascience/jupyter/kernels/kernelSelector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import {
IJupyterSessionManagerFactory,
IKernelDependencyService,
INotebookMetadataLive,
INotebookProviderConnection
INotebookProviderConnection,
KernelInterpreterDependencyResponse
} from '../../types';
import { createDefaultKernelSpec, getDisplayNameOrNameOfKernelConnection } from './helpers';
import { KernelSelectionProvider } from './kernelSelections';
Expand Down Expand Up @@ -504,6 +505,8 @@ export class KernelSelector implements IKernelSelectionUsage {
if (!kernelSpec && !activeInterpreter) {
return;
} else if (!kernelSpec && activeInterpreter) {
await this.installDependenciesIntoInterpreter(activeInterpreter, ignoreDependencyCheck, cancelToken);
IanMatthewHuff marked this conversation as resolved.
Show resolved Hide resolved

// Return current interpreter.
return {
kind: 'startUsingPythonInterpreter',
Expand All @@ -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 };
}
}
Expand Down Expand Up @@ -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) {
joyceerhl marked this conversation as resolved.
Show resolved Hide resolved
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`.
Expand Down
38 changes: 4 additions & 34 deletions src/client/datascience/kernel-launcher/kernelFinder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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');
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
IanMatthewHuff marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 1 addition & 7 deletions src/test/datascience/kernelFinder.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -325,7 +321,6 @@ suite('Kernel Finder', () => {
platformService.object,
fileSystem.object,
pathUtils.object,
instance(installer),
context.object,
instance(workspaceService),
instance(executionFactory),
Expand Down Expand Up @@ -408,7 +403,6 @@ suite('Kernel Finder', () => {
platformService.object,
fileSystem.object,
pathUtils.object,
instance(installer),
context.object,
instance(workspaceService),
instance(executionFactory),
Expand Down