Skip to content

Commit 0b586aa

Browse files
rchiodorchiodo
authored
Attempt 3 for fix interpreter mismatch in notebook (#11010)
* Attempt to fix interpreter mismatch in another branch * Add news entries * Use clone deep and assume it's a full interpreter * Actually fix the click on markdown
1 parent 8cd1485 commit 0b586aa

File tree

11 files changed

+73
-42
lines changed

11 files changed

+73
-42
lines changed

‎10926.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Make sure the same python is used for the data viewer as the notebook so that pandas can be found.

‎10953.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Make sure the interpreter in the notebook matches the kernel.

‎src/client/datascience/interactive-ipynb/nativeEditor.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,8 @@ import { nbformat } from '@jupyterlab/coreutils';
8888
// tslint:disable-next-line: no-require-imports
8989
import cloneDeep = require('lodash/cloneDeep');
9090
import { concatMultilineStringInput } from '../../../datascience-ui/common';
91+
import { ServerStatus } from '../../../datascience-ui/interactive-common/mainState';
92+
import { isTestExecution } from '../../common/constants';
9193
import { KernelSwitcher } from '../jupyter/kernels/kernelSwitcher';
9294

9395
const nativeEditorDir = path.join(EXTENSION_ROOT_DIR, 'out', 'datascience-ui', 'notebook');
@@ -671,10 +673,23 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
671673
sendTelemetryEvent(telemetryEvent);
672674
}
673675

674-
private loadCellsComplete() {
676+
private async loadCellsComplete() {
675677
if (!this.loadedAllCells) {
676678
this.loadedAllCells = true;
677679
sendTelemetryEvent(Telemetry.NotebookOpenTime, this.startupTimer.elapsedTime);
678680
}
681+
682+
// If we don't have a server right now, at least show our kernel name (this seems to slow down tests
683+
// too much though)
684+
if (!isTestExecution()) {
685+
const metadata = await this.getNotebookMetadata();
686+
if (!this.notebook && metadata?.kernelspec) {
687+
this.postMessage(InteractiveWindowMessages.UpdateKernel, {
688+
jupyterServerStatus: ServerStatus.NotStarted,
689+
localizedUri: '',
690+
displayName: metadata.kernelspec.display_name ?? metadata.kernelspec.name
691+
}).ignoreErrors();
692+
}
693+
}
679694
}
680695
}

