Debug attach first steps by IanMatthewHuff · Pull Request #6354 · microsoft/vscode-python

IanMatthewHuff · 2019-06-27T01:00:01Z

For #5900

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)
The wiki is updated with any design decisions/details.

IanMatthewHuff · 2019-06-27T01:01:22Z

This review might be a bit premature. Note the lack of tests :). I'd like to get some feedback on the structure and if we want to keep parallelizing this work it might be nice to get the first pass in. And then we could do both the connection to the file hashing and fleshing out the tests for this in parallel. #ByDesign

rchiodo · 2019-06-27T15:11:07Z

    show() : Promise<void>;
-    addCode(code: string, file: string, line: number, editor?: TextEditor) : Promise<void>;
-    // tslint:disable-next-line:no-any
+    addCode(code: string, file: string, line: number, editor?: TextEditor, debug?: boolean) : Promise<void>;


I think I'd rather have another function, like debugCode instead of a flag. Debugging feels like a first level operation to me? Thoughts? #Resolved

Heh, I had it as a function initially and then went back to a flag, I guess my feeling was that the interfaces were getting super bulky. The implementation is really close to the same, but the top level debug function could just pass a flag into an internal implementation function. I guess if I'm looking at a new interface I don't think that I would expect debug to be a feature on adding code so yeah, we can move to a top level function.

In reply to: 298228646 [](ancestors = 298228646)

I think we could probably split this interface into two or 3 parts. The add/execute part. The undo/redo/ui type parts. The preview part (which will be going away soon)

In reply to: 298239799 [](ancestors = 298239799,298228646)

But I don't think we should do it now :)

In reply to: 298240791 [](ancestors = 298240791,298239799,298228646)

rchiodo · 2019-06-27T15:12:48Z

+
+        // Stop our debugging UI session, no await as we just want it stopped
+        this.commandManager.executeCommand('workbench.action.debug.stop');
+    }


Feels like this entire thing could be in a separate class? IJupyterDebug or something like that? #Resolved

The actual code for this is pretty easy, but I do feel like I'm struggling a bit to get the class / interface structure correct. That's why I have it drawn up over on the whiteboard :P. I think that I want the InteractiveWindow to be aware and in charge of the start and stop debugger ide commands. But I want the server to control starting up the debugger and keeping track of the port / host. I'll try to see if I can refactor some of this out of the Interactive Window / Server and maybe into a new interface this morning and push a new commit.

In reply to: 298229482 [](ancestors = 298229482)

Do either the interactive window or the jupyter server need to know about ptvsd though? Each just talks to 'a debugger'. That thing knows about ptvsd so in case we need to use a different debugger (or the commands change how they interact), it's isolated in one spot?

In reply to: 298258803 [](ancestors = 298258803,298229482)

rchiodo · 2019-06-27T15:13:45Z

+                await this.debugService.startDebugging(undefined, config);
+
+                // tslint:disable-next-line:no-multiline-string
+                await this.jupyterServer.execute(`import ptvsd\r\nptvsd.wait_for_attach()`, Identifiers.EmptyFileName, 0, uuid(), undefined, true);


wait_for_attach [](start = 72, length = 15)

If you do this more than once, I assume it works okay? #ByDesign

Yup, we've detatched when we kill the debugger so we are fine for another wait here. And if we somehow didn't correctly kill our debugger session off (if we are still attached) then this operation is just a no op. That's how we got the ordering right here, by putting it after the IDE debug session start we'll either wait for the connection if it's slow or just no-op on the wait if we are already connected.

In reply to: 298229981 [](ancestors = 298229981)

rchiodo · 2019-06-27T15:15:19Z

            // Try to connect to our jupyter process. Check our setting for the number of tries
            let tryCount = 0;
            const maxTries = this.configuration.getSettings().datascience.jupyterLaunchRetries;
+            const enableDebugging = this.configuration.getSettings().datascience.enableDebugging ? this.configuration.getSettings().datascience.enableDebugging : false;


configuration [](start = 41, length = 13)

