Interactive Window Ordering Fix #8233

IanMatthewHuff · 2021-11-10T19:36:02Z

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

src/test/datascience/editor-integration/codewatcher.unit.test.ts

IanMatthewHuff · 2021-11-10T22:02:50Z

I fixed up the Unit tests. Will take a peek at what we currently have in the .vscode tests. If we don't have coverage for this situation there I'll add a test before submission for queuing multiple sets of cells and making sure they come out in order.

IanMatthewHuff · 2021-11-10T21:57:02Z

src/client/datascience/editor-integration/codewatcher.ts

+                }
+            } else {
+                // Failure. Print our failure message and clear the queue
+                await this.addErrorMessage(file, leftCount);


Right now I have a failure to execute a cell just clear out the queue, didn't feel worth tracking trying to see which command queued which cells.

IanMatthewHuff · 2021-11-10T22:01:09Z

src/client/datascience/editor-integration/codewatcher.ts

+        debug?: boolean
+    ) {
+        // Either start up this addCode, or queue it for later
+        if (this.executingAddCode) {


I feel like TS / JS has a bunch of way to do this type of queuing and they all always look a bit janky to me. Speak up if it looks like I'm doing something unreasonable here, but this way felt the cleanest to me. (Had a promise chain thing I was initially working with that felt hard to read / debug).

I think you could just chain the promise. Like the interactive window does itself.

Essentially you have a single promise - addCodePromise that you init to a resolved promise.

addCode changes to do something like so:

// Chain execution promises so that cells are executed in the right order if (this.addCodePromise) { this.addCodePromise= this.addCodePromise.then(() => this.addCodeImpl(code, fileUri, line, isDebug) ); } else { this.addCodePromise= this.addCodeImpl(code, fileUri, line, isDebug); } try { return await this.addCodePromise; } catch (exc) { // Rethrow, but clear promise so we can execute again this.addCodePromise= undefined; throw exc; }

Old addCode method becomes addCodeImpl

Actually feels like we could have a method/service to do this cause we do this a lot.

This would also leave the old code the same as the behavior wouldn't have to account for a queue.

Trying to think of what that generic service might be.

It would look like a function. A function which returns a promise.

I wonder if we could do it with a decorator. That would be even better. Something like so:

@chainable public addCode(...): Promise

Where the chainable decorator would do the code I pasted above.

I agree, and would suggest moving all of that into the existing addCode method, unless i'm reading it incorrectly

I suggest the addCode method as that the choke point, thats what everything calls today.
Perhaps rename from addCode to queueAddCode or the like and make it sync

Then we could use that in both the interractive window and here (and probably ten other places where we chain stuff)

I had initially started with the promise chain starting from a resolved promise. Felt like it was getting icky with the promises resolving to false (instead of throwing) and the error handler function having to get tacked on. But I'm not adverse to going back. Let me restart from there and see if I can get it looking better.

I tried this on the current interactive window 'submitCodeImpl' and it seems to work. Then we can use this to do promise chaining anywhere:

// Turn a method that returns a promise into something that uses a chain of promises export function chainPromise() { // eslint-disable-next-line @typescript-eslint/no-explicit-any return function (_target: Object, propertyKey: string, descriptor: TypedPropertyDescriptor<any>) { const originalMethod = descriptor.value; const chainedKey = `chained${propertyKey}`; // eslint-disable-next-line , @typescript-eslint/no-explicit-any descriptor.value = function (...args: any[]) { // Check for promise in the object. let currentValue = (this as any)[chainedKey] as Promise<any>; if (currentValue) { currentValue = currentValue.then(() => originalMethod.apply(this, args)); } else { currentValue = originalMethod.apply(this, args); } // Save promise in object (this as any)[chainedKey] = currentValue; // If promise fails, clear it. return currentValue .then((r) => r) .catch((e) => { (this as any)[chainedKey] = undefined; throw e; }); }; return descriptor; }; }

This changes submitCodeImpl (or your addCode) to this instead:

@chainPromise() private async submitCodeImpl(code: string, fileUri: Uri, line: number, isDebug: boolean) { // Internals don't worry about chaining anymore }

Hmm, thought I put a comment here. I didn't like the decorator for this (though your implementation is rad clever) it feels like really core functionality to be putting into a decorator (versus timing / telemetry) and not easy to debug and the mild differences between something like the addCode here and submitCode in the interactive window (things like the false return value handling) didn't lend to sharing one decorator.

src/client/datascience/editor-integration/codewatcher.ts

codecov-commenter · 2021-11-10T22:27:30Z

Codecov Report

Merging #8233 (c85d0d5) into main (57dcd19) will increase coverage by 0%.
The diff coverage is 92%.

@@          Coverage Diff           @@
##            main   #8233    +/-   ##
======================================
  Coverage     71%     72%            
======================================
  Files        368     371     +3     
  Lines      22591   23058   +467     
  Branches    3418    3507    +89     
======================================
+ Hits       16212   16632   +420     
- Misses      4984    4986     +2     
- Partials    1395    1440    +45

Impacted Files	Coverage Δ
...ient/datascience/editor-integration/codewatcher.ts	`69% <92%> (+<1%)`	⬆️
src/client/datascience/notebook/integration.ts	`84% <0%> (-16%)`	⬇️
src/client/datascience/activation.ts	`86% <0%> (-6%)`	⬇️
...datascience/interactive-common/notebookProvider.ts	`69% <0%> (-6%)`	⬇️
src/client/common/utils/misc.ts	`62% <0%> (-5%)`	⬇️
...interpreter/jupyterInterpreterDependencyService.ts	`75% <0%> (-4%)`	⬇️
...ce/raw-kernel/liveshare/hostRawNotebookProvider.ts	`76% <0%> (-3%)`	⬇️
...datascience/jupyter/liveshare/hostJupyterServer.ts	`61% <0%> (-3%)`	⬇️
...science/jupyter/kernels/kernelDependencyService.ts	`82% <0%> (-3%)`	⬇️
...ient/datascience/kernel-launcher/kernelLauncher.ts	`82% <0%> (-3%)`	⬇️
... and 69 more

src/client/datascience/editor-integration/codewatcher.ts

DonJayamanne

.

DonJayamanne · 2021-11-10T22:28:26Z

src/client/datascience/editor-integration/codewatcher.ts

+        debug?: boolean
+    ) {
+        // Either start up this addCode, or queue it for later
+        if (this.executingAddCode) {


I agree, and would suggest moving all of that into the existing addCode method, unless i'm reading it incorrectly

DonJayamanne · 2021-11-10T22:29:19Z

src/client/datascience/editor-integration/codewatcher.ts

+        debug?: boolean
+    ) {
+        // Either start up this addCode, or queue it for later
+        if (this.executingAddCode) {


I suggest the addCode method as that the choke point, thats what everything calls today.
Perhaps rename from addCode to queueAddCode or the like and make it sync

IanMatthewHuff · 2021-11-17T23:01:35Z

src/test/datascience/editor-integration/codewatcher.unit.test.ts

@@ -560,6 +560,81 @@ testing2`;
        document.verifyAll();
    });

+    test('Test two command ordering correct ordering', async () => {


Usually I like our vscode tests over unit, but one of these tests actually caught an issue in an earlier checkin. And the way they are set up they actually work pretty good for testing this scenario. I put this test in the old bits and it failed with the expected failure (trying to interleave instead of running each in order).

IanMatthewHuff · 2021-11-17T23:06:43Z

src/client/datascience/editor-integration/codewatcher.ts

+            await this.addErrorMessage(file, leftCount);
+
+            // Throw to break out of the promise chain
+            throw new InteractiveCellResultError();


Slight difference from something like submitCodeImpl as we want to abort the entire chain not just for exceptions (which we want to bubble up) but also for a error in the code, which returns as a false value from .addCode.

Ian Huff added 5 commits November 10, 2021 10:45

working with basic ordering fix

858de92

cancellation works

cd3280c

chop out old commented code

cede1a3

comment cleanup

9613f5e

fix unit tests and add news

c318101

IanMatthewHuff commented Nov 10, 2021

View reviewed changes

src/test/datascience/editor-integration/codewatcher.unit.test.ts Outdated Show resolved Hide resolved

IanMatthewHuff commented Nov 10, 2021

View reviewed changes

IanMatthewHuff marked this pull request as ready for review November 10, 2021 22:06

IanMatthewHuff requested a review from a team as a code owner November 10, 2021 22:06

DonJayamanne reviewed Nov 10, 2021

View reviewed changes

src/client/datascience/editor-integration/codewatcher.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Nov 10, 2021

View reviewed changes

Ian Huff added 4 commits November 17, 2021 13:43

revert test changes

b98ec1f

updated code from other branch

f201421

error in finished check

abfa10e

working unit test (fails in old bits)

c85d0d5

IanMatthewHuff commented Nov 17, 2021

View reviewed changes

DonJayamanne approved these changes Nov 17, 2021

View reviewed changes

rchiodo approved these changes Nov 17, 2021

View reviewed changes

IanMatthewHuff merged commit 48c209d into main Nov 17, 2021

IanMatthewHuff deleted the dev/ianhu/iwOrderingFix branch November 17, 2021 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interactive Window Ordering Fix #8233

Interactive Window Ordering Fix #8233

IanMatthewHuff commented Nov 10, 2021 •

edited

Loading

IanMatthewHuff commented Nov 10, 2021

IanMatthewHuff Nov 10, 2021

IanMatthewHuff Nov 10, 2021

rchiodo Nov 10, 2021 •

edited

Loading

rchiodo Nov 10, 2021

rchiodo Nov 10, 2021

DonJayamanne Nov 10, 2021

DonJayamanne Nov 10, 2021

rchiodo Nov 10, 2021

IanMatthewHuff Nov 10, 2021

rchiodo Nov 11, 2021 •

edited

Loading

IanMatthewHuff Nov 17, 2021

codecov-commenter commented Nov 10, 2021 •

edited

Loading

DonJayamanne left a comment

DonJayamanne Nov 10, 2021

DonJayamanne Nov 10, 2021

IanMatthewHuff Nov 17, 2021

IanMatthewHuff Nov 17, 2021

Interactive Window Ordering Fix #8233

Interactive Window Ordering Fix #8233

Conversation

IanMatthewHuff commented Nov 10, 2021 • edited Loading

IanMatthewHuff commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo Nov 10, 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

rchiodo Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 10, 2021 • edited Loading

Codecov Report

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

IanMatthewHuff commented Nov 10, 2021 •

edited

Loading

rchiodo Nov 10, 2021 •

edited

Loading

rchiodo Nov 11, 2021 •

edited

Loading

codecov-commenter commented Nov 10, 2021 •

edited

Loading