Minor refactoring #7837

DonJayamanne · 2021-10-06T17:55:59Z

Was looking at fixing #7697, but then realized its not going to be easy.

This PR only refactors some code that I came across while attempting to fix the issue (fixes have been reverted, but left refactoring)

When we start debugger, why should we clera the list of variables?
- I think we should not clear variables every time just because we opened another panel
- I think we can easily cache the variables & fetch variables ONLY when execution count has changed (or restarted kernels, etc)
Right now, when we start debugging, we hide variable view & then display it
- & now we're fetching variables all over again unnecessarily

This can be solved better by only fetching variables when reqruied.
E.g. when using run by line and running to completion I can see variables being requested 4 times, when it should be fetched just once.

codecov-commenter · 2021-10-06T18:25:54Z

Codecov Report

Merging #7837 (be37e66) into main (d448a02) will decrease coverage by 0%.
The diff coverage is 80%.

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

@@          Coverage Diff           @@
##            main   #7837    +/-   ##
======================================
- Coverage     70%     69%    -1%     
======================================
  Files        363     362     -1     
  Lines      22299   22274    -25     
  Branches    3373    3373            
======================================
- Hits       15726   15486   -240     
- Misses      5215    5450   +235     
+ Partials    1358    1338    -20

Impacted Files	Coverage Δ
src/client/datascience/jupyter/kernels/types.ts	`100% <ø> (ø)`
src/client/datascience/types.ts	`100% <ø> (ø)`
src/client/datascience/jupyter/kernels/helpers.ts	`71% <33%> (-1%)`	⬇️
...client/datascience/commands/activeEditorContext.ts	`87% <42%> (-5%)`	⬇️
...lient/datascience/variablesView/notebookWatcher.ts	`85% <84%> (-4%)`	⬇️
...ient/datascience/jupyter/kernels/kernelProvider.ts	`97% <90%> (-1%)`	⬇️
...atascience/interactive-window/interactiveWindow.ts	`54% <100%> (+<1%)`	⬆️
src/client/datascience/jupyter/kernels/kernel.ts	`73% <100%> (ø)`
...t/common/application/webviewPanels/webviewPanel.ts	`8% <0%> (-62%)`	⬇️
...cience/data-viewing/jupyterVariableDataProvider.ts	`12% <0%> (-54%)`	⬇️
... and 25 more

DonJayamanne · 2021-10-06T19:16:59Z

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

@@ -164,6 +164,7 @@ export interface IKernelProvider extends IAsyncDisposable {
    readonly kernels: Readonly<IKernel[]>;
    onDidRestartKernel: Event<IKernel>;
    onDidDisposeKernel: Event<IKernel>;
+    onKernelStatusChanged: Event<{status: ServerStatus, kernel: IKernel}>;


Moved kernel state change from INotebook to IKernelProvider

DonJayamanne · 2021-10-06T19:17:35Z

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

@@ -611,7 +609,7 @@ export class Kernel implements IKernel {
    }
 }

