Make a timeout for interrupt ask for a restart #4414

rchiodo · 2021-01-15T23:23:49Z

The jupyterNotebook class was being used for interrupting. The problem is the jupyterNotebook assumes it was the one that started an execution and waits for the last pending execution to finish before saying in interrupt succeeded. Since there wasn't any, interrupt returns super fast and doesn't wait for the keyboard exception.

Changed interrupt to happen in the cellExecution class instead.

Also added an interruptTimedOut event because interrupt doesn't actually go through the notebook editor (where the code lives to show a restart if a timeout occurs). This allows the UI for this to remain in the notebook editor.

Interrupt is working for me in native notebooks now but only if I make sure the jsonrpc stuff is setup.

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

codecov-io · 2021-01-15T23:46:32Z

Codecov Report

Merging #4414 (629609a) into main (e8f311a) will decrease coverage by 0%.
The diff coverage is 52%.

@@          Coverage Diff          @@
##            main   #4414   +/-   ##
=====================================
- Coverage     75%     75%   -1%     
=====================================
  Files        392     392           
  Lines      25627   25704   +77     
  Branches    3661    3673   +12     
=====================================
+ Hits       19371   19399   +28     
- Misses      4753    4790   +37     
- Partials    1503    1515   +12

Impacted Files	Coverage Δ
src/client/datascience/jupyter/kernels/types.ts	`72% <ø> (ø)`
.../client/datascience/notebook/kernelWithMetadata.ts	`85% <0%> (ø)`
src/client/datascience/notebook/notebookEditor.ts	`44% <16%> (ø)`
src/client/datascience/jupyter/kernels/kernel.ts	`66% <38%> (-8%)`	⬇️
...lient/datascience/jupyter/kernels/cellExecution.ts	`75% <52%> (-3%)`	⬇️
...ent/datascience/jupyter/kernels/kernelExecution.ts	`80% <68%> (-1%)`	⬇️
...ient/datascience/jupyter/kernels/kernelProvider.ts	`84% <88%> (-2%)`	⬇️
src/client/datascience/baseJupyterSession.ts	`76% <100%> (+<1%)`	⬆️
...ience/notebook/emptyNotebookCellLanguageService.ts	`65% <0%> (-5%)`	⬇️
...client/datascience/raw-kernel/rawJupyterSession.ts	`66% <0%> (-4%)`	⬇️
... and 22 more

IanMatthewHuff · 2021-01-16T00:45:18Z

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

@@ -118,6 +120,7 @@ export class CellExecution {
    private disposables: IDisposable[] = [];
    private cancelHandled = false;
    private requestHandlerChain = Promise.resolve();
+    private activeExecutions: { execution: Promise<void>; session: IJupyterSession }[] = [];


I've not played around with new native execution much, so this might just be a misunderstanding on my part (looking over it more now). But I thought that a new CellExecution was created each time we executed a notebook cell. So why would we be getting multiple active executions on one CellExecution?

The way the code is structured it's possible? There's nothing that prevents (from the CellExecution class) there being multiple executions for the same cell. But the calling class does prevent that.

I don't see how we can have more than one item in the array, if we do its incorrect or i'm mistaken.
Can we not have an array instead. As we create new instances of this class for each execution (even if we run the same cell multiple times).

DonJayamanne

Will look into this, please note we have tests today to ensure users are presented with an option to restart the kernel when interrupt fails.

src/test/datascience/notebook/interrupRestart.vscode.test.ts

Don'nt understand the issue, will read that soon & review the code..

DonJayamanne · 2021-01-19T18:06:26Z

However we can make native notebooks automatically restart if they take too long like we do for the old editor.

We have tests that do this today. So not sure what this is fixing.
Tests are here src/test/datascience/notebook/interrupRestart.vscode.test.ts

rchiodo · 2021-01-19T18:55:38Z

However we can make native notebooks automatically restart if they take too long like we do for the old editor.

We have tests that do this today. So not sure what this is fixing.
Tests are here src/test/datascience/notebook/interrupRestart.vscode.test.ts

The test passes if the interrupt succeeds. The old code would ALWAYS think it succeeded while the cell UI would actually still be running.

this line here:

        // Wait for interruption or message prompting to restart kernel to be displayed.
        // Interrupt can fail sometimes and then we display message prompting user to restart kernel.
        await waitForCondition(
            async () => deferred.completed || showInformationMessage.called,
            30_000, // Wait for completion or interrupt timeout.
            'Execution not cancelled'
        );

If the interrupt succeeds, deferred.completed would be true.

DonJayamanne · 2021-01-19T19:29:53Z

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

@@ -165,6 +168,23 @@ export class CellExecution {
        );
    }

