Enable Web Debugging #10040
Enable Web Debugging #10040
Conversation
…tem usage in kernelDebugAdapter
|
@rchiodo To make sure. Per my reading of how the web tests work, this change wouldn't need a new web test as it's not adding new web only functionality just unifying web and node. However I do think that we have a possible missing test hole checking for if the temp files are deleted. So I'll add an issue for that and add one if missing. |
|
I think @DonJayamanne just hit some of the same files. Will pull and update (see how git handles two renames / two moves on the same file) |
Codecov Report
@@ Coverage Diff @@
## main #10040 +/- ##
======================================
Coverage 63% 64%
======================================
Files 216 216
Lines 10127 10130 +3
Branches 1612 1613 +1
======================================
+ Hits 6474 6485 +11
+ Misses 3133 3123 -10
- Partials 520 522 +2
|
| pass | ||
| del _VSCODE_fileList`; | ||
|
|
||
| return executeSilently(this.jupyterSession, deleteFilesCode, { |
There was a problem hiding this comment.
Any idea why the execution is set to silent: false, is this intentional
vscode-jupyter/src/kernels/helpers.ts
Line 1452 in bf8e4ad
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Jupyter has doc on the execute params here which detail it a bit more: https://jupyter-client.readthedocs.io/en/latest/messaging.html#execute
There was a problem hiding this comment.
@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.
| @@ -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() { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Fixes #9973
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).