-export async function executeSilently(session: IJupyterSession, code: string): Promise<nbformat.IOutput[]> {
+export async function executeSilently(session: Pick<JupyterKernel.IKernelConnection, 'requestExecute'>, code: string): Promise<nbformat.IOutput[]> {


We're using Jupyter lab API types directly, not IJupyterSession (makes it easier to re-factor later on to use pure Jupyter npm API)

DonJayamanne · 2021-10-06T19:18:10Z

src/client/datascience/commands/activeEditorContext.ts

        ) {
            this.updateContextOfActiveInteractiveWindowKernel();
-        } else if (activeEditor && activeEditor.document.uri.toString() === notebook.identity.toString()) {
-            this.updateContextOfActiveNotebookKernel(activeEditor);
+        } else if (kernel.notebookDocument.notebookType === JupyterNotebookView && kernel.notebookDocument === this.vscNotebook.activeNotebookEditor?.document) {


Some cleanup to use IKernel
Also comparing notebook documents instead of comparing strings.

DonJayamanne · 2021-10-06T19:18:50Z

src/client/datascience/variablesView/notebookWatcher.ts

@@ -172,56 +190,7 @@ export class NotebookWatcher implements INotebookWatcher {
    }

    // Check to see if this event was on the active notebook
-    private async isActiveNotebookEvent(kernelStateEvent: KernelStateEventArgs): Promise<boolean> {


Cleaned up to use a dictionary instead of an array, we always have notebook documents, hence 1-1 relationship.

DonJayamanne · 2021-10-06T19:19:15Z

src/client/datascience/variablesView/notebookWatcher.ts

                    kernelStateEvent.cell.executionSummary?.executionOrder
                );
            }

            // Next, if this is the active document, send out our notifications
            if (
-                (await this.isActiveNotebookEvent(kernelStateEvent)) &&
+                this.isActiveNotebookEvent(kernelStateEvent) &&


Removed a lot of awaits

DonJayamanne · 2021-10-06T19:20:14Z

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

@@ -164,6 +164,7 @@ export interface IKernelProvider extends IAsyncDisposable {
    readonly kernels: Readonly<IKernel[]>;
    onDidRestartKernel: Event<IKernel>;
    onDidDisposeKernel: Event<IKernel>;
+    onKernelStatusChanged: Event<{ status: ServerStatus; kernel: IKernel }>;


Moved from INotebook to IKernel, easier & looking at NotebookWatcher, we were using Inotebook to detect changes in IKernel,
this change also makes it easier to remove INotbeook (fewer usages)

DonJayamanne · 2021-10-06T19:20:49Z

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

@@ -611,7 +609,10 @@ export class Kernel implements IKernel {
    }
 }

-export async function executeSilently(session: IJupyterSession, code: string): Promise<nbformat.IOutput[]> {
+export async function executeSilently(
+    session: Pick<JupyterKernel.IKernelConnection, 'requestExecute'>,


No longer using our types, directly using Jupyter Lab API types.
Much easier to refactor later on (as we have fewer layers)

DonJayamanne · 2021-10-06T19:21:39Z

src/client/datascience/interactive-window/interactiveWindow.ts

    }
    public get notebookEditor(): NotebookEditor | undefined {
        return this._notebookEditor;
    }
+    public get notebookDocument(): NotebookDocument | undefined {


Exposed a property notebookDocument, we have a lot of notebookUri.toString() comparisons of strings, instead of just comparing the object refs.

DonJayamanne · 2021-10-06T20:54:09Z

src/client/datascience/data-viewing/dataViewer.ts

@@ -116,7 +125,7 @@ export class DataViewer extends WebviewPanelHost<IDataViewerMapping> implements
    public get notebook(): INotebook | undefined {
        if (this.dataProvider && 'notebook' in this.dataProvider) {
            // eslint-disable-next-line @typescript-eslint/no-explicit-any
-            return (this.dataProvider as any).notebook;
+            return this.dataProvider.notebook;


Changed the type of dataProvider the benefit is strong typeing yes, but this way when we look at removing INotebook we won't miss such instances (i.e. this change was for future proofing changes related to INotebook)

DonJayamanne commented Oct 6, 2021

View reviewed changes

DonJayamanne changed the title ~~Fixes to variable viewer~~ Minor refactoring Oct 6, 2021

DonJayamanne marked this pull request as ready for review October 6, 2021 19:26

DonJayamanne requested a review from a team as a code owner October 6, 2021 19:26

DonJayamanne requested review from IanMatthewHuff and rchiodo October 6, 2021 19:26

DonJayamanne added 2 commits October 6, 2021 13:42

remove inotebook

8f66511

Clean up INotebook usages

b84eb3a

DonJayamanne force-pushed the removeINotebookAgain branch from be37e66 to b84eb3a Compare October 6, 2021 20:47

Misc

ea04d00

DonJayamanne commented Oct 6, 2021

View reviewed changes

rchiodo approved these changes Oct 6, 2021

View reviewed changes

DonJayamanne merged commit 7522433 into main Oct 6, 2021

DonJayamanne deleted the removeINotebookAgain branch October 6, 2021 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor refactoring #7837

Minor refactoring #7837

DonJayamanne commented Oct 6, 2021 •

edited

Loading

codecov-commenter commented Oct 6, 2021 •

edited

Loading

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

DonJayamanne Oct 6, 2021

Minor refactoring #7837

Minor refactoring #7837

Conversation

DonJayamanne commented Oct 6, 2021 • edited Loading

codecov-commenter commented Oct 6, 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

DonJayamanne commented Oct 6, 2021 •

edited

Loading

codecov-commenter commented Oct 6, 2021 •

edited

Loading