Debugging (run by line) spike #6332

DavidKutu · 2021-06-18T21:57:04Z

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
Has a news entry file (remember to thank yourself!).
Appropriate comments and documentation strings in the code.
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated.
Test plan is updated as appropriate.
package-lock.json has been regenerated by running npm install (if dependencies have changed).

-remove debugger UI code -remove logs -normalize paths

cast on the kernel debug adapter

codecov-commenter · 2021-06-18T22:22:32Z

Codecov Report

Merging #6332 (e76e780) into main (b45c387) will decrease coverage by 0%.
The diff coverage is 30%.

❗ Current head e76e780 differs from pull request most recent head be54e0c. Consider uploading reports for the commit be54e0c to get more accurate results

@@          Coverage Diff           @@
##            main   #6332    +/-   ##
======================================
- Coverage     69%     69%    -1%     
======================================
  Files        407     410     +3     
  Lines      27918   28253   +335     
  Branches    4138    4186    +48     
======================================
+ Hits       19487   19598   +111     
- Misses      6774    6989   +215     
- Partials    1657    1666     +9

Impacted Files	Coverage Δ
src/client/common/installer/productInstaller.ts	`41% <0%> (-7%)`	⬇️
src/client/common/types.ts	`100% <ø> (ø)`
src/client/datascience/baseJupyterSession.ts	`70% <0%> (-2%)`	⬇️
...science/jupyter/kernels/kernelDependencyService.ts	`79% <0%> (-10%)`	⬇️
src/client/debugger/jupyter/kernelDebugAdapter.ts	`5% <5%> (ø)`
src/client/debugger/jupyter/debuggingManager.ts	`36% <36%> (ø)`
...client/datascience/commands/activeEditorContext.ts	`88% <68%> (-4%)`	⬇️
src/client/common/utils/localize.ts	`95% <100%> (+<1%)`	⬆️
src/client/datascience/constants.ts	`99% <100%> (+<1%)`	⬆️
...lient/datascience/jupyter/kernels/cellExecution.ts	`70% <100%> (+<1%)`	⬆️
... and 12 more

- start the kernel when starting a debug session (if its not started already) -toggle the breapoint margin automatically - handle the case when we attach and the kernel is stopped

and dump them all when the debug session starts

IanMatthewHuff · 2021-06-23T15:44:05Z

src/client/debugger/jupyter/debuggingCellMap.ts

