prevent debugpy from getting stuck #7612

DavidKutu · 2021-09-21T23:38:54Z

-prevent debugpy from getting stuck by running code
-if we can't continue in RBL, disconnect

For #7586, #7584 and parts 1 and 7 of #7218

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).

by running code -throttle all debugging functions -if we can't continue in RBL, disconnect -have run and debug button respect 'notebook.consolidatedRunButton' setting

roblourens · 2021-09-21T23:49:09Z

package.json

                }
            ],
            "notebook/cell/execute": [
                {
                    "command": "jupyter.runAndDebugCell",
-                    "when": "notebookType == jupyter-notebook && jupyter.ispythonnotebook && notebookCellType == code && isWorkspaceTrusted && !jupyter.notebookeditor.runByLineInProgress"
+                    "when": "notebookType == jupyter-notebook && jupyter.ispythonnotebook && notebookCellType == code && isWorkspaceTrusted && config.notebook.consolidatedRunButton"


Why this change? It always goes in that menu regardless of the setting by design. Probably will rename the setting or rethink this but I don't want it on the cell toolbar.

why not?

I moved it because some people will probably want the setting to get rid of the drop down on the run button

I'm not going to add another button to every cell toolbar unless masses of angry users kick down my door

roblourens · 2021-09-21T23:51:24Z

src/client/debugger/jupyter/debuggingManager.ts

+
+            this.commandManager.registerCommand(
+                DSCommands.RunByLineNext,
+                throttle((cell: NotebookCell | undefined) => {


Why throttle this? When I am doing normal javascript debugging, I can step as fast as I want. I can even hold down F10 to step really fast. The same should work here.

Because that seems to be the issue, clicking fast enough causes debugpy to freeze.

The changes to the controller are what gets it out of the situation (finishes that frozen session and un freezes debugpy).
The throttling is meant to prevent the situation.

Its removable, but I'd rather have at least some.

It's hard to believe that debugpy just can't step fast. I can debug a normal python script and step over 50 lines in a couple seconds

I agree, I think we need to identify the root cause, this feels like a bandaid

I think they problem is not because it happens fast, it's because we start multiple debug sessions instead of checkingbif we have already started one

roblourens · 2021-09-21T23:53:18Z

src/client/debugger/jupyter/debuggingManager.ts

-                        cell = editor?.document.cellAt(range.start);
+            this.commandManager.registerCommand(
+                DSCommands.RunAndDebugCell,
+                throttle(async (cell: NotebookCell | undefined) => {


I worry it will feel broken if this command sometimes doesn't work if stuff happens too quickly

Yeah throttle of 1 second seems like way too much. Maybe like 100ms

codecov-commenter · 2021-09-21T23:59:38Z

Codecov Report

Merging #7612 (58237c8) into main (6fb16b4) will increase coverage by 2%.
The diff coverage is 37%.

❗ Current head 58237c8 differs from pull request most recent head 8c53b34. Consider uploading reports for the commit 8c53b34 to get more accurate results

@@          Coverage Diff           @@
##            main   #7612    +/-   ##
======================================
+ Coverage     66%     68%    +2%     
======================================
  Files        365     363     -2     
  Lines      22660   22550   -110     
  Branches    3445    3429    -16     
======================================
+ Hits       15111   15516   +405     
+ Misses      6263    5704   -559     
- Partials    1286    1330    +44

Impacted Files	Coverage Δ
src/client/debugger/jupyter/debuggingManager.ts	`61% <35%> (+35%)`	⬆️
src/client/debugger/jupyter/debugControllers.ts	`71% <50%> (+61%)`	⬆️
...ient/datascience/jupyter/kernels/kernelSelector.ts	`57% <0%> (-24%)`	⬇️
...lient/datascience/notebookAndInteractiveTracker.ts	`76% <0%> (-8%)`	⬇️
...ent/datascience/jupyter/jupyterNotebookProvider.ts	`70% <0%> (-5%)`	⬇️
...rc/client/datascience/notebookStorage/baseModel.ts	`70% <0%> (-4%)`	⬇️
...ient/datascience/raw-kernel/rawNotebookProvider.ts	`66% <0%> (-4%)`	⬇️
src/client/datascience/telemetry/telemetry.ts	`72% <0%> (-3%)`	⬇️
...datascience/interactive-common/notebookProvider.ts	`79% <0%> (-3%)`	⬇️
src/client/datascience/webviews/webviewHost.ts	`77% <0%> (-2%)`	⬇️
... and 55 more

DonJayamanne · 2021-09-22T02:08:50Z

src/client/debugger/jupyter/debugControllers.ts

            return;
        }

        void this.debugAdapter.stepIn(this.lastPausedThreadId);
    }

    public stop(): void {
+        void executeDebugPyFix(this.kernel);


Some comments would be good here, didn't make sense why this is required
Why do we need to setup the debugger again when it's already running

I'm also curious about this, we execute this code when we start, why do we have to execute it when we stop?

DonJayamanne · 2021-09-22T02:09:30Z

src/client/debugger/jupyter/debugControllers.ts

+    await debugAdapter.dumpCell(debugCell.index);
+}
+
+async function executeDebugPyFix(kernel: IKernel) {
    // remove this if when https://github.com/microsoft/debugpy/issues/706 is fixed and ipykernel ships it
    // executing this code restarts debugpy and fixes https://github.com/microsoft/vscode-jupyter/issues/7251
    if (kernel) {


This is always not undefined

DonJayamanne · 2021-09-22T02:13:20Z

src/client/debugger/jupyter/debuggingManager.ts

+            this.commandManager.registerCommand(
+                DSCommands.DebugNotebook,
+                throttle(async () => {
+                    const editor = this.vscNotebook.activeNotebookEditor;


I think the solution is, if we're already debugging or have started then return from here.
I'd if in progress then get out, mashed three throttle unnecessary.

DonJayamanne · 2021-09-22T02:14:14Z

src/client/debugger/jupyter/debuggingManager.ts

-                        this.updateToolbar(true);
-                        this.updateCellToolbar(true);
-                        await this.startDebuggingCell(editor.document, KernelDebugMode.RunByLine, cell);
+                    if (editor) {


Same comment as earlier

DonJayamanne

We don't need the throttle

-check if we're already debugging -change code that we execute to get unfrozen

DonJayamanne · 2021-09-22T21:38:36Z

src/client/debugger/jupyter/debuggingManager.ts

@@ -95,6 +95,9 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
            this.commandManager.registerCommand(DSCommands.DebugNotebook, async () => {
                const editor = this.vscNotebook.activeNotebookEditor;
                if (editor) {
+                    if (this.notebookToDebugger.has(editor.document)) {


Not sure this is going to be enough.
If user clicks this button while checkforIpykernel6 is still running, then yo'ull end up with duplicate sessions.

I think you can move this entire if condition from line 98 to 109 into a seprate fucntion, this has been repeated in 3 places, and then you can easily check if that method has already been called once before if yes, then return.

a debugging session

roblourens · 2021-09-22T23:50:10Z

src/client/debugger/jupyter/debuggingManager.ts

@@ -232,6 +212,50 @@ export class DebuggingManager implements IExtensionSingleActivationService, IDeb
        this.runByLineInProgress.set(runningByLine).ignoreErrors();
    }

+    private async tryToStartDebugging(mode: KernelDebugMode, editor?: NotebookEditor, cell?: NotebookCell) {
+        if (editor) {
+            if (this.notebookInProgress.has(editor.document)) {


This is checking the case where debugging is starting, but doesn't it still need to check this.notebookToDebugger.has?

DonJayamanne · 2021-09-22T23:55:53Z

src/client/debugger/jupyter/debuggingManager.ts

+                            if (cell) {
+                                this.updateToolbar(true);
+                                this.updateCellToolbar(true);
+                                await this.startDebuggingCell(editor.document, KernelDebugMode.RunByLine, cell);


in line 233 we're not awaiting, but here we're awaiting.
Confused why that's the case, comments would help.

It'll be good to await them all

DonJayamanne · 2021-09-23T00:43:39Z

src/client/debugger/jupyter/debuggingManager.ts

+                    case KernelDebugMode.Cell:
+                        if (cell) {
+                            this.updateToolbar(true);
+                            await this.startDebuggingCell(editor.document, KernelDebugMode.Cell, cell);


I'm sorry, but I'm not convinced we need awaits here
The old code didn't have awaits, feels like there's something we're not understanding here
I.e. why do we need an await or what happens if there is no await?

Would like to have some comments why we must await, e.g. tomorrow what would stop working if we removed it.

it helps with keeping the context keys aligned with the status of the debugger. My most recent commit puts them after the awaits.

roblourens · 2021-09-23T01:39:00Z

It took longer (this is after many attempts) but I can still break it

and context keys shows its not

DavidKutu · 2021-09-23T07:01:44Z

@roblourens, the latest commit should catch your scenario

roblourens · 2021-09-23T16:09:52Z

src/client/debugger/jupyter/debuggingManager.ts

+        }
+
+        if (this.isDebugging(editor.document)) {
+            this.updateToolbar(true);


Why do you have to do this here? If you are already debugging, shouldn't it already be in this state?

it should, but that's the buggy state you get trapped in.

this would correct the context keys

Not sure i like this, feels like we're re-executing some code & that seems to fix a problem (but we don't know why).
I would like us to fix the root cause, why is this required in the first place

The root cause is out of our hands, its debugpy freezing inside ipykernel.

And I disagree, we should be trying to fix root causes, but since the root cause is not on our code, we should also try to fix it if it happens.

its debugpy freezing inside ipykernel.

If that's the cause, how can updating the toolbar fix anything in the kernel (i.e. updating a UI will not address anything in ipykernel freezing).
I think Rob and I would like to know why we need lines 225-230 when we're already debugging,
I dont' think this has to do with ipykernel state.
Unless I'm missing something here.

It seems like the root cause is us having the context keys out of sync with the RBL lifecycle. Your latest commit is a good one towards fixing that, I think previously we reset the context keys too early, before debugging actually stopped.

Fundamentally, at this point in the code, two things are out of sync. It's hard to point at one and say this one is always right. It should be possible to write the code so that they are always in sync.

start

DonJayamanne

.

DonJayamanne

Removing the unblocking request.

DonJayamanne

.

DonJayamanne · 2021-09-23T19:24:23Z

src/client/debugger/jupyter/debuggingManager.ts

                        break;
                    case KernelDebugMode.Cell:
                        if (cell) {
                            await this.startDebuggingCell(editor.document, KernelDebugMode.Cell, cell);
-                            this.updateToolbar(true);
+                            if (this.isDebugging(editor.document)) {


Under what circustances will this if condition be false, i don't think this can ever be false, its aleways true, it should be true, else the previous await should throw an error.
Having this if makes it look as though the previous code can run to completion without errors & not start the debugger (which i think is impossible)

I guess that's true, it just feels safer.
I'll remove them

DonJayamanne · 2021-09-23T19:24:38Z

src/client/debugger/jupyter/debuggingManager.ts

                        }
                        break;
                    case KernelDebugMode.RunByLine:
                        if (cell) {
                            await this.startDebuggingCell(editor.document, KernelDebugMode.RunByLine, cell);
-                            this.updateToolbar(true);
-                            this.updateCellToolbar(true);
+                            if (this.isDebugging(editor.document)) {


As earlier, i don't think this if condition is required.

DavidKutu · 2021-09-23T23:03:08Z

src/client/debugger/jupyter/debugControllers.ts

            return;
        }

        void this.debugAdapter.stepIn(this.lastPausedThreadId);
    }

    public stop(): void {
+        // When debugpy gets stuck, running a cell fixes it and allows us to start another debugging session
+        void this.kernel.executeHidden('pass');


I'll file a bug in ipykernel, and mention it here, but I don't see what else we can do

roblourens · 2021-09-24T00:44:13Z

src/client/debugger/jupyter/debugControllers.ts

@@ -57,13 +57,16 @@ export class RunByLineController implements IDebuggingDelegate {
    public continue(): void {
        if (typeof this.lastPausedThreadId !== 'number') {
            traceVerbose(`No paused thread, can't do RBL`);
+            this.stop();


With this, if I click the RBL button and then click it again before the first pause, it disconnects. That's not that bad of a problem but could be annoying, especially since it will then run the whole cell, even if I didn't want/expect it to.

I guess this is working around a case where we attach to debugpy and it doesn't respond, but if we have fixed the issue on our end of trying to attach twice at a time, then I don't understand how else we get into that case

roblourens · 2021-09-24T00:45:07Z

Played with it for awhile and was not able to totally break it, nice!

-prevent debugpy from getting stuck

f827552

by running code -throttle all debugging functions -if we can't continue in RBL, disconnect -have run and debug button respect 'notebook.consolidatedRunButton' setting

DavidKutu requested a review from a team as a code owner September 21, 2021 23:38

DavidKutu requested a review from roblourens September 21, 2021 23:39

roblourens reviewed Sep 21, 2021

View reviewed changes

DavidKutu requested review from roblourens and rchiodo September 22, 2021 01:14

DonJayamanne reviewed Sep 22, 2021

View reviewed changes

DonJayamanne requested changes Sep 22, 2021

View reviewed changes

David Kutugata added 2 commits September 22, 2021 13:50

undo debug cell button changes

014082d

-remove throttle

737cb03

-check if we're already debugging -change code that we execute to get unfrozen

DavidKutu requested a review from DonJayamanne September 22, 2021 21:30

DonJayamanne reviewed Sep 22, 2021

View reviewed changes

David Kutugata added 2 commits September 22, 2021 16:00

simplify into a function

2b4d0bc

return if notebook is in process of starting

c780500

a debugging session

DavidKutu requested a review from DonJayamanne September 22, 2021 23:30

oops

147ef55

roblourens reviewed Sep 22, 2021

View reviewed changes

Check if its already debugging

42a0e79

DonJayamanne reviewed Sep 22, 2021

View reviewed changes

imrpove readability

d55d4e9

DavidKutu requested review from DonJayamanne and roblourens September 23, 2021 00:00

DonJayamanne reviewed Sep 23, 2021

View reviewed changes

fix context keys in case session is going,

e1cb175

and context keys shows its not

roblourens reviewed Sep 23, 2021

View reviewed changes

change context keys after awaiting the debugger to

52fcf32

start

DonJayamanne reviewed Sep 23, 2021

View reviewed changes

DonJayamanne approved these changes Sep 23, 2021

View reviewed changes

DonJayamanne reviewed Sep 23, 2021

View reviewed changes

more context key management

9889241

DonJayamanne reviewed Sep 23, 2021

View reviewed changes

David Kutugata added 3 commits September 23, 2021 12:40

remove isDebugging conditions

7096f8c

fix failing test

ed8b70a

wait more

eb19626

DavidKutu commented Sep 23, 2021

View reviewed changes

David Kutugata added 2 commits September 23, 2021 16:27

try to fix tests

58237c8

tests

8c53b34

DavidKutu merged commit c6d0357 into main Sep 24, 2021

DavidKutu deleted the david/RBLDoesntStart branch September 24, 2021 00:18

roblourens reviewed Sep 24, 2021

View reviewed changes

roblourens mentioned this pull request Sep 9, 2022

debugpy gets "stuck" while using run by line in vscode jupyter notebook microsoft/debugpy#1051

Closed

prevent debugpy from getting stuck #7612

prevent debugpy from getting stuck #7612

Conversation

DavidKutu commented Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 21, 2021 • edited Loading

Codecov Report

DonJayamanne Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblourens commented Sep 23, 2021

DavidKutu commented Sep 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblourens commented Sep 24, 2021

DavidKutu commented Sep 21, 2021 •

edited

Loading

codecov-commenter commented Sep 21, 2021 •

edited

Loading

DonJayamanne Sep 22, 2021 •

edited

Loading

DonJayamanne Sep 23, 2021 •

edited

Loading