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

Select kernel based on metadata in notebook #14217

Merged
merged 3 commits into from
Oct 2, 2020
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/14213.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use the kernel defined in the metadata of Notebook instead of using the default workspace interpreter.
8 changes: 3 additions & 5 deletions src/client/datascience/jupyter/kernels/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ import {

// Helper functions for dealing with kernels and kernelspecs

export const defaultKernelSpecName = 'python_defaultSpec_';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this, not required.


// https://jupyter-client.readthedocs.io/en/stable/kernels.html
const connectionFilePlaceholder = '{connection_file}';

Expand Down Expand Up @@ -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: {},
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/jupyter/kernels/kernelSelector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,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<KernelConnectionMetadata> {
const kernelSpec = createDefaultKernelSpec(interpreter.displayName);
const kernelSpec = createDefaultKernelSpec(interpreter);
return { kernelSpec, interpreter, kind: 'startUsingPythonInterpreter' };
}

Expand Down
5 changes: 4 additions & 1 deletion src/client/datascience/jupyter/kernels/kernelSwitcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
44 changes: 20 additions & 24 deletions src/client/datascience/kernel-launcher/kernelFinder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required, its not possible for us to get to this state.
Having the condition doesn't hurt, but its invalid code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not possible for the kernel spec to not start off as the default one? What if it comes from an external notebook?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method gets executed only in KernelSelector and kernel selector first searches for an interpreter based on metadata in notebook, if not found then it comes in here.
At that point, its impossible for the metadata to contain the name of a default kernel spec.

DefaultKernelSpec is something we generate, and that happens well after searching for a kernel.
Hence this code was not necessary.

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();
Expand Down
5 changes: 1 addition & 4 deletions src/client/datascience/kernel-launcher/kernelProcess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
37 changes: 22 additions & 15 deletions src/client/datascience/notebookStorage/baseModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,22 @@ export function updateNotebookMetadata(
kernelConnection && kernelConnectionMetadataHasKernelModel(kernelConnection)
? kernelConnection.kernelModel
: kernelConnection?.kernelSpec;
if (kernelSpecOrModel && !metadata.kernelspec) {
if (kernelConnection?.kind === 'startUsingPythonInterpreter') {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix.
We were doing this too late. This check needs to be done first.

// 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 || '',
Expand All @@ -91,20 +106,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 || '';
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 };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 0 additions & 20 deletions src/test/datascience/kernelFinder.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand Down