+import { ICell, IDebuggingCellMap, INotebookExecutionLogger } from '../../datascience/types';
+
+@injectable()
+export class DebuggingCellMap implements IDebuggingCellMap, INotebookExecutionLogger {


Just a draft here, but this class could use some class comments about what it's responsible for.

DonJayamanne · 2021-06-23T16:37:32Z

src/client/datascience/types.ts

@@ -272,6 +273,7 @@ export const INotebookExecutionLogger = Symbol('INotebookExecutionLogger');
 export interface INotebookExecutionLogger extends IDisposable {
    preExecute(cell: ICell, silent: boolean): Promise<void>;
    postExecute(cell: ICell, silent: boolean, language: string, resource: Uri): Promise<void>;
+    nativePostExecute(cell: NotebookCell): Promise<void>;


Can we make this an optional method, this way you don't need to modify all other classes.

Suggested change

nativePostExecute(cell: NotebookCell): Promise<void>;

nativePostExecute?(cell: NotebookCell): Promise<void>;

IanMatthewHuff · 2021-06-23T16:47:10Z

src/client/datascience/baseJupyterSession.ts

@@ -63,6 +63,12 @@ export abstract class BaseJupyterSession implements IJupyterSession {
        }
        return this.onStatusChangedEvent.event;
    }
+    public get onIOPubMessageSignal(): Event<KernelMessage.IIOPubMessage> {


Might want to remove the "Signal" from the name here, it's only a Signal down in the Juypter code doesn't really make sense to have that on the name here.

IanMatthewHuff · 2021-06-24T12:55:22Z

src/client/debugger/jupyter/debuggingManager.ts

+    ) {}
+
+    public async activate() {
+        vscode.debug.breakpoints; // start to fetch breakpoints


In the API this looks like a non-async array, why are the results just ignored here?

This seems really weird to do on extension activation? Fetch breakpoints?

rchiodo · 2021-06-24T23:13:30Z

src/client/common/installer/productInstaller.ts

+            .get<IPythonExecutionFactory>(IPythonExecutionFactory)
+            .createActivatedEnvironment({ resource: uri, interpreter, allowEnvironmentFetchExceptions: true });
+
+        const output = pythonProcess.execObservable(['-m', 'pip', 'show', executableName], {});


Does this work for conda? Meaning will running pip show give you the correct information in a conda environment?

Why not use -m ipython --version or the existing api as mentioned by ian

This is the reason:

I think the python api uses the bottom command, and it gives the version of something different (its ipython) and that wouldn't work. I'll check their code, but for this PR we might want to do it ourselves.

And fix it for Conda of course.

I'm not a python expert (could consider asking the python folks). But an easier way if we can't use the python -m version check or python api then might be to activate the environment and do like this:

python -c "import ipykernel; print(ipykernel.version)"

I'll try it, although it would be nice to not have to activate it

rchiodo · 2021-06-24T23:15:44Z

src/client/datascience/jupyter/kernels/kernelDependencyService.ts

+        interpreter: PythonEnvironment,
+        _token?: CancellationToken
+    ): Promise<boolean> {
+        return this.installer.getVersion(Product.ipykernel, interpreter).then((version) => {


Would async/await be more readable here? You forgot a 'catch' clause I think. Might be easier with try, await getVersion, parse versoin, catch - return false

cleanup commit
although, I will check if we can accomplish this with the python api.

There's the issue that python -m ipykernel --version returns the ipython version instead of the ipykernel version. Hopefully the python api deals with that.

rchiodo · 2021-06-24T23:20:32Z

src/client/debugger/jupyter/debuggingManager.ts

+                }
+            }),
+
+            vscode.commands.registerCommand('jupyter.debugNotebook', () => {


I think we have commands in a 'commands.ts' file? So that this string is one spot.

cleanup commit

rchiodo · 2021-06-24T23:24:58Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+            await this.debugInfo();
+        }
+
+        // after disconnecting, hide the breakpoint margin


Can you elaborate a bit in this comment? I assume detaching is the same as hitting the 'debug' button on the toolbar. Can the user also cause this by just hitting the stop button during debugging?

Seems weird to me. Stopping during debugging should just interrrupt the cell I think.

normally nothing needs to happen here. But since vscode has a command that shows and hides the breakpoint margin for notebooks, we need to hide it at some point. And the best time to do it is on the disconnect command.

rchiodo · 2021-06-24T23:25:55Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+                            const cell = this.fileToCell.get(source.path);
+                            if (cell) {
+                                source.name = path.basename(cell.document.uri.path);
+                                if (cell.index >= 0) {
+                                    source.name += `, Cell ${cell.index + 1}`;
+                                }
+                                source.path = cell.document.uri.toString();


I think this is common with the code above? Meaning can you make a separate function or lambda?

cleanup commit

rchiodo · 2021-06-24T23:27:21Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+        const cell = this.notebookDocument.getCells().find((c) => c.document.uri.toString() === uri);
+        if (cell) {
+            try {
+                const response = await this.session.customRequest('dumpCell', { code: cell.document.getText() });


I guess this means the kernel has to support this request. I guess it's okay cause ipykernel is probably the only reasonable case for debugging, but maybe we should flag this somehow so we don't forget it when we try getting debugging working elsewhere.

What do you mean by flagging? With a comment?

Yeah with a comment here and maybe an issue? ipykernel supports 'dumpcell' but does xeus?

move the debug context to its own function

IanMatthewHuff · 2021-06-25T15:01:43Z

src/client/common/installer/productInstaller.ts

@@ -126,6 +126,54 @@ export abstract class BaseInstaller {
            .install(product, resource, cancel, reInstallAndUpdate);
    }

+    public async getVersion(product: Product, resource?: InterpreterUri): Promise<string | undefined> {


Did using the pythonAPI function not work here?

This is the reason:

I think the python api uses the bottom command, and it gives the version of something different (its ipython) and that wouldn't work. I'll check their code, but for this PR we might want to do it ourselves.

And fix it for Conda of course.

For modules (which ipykernel should be treated as), I believe the Python API uses

python -c 'import ipykernel; print(ipykernel.__version__)'

which would yield the right result

https://github.com/microsoft/vscode-python/blob/362eb34cafcbc513919eb4d78ea0521a0ed6738a/src/client/common/process/internal/python.ts#L104-L112

IanMatthewHuff · 2021-06-25T15:06:45Z

src/client/datascience/commands/activeEditorContext.ts

@@ -173,6 +180,31 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
        }
        this.updateContextOfActiveNotebookKernel(activeEditor);
    }
+    private async updateDebugContext(activeEditor?: INotebookEditor, interpreter?: PythonEnvironment) {


Where is the second parameter case (interpreter) coming from? The only usage of this looks to just be passed an editor.

That one used to come from kernel status change, but that didn't work right. I need to create an event for kernel change.

IanMatthewHuff · 2021-06-25T15:15:07Z

src/client/debugger/jupyter/debuggingManager.ts

+
+    private getDebuggerByUri(document: vscode.NotebookDocument): Debugger | undefined {
+        for (const [doc, dbg] of this.notebookToDebugger.entries()) {
+            if (document.uri.toString() === doc.uri.toString()) {


Per-discussion here: #6358 (comment) I believe we can just compare on documents.

cleanup commit

IanMatthewHuff · 2021-06-25T15:21:09Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+            version: '5.2',
+            msg_type: 'debug_request',
+            username: 'vscode',
+            session: randomBytes(8).toString('hex')


This might work for the spike, but for JMP messages session should be unique per session. It's not random per message. You can get the id of an existing session off the ISession on the IJupyterSession.

IanMatthewHuff · 2021-06-25T15:28:25Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+
+                    this.sendMessage.fire(msg.content);
+                };
+                control.onStdin = (msg) => {


Does anything actually come in on stdin?

yeah, nothing, I'll remove it

DonJayamanne · 2021-06-25T15:38:12Z

package.json

+        "debuggers": [
+            {
+                "type": "kernel",
+                "label": "Kernel Debug"


II think this should be called Jupyter Python or Python Kernel or similar, as it's specific to python not just any kernel.
This will matter in full debugging, user needs to select the right debug config (based on the label)

I changed it to Python Kernel on the cleanup commit

IanMatthewHuff · 2021-06-25T15:40:07Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+    /**
+     * Dump content of given cell into a tmp file and return path to file.
+     */
+    private async dumpCell(uri: string): Promise<void> {


Who has ownership of the file here? Looks like ipykernel returns back a sourcePath for the file created in a success message. But I'm not seeing cleanup on the ipykernel side.

IanMatthewHuff · 2021-06-25T15:45:26Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+        const response = await this.session.customRequest('debugInfo');
+
+        // If there's breakpoints at this point, delete them
+        if (response.breakpoints.lenght > 0) {


This looks misspelled, "length." Sorry in a PR I can't see the type easily is it an any? Not sure why this would build.

cleanup commit
I created types for these responses to avoid that

IanMatthewHuff · 2021-06-25T15:47:30Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+                command: 'setBreakpoints',
+                arguments: {
+                    source: {
+                        path: response.breakpoints[0].source


Will they all have the same source here?

cleanup commit
Small change, but instead of deleting the breakpoints on the kernel, I instead sent a message to VS Code to register them.

DavidKutu · 2021-06-25T23:08:46Z

To do

Add context for switching kernels and notebook opened
cache valid kernels so the button enables faster
don't make up a random session ids
delete temp files (the kernel is not doing this)
use python api for checking ipykernel version
use IDisposableRegistry
check if we need the vscode.debug.breakpoints on activate

rchiodo · 2021-06-28T18:20:34Z

src/client/datascience/commands/activeEditorContext.ts

+        if (activeEditor) {
+            this.notebookProvider
+                .getOrCreateNotebook({ identity: activeEditor.file, resource: activeEditor.file, getOnly: true })
+                .then(async (nb) => {


why not just an await here instead of chaining stuff?

rchiodo · 2021-06-28T18:22:37Z

src/client/debugger/jupyter/kernelDebugAdapter.ts

+        const cell = this.notebookDocument.getCells().find((c) => c.document.uri.toString() === uri);
+        if (cell) {
+            try {
+                const response = await this.session.customRequest('dumpCell', { code: cell.document.getText() });


Yeah with a comment here and maybe an issue? ipykernel supports 'dumpcell' but does xeus?

DavidKutu · 2021-06-28T18:26:16Z

@rchiodo for your second comment, I added this at the top of the file

vscode-jupyter/src/client/debugger/jupyter/kernelDebugAdapter.ts

Line 89 in e3bdfd1

// For info on the custom requests implemented by jupyter see:

I put it there because the whole debug adapter need support for those 2 special messages.

-pr comment on chaining stuff vs await

David added 9 commits June 16, 2021 10:34

initial work

b102702

debugger launches and sends messages

9d581cf

print ipykernel debug messages

bc9f44f

send messages back and forth

57ee57b

dispose controlFutures

4626bbf

-add iopub message signal

e9bfc0d

-remove debugger UI code -remove logs -normalize paths

remove messaging

1e4d9f9

cast on the kernel debug adapter

remove unused type

3547663

remove xeus name

e122789

oops

a83d4b9

DavidKutu changed the title ~~David/debuging~~ Debugging (run by line) spike Jun 18, 2021

David added 8 commits June 18, 2021 15:58

lint

da52ef4

lint

74eb0a5

- add the debug comnd as a button in the nb toolbar

18f7691

- start the kernel when starting a debug session (if its not started already) -toggle the breapoint margin automatically - handle the case when we attach and the kernel is stopped

-create a static obj that saves undumped cells

d82d249

and dump them all when the debug session starts

Merge branch 'main' into david/debuging

980900e

lint

9104c83

lint

7726a1e

lint

90a258f

IanMatthewHuff reviewed Jun 23, 2021

View reviewed changes

DonJayamanne reviewed Jun 23, 2021

View reviewed changes

IanMatthewHuff reviewed Jun 23, 2021

View reviewed changes

David added 6 commits June 23, 2021 10:17

make sure kernel is running

b46d151

naming changes

e387ef4

add comments to describe DebuggingCellMap

fd69b59

oops

80c372d

lint

13253e2

remove work in progress stuff

f3b0057

IanMatthewHuff reviewed Jun 24, 2021

View reviewed changes

DavidKutu marked this pull request as ready for review June 24, 2021 23:11

DavidKutu requested a review from a team as a code owner June 24, 2021 23:11

rchiodo reviewed Jun 24, 2021

View reviewed changes

disable restart and interrupt while debugging

95b16ed

move the debug context to its own function

IanMatthewHuff reviewed Jun 25, 2021

View reviewed changes

DonJayamanne reviewed Jun 25, 2021

View reviewed changes

IanMatthewHuff reviewed Jun 25, 2021

View reviewed changes

David added 2 commits June 25, 2021 15:52

cleanup

d4fab24

remove calls to console

e3bdfd1

David added 3 commits June 25, 2021 16:09

lint

d794b15

add hidden setting

59c653d

Merge branch 'main' into david/debuging

e76e780

rchiodo approved these changes Jun 28, 2021

View reviewed changes

-use constant for 'Python Kernel Debug Adapter'

be54e0c

-pr comment on chaining stuff vs await

DavidKutu merged commit ce2739a into main Jun 28, 2021

DavidKutu deleted the david/debuging branch June 28, 2021 20:07

DavidKutu mentioned this pull request Jun 28, 2021

Pending work for basic debugging #6391

Closed

	nativePostExecute(cell: NotebookCell): Promise<void>;
	nativePostExecute?(cell: NotebookCell): Promise<void>;

Debugging (run by line) spike #6332

Debugging (run by line) spike #6332

Uh oh!

Conversation

DavidKutu commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DavidKutu commented Jun 18, 2021 •

edited

Loading

codecov-commenter commented Jun 18, 2021 •

edited

Loading

DavidKutu commented Jun 25, 2021 •

edited

Loading