execute hidden code with the executionQueue #7763

DavidKutu · 2021-09-30T01:49:09Z

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

DavidKutu · 2021-09-30T01:51:38Z

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

        const stopWatch = new StopWatch();
        const notebookPromise = this.startNotebook();
-        const promise = notebookPromise.then((nb) => executeSilently(nb.session, code));
+
+        const promise = this.kernelExecution.executeHidden(notebookPromise, code, doc);


should we do this for all the uses of executeSilently?

No we should not, this has the potential to break variable viewer and others.
I think we'd should have two methods.
And we might want to rename the executeCell to queueAndExecuteCell,
Similarly executeHidden and queueAndExeceuteHidden

We'd should not because if we have 10 cells and we're thinking all, then variables will never refresh because those hidden requests will get queued and run only after all 10 cells finish. Similarly plenty of other pitfalls.
Never this new method should be banned to make it obvious how the code will be executed, ie the fact that it's queued.

DonJayamanne · 2021-09-30T02:04:29Z

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

@@ -147,7 +147,7 @@ export interface IKernel extends IAsyncDisposable {
    interrupt(): Promise<InterruptResult>;
    restart(): Promise<void>;
    executeCell(cell: NotebookCell): Promise<NotebookCellRunState>;
-    executeHidden(code: string): Promise<nbformat.IOutput[]>;
+    executeHidden(code: string, doc: NotebookDocument): Promise<nbformat.IOutput[]>;


We don't need to pass the document

It's already available in the class

We need it here to get the executionQueue

I don't see it in the class, there's documentExecutions which uses documents as keys, but there's no document in the class

Check line 113 of kernel.ts file

DonJayamanne · 2021-09-30T02:07:18Z

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

-        }
-        const cellExecution = this.executionFactory.create(cell, this.metadata);
-        this.queueOfCellsToExecute.push(cellExecution);
+    public queueCell(cell?: NotebookCell, code?: string): void {


This api allows me to pass two empty arguments.
It should instead be code : NotebookCell | string
This way it's impossible to pass undefined and it's obvious what's expected without having to look at the code

I think it's better to return a promise from this method, then we won't need waitForHidden.
Else you have two methods to deal with.

DonJayamanne · 2021-09-30T02:10:38Z

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

+interface Execution {
+    cellExecution?: CellExecution;
+    codeExecution?: CodeExecution;
+}


This means we can have a cake with both properties as empty, which is not correct.
We should change this to type Execution = CellExecution | CodeExecution

DonJayamanne · 2021-09-30T02:12:06Z

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

+            this.queueToExecute.push({ codeExecution });
+
+            traceInfo('Hidden cell queued for execution', codeExecution.code.substring(0, 50));
+        }



We have missed an else clause and that results in bugs, the suggestion I made will ensure typescript types will prevent such bugs

DonJayamanne · 2021-09-30T02:12:18Z

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

+                traceCellMessage(toExecute.cellExecution.cell, 'Before Execute individual cell');
+            } else if (toExecute.codeExecution) {
+                traceInfo('Before Execute hidden code', toExecute.codeExecution.code.substring(0, 50));
+            }


We have missed an else clause and that results in bugs, the suggestion I made will ensure typescript types will prevent such bugs

codecov-commenter · 2021-09-30T02:22:09Z

Codecov Report

Merging #7763 (0d76176) into main (f56ba6e) will decrease coverage by 0%.
The diff coverage is 69%.

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

@@          Coverage Diff           @@
##            main   #7763    +/-   ##
======================================
- Coverage     68%     68%    -1%     
======================================
  Files        363     364     +1     
  Lines      22593   22714   +121     
  Branches    3437    3456    +19     
======================================
+ Hits       15530   15607    +77     
- Misses      5725    5756    +31     
- Partials    1338    1351    +13

