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

Enable Web Debugging #10040

Merged
merged 13 commits into from
May 16, 2022
1 change: 1 addition & 0 deletions news/2 Fixes/9973.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support notebook debugging in the web extension.
19 changes: 9 additions & 10 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@
"title": "%jupyter.command.jupyter.debug.title%",
"icon": "$(bug)",
"category": "Jupyter",
"enablement": "notebookKernelCount > 0 && !jupyter.notebookeditor.debuggingInProgress && !jupyter.notebookeditor.runByLineInProgress && !jupyter.webExtension"
"enablement": "notebookKernelCount > 0 && !jupyter.notebookeditor.debuggingInProgress && !jupyter.notebookeditor.runByLineInProgress"
sadasant marked this conversation as resolved.
Show resolved Hide resolved
},
{
"command": "jupyter.filterKernels",
Expand All @@ -304,28 +304,27 @@
"title": "%jupyter.command.jupyter.runByLine.title%",
"icon": "$(debug-line-by-line)",
"category": "Jupyter",
"enablement": "notebookKernelCount > 0 && !jupyter.notebookeditor.debuggingInProgress && !jupyter.notebookeditor.runByLineInProgress && !jupyter.webExtension"
"enablement": "notebookKernelCount > 0 && !jupyter.notebookeditor.debuggingInProgress && !jupyter.notebookeditor.runByLineInProgress"
},
{
"command": "jupyter.runAndDebugCell",
"title": "%jupyter.command.jupyter.debugCell.title%",
"icon": "$(debug-alt-small)",
"category": "Jupyter",
"enablement": "!jupyter.webExtension"
"category": "Jupyter"
},
{
"command": "jupyter.runByLineNext",
"title": "%jupyter.command.jupyter.runByLineNext.title%",
"icon": "$(debug-line-by-line)",
"category": "Jupyter",
"enablement": "jupyter.notebookeditor.runByLineInProgress && !jupyter.webExtension"
"enablement": "jupyter.notebookeditor.runByLineInProgress"
},
{
"command": "jupyter.runByLineStop",
"title": "%jupyter.command.jupyter.runByLineStop.title%",
"icon": "$(debug-continue-small)",
"category": "Jupyter",
"enablement": "notebookKernelCount > 0 && jupyter.notebookeditor.runByLineInProgress && !jupyter.webExtension"
"enablement": "notebookKernelCount > 0 && jupyter.notebookeditor.runByLineInProgress"
},
{
"command": "jupyter.viewOutput",
Expand Down Expand Up @@ -383,25 +382,25 @@
"command": "jupyter.debugcell",
"title": "%jupyter.command.jupyter.debugcell.title%",
"category": "Jupyter",
"enablement": "isWorkspaceTrusted && !jupyter.webExtension"
"enablement": "isWorkspaceTrusted"
},
{
"command": "jupyter.debugstepover",
"title": "%jupyter.command.jupyter.debugstepover.title%",
"category": "Jupyter",
"enablement": "isWorkspaceTrusted && !jupyter.webExtension"
"enablement": "isWorkspaceTrusted"
},
{
"command": "jupyter.debugstop",
"title": "%jupyter.command.jupyter.debugstop.title%",
"category": "Jupyter",
"enablement": "isWorkspaceTrusted && !jupyter.webExtension"
"enablement": "isWorkspaceTrusted"
},
{
"command": "jupyter.debugcontinue",
"title": "%jupyter.command.jupyter.debugcontinue.title%",
"category": "Jupyter",
"enablement": "isWorkspaceTrusted && !jupyter.webExtension"
"enablement": "isWorkspaceTrusted"
},
{
"command": "jupyter.insertCellBelowPosition",
Expand Down
3 changes: 1 addition & 2 deletions src/kernels/debugging/jupyterDebugService.node.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import { IDisposable } from '@fluentui/react';
import { inject, injectable } from 'inversify';
import * as net from 'net';
import * as path from '../../platform/vscode-path/path';
Expand All @@ -24,7 +23,7 @@ import {
} from 'vscode';
import { DebugProtocol } from 'vscode-debugprotocol';
import { traceInfo, traceError } from '../../platform/logging';
import { IDisposableRegistry } from '../../platform/common/types';
import { IDisposable, IDisposableRegistry } from '../../platform/common/types';
import { createDeferred } from '../../platform/common/utils/async';
import { noop } from '../../platform/common/utils/misc';
import { EXTENSION_ROOT_DIR } from '../../platform/constants.node';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,8 @@ import {
IKernelDebugAdapterConfig,
KernelDebugMode
} from '../types';
import {
assertIsDebugConfig,
getMessageSourceAndHookIt,
isShortNamePath,
shortNameMatchesLongName
} from './helper.node';
import { IFileSystem } from '../../common/platform/types';
import { assertIsDebugConfig, getMessageSourceAndHookIt, isShortNamePath, shortNameMatchesLongName } from './helper';
import { executeSilently } from '../../../kernels/helpers';

// For info on the custom requests implemented by jupyter see:
// https://jupyter-client.readthedocs.io/en/stable/messaging.html#debug-request
Expand All @@ -64,7 +59,6 @@ export class KernelDebugAdapter implements DebugAdapter, IKernelDebugAdapter, ID
private session: DebugSession,
private notebookDocument: NotebookDocument,
private readonly jupyterSession: IJupyterSession,
private fs: IFileSystem,
private readonly kernel: IKernel | undefined,
private readonly platformService: IPlatformService
) {
Expand Down Expand Up @@ -195,16 +189,11 @@ export class KernelDebugAdapter implements DebugAdapter, IKernelDebugAdapter, ID
}

