Skip to content

Commit 641bece

Browse files
IanMatthewHuffIanMatthewHuff
authored
Don't register kernel spec on raw kernel switch to interpreter (#11676)
1 parent 4b06a77 commit 641bece

10 files changed

Lines changed: 163 additions & 65 deletions

File tree

‎news/2 Fixes/11575.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Don't register a kernelspec when switching to an interpreter in raw kernel mode.

‎news/2 Fixes/11672.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
For direct kernel connection, don't replace a notebook's metadata default kernelspec with a new kernelspec on startup.

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved.
2+
// Licensed under the MIT License.
3+
'use strict';
4+
5+
import type { Kernel } from '@jupyterlab/services';
6+
import { IJupyterKernelSpec } from '../../types';
7+
import { JupyterKernelSpec } from './jupyterKernelSpec';
8+
9+
// Helper functions for dealing with kernels and kernelspecs
10+
11+
export const defaultKernelSpecName = 'python_defaultSpec_';
12+
13+
// https://jupyter-client.readthedocs.io/en/stable/kernels.html
14+
const connectionFilePlaceholder = '{connection_file}';
15+
16+
// Find the index of the connection file placeholder in a kernelspec
17+
export function findIndexOfConnectionFile(kernelSpec: Readonly<IJupyterKernelSpec>): number {
18+
return kernelSpec.argv.indexOf(connectionFilePlaceholder);
19+
}
20+
export function createDefaultKernelSpec(displayName?: string): IJupyterKernelSpec {
21+
// This creates a default kernel spec. When launched, 'python' argument will map to using the interpreter
22+
// associated with the current resource for launching.
23+
const defaultSpec: Kernel.ISpecModel = {
24+
name: defaultKernelSpecName + Date.now().toString(),
25+
language: 'python',
26+
display_name: displayName || 'Python 3',
27+
metadata: {},
28+
argv: ['python', '-m', 'ipykernel_launcher', '-f', connectionFilePlaceholder],
29+
env: {},
30+
resources: {}
31+
};
32+
33+
return new JupyterKernelSpec(defaultSpec);
34+
}

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import { IKernelFinder } from '../../kernel-launcher/types';
2020
import { reportAction } from '../../progress/decorator';
2121
import { ReportableAction } from '../../progress/types';
2222
import { IJupyterKernelSpec, IJupyterSessionManager, IKernelDependencyService } from '../../types';
23+
import { createDefaultKernelSpec } from './helpers';
2324
import { KernelSelectionProvider } from './kernelSelections';
2425
import { KernelService } from './kernelService';
2526
import { IKernelSpecQuickPickItem, LiveKernelModel } from './types';
@@ -358,7 +359,7 @@ export class KernelSelector {
358359
// First use our kernel finder to locate a kernelspec on disk
359360
selection.kernelSpec = await this.kernelFinder.findKernelSpec(
360361
resource,
361-
notebookMetadata?.kernelspec?.name,
362+
notebookMetadata?.kernelspec,
362363
cancelToken
363364
);
364365

@@ -388,7 +389,7 @@ export class KernelSelector {
388389
return {};
389390
}
390391
// Check if ipykernel is installed in this kernel.
391-
if (selection.selection.interpreter) {
392+
if (selection.selection.interpreter && type === 'jupyter') {
392393
sendTelemetryEvent(Telemetry.SwitchToInterpreterAsKernel);
393394
return this.useInterpreterAsKernel(
394395
resource,
@@ -399,6 +400,8 @@ export class KernelSelector {
399400
false,
400401
cancelToken
401402
);
403+
} else if (selection.selection.interpreter && type === 'raw') {
404+
return this.useInterpreterAndDefaultKernel(selection.selection.interpreter);
402405
} else if (selection.selection.kernelModel) {
403406
sendTelemetryEvent(Telemetry.SwitchToExistingKernel, undefined, {
404407
language: this.computeLanguage(selection.selection.kernelModel.language)
@@ -425,6 +428,13 @@ export class KernelSelector {
425428
return {};
426429
}
427430
}
431+
432+
// When switching to an interpreter in raw kernel mode then just create a default kernelspec for that interpreter to use
433+
private async useInterpreterAndDefaultKernel(interpreter: PythonInterpreter): Promise<KernelSpecInterpreter> {
434+
const kernelSpec = createDefaultKernelSpec(interpreter.displayName);
435+
return { kernelSpec, interpreter };
436+
}
437+
428438
/**
429439
* Use the provided interpreter as a kernel.
430440
* If `displayNameOfKernelNotFound` is provided, then display a message indicating we're using the `current interpreter`.

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

Lines changed: 44 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// Licensed under the MIT License.
33
'use strict';
44

5-
import { Kernel } from '@jupyterlab/services';
5+
import type { nbformat } from '@jupyterlab/coreutils';
66
import { inject, injectable, named } from 'inversify';
77
import * as path from 'path';
88
import { CancellationToken, CancellationTokenSource } from 'vscode';
@@ -14,6 +14,7 @@ import { IExtensionContext, IInstaller, InstallerResponse, IPathUtils, Product,
1414
import { IInterpreterLocatorService, IInterpreterService, KNOWN_PATH_SERVICE } from '../../interpreter/contracts';
1515
import { captureTelemetry } from '../../telemetry';
1616
import { Telemetry } from '../constants';
17+
import { createDefaultKernelSpec, defaultKernelSpecName } from '../jupyter/kernels/helpers';
1718
import { JupyterKernelSpec } from '../jupyter/kernels/jupyterKernelSpec';
1819
import { IJupyterKernelSpec } from '../types';
1920
import { getKernelInterpreter } from './helpers';
@@ -27,14 +28,6 @@ const macJupyterPath = path.join('Library', 'Jupyter', 'kernels');
2728
const baseKernelPath = path.join('share', 'jupyter', 'kernels');
2829

2930
const cacheFile = 'kernelSpecPathCache.json';
30-
const defaultSpecName = 'python_defaultSpec_';
31-
32-
// https://jupyter-client.readthedocs.io/en/stable/kernels.html
33-
const connectionFilePlaceholder = '{connection_file}';
34-
35-
export function findIndexOfConnectionFile(kernelSpec: Readonly<IJupyterKernelSpec>): number {
36-
return kernelSpec.argv.indexOf(connectionFilePlaceholder);
37-
}
3831

3932
// This class searches for a kernel that matches the given kernel name.
4033
// First it searches on a global persistent state, then on the installed python interpreters,
@@ -68,40 +61,49 @@ export class KernelFinder implements IKernelFinder {
6861
@captureTelemetry(Telemetry.KernelFinderPerf)
6962
public async findKernelSpec(
7063
resource: Resource,
71-
kernelName?: string,
64+
kernelSpecMetadata?: nbformat.IKernelspecMetadata,
7265
cancelToken?: CancellationToken
7366
): Promise<IJupyterKernelSpec> {
7467
this.cache = await this.readCache();
7568
let foundKernel: IJupyterKernelSpec | undefined;
7669

77-
if (kernelName && !kernelName.includes(defaultSpecName)) {
78-
let kernelSpec = await this.searchCache(kernelName);
70+
const kernelName = kernelSpecMetadata?.name;
7971

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

84-
// Check in active interpreter first
85-
kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource);
77+
if (kernelSpec) {
78+
return kernelSpec;
79+
}
8680

87-
if (kernelSpec) {
88-
this.writeCache(this.cache).ignoreErrors();
89-
return kernelSpec;
90-
}
81+
// Check in active interpreter first
82+
kernelSpec = await this.getKernelSpecFromActiveInterpreter(kernelName, resource);
9183

92-
const diskSearch = this.findDiskPath(kernelName);
93-
const interpreterSearch = this.getInterpreterPaths(resource).then((interpreterPaths) => {
94-
return this.findInterpreterPath(interpreterPaths, kernelName);
95-
});
84+
if (kernelSpec) {
85+
this.writeCache(this.cache).ignoreErrors();
86+
return kernelSpec;
87+
}
9688

97-
let result = await Promise.race([diskSearch, interpreterSearch]);
98-
if (!result) {
99-
const both = await Promise.all([diskSearch, interpreterSearch]);
100-
result = both[0] ? both[0] : both[1];
101-
}
89+
const diskSearch = this.findDiskPath(kernelName);
90+
const interpreterSearch = this.getInterpreterPaths(resource).then((interpreterPaths) => {
91+
return this.findInterpreterPath(interpreterPaths, kernelName);
92+
});
93+
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+
}
10299

103-
foundKernel = result ? result : await this.getDefaultKernelSpec(resource);
100+
foundKernel = result ? result : await this.getDefaultKernelSpec(resource);
101+
} else {
102+
// For a previous default kernel spec, just use it again
103+
foundKernel = this.reuseExistingDefaultSpec(kernelSpecMetadata);
104+
}
104105
} else {
106+
// If we don't have kernel metadata then just get a default spec to use
105107
foundKernel = await this.getDefaultKernelSpec(resource);
106108
}
107109

@@ -132,6 +134,10 @@ export class KernelFinder implements IKernelFinder {
132134
return this.workspaceToKernels.get(workspaceFolderId)!;
133135
}
134136

137+
private reuseExistingDefaultSpec(kernelMetadata: nbformat.IKernelspecMetadata): IJupyterKernelSpec {
138+
return createDefaultKernelSpec(kernelMetadata.display_name);
139+
}
140+
135141
private async findResourceKernelSpecs(resource: Resource): Promise<IJupyterKernelSpec[]> {
136142
const results: IJupyterKernelSpec[] = [];
137143

@@ -184,7 +190,11 @@ export class KernelFinder implements IKernelFinder {
184190
traceError(`Failed to parse kernelspec ${specPath}`);
185191
return undefined;
186192
}
187-
return new JupyterKernelSpec(kernelJson, specPath);
193+
const kernelSpec: IJupyterKernelSpec = new JupyterKernelSpec(kernelJson, specPath);
194+
195+
// Some registered kernel specs do not have a name, in this case use the last part of the path
196+
kernelSpec.name = kernelJson?.name || path.basename(path.dirname(specPath));
197+
return kernelSpec;
188198
}
189199

190200
// For the given resource, find atll the file paths for kernel specs that wewant to associate with this
@@ -319,18 +329,7 @@ export class KernelFinder implements IKernelFinder {
319329
private async getDefaultKernelSpec(resource: Resource): Promise<IJupyterKernelSpec> {
320330
const activeInterpreter = await this.interpreterService.getActiveInterpreter(resource);
321331

322-
// This creates a default kernel spec. When launched, 'python' argument will map to using the interpreter
323-
// associated with the current resource for launching.
324-
const defaultSpec: Kernel.ISpecModel = {
325-
name: defaultSpecName + Date.now().toString(),
326-
language: 'python',
327-
display_name: activeInterpreter?.displayName ? activeInterpreter.displayName : 'Python 3',
328-
metadata: {},
329-
argv: ['python', '-m', 'ipykernel_launcher', '-f', connectionFilePlaceholder],
330-
env: {},
331-
resources: {}
332-
};
333-
return new JupyterKernelSpec(defaultSpec);
332+
return createDefaultKernelSpec(activeInterpreter?.displayName);
334333
}
335334

336335
private async readCache(): Promise<string[]> {
@@ -369,12 +368,7 @@ export class KernelFinder implements IKernelFinder {
369368
});
370369

371370
if (kernelJsonFile) {
372-
const spec = await this.getKernelSpec(kernelJsonFile);
373-
374-
if (spec) {
375-
spec.name = kernelName;
376-
return spec;
377-
}
371+
return this.getKernelSpec(kernelJsonFile);
378372
}
379373

380374
return undefined;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ import { noop, swallowExceptions } from '../../common/utils/misc';
1414
import { PythonInterpreter } from '../../interpreter/contracts';
1515
import { captureTelemetry } from '../../telemetry';
1616
import { Telemetry } from '../constants';
17+
import { findIndexOfConnectionFile } from '../jupyter/kernels/helpers';
1718
import { IJupyterKernelSpec } from '../types';
18-
import { findIndexOfConnectionFile } from './kernelFinder';
1919
import { PythonKernelLauncherDaemon } from './kernelLauncherDaemon';
2020
import { IKernelConnection, IKernelProcess, IPythonKernelDaemon, PythonKernelDiedError } from './types';
2121

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Licensed under the MIT License.
33
'use strict';
44

5+
import type { nbformat } from '@jupyterlab/coreutils';
56
import { SpawnOptions } from 'child_process';
67
import { CancellationToken, Event } from 'vscode';
78
import { InterpreterUri } from '../../common/installer/types';
@@ -46,7 +47,7 @@ export const IKernelFinder = Symbol('IKernelFinder');
4647
export interface IKernelFinder {
4748
findKernelSpec(
4849
interpreterUri: InterpreterUri,
49-
kernelName?: string,
50+
kernelSpecMetadata?: nbformat.IKernelspecMetadata,
5051
cancelToken?: CancellationToken
5152
): Promise<IJupyterKernelSpec>;
5253
listKernelSpecs(resource: Resource): Promise<IJupyterKernelSpec[]>;

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

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33
import { nbformat } from '@jupyterlab/coreutils';
4-
import { assert } from 'chai';
4+
import { assert, expect } from 'chai';
55
import * as sinon from 'sinon';
66
import { anything, capture, instance, mock, verify, when } from 'ts-mockito';
77
import { CancellationToken } from 'vscode-jsonrpc';
@@ -15,6 +15,7 @@ import { noop } from '../../../../client/common/utils/misc';
1515
import { Architecture } from '../../../../client/common/utils/platform';
1616
import { StopWatch } from '../../../../client/common/utils/stopWatch';
1717
import { JupyterSessionManager } from '../../../../client/datascience/jupyter/jupyterSessionManager';
18+
import { defaultKernelSpecName } from '../../../../client/datascience/jupyter/kernels/helpers';
1819
import { KernelDependencyService } from '../../../../client/datascience/jupyter/kernels/kernelDependencyService';
1920
import { KernelSelectionProvider } from '../../../../client/datascience/jupyter/kernels/kernelSelections';
2021
import { KernelSelector } from '../../../../client/datascience/jupyter/kernels/kernelSelector';
@@ -25,7 +26,7 @@ import { IJupyterKernelSpec, IJupyterSessionManager } from '../../../../client/d
2526
import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../../../client/interpreter/contracts';
2627
import { InterpreterService } from '../../../../client/interpreter/interpreterService';
2728

28-
// tslint:disable: max-func-body-length
29+
// tslint:disable: max-func-body-length no-unused-expression
2930

3031
suite('Data Science - KernelSelector', () => {
3132
let kernelSelectionProvider: KernelSelectionProvider;
@@ -504,6 +505,36 @@ suite('Data Science - KernelSelector', () => {
504505
appShell.showInformationMessage(localize.DataScience.fallBackToRegisterAndUseActiveInterpeterAsKernel())
505506
).never();
506507
});
508+
test('For a raw connection, if an interpreter is selected return it along with a default kernelspec', async () => {
509+
when(dependencyService.areDependenciesInstalled(interpreter, anything())).thenResolve(true);
510+
when(
511+
kernelSelectionProvider.getKernelSelectionsForLocalSession(anything(), 'raw', anything(), anything())
512+
).thenResolve([]);
513+
when(appShell.showQuickPick(anything(), anything(), anything())).thenResolve({
514+
selection: { interpreter, kernelSpec: undefined }
515+
// tslint:disable-next-line: no-any
516+
} as any);
517+
518+
const kernel = await kernelSelector.selectLocalKernel(undefined, 'raw', new StopWatch());
519+
520+
assert.isOk(kernel.interpreter === interpreter);
521+
expect(kernel.kernelSpec, 'Should have kernelspec').to.not.be.undefined;
522+
expect(kernel.kernelSpec!.name, 'Spec should have default name').to.include(defaultKernelSpecName);
523+
});
524+
test('For a raw connection, if a kernel spec is selected return it with the interpreter', async () => {
525+
when(dependencyService.areDependenciesInstalled(interpreter, anything())).thenResolve(true);
526+
when(kernelService.findMatchingInterpreter(kernelSpec, anything())).thenResolve(interpreter);
527+
when(
528+
kernelSelectionProvider.getKernelSelectionsForLocalSession(anything(), 'raw', anything(), anything())
529+
).thenResolve([]);
530+
when(appShell.showQuickPick(anything(), anything(), anything())).thenResolve({
531+
selection: { interpreter: undefined, kernelSpec }
532+
// tslint:disable-next-line: no-any
533+
} as any);
534+
const kernel = await kernelSelector.selectLocalKernel(undefined, 'raw', new StopWatch());
535+
expect(kernel.kernelSpec).to.equal(kernelSpec);
536+
expect(kernel.interpreter).to.equal(interpreter);
537+
});
507538
});
508539
// tslint:disable-next-line: max-func-body-length
509540
suite('Get a kernel for local sessions', () => {

0 commit comments

Comments
 (0)