Impacted Files	Coverage Δ
...lient/datascience/jupyter/kernels/cellExecution.ts	`76% <0%> (ø)`
src/client/datascience/jupyter/kernels/types.ts	`100% <ø> (ø)`
...lient/datascience/jupyter/kernels/codeExecution.ts	`51% <51%> (ø)`
...ent/datascience/jupyter/kernels/kernelExecution.ts	`69% <80%> (+<1%)`	⬆️
src/client/datascience/jupyter/kernels/kernel.ts	`75% <87%> (+<1%)`	⬆️
.../datascience/jupyter/kernels/cellExecutionQueue.ts	`90% <91%> (+2%)`	⬆️
...atascience/interactive-window/interactiveWindow.ts	`53% <100%> (ø)`
...t/datascience/notebook/vscodeNotebookController.ts	`77% <100%> (ø)`
src/client/debugger/jupyter/helper.ts	`57% <100%> (ø)`
...client/datascience/kernel-launcher/kernelDaemon.ts	`56% <0%> (-2%)`	⬇️
... and 5 more

DonJayamanne · 2021-09-30T03:04:14Z

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

+                if (item.cellExecution) {
+                    void item.cellExecution.cancel(forced);
+                } else {
+                    void item.codeExecution?.cancel(forced);


We must await, that's why we have promise.all

DonJayamanne · 2021-09-30T03:06:00Z

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


        return Promise.all(cellsToCheck.map((cell) => cell.result));
    }
