Fixes to flaky Installer Prompt tests #9358

DonJayamanne · 2022-03-15T03:52:23Z

codecov-commenter · 2022-03-15T19:53:39Z

Codecov Report

Merging #9358 (278b2dc) into main (6503ab0) will increase coverage by 0%.
The diff coverage is 50%.

❗ Current head 278b2dc differs from pull request most recent head 866f41b. Consider uploading reports for the commit 866f41b to get more accurate results

@@          Coverage Diff          @@
##            main   #9358   +/-   ##
=====================================
  Coverage     74%     74%           
=====================================
  Files        219     219           
  Lines      10939   10939           
  Branches    1571    1571           
=====================================
+ Hits        8115    8153   +38     
+ Misses      2195    2147   -48     
- Partials     629     639   +10

Impacted Files	Coverage Δ
src/client/telemetry/index.ts	`63% <ø> (ø)`
src/client/common/utils/async.ts	`50% <50%> (ø)`
...lient/datascience/variablesView/notebookWatcher.ts	`93% <0%> (-2%)`	⬇️
src/client/logging/trace.ts	`80% <0%> (-1%)`	⬇️
src/client/datascience/dataScienceSurveyBanner.ts	`66% <0%> (ø)`
src/client/common/process/pythonDaemonPool.ts	`76% <0%> (+1%)`	⬆️
src/client/common/process/proc.ts	`78% <0%> (+2%)`	⬆️
.../client/datascience/plotting/plotViewerProvider.ts	`41% <0%> (+2%)`	⬆️
...ent/datascience/progress/kernelProgressReporter.ts	`81% <0%> (+3%)`	⬆️
src/client/common/featureDeprecationManager.ts	`37% <0%> (+3%)`	⬆️
... and 8 more

DonJayamanne · 2022-03-17T04:06:32Z

Still flaky,

DonJayamanne · 2022-03-18T00:44:10Z

