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

Keybindings and telemetry #7183

Merged
merged 6 commits into from
Aug 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,24 @@
"command": "jupyter.runAndDebugCell",
"key": "ctrl+alt+shift+enter",
"mac": "ctrl+shift+enter"
},
{
"command": "jupyter.runByLine",
"key": "f10",
"mac": "f10",
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity is the 'mac' entry necessary here? I assumed 'key' was sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

IDK, I don't have a mac to test it.

"when": "!jupyter.notebookeditor.debuggingInProgress && !jupyter.notebookeditor.runByLineInProgress"
},
{
"command": "jupyter.runByLineContinue",
"key": "f10",
"mac": "f10",
"when": "jupyter.notebookeditor.runByLineInProgress"
Copy link
Member

Choose a reason for hiding this comment

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

Out of interest, what happens if I'm debugging something in my workspace (not a notebook) and then I start run by line?

Copy link
Author

Choose a reason for hiding this comment

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

The debugging manager checks for the active editor here. Unless you're on the editor, nothing will happen on the run by line side.

},
{
"command": "jupyter.runByLineStop",
"key": "ctrl+enter",
"mac": "ctrl+enter",
"when": "jupyter.notebookeditor.runByLineInProgress"
}
],
"commands": [
Expand Down
4 changes: 3 additions & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,9 @@
"DataScience.interactiveWindowModeBannerSwitchAlways": "Always",
"DataScience.interactiveWindowModeBannerSwitchNo": "No",
"DataScience.ipykernelNotInstalled": "IPyKernel not installed into interpreter {0}",
"DataScience.needIpykernel6": "Ipykernel 6 is needed for debugging, click Install to continue. Or you can run 'pip install ipykernel==6.0.3/conda install ipykernel=6'",
"DataScience.needIpykernel6": "Ipykernel setup required for this feature",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure this has been discussed, but could this message be more specific, and/or could we update the variable name needIpykernel6 to match the new message?

"DataScience.setup": "Setup",
"DataScience.startingRunByLine": "Starting Run by Line",
"DataScience.illegalEditorConfig": "CustomEditor and NativeNotebook experiments cannot be turned on together",
"DataScience.pythonExtensionRequired": "The Python extension is required to perform that task. Click Yes to open Python extension installation page.",
"DataScience.pythonExtensionRequiredToRunNotebook": "Python Extension required to run Python notebooks.",
Expand Down
1 change: 1 addition & 0 deletions src/client/common/application/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,4 +172,5 @@ export interface ICommandNameArgumentTypeMapping extends ICommandNameWithoutArgu
[DSCommands.RunByLine]: [NotebookCell];
[DSCommands.RunAndDebugCell]: [NotebookCell];
[DSCommands.RunByLineContinue]: [NotebookCell];
[DSCommands.RunByLineStop]: [];
}
7 changes: 3 additions & 4 deletions src/client/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -940,10 +940,9 @@ export namespace DataScience {
'DataScience.ipykernelNotInstalled',
'IPyKernel not installed into interpreter {0}'
);
export const needIpykernel6 = localize(
'DataScience.needIpykernel6',
"Ipykernel 6 is needed for debugging, click Install to continue. Or you can run 'pip install ipykernel==6.0.3/conda install ipykernel=6'"
);
export const needIpykernel6 = localize('DataScience.needIpykernel6', 'Ipykernel setup required for this feature');
export const setup = localize('DataScience.setup', 'Setup');
export const startingRunByLine = localize('DataScience.startingRunByLine', 'Starting Run by Line');
export const showDataViewerFail = localize(
'DataScience.showDataViewerFail',
'Failed to create the Data Viewer. Check the Jupyter tab of the Output window for more info.'
Expand Down
1 change: 1 addition & 0 deletions src/client/datascience/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ export namespace Commands {
export const RunByLine = 'jupyter.runByLine';
export const RunAndDebugCell = 'jupyter.runAndDebugCell';
export const RunByLineContinue = 'jupyter.runByLineContinue';
export const RunByLineStop = 'jupyter.runByLineStop';
}

export namespace CodeLensCommands {
Expand Down
11 changes: 11 additions & 0 deletions src/client/debugger/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,14 @@
'use strict';

export const pythonKernelDebugAdapter = 'Python Kernel Debug Adapter';

export enum DebuggingTelemetry {
clickedOnSetup = 'DATASCIENCE.DEBUGGING.CLICKED_ON_SETUP',
closedModal = 'DATASCIENCE.DEBUGGING.CLOSED_MODAL',
ipykernel6Status = 'DATASCIENCE.DEBUGGING.IPYKERNEL6_STATUS',
clickedRunByLine = 'DATASCIENCE.DEBUGGING.CLICKED_RUNBYLINE',
successfullyStartedRunByLine = 'DATASCIENCE.DEBUGGING.SUCCESSFULLY_STARTED_RUNBYLINE',
clickedRunAndDebugCell = 'DATASCIENCE.DEBUGGING.CLICKED_RUN_AND_DEBUG_CELL',
successfullyStartedRunAndDebugCell = 'DATASCIENCE.DEBUGGING.SUCCESSFULLY_STARTED_RUN_AND_DEBUG_CELL',
endedSession = 'DATASCIENCE.DEBUGGING.ENDED_SESSION'
}
99 changes: 75 additions & 24 deletions src/client/debugger/jupyter/debuggingManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ import {
DebugSessionOptions,
DebugConfiguration,
EventEmitter,
DebugProtocolMessage
DebugProtocolMessage,
ProgressLocation
} from 'vscode';
import * as path from 'path';
import { IKernel, IKernelProvider } from '../../datascience/jupyter/kernels/types';
import { IDisposable, IInstaller, Product, ProductInstallStatus } from '../../common/types';
import { IDisposable, Product, ProductInstallStatus } from '../../common/types';
import { IKernelDebugAdapterConfig, KernelDebugAdapter, KernelDebugMode } from './kernelDebugAdapter';
import { INotebookProvider } from '../../datascience/types';
import { IExtensionSingleActivationService } from '../../activation/types';
Expand All @@ -34,8 +35,9 @@ import { Commands as DSCommands } from '../../datascience/constants';
import { IFileSystem } from '../../common/platform/types';
import { IDebuggingManager } from '../types';
import { DebugProtocol } from 'vscode-debugprotocol';
import { pythonKernelDebugAdapter } from '../constants';
import { DebuggingTelemetry, pythonKernelDebugAdapter } from '../constants';
import { IPythonInstaller } from '../../api/types';
import { sendTelemetryEvent } from '../../telemetry';

class Debugger {
private resolveFunc?: (value: DebugSession) => void;
Expand Down Expand Up @@ -93,8 +95,7 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
@inject(IVSCodeNotebook) private readonly vscNotebook: IVSCodeNotebook,
@inject(IFileSystem) private fs: IFileSystem,
@inject(IPythonInstaller) private pythonInstaller: IPythonInstaller,
@inject(IInstaller) private readonly installer: IInstaller
@inject(IPythonInstaller) private pythonInstaller: IPythonInstaller
) {
this.debuggingInProgress = new ContextKey(EditorContexts.DebuggingInProgress, this.commandManager);
this.runByLineInProgress = new ContextKey(EditorContexts.RunByLineInProgress, this.commandManager);
Expand Down Expand Up @@ -173,36 +174,77 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
this.updateToolbar(true);
void this.startDebugging(editor.document);
} else {
void this.installIpykernel6(editor.document);
void this.installIpykernel6();
}
} else {
void this.appShell.showErrorMessage(DataScience.noNotebookToDebug());
}
}),