+    public async waitForHiddenOutput(code: string): Promise<nbformat.IOutput[]> {


This won't work if we have multiple expositions with the same code, e.g. pass or the like..
It will return the result from the first matching Execution with the same code

DonJayamanne · 2021-09-30T03:06:22Z

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

+        const execution = queue.find((exec) => exec.code === code);
+
+        if (execution) {
+            return Promise.resolve(execution.output);


Promise.resolve is unnecessary

DavidKutu · 2021-09-30T03:36:18Z

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

-        }
-        const cellExecution = this.executionFactory.create(cell, this.metadata);
-        this.queueOfCellsToExecute.push(cellExecution);
+    public queueCell(code: NotebookCell | string): Promise<nbformat.IOutput[]> {


I couldn't figure out how to return both types (Promise<NotebookCellRunState[] | nbformat.IOutput[]>) here and then use instanceof nbformat.IOutput[] on kernelExecution.ts.

If you do, let me know @DonJayamanne

I'd created two methods

Yeah, and as is the naming is quite awkward here at least in my view. It's called queueCell, but it takes a parameter called 'code' now not cell. And then the paths below are harder to read and easy to miss whens on. @DonJayamanne 's suggestion is good. If it was one method it would be better with a more explicit name like queueExecution, then casting early to either CodeExecution or CellExecution so that the code after that is easier to read, but two methods sounds like a legit idea.

+1 on a separate method

DonJayamanne · 2021-09-30T04:42:51Z

src/client/debugger/jupyter/helper.ts

@@ -7,7 +7,7 @@ import { IKernelDebugAdapterConfig, KernelDebugMode } from '../types';

 export async function isUsingIpykernel6OrLater(kernel: IKernel): Promise<boolean> {
    const code = 'import ipykernel\nprint(ipykernel.__version__)';
-    const output = await kernel.executeHidden(code);
+    const output = await kernel.queueAndExeceuteHidden(code);


This should not be queued, only the debugger start method should be queued, all other (older) methods that used hidden Execution b should not have to be queued

Not queuing that is the root of the issue

IanMatthewHuff · 2021-09-30T16:06:40Z

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

@@ -132,4 +171,28 @@ export class CellExecutionQueue {
            }
        }
    }
+
+    private getCellExecutions(executionList: Execution[]): CellExecution[] {


Not a perf difference I believe, but this could look cleaner just using .filter

I agree, personally we don't need these two methods, i'd just move the filtering into the place where we call this method, else we just iterate throught the same set of list twice.

IanMatthewHuff · 2021-09-30T16:07:58Z

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

+}
+
+/**
+ * Responsible for execution of an individual cell and manages the state of the cell as it progresses through the execution phases.


This is picky, but that's kinda how I am 😆 . The comments here look copied over from the other class, they should be updated for the new class (i.e. this is not responsible for execution of an individual cell).

rchiodo · 2021-09-30T16:21:19Z

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

+ * Further details here https://github.com/microsoft/vscode-jupyter/issues/232 & https://github.com/jupyter/jupyter_client/issues/297
+ *
+ */
+export class CodeExecution implements IDisposable {


It seems weird that we'd need an entirely new class just to execute a string instead of a notebook cell. Could CellExecution not use this instead?

Meaning derive from it or use this object internally to do execution?

Valid request, however I think it gets way too messy (lots of if/else), i personally think this is cleaner than trying to get a single class to do both.
Also i don't think execute hidden will require execution of code that has widgets & other complexities.

No not a single class. The other class would use this one. This one knows only about code.

The other one uses this class internally to actually do an execute. It just listens to the promise and updates the cell outputs on the cell.

Otherwise we end up with the same code doing iopub listening and the potential for having to fix things in two spots if something is broken.

The other class would use this one. This one knows only about code.

I'm still not convinced, this is simple and small, it just gets messy. We don't need widgets, etc, hence this code is sufficient as is.

code doing iopub listening and the potential for having to fix things in two

I'd perfer to refactor when that happens, right now this feels much simpler to me, but thats just me.

roblourens · 2021-09-30T16:44:28Z

Please add me as a reviewer on PRs like this so I get a notification. I don't quite follow this though, how does it actually queue RBL?

DonJayamanne · 2021-09-30T16:56:26Z

how does it actually queue RBL?

Assume we have 10 cell and we're hit run all, then we hit RBL for the last cell, ideally what should happen (i plan on bringing this up in standup to discuss expected behavior) is we should start RBL after the 10 cells.
In jupyter all executions are queued (if you're already running a cell & attempt to run another cell, all that happens is, it gets put into a queue & it will run after all previous executions).
Thus, RBL will happen after all previous pending execution requests.

The down side of this is, RBL could start 10 minutes after you clicked RBL, (i'll be bringing up this issue & a few others related to RBL in standup)
Its possible this solution of queueing is the right solution, we could even just as well disable RBL when we have pending executions... lets discuss in standup

DavidKutu · 2021-09-30T17:54:09Z

As we discussed on standup, we'll wait for after release for this. And there's a chance we'll drop this and just not allow run by line/debugging when the kernel is running.

roblourens · 2021-09-30T18:49:35Z

@DonJayamanne I get it, you're describing how queueing should work, I don't see how this PR implements that. It seems like individual executions will be queued but not the operation of starting RBL itself. But anyway, like we discussed we can revisit it later

DonJayamanne · 2021-10-15T19:59:49Z

I think we should re-vist this, I"m actually keen on taking some of this work, as it ensures hidden cells & others all go through the same queu, there are times when they must (& that's what this PR was trying to address)

Will leave open for now, will create an issue so we track this as an engineering/debt task

execute hidden code with the executionQueue

ed80d14

DavidKutu requested a review from a team as a code owner September 30, 2021 01:49

DavidKutu commented Sep 30, 2021

View reviewed changes

DonJayamanne reviewed Sep 30, 2021

View reviewed changes

lint

9eaa2ce

DonJayamanne reviewed Sep 30, 2021

View reviewed changes

remove notebookDocument parameter

0f86310

DonJayamanne reviewed Sep 30, 2021

View reviewed changes

David added 4 commits September 29, 2021 20:06

use two types

e25d761

add queueAndExeceuteHidden method

5a8a28a

return promise

d245d3d

lint

3bad589

DavidKutu commented Sep 30, 2021

View reviewed changes

David added 2 commits September 29, 2021 20:39

remove Promise.resolve

0d76176

compare an id instead of the code

3876790

DavidKutu requested a review from DonJayamanne September 30, 2021 04:05

DonJayamanne reviewed Sep 30, 2021

View reviewed changes

IanMatthewHuff reviewed Sep 30, 2021

View reviewed changes

rchiodo reviewed Sep 30, 2021

View reviewed changes

DavidKutu requested a review from roblourens September 30, 2021 17:47

DonJayamanne closed this Oct 27, 2021

execute hidden code with the executionQueue #7763

execute hidden code with the executionQueue #7763

Conversation

DavidKutu commented Sep 30, 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

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 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne Sep 30, 2021 • edited Loading

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 30, 2021 • edited Loading

Codecov Report

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

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 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblourens commented Sep 30, 2021

DonJayamanne commented Sep 30, 2021

DavidKutu commented Sep 30, 2021

roblourens commented Sep 30, 2021

DonJayamanne commented Oct 15, 2021

DonJayamanne Sep 30, 2021 •

edited

Loading

DonJayamanne Sep 30, 2021 •

edited

Loading

codecov-commenter commented Sep 30, 2021 •

edited

Loading

DonJayamanne Sep 30, 2021 •

edited

Loading