Skip to content

Commit

Permalink
Fix interactive window debugging with ipykernel6 (#6691)
Browse files Browse the repository at this point in the history
  • Loading branch information
DonJayamanne authored Jul 17, 2021
1 parent b2f352d commit 2004ee9
Show file tree
Hide file tree
Showing 15 changed files with 127 additions and 64 deletions.
1 change: 0 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,6 @@ module.exports = {
'src/client/datascience/jupyter/interpreter/jupyterInterpreterSelector.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts',
'src/client/datascience/jupyter/commandLineSelector.ts',
'src/client/datascience/jupyter/jupyterDebugger.ts',
'src/client/datascience/jupyter/liveshare/roleBasedFactory.ts',
'src/client/datascience/jupyter/liveshare/responseQueue.ts',
'src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts',
Expand Down
1 change: 1 addition & 0 deletions news/2 Fixes/6534.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix debugging in `Interactive Window` when using `IPyKernel 6`
8 changes: 8 additions & 0 deletions src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,14 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
if (debugInfo.runByLine && debugInfo.hashFileName) {
await this.jupyterDebugger.startRunByLine(this._notebook, debugInfo.hashFileName);
} else if (!debugInfo.runByLine) {
await this._notebook.execute(
`import os;os.environ["IPYKERNEL_CELL_NAME"] = '${file.replace(/\\/g, '\\\\')}'`,
file,
0,
uuid(),
undefined,
true
);
await this.jupyterDebugger.startDebugging(this._notebook);
} else {
throw Error('Missing hash file name when running by line');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,10 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {
}

private registerKernel(notebookDocument: NotebookDocument, controller: VSCodeNotebookController) {
const kernel = this.kernelProvider.getOrCreate(notebookDocument.uri, {
const kernel = this.kernelProvider.getOrCreate(notebookDocument, {
metadata: controller.connection,
controller: controller.controller
controller: controller.controller,
resourceUri: this.owner
});
this.kernelLoadPromise = kernel?.start({ disableUI: false, document: notebookDocument });
this.kernel = kernel;
Expand Down Expand Up @@ -379,6 +380,11 @@ export class NativeInteractiveWindow implements IInteractiveWindowLoadable {
}

if (isDebug) {
await this.kernel!.executeHidden(
`import os;os.environ["IPYKERNEL_CELL_NAME"] = '${file.replace(/\\/g, '\\\\')}'`,
file,
this.notebookDocument
);
await this.jupyterDebugger.startDebugging(notebook);
}

Expand Down
21 changes: 16 additions & 5 deletions src/client/datascience/jupyter/jupyterDebugger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import { DebugConfiguration, Disposable } from 'vscode';
import * as vsls from 'vsls/vscode';
import { concatMultilineString } from '../../../datascience-ui/common';
import { ServerStatus } from '../../../datascience-ui/interactive-common/mainState';
import { IPythonDebuggerPathProvider } from '../../api/types';
import { IPythonDebuggerPathProvider, IPythonInstaller } from '../../api/types';
import { traceInfo, traceWarning } from '../../common/logger';
import { IPlatformService } from '../../common/platform/types';
import { IConfigurationService } from '../../common/types';
import { IConfigurationService, Product, ProductInstallStatus } from '../../common/types';
import * as localize from '../../common/utils/localize';
import { traceCellResults } from '../common';
import { Identifiers } from '../constants';
Expand Down Expand Up @@ -39,13 +39,15 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener {
private readonly tracingEnableCode: string;
private readonly tracingDisableCode: string;
private runningByLine: boolean = false;
private isUsingPyKernel6OrLater?: boolean;
constructor(
@inject(IPythonDebuggerPathProvider) private readonly debuggerPathProvider: IPythonDebuggerPathProvider,
@inject(IConfigurationService) private configService: IConfigurationService,
@inject(IJupyterDebugService)
@named(Identifiers.MULTIPLEXING_DEBUGSERVICE)
private debugService: IJupyterDebugService,
@inject(IPlatformService) private platform: IPlatformService
@inject(IPlatformService) private platform: IPlatformService,
@inject(IPythonInstaller) private installer: IPythonInstaller
) {
this.debuggerPackage = 'debugpy';
this.enableDebuggerCode = `import debugpy;debugpy.listen(('localhost', 0))`;
Expand Down Expand Up @@ -78,7 +80,13 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener {
}

public async startDebugging(notebook: INotebook): Promise<void> {
const result = await this.installer.isProductVersionCompatible(
Product.ipykernel,
'>=6.0.0',
notebook.getKernelConnection()?.interpreter
);
const settings = this.configService.getSettings(notebook.resource);
this.isUsingPyKernel6OrLater = result === ProductInstallStatus.Installed;
return this.startDebugSession(
(c) => this.debugService.startDebugging(undefined, c),
notebook,
Expand Down Expand Up @@ -275,12 +283,15 @@ export class JupyterDebugger implements IJupyterDebugger, ICellHashListener {

private buildSourceMap(fileHash: IFileHashes): ISourceMapRequest {
const sourceMapRequest: ISourceMapRequest = { source: { path: fileHash.file }, pydevdSourceMaps: [] };

sourceMapRequest.pydevdSourceMaps = fileHash.hashes.map((cellHash) => {
return {
line: cellHash.line,
endLine: cellHash.endLine,
runtimeSource: { path: `<ipython-input-${cellHash.executionCount}-${cellHash.hash}>` },
runtimeSource: {
path: this.isUsingPyKernel6OrLater
? fileHash.file
: `<ipython-input-${cellHash.executionCount}-${cellHash.hash}>`
},
runtimeLine: cellHash.runtimeLine
};
});
Expand Down
35 changes: 22 additions & 13 deletions src/client/datascience/jupyter/kernels/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { ServerStatus } from '../../../../datascience-ui/interactive-common/main
import { IApplicationShell } from '../../../common/application/types';
import { traceError, traceInfo, traceInfoIf, traceWarning } from '../../../common/logger';
import { IFileSystem } from '../../../common/platform/types';
import { IConfigurationService, IDisposableRegistry, IExtensionContext } from '../../../common/types';
import { IConfigurationService, IDisposableRegistry, IExtensionContext, Resource } from '../../../common/types';
import { createDeferred, Deferred } from '../../../common/utils/async';
import { noop } from '../../../common/utils/misc';
import { StopWatch } from '../../../common/utils/stopWatch';
Expand Down Expand Up @@ -90,7 +90,8 @@ export class Kernel implements IKernel {
private readonly kernelExecution: KernelExecution;
private startCancellation = new CancellationTokenSource();
constructor(
public readonly uri: Uri,
public readonly notebookUri: Uri,
public readonly resourceUri: Resource,
public readonly kernelConnectionMetadata: Readonly<KernelConnectionMetadata>,
private readonly notebookProvider: INotebookProvider,
private readonly disposables: IDisposableRegistry,
Expand Down Expand Up @@ -155,7 +156,7 @@ export class Kernel implements IKernel {
return interruptResultPromise;
}
public async dispose(): Promise<void> {
traceInfo(`Dispose kernel ${this.uri.toString()}`);
traceInfo(`Dispose kernel ${this.notebookUri.toString()}`);
this.restarting = undefined;
this._notebookPromise = undefined;
if (this.notebook) {
Expand Down Expand Up @@ -221,8 +222,8 @@ export class Kernel implements IKernel {
);
traceInfo(`Starting Notebook in kernel.ts id = ${this.kernelConnectionMetadata.id}`);
this.notebook = await this.notebookProvider.getOrCreateNotebook({
identity: this.uri,
resource: this.uri,
identity: this.notebookUri,
resource: this.resourceUri,
disableUI: options?.disableUI,
getOnly: false,
metadata: getNotebookMetadata(options.document), // No need to pass this, as we have a kernel connection (metadata is required in lower layers to determine the kernel connection).
Expand All @@ -240,7 +241,7 @@ export class Kernel implements IKernel {
}
await this.initializeAfterStart(SysInfoReason.Start, options.document);
sendKernelTelemetryEvent(
this.uri,
this.resourceUri,
Telemetry.PerceivedJupyterStartupNotebook,
stopWatch.elapsedTime
);
Expand Down Expand Up @@ -312,7 +313,7 @@ export class Kernel implements IKernel {
}

// Set the notebook property on the matching editor
const editor = this.editorProvider.editors.find((item) => this.fs.arePathsSame(item.file, this.uri));
const editor = this.editorProvider.editors.find((item) => this.fs.arePathsSame(item.file, this.notebookUri));
if (editor) {
editor.notebook = this.notebook;
}
Expand All @@ -321,7 +322,7 @@ export class Kernel implements IKernel {
this.hookedNotebookForEvents.add(this.notebook);
this.notebook.kernelSocket.subscribe(this._kernelSocket);
this.notebook.onDisposed(() => {
traceInfo(`Kernel got disposed as a result of notebook.onDisposed ${this.uri.toString()}`);
traceInfo(`Kernel got disposed as a result of notebook.onDisposed ${this.notebookUri.toString()}`);
this._notebookPromise = undefined;
this._onDisposed.fire();
});
Expand All @@ -340,7 +341,9 @@ export class Kernel implements IKernel {
}
if (isPythonKernelConnection(this.kernelConnectionMetadata)) {
await this.disableJedi();
await this.notebook.setLaunchingFile(this.uri.fsPath);
if (this.resourceUri) {
await this.notebook.setLaunchingFile(this.resourceUri.fsPath);
}
await this.initializeMatplotlib();
}
await this.notebook
Expand Down Expand Up @@ -428,13 +431,13 @@ export class Kernel implements IKernel {
if (!this.notebook) {
return;
}
const settings = this.configService.getSettings(this.uri);
const settings = this.configService.getSettings(this.resourceUri);
if (settings && settings.themeMatplotlibPlots) {
const matplobInit = settings.enablePlotViewer
? CodeSnippets.MatplotLibInitSvg
: CodeSnippets.MatplotLibInitPng;

traceInfo(`Initialize matplotlib for ${this.uri.toString()}`);
traceInfo(`Initialize matplotlib for ${(this.resourceUri || this.notebookUri).toString()}`);
await this.executeSilently(matplobInit);
const useDark = this.appShell.activeColorTheme.kind === ColorThemeKind.Dark;
if (!settings.ignoreVscodeTheme) {
Expand All @@ -447,7 +450,7 @@ export class Kernel implements IKernel {
}
} else {
const configInit = !settings || settings.enablePlotViewer ? CodeSnippets.ConfigSvg : CodeSnippets.ConfigPng;
traceInfoIf(isCI, `Initialize config for plots for ${this.uri.toString()}`);
traceInfoIf(isCI, `Initialize config for plots for ${(this.resourceUri || this.notebookUri).toString()}`);
await this.executeSilently(configInit);
}
}
Expand All @@ -456,7 +459,13 @@ export class Kernel implements IKernel {
return;
}
const deferred = createDeferred<void>();
const observable = this.notebook.executeObservable(code, this.uri.fsPath, 0, uuid(), true);
const observable = this.notebook.executeObservable(
code,
(this.resourceUri || this.notebookUri).fsPath,
0,
uuid(),
true
);
const subscription = observable.subscribe(
noop,
(ex) => deferred.reject(ex),
Expand Down
6 changes: 3 additions & 3 deletions src/client/datascience/jupyter/kernels/kernelExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ export class KernelExecution implements IDisposable {
() => {
// We're only interested in restarts of the kernel associated with this document.
const executionQueue = this.documentExecutions.get(document);
if (kernel !== this.kernelProvider.get(document.uri) || !executionQueue) {
if (kernel !== this.kernelProvider.get(document) || !executionQueue) {
return;
}

Expand All @@ -243,9 +243,9 @@ export class KernelExecution implements IDisposable {
);
}
private async getKernel(document: NotebookDocument): Promise<IKernel> {
let kernel = this.kernelProvider.get(document.uri);
let kernel = this.kernelProvider.get(document);
if (!kernel) {
kernel = this.kernelProvider.getOrCreate(document.uri, {
kernel = this.kernelProvider.getOrCreate(document, {
metadata: this.metadata,
controller: this.controller
});
Expand Down
44 changes: 23 additions & 21 deletions src/client/datascience/jupyter/kernels/kernelProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
'use strict';

import { inject, injectable } from 'inversify';
import { Uri } from 'vscode';
import { NotebookDocument } from 'vscode';
import { IApplicationShell } from '../../../common/application/types';
import { traceInfo, traceWarning } from '../../../common/logger';
import { IFileSystem } from '../../../common/platform/types';
Expand All @@ -16,6 +16,7 @@ import {
IExtensionContext
} from '../../../common/types';
import { noop } from '../../../common/utils/misc';
import { InteractiveWindowView } from '../../notebook/constants';
import {
IDataScienceErrorHandler,
IJupyterServerUriStorage,
Expand All @@ -27,7 +28,7 @@ import { IKernel, IKernelProvider, KernelOptions } from './types';

@injectable()
export class KernelProvider implements IKernelProvider {
private readonly kernelsByUri = new Map<string, { options: KernelOptions; kernel: IKernel }>();
private readonly kernelsByNotebook = new WeakMap<NotebookDocument, { options: KernelOptions; kernel: IKernel }>();
private readonly pendingDisposables = new Set<IAsyncDisposable>();
constructor(
@inject(IAsyncDisposableRegistry) private asyncDisposables: IAsyncDisposableRegistry,
Expand All @@ -44,26 +45,27 @@ export class KernelProvider implements IKernelProvider {
this.asyncDisposables.push(this);
}

public get(uri: Uri): IKernel | undefined {
return this.kernelsByUri.get(uri.toString())?.kernel;
public get(notebook: NotebookDocument): IKernel | undefined {
return this.kernelsByNotebook.get(notebook)?.kernel;
}
public async dispose() {
const items = Array.from(this.pendingDisposables.values());
this.pendingDisposables.clear();
await Promise.all(items);
}
public getOrCreate(uri: Uri, options: KernelOptions): IKernel | undefined {
const existingKernelInfo = this.kernelsByUri.get(uri.toString());
public getOrCreate(notebook: NotebookDocument, options: KernelOptions): IKernel | undefined {
const existingKernelInfo = this.kernelsByNotebook.get(notebook);
if (existingKernelInfo && existingKernelInfo.options.metadata.id === options.metadata.id) {
return existingKernelInfo.kernel;
}
const resourceUri = notebook.notebookType === InteractiveWindowView ? options.resourceUri : notebook.uri;
this.disposeOldKernel(notebook);

this.disposeOldKernel(uri);

const waitForIdleTimeout = this.configService.getSettings(uri).jupyterLaunchTimeout;
const interruptTimeout = this.configService.getSettings(uri).jupyterInterruptTimeout;
const waitForIdleTimeout = this.configService.getSettings(resourceUri).jupyterLaunchTimeout;
const interruptTimeout = this.configService.getSettings(resourceUri).jupyterInterruptTimeout;
const kernel = new Kernel(
uri,
notebook.uri,
resourceUri,
options.metadata,
this.notebookProvider,
this.disposables,
Expand All @@ -80,31 +82,31 @@ export class KernelProvider implements IKernelProvider {
this.configService
);
this.asyncDisposables.push(kernel);
this.kernelsByUri.set(uri.toString(), { options, kernel });
this.deleteMappingIfKernelIsDisposed(uri, kernel);
this.kernelsByNotebook.set(notebook, { options, kernel });
this.deleteMappingIfKernelIsDisposed(notebook, kernel);
return kernel;
}
/**
* If a kernel has been disposed, then remove the mapping of Uri + Kernel.
*/
private deleteMappingIfKernelIsDisposed(uri: Uri, kernel: IKernel) {
private deleteMappingIfKernelIsDisposed(notebook: NotebookDocument, kernel: IKernel) {
kernel.onDisposed(
() => {
// If the same kernel is associated with this document & it was disposed, then delete it.
if (this.kernelsByUri.get(uri.toString())?.kernel === kernel) {
this.kernelsByUri.delete(uri.toString());
if (this.kernelsByNotebook.get(notebook)?.kernel === kernel) {
this.kernelsByNotebook.delete(notebook);
traceInfo(
`Kernel got disposed, hence there is no longer a kernel associated with ${uri.toString()}`,
kernel.uri.toString()
`Kernel got disposed, hence there is no longer a kernel associated with ${notebook.uri.toString()}`,
kernel.notebookUri.toString()
);
}
},
this,
this.disposables
);
}
private disposeOldKernel(uri: Uri) {
const kernelToDispose = this.kernelsByUri.get(uri.toString());
private disposeOldKernel(notebook: NotebookDocument) {
const kernelToDispose = this.kernelsByNotebook.get(notebook);
if (kernelToDispose) {
this.pendingDisposables.add(kernelToDispose.kernel);
kernelToDispose.kernel
Expand All @@ -113,7 +115,7 @@ export class KernelProvider implements IKernelProvider {
.finally(() => this.pendingDisposables.delete(kernelToDispose.kernel))
.catch(noop);
}
this.kernelsByUri.delete(uri.toString());
this.kernelsByNotebook.delete(notebook);
}
}

Expand Down
Loading

0 comments on commit 2004ee9

Please sign in to comment.