src/kernels/helpers.ts

    switch (context) {
        case 'start':
-            return (k: IKernel) => k.start();
+            return (k: IKernel) => k.start(options);


Keep track of whether UI is to be displayed or not.
When we use auto start we need to go through his same code.

DonJayamanne · 2022-03-18T00:45:04Z

src/kernels/installer/moduleInstaller.ts

@@ -55,25 +55,33 @@ export abstract class ModuleInstaller implements IModuleInstaller {
        const environmentService = this.serviceContainer.get<IEnvironmentVariablesService>(
            IEnvironmentVariablesService
        );
+        if (cancelTokenSource.token.isCancellationRequested) {


You'll see these in a few places, while running the tests found that these were causing issues.
Basically we had stopped cell execution (either closing a document or interrupt), & these code paths would continue running.

DonJayamanne · 2022-03-18T00:45:28Z

src/kernels/installer/moduleInstaller.ts

            // When the progress is canceled notify caller
            token.onCancellationRequested(() => {
                cancelTokenSource.cancel();
+                deferred.resolve();


Another fix, if token was cancelled, we should stop the installation

DonJayamanne · 2022-03-18T00:45:40Z

src/kernels/installer/productInstaller.ts

@@ -117,6 +117,9 @@ abstract class BaseInstaller {
        if (!installer) {
            return InstallerResponse.Ignore;
        }
+        if (cancelTokenSource.token.isCancellationRequested) {


You'll see these in a few places, while running the tests found that these were causing issues.
Basically we had stopped cell execution (either closing a document or interrupt), & these code paths would continue running.

DonJayamanne · 2022-03-18T00:48:24Z

src/kernels/kernel.ts

+    /**
+     * Used for debugging purposes, ability to uniquely identify kernels.
+     */
+    public readonly id: string;


This was very useful, found we were creating a few kernels, initially I thought they were the same after adding this, found that we were actually creating new instances

DonJayamanne · 2022-03-18T00:50:01Z

src/kernels/kernel.ts

@@ -248,20 +263,23 @@ export class Kernel implements IKernel {
        const disposeImpl = async () => {
            traceInfo(`Dispose kernel ${(this.resourceUri || this.notebookDocument.uri).toString()}`);
            this.restarting = undefined;
+            const promises: Promise<void>[] = [];
+            promises.push(this.kernelExecution.cancel());


Found that when shutting down a kernel, we didn't cancel the pending executions, all we did was dispose that object.
This is kinda debt

DonJayamanne · 2022-03-18T00:50:55Z

src/kernels/kernel.ts

        if (!options.disableUI) {
            this.startupUI.disableUI = false;
        }
+        options.onDidChangeDisableUI(() => {


This was missing, if we have auto start, then we don't display, but when running cells, the errors are not displayed as we're using the stale state of hiding the UI.

DonJayamanne · 2022-03-18T00:52:05Z

src/kernels/kernelDependencyService.ts

        if (!promise) {
+            const cancelTokenSource = new CancellationTokenSource();
+            const disposable = token.onCancellationRequested(() => {


This was a bug, basically we stop execution, but this part continues, as we created a whole new cancellation token and ignored the one that was passed into this.

DonJayamanne · 2022-03-18T00:57:08Z

src/notebooks/controllers/vscodeNotebookController.ts

+        doc: NotebookDocument,
+        options: IDisplayOptions = new DisplayOptions(false)
+    ): Promise<IKernel> {
+        let currentPromise = VSCodeNotebookController.connections.get(doc);


This is one of the bugs that was fixed, its an easy to repro issue:

Open a notebook, select a kernel A without ipykernel

Run all cells

In the prompt displayed select the option to change the kernel to B

We still get error message indicating ipykernel isnt found in A

Run all cells again, and we'll still get errors about ipykernel not in A

The problem was we had connectToKernel function called from different places and they all had their own while loops, and thats what caused issues (causing multiple kernels to get created, one gets created and disposes the other, and even after we change the controller, the previous loop will treat the older controller as active and weird things happend).

Basically we were running connectToKernel function and had multiple while loops (instead of one) = now we cache it per doc.

DonJayamanne · 2022-03-18T00:57:46Z

src/notebooks/controllers/vscodeNotebookController.ts

@@ -584,7 +658,7 @@ export class VSCodeNotebookController implements Disposable {
            isLocalConnection(this.kernelConnection)
        ) {
            // Startup could fail due to missing dependencies or the like.
-            void newKernel.start({ disableUI: true });
+            this.connectToKernel(document, new DisplayOptions(true)).catch(noop);


THis was another fix, here we start newKernel.start our selves, but connectToKernel function will do something else and those two would step on each others toes.

DonJayamanne · 2022-03-18T00:59:20Z

src/notebooks/execution/cellExecutionQueue.ts

@@ -127,8 +128,14 @@ export class CellExecutionQueue implements Disposable {

            let executionResult: NotebookCellRunState | undefined;
            try {
-                await cellToExecute.start(kernelConnection);
-                executionResult = await cellToExecute.result;
+                if (cellToExecute.cell.notebook.isClosed) {


Found while debugging the tests.
Basically we stopped some tests early (which was not right), while some cells were still getting executed.
This ensures we don't run cells after a document has been closed.
The logs showed cells with index -1 were running.

src/kernels/helpers.ts

DonJayamanne force-pushed the issue9186 branch from 802c605 to 457711f Compare March 15, 2022 19:24

DonJayamanne force-pushed the issue9186 branch 3 times, most recently from cecd7ef to 02254fe Compare March 17, 2022 00:44

DonJayamanne changed the title ~~Add logging to identify test failures~~ Fixes to flaky Installer Prompt tests Mar 17, 2022

DonJayamanne marked this pull request as ready for review March 17, 2022 02:23

DonJayamanne requested a review from a team as a code owner March 17, 2022 02:23

DonJayamanne requested review from rchiodo, amunger and IanMatthewHuff and removed request for a team, IanMatthewHuff, amunger and rchiodo March 17, 2022 02:23

DonJayamanne marked this pull request as draft March 17, 2022 04:05

DonJayamanne force-pushed the issue9186 branch from a8ffba1 to 0a06222 Compare March 17, 2022 21:09

DonJayamanne commented Mar 18, 2022

View reviewed changes

Fixes to kernel installation prmopts and tests

278b2dc

DonJayamanne force-pushed the issue9186 branch from d2b3d8f to 278b2dc Compare March 21, 2022 17:10

DonJayamanne marked this pull request as ready for review March 21, 2022 17:11

DonJayamanne requested review from rchiodo, IanMatthewHuff and amunger March 21, 2022 17:11

rchiodo reviewed Mar 21, 2022

View reviewed changes

src/kernels/helpers.ts Outdated Show resolved Hide resolved

rchiodo approved these changes Mar 21, 2022

View reviewed changes

DonJayamanne added 2 commits March 22, 2022 04:32

Fixes

043a74e

oops

866f41b

DonJayamanne merged commit 7b5cd3c into main Mar 21, 2022

DonJayamanne deleted the issue9186 branch March 21, 2022 20:45

DonJayamanne mentioned this pull request Mar 21, 2022

Ensure we are prompted to install IPyKernel and it gets installed into the right Environment #9435

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to flaky Installer Prompt tests #9358

Fixes to flaky Installer Prompt tests #9358

DonJayamanne commented Mar 15, 2022

codecov-commenter commented Mar 15, 2022 •

edited

Loading

DonJayamanne commented Mar 17, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022

DonJayamanne Mar 18, 2022 •

edited

Loading

DonJayamanne Mar 18, 2022

Fixes to flaky Installer Prompt tests #9358

Fixes to flaky Installer Prompt tests #9358

Conversation

DonJayamanne commented Mar 15, 2022

codecov-commenter commented Mar 15, 2022 • edited Loading

Codecov Report

DonJayamanne commented Mar 17, 2022

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 Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 15, 2022 •

edited

Loading

DonJayamanne Mar 18, 2022 •

edited

Loading