Skip to content

Commit 8022bfe

Browse files
DonJayamanneDonJayamanne
authored
Select kernel based on metadata in notebook (#14217) (#14234)
* Fix picking kernel based on metadata
1 parent f4762cb commit 8022bfe

File tree

9 files changed

+52
-74
lines changed

9 files changed

+52
-74
lines changed

‎news/2 Fixes/14213.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Use the kernel defined in the metadata of Notebook instead of using the default workspace interpreter.

‎src/client/datascience/jupyter/kernels/helpers.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,6 @@ import {
2525

2626
// Helper functions for dealing with kernels and kernelspecs
2727

28-
export const defaultKernelSpecName = 'python_defaultSpec_';
29-
3028
// https://jupyter-client.readthedocs.io/en/stable/kernels.html
3129
const connectionFilePlaceholder = '{connection_file}';
3230

@@ -136,13 +134,13 @@ export function getKernelConnectionLanguage(kernelConnection?: KernelConnectionM
136134
return model?.language || kernelSpec?.language;
137135
}
138136
// Create a default kernelspec with the given display name
139-
export function createDefaultKernelSpec(displayName?: string): IJupyterKernelSpec {
137+
export function createDefaultKernelSpec(interpreter?: PythonEnvironment): IJupyterKernelSpec {
140138
// This creates a default kernel spec. When launched, 'python' argument will map to using the interpreter
141139
// associated with the current resource for launching.
142140
const defaultSpec: Kernel.ISpecModel = {
143-
name: defaultKernelSpecName + Date.now().toString(),
141+
name: interpreter?.displayName || 'Python 3',
144142
language: 'python',
145-
display_name: displayName || 'Python 3',
143+
display_name: interpreter?.displayName || 'Python 3',
146144
metadata: {},
147145
argv: ['python', '-m', 'ipykernel_launcher', '-f', connectionFilePlaceholder],
148146
env: {},

‎src/client/datascience/jupyter/kernels/kernelSelector.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -549,7 +549,7 @@ export class KernelSelector implements IKernelSelectionUsage {
549549

550550
// When switching to an interpreter in raw kernel mode then just create a default kernelspec for that interpreter to use
551551
private async useInterpreterAndDefaultKernel(interpreter: PythonEnvironment): Promise<KernelConnectionMetadata> {
552-
const kernelSpec = createDefaultKernelSpec(interpreter.displayName);
552+
const kernelSpec = createDefaultKernelSpec(interpreter);
553553
return { kernelSpec, interpreter, kind: 'startUsingPythonInterpreter' };
554554
}
555555

‎src/client/datascience/jupyter/kernels/kernelSwitcher.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,10 @@ export class KernelSwitcher {
9090
const kernelSpec = kernelConnectionMetadataHasKernelSpec(kernelConnection) ? kernelConnection : undefined;
9191
const kernelName = kernelSpec?.kernelSpec?.name || kernelModel?.kernelModel?.name;
9292
// One of them is bound to be non-empty.
93-
const displayName = kernelModel?.kernelModel?.display_name || kernelName || '';
93+
const displayName =
94+
kernelConnection.kind === 'startUsingPythonInterpreter'
95+
? kernelConnection.interpreter.displayName || kernelConnection.interpreter.path
96+
: kernelModel?.kernelModel?.display_name || kernelName || '';
9497
const options: ProgressOptions = {
9598
location: ProgressLocation.Notification,
9699
cancellable: false,

‎src/client/datascience/kernel-launcher/kernelFinder.ts

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import { IInterpreterLocatorService, IInterpreterService, KNOWN_PATH_SERVICE } f
1616
import { captureTelemetry } from '../../telemetry';
1717
import { getRealPath } from '../common';
1818
import { Telemetry } from '../constants';
19-
import { defaultKernelSpecName } from '../jupyter/kernels/helpers';
2019
import { JupyterKernelSpec } from '../jupyter/kernels/jupyterKernelSpec';
2120
import { IDataScienceFileSystem, IJupyterKernelSpec } from '../types';
2221
import { IKernelFinder } from './types';
@@ -70,35 +69,32 @@ export class KernelFinder implements IKernelFinder {
7069
const kernelName = kernelSpecMetadata?.name;
7170

7271
if (kernelSpecMetadata && kernelName) {
73-
// For a non default kernelspec search for it
74-
if (!kernelName.includes(defaultKernelSpecName)) {
75-
let kernelSpec = await this.searchCache(kernelName);
72+
let kernelSpec = await this.searchCache(kernelName);
7673

77-
if (kernelSpec) {
78-
return kernelSpec;
79-
}
74+
if (kernelSpec) {
75+
return kernelSpec;
76+
}
8077

81-
// Check in active interpreter first
82-
kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource);
78+
// Check in active interpreter first
79+
kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource);
8380

84-
if (kernelSpec) {
85-
this.writeCache().ignoreErrors();
86-
return kernelSpec;
87-
}
88-
89-
const diskSearch = this.findDiskPath(kernelName);
90-
const interpreterSearch = this.getInterpreterPaths(resource).then((interpreterPaths) => {
91-
return this.findInterpreterPath(interpreterPaths, kernelName);
92-
});
81+
if (kernelSpec) {
82+
this.writeCache().ignoreErrors();
83+
return kernelSpec;
84+
}
9385

94-
let result = await Promise.race([diskSearch, interpreterSearch]);
95-
if (!result) {
96-
const both = await Promise.all([diskSearch, interpreterSearch]);
97-
result = both[0] ? both[0] : both[1];
98-
}
86+
const diskSearch = this.findDiskPath(kernelName);
87+
const interpreterSearch = this.getInterpreterPaths(resource).then((interpreterPaths) => {
88+
return this.findInterpreterPath(interpreterPaths, kernelName);
89+
});
9990

100-
foundKernel = result;
91+
let result = await Promise.race([diskSearch, interpreterSearch]);
92+
if (!result) {
93+
const both = await Promise.all([diskSearch, interpreterSearch]);
94+
result = both[0] ? both[0] : both[1];
10195
}
96+
97+
foundKernel = result;
10298
}
10399

104100
this.writeCache().ignoreErrors();

‎src/client/datascience/kernel-launcher/kernelProcess.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,10 +151,7 @@ export class KernelProcess implements IKernelProcess {
151151
let kernelSpec = this._kernelConnectionMetadata.kernelSpec;
152152
// If there is no kernelspec & when launching a Python process, generate a dummy `kernelSpec`
153153
if (!kernelSpec && this._kernelConnectionMetadata.kind === 'startUsingPythonInterpreter') {
154-
kernelSpec = createDefaultKernelSpec(
155-
this._kernelConnectionMetadata.interpreter.displayName ||
156-
this._kernelConnectionMetadata.interpreter.path
157-
);
154+
kernelSpec = createDefaultKernelSpec(this._kernelConnectionMetadata.interpreter);
158155
}
159156
// We always expect a kernel spec.
160157
if (!kernelSpec) {

‎src/client/datascience/notebookStorage/baseModel.ts

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,22 @@ export function updateNotebookMetadata(
5656
kernelConnection && kernelConnectionMetadataHasKernelModel(kernelConnection)
5757
? kernelConnection.kernelModel
5858
: kernelConnection?.kernelSpec;
59-
if (kernelSpecOrModel && !metadata.kernelspec) {
59+
if (kernelConnection?.kind === 'startUsingPythonInterpreter') {
60+
// Store interpreter name, we expect the kernel finder will find the corresponding interpreter based on this name.
61+
const name = kernelConnection.interpreter.displayName || '';
62+
if (metadata.kernelspec?.name !== name || metadata.kernelspec?.display_name !== name) {
63+
changed = true;
64+
metadata.kernelspec = {
65+
name,
66+
display_name: name,
67+
metadata: {
68+
interpreter: {
69+
hash: sha256().update(kernelConnection.interpreter.path).digest('hex')
70+
}
71+
}
72+
};
73+
}
74+
} else if (kernelSpecOrModel && !metadata.kernelspec) {
6075
// Add a new spec in this case
6176
metadata.kernelspec = {
6277
name: kernelSpecOrModel.name || kernelSpecOrModel.display_name || '',
@@ -78,20 +93,12 @@ export function updateNotebookMetadata(
7893
metadata.kernelspec.display_name = displayName;
7994
kernelId = kernelSpecOrModel.id;
8095
}
81-
} else if (kernelConnection?.kind === 'startUsingPythonInterpreter') {
82-
// Store interpreter name, we expect the kernel finder will find the corresponding interpreter based on this name.
83-
const name = kernelConnection.interpreter.displayName || kernelConnection.interpreter.path;
84-
if (metadata.kernelspec?.name !== name || metadata.kernelspec?.display_name !== name) {
85-
changed = true;
86-
metadata.kernelspec = {
87-
name,
88-
display_name: name,
89-
metadata: {
90-
interpreter: {
91-
hash: sha256().update(kernelConnection.interpreter.path).digest('hex')
92-
}
93-
}
94-
};
96+
try {
97+
// This is set only for when we select an interpreter.
98+
// tslint:disable-next-line: no-any
99+
delete (metadata.kernelspec as any).metadata;
100+
} catch {
101+
// Noop.
95102
}
96103
}
97104
return { changed, kernelId };

‎src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import { Architecture } from '../../../../client/common/utils/platform';
1919
import { StopWatch } from '../../../../client/common/utils/stopWatch';
2020
import { JupyterSessionManager } from '../../../../client/datascience/jupyter/jupyterSessionManager';
2121
import { JupyterSessionManagerFactory } from '../../../../client/datascience/jupyter/jupyterSessionManagerFactory';
22-
import { defaultKernelSpecName } from '../../../../client/datascience/jupyter/kernels/helpers';
2322
import { KernelDependencyService } from '../../../../client/datascience/jupyter/kernels/kernelDependencyService';
2423
import { KernelSelectionProvider } from '../../../../client/datascience/jupyter/kernels/kernelSelections';
2524
import { KernelSelector } from '../../../../client/datascience/jupyter/kernels/kernelSelector';
@@ -453,9 +452,6 @@ suite('DataScience - KernelSelector', () => {
453452

454453
assert.deepEqual(kernel?.interpreter, interpreter);
455454
expect((kernel as any)?.kernelSpec, 'Should have kernelspec').to.not.be.undefined;
456-
expect((kernel as any)?.kernelSpec!.name, 'Spec should have default name').to.include(
457-
defaultKernelSpecName
458-
);
459455
});
460456
test('For a raw connection, if a kernel spec is selected return it with the interpreter', async () => {
461457
when(dependencyService.areDependenciesInstalled(interpreter, anything())).thenResolve(true);

‎src/test/datascience/kernelFinder.unit.test.ts

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import { PythonExecutionFactory } from '../../client/common/process/pythonExecut
1414
import { IExtensionContext, IPathUtils, Resource } from '../../client/common/types';
1515
import { Architecture } from '../../client/common/utils/platform';
1616
import { IEnvironmentVariablesProvider } from '../../client/common/variables/types';
17-
import { defaultKernelSpecName } from '../../client/datascience/jupyter/kernels/helpers';
1817
import { JupyterKernelSpec } from '../../client/datascience/jupyter/kernels/jupyterKernelSpec';
1918
import { KernelFinder } from '../../client/datascience/kernel-launcher/kernelFinder';
2019
import { IKernelFinder } from '../../client/datascience/kernel-launcher/types';
@@ -526,25 +525,6 @@ suite('Kernel Finder', () => {
526525
fileSystem.reset();
527526
});
528527

529-
test('Kernel metadata already has a default spec, and kernel spec not found, then return undefined', async () => {
530-
setupFileSystem();
531-
fileSystem
532-
.setup((fs) => fs.readLocalFile(typemoq.It.isAnyString()))
533-
.returns((pathParam: string) => {
534-
if (pathParam.includes(cacheFile)) {
535-
return Promise.resolve('[]');
536-
}
537-
return Promise.resolve('{}');
538-
});
539-
// get default kernel
540-
const spec = await kernelFinder.findKernelSpec(resource, {
541-
name: defaultKernelSpecName,
542-
display_name: 'TargetDisplayName'
543-
});
544-
assert.isUndefined(spec);
545-
fileSystem.reset();
546-
});
547-
548528
test('Look for KernelA with no cache, find KernelA and KenelB, then search for KernelB and find it in cache', async () => {
549529
setupFileSystem();
550530
fileSystem

0 commit comments

Comments
 (0)