-
Notifications
You must be signed in to change notification settings - Fork 301
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
Changes from 5 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -277,6 +277,24 @@ | |
"command": "jupyter.runAndDebugCell", | ||
"key": "ctrl+alt+shift+enter", | ||
"mac": "ctrl+shift+enter" | ||
}, | ||
{ | ||
"command": "jupyter.runByLine", | ||
"key": "f10", | ||
"mac": "f10", | ||
"when": "!jupyter.notebookeditor.debuggingInProgress && !jupyter.notebookeditor.runByLineInProgress" | ||
}, | ||
{ | ||
"command": "jupyter.runByLineContinue", | ||
"key": "f10", | ||
"mac": "f10", | ||
"when": "jupyter.notebookeditor.runByLineInProgress" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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": [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
"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.", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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; | ||
|
@@ -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); | ||
|
@@ -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, () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||
|
@@ -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()); | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometimes it installs it in the wrong conda env. There might be more, but that's what I've seen so far. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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-Debugging-for-Notebooks' | ||
); | ||
} else { | ||
sendTelemetryEvent(DebuggingTelemetry.closedModal); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.