Keybindings and telemetry #7183
Conversation
-add keybindings -add progress notification when run by line starts -disconnect when the kernel is disposed
Codecov Report
@@ Coverage Diff @@
## main #7183 +/- ##
======================================
- Coverage 64% 64% -1%
======================================
Files 362 362
Lines 22887 22932 +45
Branches 3430 3446 +16
======================================
- Hits 14858 14801 -57
- Misses 6715 6820 +105
+ Partials 1314 1311 -3
|
| 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.
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.
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.
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.
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.
There was a problem hiding this comment.
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.
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.
We don't, but we will starting next month. This PR has new telemetry for that.
| "command": "jupyter.runByLineContinue", | ||
| "key": "f10", | ||
| "mac": "f10", | ||
| "when": "jupyter.notebookeditor.runByLineInProgress" |
There was a problem hiding this comment.
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.
The debugging manager checks for the active editor here. Unless you're on the editor, nothing will happen on the run by line side.
| 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.
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?
| { | ||
| "command": "jupyter.runByLine", | ||
| "key": "f10", | ||
| "mac": "f10", |
There was a problem hiding this comment.
Out of curiosity is the 'mac' entry necessary here? I assumed 'key' was sufficient.
There was a problem hiding this comment.
IDK, I don't have a mac to test it.
| @@ -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.
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?
For #7133, #7124
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).