this.commandManager.registerCommand(DSCommands.RunByLine, async (cell: NotebookCell) => {
this.commandManager.registerCommand(DSCommands.RunByLine, async (cell: NotebookCell | undefined) => {
sendTelemetryEvent(DebuggingTelemetry.clickedRunByLine);
void this.appShell.withProgress(
{ location: ProgressLocation.Notification, title: DataScience.startingRunByLine() },
async () => {
const editor = this.vscNotebook.activeNotebookEditor;
if (!cell) {
const range = editor?.selections[0];
if (range) {
cell = editor?.document.cellAt(range.start);
}
}

if (!cell) {
return;
}

if (editor) {
if (await this.checkForIpykernel6(editor.document)) {
this.updateToolbar(true);
this.updateCellToolbar(true);
await this.startDebuggingCell(editor.document, KernelDebugMode.RunByLine, cell);
} else {
void this.installIpykernel6();
}
} else {
void this.appShell.showErrorMessage(DataScience.noNotebookToDebug());
}
}
);
}),

this.commandManager.registerCommand(DSCommands.RunByLineContinue, (cell: NotebookCell | undefined) => {
const editor = this.vscNotebook.activeNotebookEditor;
if (editor) {
if (await this.checkForIpykernel6(editor.document)) {
this.updateToolbar(true);
this.updateCellToolbar(true);
void this.startDebuggingCell(editor.document, KernelDebugMode.RunByLine, cell);
} else {
void this.installIpykernel6(editor.document);
if (!cell) {
const range = editor?.selections[0];
if (range) {
cell = editor?.document.cellAt(range.start);
}
} else {
void this.appShell.showErrorMessage(DataScience.noNotebookToDebug());
}
}),

this.commandManager.registerCommand(DSCommands.RunByLineContinue, (cell: NotebookCell) => {
if (!cell) {
return;
}

const adapter = this.notebookToDebugAdapter.get(cell.notebook);
if (adapter && adapter.debugCellUri?.toString() === cell.document.uri.toString()) {
adapter.runByLineContinue();
}
}),

this.commandManager.registerCommand(DSCommands.RunByLineStop, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Question is the cell specificity needed here? For any editor there would only be one RBL session. So do we need to look for selection and range? Or could we just stop RBL based on the editor?

Copy link
Author

Choose a reason for hiding this comment

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

good point

const editor = this.vscNotebook.activeNotebookEditor;
if (editor) {
const adapter = this.notebookToDebugAdapter.get(editor.document);
if (adapter) {
sendTelemetryEvent(DebuggingTelemetry.endedSession, undefined, { reason: 'withKeybinding' });
adapter.disconnect();
}
}
}),

this.commandManager.registerCommand(DSCommands.RunAndDebugCell, async (cell: NotebookCell | undefined) => {
sendTelemetryEvent(DebuggingTelemetry.clickedRunAndDebugCell);
const editor = this.vscNotebook.activeNotebookEditor;
if (!cell) {
const range = editor?.selections[0];
Expand All @@ -220,7 +262,7 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
this.updateToolbar(true);
void this.startDebuggingCell(editor.document, KernelDebugMode.Cell, cell);
} else {
void this.installIpykernel6(editor.document);
void this.installIpykernel6();
}
} else {
void this.appShell.showErrorMessage(DataScience.noNotebookToDebug());
Expand Down Expand Up @@ -345,22 +387,31 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
controller?.connection.interpreter
);

if (result === ProductInstallStatus.Installed) {
sendTelemetryEvent(DebuggingTelemetry.ipykernel6Status, undefined, { status: 'installed' });
} else {
sendTelemetryEvent(DebuggingTelemetry.ipykernel6Status, undefined, { status: 'notInstalled' });
}
return result === ProductInstallStatus.Installed;
} catch {
return false;
}
}

private async installIpykernel6(doc: NotebookDocument) {
private async installIpykernel6() {
const response = await this.appShell.showInformationMessage(
DataScience.needIpykernel6(),
{ modal: true },
DataScience.jupyterInstall()
DataScience.setup()
);

if (response === DataScience.jupyterInstall()) {
const controller = this.notebookControllerManager.getSelectedNotebookController(doc);
void this.installer.install(Product.ipykernel, controller?.connection.interpreter, undefined, true);
if (response === DataScience.setup()) {
sendTelemetryEvent(DebuggingTelemetry.clickedOnSetup);
this.appShell.openUrl(
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems crappy to me. You try to debug and now you have to go to a website to try and install ipykernel 6? We can't do a best effort and then if it fails send them to the website?

Copy link
Author

Choose a reason for hiding this comment

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

I had this conversation with Claudia. Some people might quit if the installer doesn't work. That's double the annoyance, try to debug, click 'Intall', fail, have to go to a website. We think this way is more effective until we can fix the installer in the python extension.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sounds like it was discussed, but I have to say, seems pretty crummy for a user experience. Did the installer really fail that often? Was this just for the older users on 3.6?

Copy link
Author

Choose a reason for hiding this comment

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

Sometimes it installs it in the wrong conda env.
Sometimes it fails to activate the conda env because its using powershell.
If the python version is 3.6 or lower, the installation is a 'success' but you didn't actually upgrade to ipykernel 6, but to 5.5.5.

There might be more, but that's what I've seen so far.

Copy link
Author

Choose a reason for hiding this comment

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

I know its not ideal, its giving some trust to the user that they know how to navigate python envs. The bet is that they'll do it, after all, they're using notebooks and want to debug them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have telemetry on number of times ipykernel 6 install was tried and corresponding success on debugging a cell?

Copy link
Author

Choose a reason for hiding this comment

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

We don't, but we will starting next month. This PR has new telemetry for that.

'https://github.com/microsoft/vscode-jupyter/wiki/Setting-Up-Run-by-Line-and-Debugging-for-Notebooks'
);
} else {
sendTelemetryEvent(DebuggingTelemetry.closedModal);
}
}
}
17 changes: 17 additions & 0 deletions src/client/debugger/jupyter/kernelDebugAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import { IKernelDebugAdapter } from '../types';
import { IDisposable } from '../../common/types';
import { Commands } from '../../datascience/constants';
import { IKernel } from '../../datascience/jupyter/kernels/types';
import { sendTelemetryEvent } from '../../telemetry';
import { DebuggingTelemetry } from '../constants';

const debugRequest = (message: DebugProtocol.Request, jupyterSessionId: string): KernelMessage.IDebugRequestMsg => {
return {
Expand Down Expand Up @@ -152,6 +154,14 @@ export class KernelDebugAdapter implements DebugAdapter, IKernelDebugAdapter, ID
this.debugCellUri = notebookDocument.cellAt(configuration.__cellIndex!)?.document.uri;
}

if (configuration.__mode === KernelDebugMode.Cell) {
sendTelemetryEvent(DebuggingTelemetry.successfullyStartedRunAndDebugCell);
}

if (configuration.__mode === KernelDebugMode.RunByLine) {
sendTelemetryEvent(DebuggingTelemetry.successfullyStartedRunByLine);
}

this.jupyterSession.onIOPubMessage((msg: KernelMessage.IIOPubMessage) => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const content = msg.content as any;
Expand All @@ -166,9 +176,15 @@ export class KernelDebugAdapter implements DebugAdapter, IKernelDebugAdapter, ID

if (this.kernel) {
this.kernel.onWillRestart(() => {
sendTelemetryEvent(DebuggingTelemetry.endedSession, undefined, { reason: 'onARestart' });
this.disconnect();
});
this.kernel.onWillInterrupt(() => {
sendTelemetryEvent(DebuggingTelemetry.endedSession, undefined, { reason: 'onAnInterrupt' });
this.disconnect();
});
this.kernel.onDisposed(() => {
sendTelemetryEvent(DebuggingTelemetry.endedSession, undefined, { reason: 'onKernelDisposed' });
this.disconnect();
});
}
Expand All @@ -177,6 +193,7 @@ export class KernelDebugAdapter implements DebugAdapter, IKernelDebugAdapter, ID
(cellStateChange: NotebookCellExecutionStateChangeEvent) => {
// If a cell has moved to idle, stop the debug session
if (cellStateChange.state === NotebookCellExecutionState.Idle) {
sendTelemetryEvent(DebuggingTelemetry.endedSession, undefined, { reason: 'normally' });
this.disconnect();
}
},
Expand Down
13 changes: 13 additions & 0 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { populateTelemetryWithErrorInfo } from '../common/errors';
import { ErrorCategory, TelemetryErrorProperties } from '../common/errors/types';
import { noop } from '../common/utils/misc';
import { isPromise } from 'rxjs/internal-compatibility';
import { DebuggingTelemetry } from '../debugger/constants';

export const waitBeforeSending = 'waitBeforeSending';
/* eslint-disable @typescript-eslint/no-explicit-any */
Expand Down Expand Up @@ -1416,4 +1417,16 @@ export interface IEventNamePropertyMapping {
hasKernelSpecInMetadata: boolean; // Whether we have kernelspec info in the notebook metadata.
kernelConnectionFound: boolean; // Whether a kernel connection was found or not.
};
[DebuggingTelemetry.clickedOnSetup]: never | undefined;
[DebuggingTelemetry.closedModal]: never | undefined;
[DebuggingTelemetry.ipykernel6Status]: {
status: 'installed' | 'notInstalled';
};
[DebuggingTelemetry.clickedRunByLine]: never | undefined;
[DebuggingTelemetry.successfullyStartedRunByLine]: never | undefined;
[DebuggingTelemetry.clickedRunAndDebugCell]: never | undefined;
[DebuggingTelemetry.successfullyStartedRunAndDebugCell]: never | undefined;
[DebuggingTelemetry.endedSession]: {
reason: 'normally' | 'onKernelDisposed' | 'onAnInterrupt' | 'onARestart' | 'withKeybinding';
};
}