dispose() {
this.disposables.forEach((d) => d.dispose());
// clean temp files
this.cellToFile.forEach((tempPath) => {
const norm = path.normalize(tempPath);
try {
void this.fs.deleteLocalFile(norm);
} catch {
traceError('Error deleting temporary debug files');
}
// On dispose, delete our temp cell files
this.deleteDumpCells().catch(() => {
traceError('Error deleting temporary debug files.');
});
this.disposables.forEach((d) => d.dispose());
}

public stackTrace(args: DebugProtocol.StackTraceArguments): Thenable<DebugProtocol.StackTraceResponse['body']> {
Expand Down Expand Up @@ -275,6 +264,35 @@ export class KernelDebugAdapter implements DebugAdapter, IKernelDebugAdapter, ID
return undefined;
}

// Use our jupyter session to delete all the cells
private async deleteDumpCells() {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is identical to ContentsManager#delete considering that the dummy file is created by the Jupyter itself. In Export Web support, I found that I need to create temp files and delete them too so I'm experimenting with ContentsManager#newUntitled and ContentsManager#delete to see if they work as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Part of the diff here is that this code will be working in both the web and the node version. The ContentsManager is only available when connecting to Jupyter, so that makes sense to use when working with a .web only scenario. I could consider a .web kernelDebugAdapter and use the ContentsManager delete there, but I think that I'd only go there if this cell execution option which covers both scenarios was proving to have issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

Might also be a mild diff here in that this file is actually created by ipykernel and not Jupyter:
https://github.com/ipython/ipykernel/blob/51a613d501a86073ea1cdbd8023a168646644c6a/ipykernel/compiler.py#L83
I would assume Jupyter ContentsManager would have access to the same temp location though, so I'm guessing it could be used for the delete.

const fileValues = [...this.cellToFile.values()];
sadasant marked this conversation as resolved.
Show resolved Hide resolved
// Need to have our Jupyter Session and some dumpCell files to delete
if (this.jupyterSession && fileValues.length) {
// Create our python string of file names
const fileListString = fileValues
.map((filePath) => {
return '"' + filePath + '"';
})
.join(',');

// Insert into our delete snippet
const deleteFilesCode = `import os
_VSCODE_fileList = [${fileListString}]
sadasant marked this conversation as resolved.
Show resolved Hide resolved
for file in _VSCODE_fileList:
try:
os.remove(file)
except:
pass
del _VSCODE_fileList`;

return executeSilently(this.jupyterSession, deleteFilesCode, {
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why the execution is set to silent: false, is this intentional

silent: false,
?

Copy link
Member Author

@IanMatthewHuff IanMatthewHuff May 16, 2022

Choose a reason for hiding this comment

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

Yeah, as I recall the name is bad in terms of what silent actually means.. Silent also swallows all output and we want to see if there are errors coming in on stdout. It's actually store_history that makes it silent in terms of hiding the execution from execution count.

Copy link
Member Author

Choose a reason for hiding this comment

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

Jupyter has doc on the execute params here which detail it a bit more: https://jupyter-client.readthedocs.io/en/latest/messaging.html#execute

Copy link
Member

Choose a reason for hiding this comment

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

@IanMatthewHuff thanks for the detailed explanation, you are right store_history is the key here and our executeSilently here means skipping history and execution count.

traceErrors: true,
traceErrorsMessage: 'Error deleting temporary debugging files'
});
}
}

private async sendRequestToJupyterSession(message: DebugProtocol.ProtocolMessage) {
if (this.jupyterSession.disposed || this.jupyterSession.status === 'dead') {
traceInfo(`Skipping sending message ${message.type} because session is disposed`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { IKernel } from '../../../../kernels/types';
import { sendTelemetryEvent } from '../../../../telemetry';
import { DebuggingTelemetry } from '../../constants';
import { IDebuggingDelegate, IKernelDebugAdapter } from '../../types';
import { cellDebugSetup } from '../helper.node';
import { cellDebugSetup } from '../helper';

export class DebugCellController implements IDebuggingDelegate {
constructor(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
import * as path from '../../../vscode-path/path';
import { IKernel, IKernelProvider } from '../../../../kernels/types';
import { IConfigurationService, IDisposable } from '../../../common/types';
import { KernelDebugAdapter } from '../kernelDebugAdapter.node';
import { KernelDebugAdapter } from '../kernelDebugAdapter';
import { IExtensionSingleActivationService } from '../../../activation/types';
import { ContextKey } from '../../../common/contextKey';
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../common/application/types';
Expand All @@ -31,12 +31,11 @@ import { IPlatformService } from '../../../common/platform/types';
import { IDebuggingManager, IKernelDebugAdapterConfig, KernelDebugMode } from '../../types';
import { DebuggingTelemetry, pythonKernelDebugAdapter } from '../../constants';
import { sendTelemetryEvent } from '../../../../telemetry';
import { DebugCellController } from './debugCellControllers.node';
import { assertIsDebugConfig, IpykernelCheckResult, isUsingIpykernel6OrLater } from '../helper.node';
import { Debugger } from '../debugger.node';
import { DebugCellController } from './debugCellControllers';
import { assertIsDebugConfig, IpykernelCheckResult, isUsingIpykernel6OrLater } from '../helper';
import { Debugger } from '../debugger';
import { INotebookControllerManager } from '../../../../notebooks/types';
import { IFileSystem } from '../../../common/platform/types';
import { RunByLineController } from './runByLineController.node';
import { RunByLineController } from './runByLineController';

/**
* The DebuggingManager maintains the mapping between notebook documents and debug sessions.
Expand All @@ -58,7 +57,6 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IVSCodeNotebook) private readonly vscNotebook: IVSCodeNotebook,
@inject(IFileSystem) private fs: IFileSystem,
@inject(IConfigurationService) private settings: IConfigurationService,
@inject(IPlatformService) private platform: IPlatformService
) {
Expand Down Expand Up @@ -381,7 +379,6 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
session,
debug.document,
kernel.session,
this.fs,
kernel,
this.platform
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { sendTelemetryEvent } from '../../../../telemetry';
import { DebuggingTelemetry } from '../../constants';
import { IDebuggingDelegate, IKernelDebugAdapter, KernelDebugMode } from '../../types';
import { Commands } from '../../../common/constants';
import { cellDebugSetup } from '../helper.node';
import { cellDebugSetup } from '../helper';

export class RunByLineController implements IDebuggingDelegate {
private lastPausedThreadId: number | undefined;
Expand Down
2 changes: 1 addition & 1 deletion src/platform/serviceRegistry.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import { GlobalActivation } from './common/globalActivation';
import { PreReleaseChecker } from './common/prereleaseChecker.node';
import { IConfigurationService, IDataScienceCommandListener, IExtensionContext } from './common/types';
import { DebugLocationTrackerFactory } from './debugger/debugLocationTrackerFactory.node';
import { DebuggingManager } from './debugger/jupyter/notebook/debuggingManager.node';
import { DebuggingManager } from './debugger/jupyter/notebook/debuggingManager';
import { IDebugLocationTracker, IDebuggingManager } from './debugger/types';
import { DataScienceErrorHandler } from './errors/errorHandler';
import { IDataScienceErrorHandler } from './errors/types';
Expand Down
6 changes: 6 additions & 0 deletions src/platform/serviceRegistry.web.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import { GlobalActivation } from './common/globalActivation';
import { IExtensionSingleActivationService } from './activation/types';
import { ExtensionSideRenderer, IExtensionSideRenderer } from '../webviews/extension-side/renderer';
import { OutputCommandListener } from './logging/outputCommandListener';
import { IDebuggingManager } from './debugger/types';
import { DebuggingManager } from './debugger/jupyter/notebook/debuggingManager';
import { ExportDialog } from './export/exportDialog';
import { ExportFormat, IExport, IExportDialog, IFileConverter } from './export/types';
import { FileConverter } from './export/fileConverter.web';
Expand Down Expand Up @@ -56,4 +58,8 @@ export function registerTypes(context: IExtensionContext, serviceManager: IServi
registerApiTypes(serviceManager);
registerActivationTypes(serviceManager);
registerDevToolTypes(context, serviceManager, isDevMode);

serviceManager.addSingleton<IDebuggingManager>(IDebuggingManager, DebuggingManager, undefined, [
IExtensionSingleActivationService
]);
}
2 changes: 1 addition & 1 deletion src/test/debugger/jupyter/helpers.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Licensed under the MIT License.

import { expect } from 'chai';
import { isShortNamePath } from '../../../platform/debugger/jupyter/helper.node';
import { isShortNamePath } from '../../../platform/debugger/jupyter/helper';

suite('Debugging - Helpers', () => {
suite('isShortNamePath', async () => {
Expand Down