This feels like something that should be in the notebookserveroptions (it would determine the difference between reusing a cached server or not). The IInteractiveWindowProvider would use this setting of course to add it to the options. #Resolved

rchiodo · 2019-06-27T15:17:21Z

    }

+    public async setDebugTracing(tracingOn: boolean): Promise<void> {
+        await this.executeSilently(`from ptvsd import tracing\r\ntracing(${tracingOn ? 'True' : 'False'})`);


ptvsd [](start = 41, length = 5)

It's a bit odd that the ptvsd code is here and in the InteractiveWindow. Maybe only one of them should know about ptvsd? Or this would be a good reason to make a IJupyterDebugger object. It would have a connection to the server and would do all of the PTVSD specific stuff. (Like maintaining the connection string) #Resolved

rchiodo

🕐

IanMatthewHuff · 2019-06-27T21:44:07Z

+
+        // IANHU this current import is only for local testing with specific bits. The main ptvsd code is below
+        // tslint:disable-next-line:no-multiline-string
+        const enableDebuggerResults = await this.executeSilently(server, `import sys\r\nsys.path.append('d:/ptvsd-drop/kdrop/src')\r\nimport os\r\nos.environ["PTVSD_LOG_DIR"] = "d:/note_dbg/logs"\r\nimport ptvsd\r\nptvsd.enable_attach(('localhost', 0))`);


Just wanted to make sure that I called this out. This code currently depends on stuff that at least as of when I started was only in the private branch. Not fully sure about the best way to check this in for now, but this is what I did. I'll probably keep the logging part in as a comment even after shipping since that was really handy for debugging.

rchiodo

IanMatthewHuff added 4 commits June 26, 2019 08:19

first pass of debugging

92f3172

bp hit

33acc09

working E2E

fd074ef

review cleanup

9860504

IanMatthewHuff requested review from DavidKutu, joyceerhl and rchiodo June 27, 2019 01:00

rchiodo reviewed Jun 27, 2019

View reviewed changes

rchiodo requested changes Jun 27, 2019

View reviewed changes

IanMatthewHuff added 7 commits June 27, 2019 09:50

debugCode top level function

8d7ada0

put enable debugging on server options

e9c92e6

refactor working

5dbdb96

remove old comments

ac759a0

review changes

1d0c254

final cleanup

de4e435

add news

296266b

IanMatthewHuff commented Jun 27, 2019

View reviewed changes

rchiodo approved these changes Jun 27, 2019

View reviewed changes

IanMatthewHuff added 4 commits June 27, 2019 15:13

remove hard coded path

1e5b759

remove unused vars

b86d32d

fix functional tests

6be7ac1

merge with master changes

61ef5f4

IanMatthewHuff merged commit b28f62f into microsoft:master Jun 28, 2019

IanMatthewHuff deleted the dev/ianhu/debugAttach branch June 28, 2019 16:08

lock Bot locked as resolved and limited conversation to collaborators Jul 29, 2019

Conversation

IanMatthewHuff commented Jun 27, 2019

Uh oh!

IanMatthewHuff commented Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchiodo Jun 27, 2019 • edited by IanMatthewHuff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Jun 27, 2019 • edited by IanMatthewHuff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Jun 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Jun 27, 2019 • edited by IanMatthewHuff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Jun 27, 2019 • edited by IanMatthewHuff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Jun 27, 2019 • edited by IanMatthewHuff Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Jun 27, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IanMatthewHuff commented Jun 27, 2019 •

edited

Loading

rchiodo Jun 27, 2019 •

edited by IanMatthewHuff

Loading

rchiodo Jun 27, 2019 •

edited by IanMatthewHuff

Loading

IanMatthewHuff Jun 27, 2019 •

edited

Loading

rchiodo Jun 27, 2019 •

edited by IanMatthewHuff

Loading

rchiodo Jun 27, 2019 •

edited by IanMatthewHuff

Loading

rchiodo Jun 27, 2019 •

edited by IanMatthewHuff

Loading