‎src/client/datascience/jupyter/jupyterNotebook.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ export class JupyterNotebookBase implements INotebook {
171171
private sessionStatusChanged: Disposable | undefined;
172172
private initializedMatplotlib = false;
173173
private ioPubListeners = new Set<(msg: KernelMessage.IIOPubMessage, requestId: string) => Promise<void>>();
174+
private launchInfo: INotebookServerLaunchInfo;
174175
public get kernelSocket(): Observable<KernelSocketInformation | undefined> {
175176
return this.session.kernelSocket;
176177
}
@@ -181,7 +182,7 @@ export class JupyterNotebookBase implements INotebook {
181182
private configService: IConfigurationService,
182183
private disposableRegistry: IDisposableRegistry,
183184
private owner: INotebookServer,
184-
private launchInfo: INotebookServerLaunchInfo,
185+
_launchInfo: INotebookServerLaunchInfo,
185186
private loggers: INotebookExecutionLogger[],
186187
resource: Resource,
187188
identity: Uri,
@@ -200,8 +201,9 @@ export class JupyterNotebookBase implements INotebook {
200201
this.sessionStatusChanged = this.session.onSessionStatusChanged(statusChangeHandler);
201202
this._identity = identity;
202203
this._resource = resource;
203-
// Save our interpreter and don't change it. Later on when kernel changes
204-
// are possible, recompute it.
204+
205+
// Make a copy of the launch info so we can update it in this class
206+
this.launchInfo = cloneDeep(_launchInfo);
205207
}
206208
public get server(): INotebookServer {
207209
return this.owner;
@@ -596,15 +598,15 @@ export class JupyterNotebookBase implements INotebook {
596598
return this.launchInfo.interpreter;
597599
}
598600

599-
public setInterpreter(interpreter: PythonInterpreter) {
600-
this.launchInfo.interpreter = interpreter;
601-
}
602-
603601
public getKernelSpec(): IJupyterKernelSpec | LiveKernelModel | undefined {
604602
return this.launchInfo.kernelSpec;
605603
}
606604

607-
public async setKernelSpec(spec: IJupyterKernelSpec | LiveKernelModel, timeoutMS: number): Promise<void> {
605+
public async setKernelSpec(
606+
spec: IJupyterKernelSpec | LiveKernelModel,
607+
timeoutMS: number,
608+
interpreter: PythonInterpreter | undefined
609+
): Promise<void> {
608610
// We need to start a new session with the new kernel spec
609611
if (this.session) {
610612
// Turn off setup
@@ -625,6 +627,11 @@ export class JupyterNotebookBase implements INotebook {
625627
}
626628

627629
this.kernelChanged.fire(spec);
630+
631+
// If our new kernelspec has an interpreter, set that as our interpreter too
632+
if (interpreter) {
633+
this.launchInfo.interpreter = interpreter;
634+
}
628635
}
629636

630637
public getLoggers(): INotebookExecutionLogger[] {

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -127,12 +127,9 @@ export class KernelSwitcher {
127127
// Change the kernel. A status update should fire that changes our display
128128
await notebook.setKernelSpec(
129129
newKernel.kernelSpec || newKernel.kernelModel!,
130-
this.configService.getSettings(notebook.resource).datascience.jupyterLaunchTimeout
130+
this.configService.getSettings(notebook.resource).datascience.jupyterLaunchTimeout,
131+
newKernel.interpreter
131132
);
132-
133-
if (newKernel.interpreter) {
134-
notebook.setInterpreter(newKernel.interpreter);
135-
}
136133
};
137134

138135
const kernelDisplayName = kernel.kernelSpec?.display_name || kernel.kernelModel?.display_name;

‎src/client/datascience/jupyter/liveshare/guestJupyterNotebook.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -240,10 +240,6 @@ export class GuestJupyterNotebook
240240
return;
241241
}
242242

243-
public setInterpreter(_spec: PythonInterpreter) {
244-
noop();
245-
}
246-
247243
public getKernelSpec(): IJupyterKernelSpec | LiveKernelModel | undefined {
248244
return;
249245
}

‎src/client/datascience/jupyter/liveshare/hostJupyterServer.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
'use strict';
44
import '../../../common/extensions';
55

6+
// tslint:disable-next-line: no-require-imports
7+
import cloneDeep = require('lodash/cloneDeep');
68
import * as os from 'os';
79
import * as vscode from 'vscode';
810
import { CancellationToken } from 'vscode-jsonrpc';
@@ -22,7 +24,7 @@ import {
2224
} from '../../../common/types';
2325
import { createDeferred } from '../../../common/utils/async';
2426
import * as localize from '../../../common/utils/localize';
25-
import { IInterpreterService } from '../../../interpreter/contracts';
27+
import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts';
2628
import { IServiceContainer } from '../../../ioc/types';
2729
import { Identifiers, LiveShare, LiveShareCommands, RegExpValues } from '../../constants';
2830
import {
@@ -303,7 +305,11 @@ export class HostJupyterServer extends LiveShareParticipantHost(JupyterServerBas
303305
const kernelInfoToUse = kernelInfo?.kernelSpec || kernelInfo?.kernelModel;
304306
if (kernelInfoToUse) {
305307
launchInfo.kernelSpec = kernelInfoToUse;
306-
launchInfo.interpreter = resourceInterpreter;
308+
309+
// For the interpreter, make sure to select the one matching the kernel.
310+
launchInfo.interpreter = kernelInfoToUse.metadata?.interpreter?.path
311+
? (cloneDeep(kernelInfoToUse.metadata.interpreter) as PythonInterpreter)
312+
: resourceInterpreter;
307313
changedKernel = true;
308314
}
309315
}

‎src/client/datascience/types.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,11 @@ export interface INotebook extends IAsyncDisposable {
141141
setMatplotLibStyle(useDark: boolean): Promise<void>;
142142
getMatchingInterpreter(): PythonInterpreter | undefined;
143143
getKernelSpec(): IJupyterKernelSpec | LiveKernelModel | undefined;
144-
setKernelSpec(spec: IJupyterKernelSpec | LiveKernelModel, timeoutMS: number): Promise<void>;
145-
setInterpreter(interpeter: PythonInterpreter): void;
144+
setKernelSpec(
145+
spec: IJupyterKernelSpec | LiveKernelModel,
146+
timeoutMS: number,
147+
interpreter: PythonInterpreter | undefined
148+
): Promise<void>;
146149
getLoggers(): INotebookExecutionLogger[];
147150
registerIOPubListener(listener: (msg: KernelMessage.IIOPubMessage, requestId: string) => Promise<void>): void;
148151
registerCommTarget(

‎src/datascience-ui/interactive-common/kernelSelection.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,10 @@ export class KernelSelection extends React.Component<IKernelSelectionProps> {
7676
</div>
7777
);
7878
} else {
79+
const displayName = this.props.kernel.displayName ?? getLocString('DataScience.noKernel', 'No Kernel');
7980
return (
80-
<div className="kernel-status-section kernel-status-status" style={displayNameTextWidth}>
81-
{getLocString('DataScience.noKernel', 'No Kernel')}
81+
<div className="kernel-status-section kernel-status-status" style={displayNameTextWidth} role="button">
82+
{displayName}: {this.props.kernel.jupyterServerStatus}
8283
</div>
8384
);
8485
}

‎src/datascience-ui/native-editor/nativeCell.tsx

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ export class NativeCell extends React.Component<INativeCellProps> {
101101

102102
// Scroll into view (since we have focus). However this function
103103
// is not supported on enzyme
104-
if (this.wrapperRef.current.scrollIntoView) {
104+
// tslint:disable-next-line: no-any
105+
if ((this.wrapperRef.current as any).scrollIntoView) {
105106
this.wrapperRef.current.scrollIntoView({ behavior: 'auto', block: 'nearest', inline: 'nearest' });
106107
}
107108
}
@@ -180,10 +181,20 @@ export class NativeCell extends React.Component<INativeCellProps> {
180181
);
181182
}
182183

184+
private allowClickPropagation(elem: HTMLElement): boolean {
185+
if (this.isMarkdownCell()) {
186+
return true;
187+
}
188+
if (!elem.closest(CssConstants.ImageButtonClass) && !elem.closest(CssConstants.CellOutputWrapperClass)) {
189+
return true;
190+
}
191+
return false;
192+
}
193+
183194
private onMouseClick = (ev: React.MouseEvent<HTMLDivElement>) => {
184195
if (ev.nativeEvent.target) {
185196
const elem = ev.nativeEvent.target as HTMLElement;
186-
if (!elem.closest(CssConstants.ImageButtonClass) && !elem.closest(CssConstants.CellOutputWrapperClass)) {
197+
if (this.allowClickPropagation(elem)) {
187198
// Not a click on an button in a toolbar or in output, select the cell.
188199
ev.stopPropagation();
189200
this.lastKeyPressed = undefined;
@@ -194,7 +205,7 @@ export class NativeCell extends React.Component<INativeCellProps> {
194205

195206
private onMouseDoubleClick = (ev: React.MouseEvent<HTMLDivElement>) => {
196207
const elem = ev.nativeEvent.target as HTMLElement;
197-
if (!elem.closest(CssConstants.ImageButtonClass) && !elem.closest(CssConstants.CellOutputWrapperClass)) {
208+
if (this.allowClickPropagation(elem)) {
198209
// When we receive double click, propagate upwards. Might change our state
199210
ev.stopPropagation();
200211
this.props.focusCell(this.cellId, CursorPos.Current);

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

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -204,26 +204,23 @@ suite('Data Science - Kernel Switcher', () => {
204204
});
205205

206206
test('Switch to the selected kernel', async () => {
207-
when(notebook.setKernelSpec(anything(), anything())).thenResolve();
208-
when(notebook.setInterpreter(selectedInterpreter)).thenReturn();
207+
when(notebook.setKernelSpec(anything(), anything(), anything())).thenResolve();
209208

210209
const selection = await kernelSwitcher.switchKernel(instance(notebook));
211210

212211
assert.isOk(selection);
213212
assert.deepEqual(selection?.kernelModel, selectedKernel);
214213
assert.deepEqual(selection?.interpreter, selectedInterpreter);
215214
assert.deepEqual(selection?.kernelSpec, undefined);
216-
verify(notebook.setKernelSpec(anything(), anything())).once();
217-
verify(notebook.setInterpreter(selectedInterpreter)).once();
215+
verify(notebook.setKernelSpec(anything(), anything(), anything())).once();
218216
});
219217
test('Re-throw errors when switching to the selected kernel', async () => {
220218
const ex = new Error('Kaboom');
221-
when(notebook.setKernelSpec(anything(), anything())).thenReject(ex);
219+
when(notebook.setKernelSpec(anything(), anything(), anything())).thenReject(ex);
222220

223221
const selection = kernelSwitcher.switchKernel(instance(notebook));
224222

225223
await assert.isRejected(selection, ex.message);
226-
verify(notebook.setInterpreter(selectedInterpreter)).never();
227224
});
228225
suite('Display error if `JupyterSessionStartError` is throw and retry', () => {
229226
setup(function () {
@@ -234,13 +231,12 @@ suite('Data Science - Kernel Switcher', () => {
234231
});
235232
test('Display error', async () => {
236233
const ex = new JupyterSessionStartError(new Error('Kaboom'));
237-
when(notebook.setKernelSpec(anything(), anything())).thenReject(ex);
234+
when(notebook.setKernelSpec(anything(), anything(), anything())).thenReject(ex);
238235
when(appShell.showErrorMessage(anything(), anything(), anything())).thenResolve();
239236

240237
const selection = kernelSwitcher.switchKernel(instance(notebook));
241238

242239
await assert.isRejected(selection, ex.message);
243-
verify(notebook.setInterpreter(selectedInterpreter)).never();
244240
const message = DataScience.sessionStartFailedWithKernel().format(
245241
selectedKernel.name,
246242
Commands.ViewJupyterOutput
@@ -255,13 +251,12 @@ suite('Data Science - Kernel Switcher', () => {
255251
});
256252
test('Re-throw error if nothing is selected from prompt', async () => {
257253
const ex = new JupyterSessionStartError(new Error('Kaboom'));
258-
when(notebook.setKernelSpec(anything(), anything())).thenReject(ex);
254+
when(notebook.setKernelSpec(anything(), anything(), anything())).thenReject(ex);
259255
when(appShell.showErrorMessage(anything(), anything(), anything())).thenResolve();
260256

261257
const selection = kernelSwitcher.switchKernel(instance(notebook));
262258

263259
await assert.isRejected(selection, ex.message);
264-
verify(notebook.setInterpreter(selectedInterpreter)).never();
265260
const message = DataScience.sessionStartFailedWithKernel().format(
266261
selectedKernel.name,
267262
Commands.ViewJupyterOutput
@@ -276,7 +271,7 @@ suite('Data Science - Kernel Switcher', () => {
276271
});
277272
test('Re-throw error if cancel is selected from prompt', async () => {
278273
const ex = new JupyterSessionStartError(new Error('Kaboom'));
279-
when(notebook.setKernelSpec(anything(), anything())).thenReject(ex);
274+
when(notebook.setKernelSpec(anything(), anything(), anything())).thenReject(ex);
280275
when(appShell.showErrorMessage(anything(), anything(), anything())).thenResolve(
281276
// tslint:disable-next-line: no-any
282277
Common.cancel() as any
@@ -285,7 +280,6 @@ suite('Data Science - Kernel Switcher', () => {
285280
const selection = kernelSwitcher.switchKernel(instance(notebook));
286281

287282
await assert.isRejected(selection, ex.message);
288-
verify(notebook.setInterpreter(selectedInterpreter)).never();
289283
const message = DataScience.sessionStartFailedWithKernel().format(
290284
selectedKernel.name,
291285
Commands.ViewJupyterOutput
@@ -302,7 +296,7 @@ suite('Data Science - Kernel Switcher', () => {
302296
let firstTimeSelectingAKernel = true;
303297
let firstTimeSettingAKernel = true;
304298
const ex = new JupyterSessionStartError(new Error('Kaboom'));
305-
when(notebook.setKernelSpec(anything(), anything())).thenCall(() => {
299+
when(notebook.setKernelSpec(anything(), anything(), anything())).thenCall(() => {
306300
// If we're setting it the first time, then throw an error.
307301
if (firstTimeSettingAKernel) {
308302
firstTimeSettingAKernel = false;
@@ -342,8 +336,7 @@ suite('Data Science - Kernel Switcher', () => {
342336
assert.deepEqual(selection?.kernelModel, selectedKernelSecondTime);
343337
assert.deepEqual(selection?.interpreter, selectedInterpreter);
344338
assert.deepEqual(selection?.kernelSpec, undefined);
345-
verify(notebook.setKernelSpec(anything(), anything())).twice();
346-
verify(notebook.setInterpreter(selectedInterpreter)).once();
339+
verify(notebook.setKernelSpec(anything(), anything(), anything())).twice();
347340
verify(
348341
appShell.showErrorMessage(
349342
anything(),

0 commit comments

Comments
 (0)