+    public async interrupt(timeoutMs: number): Promise<InterruptResult> {
+        // Go through our active executions and interrupt each one.
+        const results = await Promise.all(


We might want to ensure this method cannot be called more than once & if already completed.
I.e. if this._interrupted || this._completed) { return false}
Where ``this._interruptedis a promise that contains the result for the previousinterrupt` call for this cell execution (i.e. all interrupts will be doing the same thing - pointing to the same promise)

& once the promise resolves we might want to clear that propery.
This way if user clicks multiple times, they will get the samae interrupt promise.
After user gets the interrupt error message if they try again, it should start a new interrupt sequence (without using the same - previously resolved promise).

DonJayamanne · 2021-01-19T19:30:33Z

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

@@ -394,7 +470,9 @@ export class CellExecution {
    private async execute(session: IJupyterSession, loggers: INotebookExecutionLogger[]) {
        const code = this.cell.document.getText();
        traceCellMessage(this.cell, 'Send code for execution');
-        return this.executeCodeCell(code, session, loggers);
+        const execution = this.executeCodeCell(code, session, loggers);
+        this.activeExecutions.push({ execution, session });


Might want to clear this property in the dispose method.

DonJayamanne · 2021-01-19T19:37:47Z

The old code would ALWAYS think it succeeded while the cell UI would actually still be running.

Not sure about this, as it works for me.
Anyways, like the new code. Approving it.

DonJayamanne

Minor issues,

Single item in array
Cache promise for interrupt (if user clicks interrupt multiple times)

Waiting for re review

rchiodo · 2021-01-19T21:19:02Z

@DonJayamanne can you have another look?

Make a timeout for interrupt ask for a restart

130dffb

rchiodo requested a review from a team as a code owner January 15, 2021 23:23

Fix linter

629609a

Another file to not ignore

5108bc9

DavidKutu approved these changes Jan 16, 2021

View reviewed changes

IanMatthewHuff reviewed Jan 16, 2021

View reviewed changes

DonJayamanne requested changes Jan 19, 2021

View reviewed changes

DonJayamanne reviewed Jan 19, 2021

View reviewed changes

DonJayamanne previously requested changes Jan 19, 2021

View reviewed changes

Code review feedback

ebc2064

rchiodo requested a review from DonJayamanne January 19, 2021 19:59

DonJayamanne approved these changes Jan 19, 2021

View reviewed changes

rchiodo merged commit 27cfffe into main Jan 19, 2021

rchiodo deleted the rchiodo/native_interrupt branch January 19, 2021 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make a timeout for interrupt ask for a restart #4414

Make a timeout for interrupt ask for a restart #4414

rchiodo commented Jan 15, 2021

codecov-io commented Jan 15, 2021 •

edited

Loading

IanMatthewHuff Jan 16, 2021

rchiodo Jan 16, 2021

DonJayamanne Jan 19, 2021

DonJayamanne left a comment •

edited

Loading

DonJayamanne commented Jan 19, 2021 •

edited

Loading

rchiodo commented Jan 19, 2021 •

edited

Loading

DonJayamanne Jan 19, 2021

DonJayamanne Jan 19, 2021

rchiodo Jan 19, 2021

DonJayamanne Jan 19, 2021

DonJayamanne commented Jan 19, 2021

DonJayamanne left a comment

rchiodo commented Jan 19, 2021

Make a timeout for interrupt ask for a restart #4414

Make a timeout for interrupt ask for a restart #4414

Conversation

rchiodo commented Jan 15, 2021

codecov-io commented Jan 15, 2021 • edited Loading

Codecov Report

IanMatthewHuff Jan 16, 2021

Choose a reason for hiding this comment

rchiodo Jan 16, 2021

Choose a reason for hiding this comment

DonJayamanne Jan 19, 2021

Choose a reason for hiding this comment

DonJayamanne left a comment • edited Loading

Choose a reason for hiding this comment

DonJayamanne commented Jan 19, 2021 • edited Loading

rchiodo commented Jan 19, 2021 • edited Loading

DonJayamanne Jan 19, 2021

Choose a reason for hiding this comment

DonJayamanne Jan 19, 2021

Choose a reason for hiding this comment

rchiodo Jan 19, 2021

Choose a reason for hiding this comment

DonJayamanne Jan 19, 2021

Choose a reason for hiding this comment

DonJayamanne commented Jan 19, 2021

DonJayamanne left a comment

Choose a reason for hiding this comment

rchiodo commented Jan 19, 2021

codecov-io commented Jan 15, 2021 •

edited

Loading

DonJayamanne left a comment •

edited

Loading

DonJayamanne commented Jan 19, 2021 •

edited

Loading

rchiodo commented Jan 19, 2021 •

edited

Loading