Startup the jupyter server when appropriate #9708

rchiodo · 2020-01-22T00:54:05Z

For #7232

We will now start the jupyter server process (well the daemon in most cases) when:

User opens a notebook
User opens the interactive window
User opens a workspace that activates the extension and the user has done either of the previous two in the last 7 days

On my machine (once the server has started), execution is basically instantaneous.

… from opening

rchiodo · 2020-01-22T00:54:54Z

And to be noted, our existing functional tests caught a bug in my implementation. Woot.

news/1 Enhancements/7232.md

DonJayamanne · 2020-01-22T16:36:00Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

@@ -142,6 +152,11 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
            localize.DataScience.nativeEditorTitle(),
            ViewColumn.Active
        );
+
+        // Start the server as soon as we open if allowed
+        if (!this.configService.getSettings().datascience.disableJupyterAutoStart) {


I think auto starting of notebooks should be done outside the editor.
I'd prefer the editor focus on editing alone. #Resolved

The editor has always started the server, just not when it opens.

In reply to: 369669813 [](ancestors = 369669813)

I think it's a good time to move this out.
At least auto starting. Feels like to much is happening in this space and we keep wrong more stuff. #Resolved

Disagree. The internal implementation of the notebook (interactiveBase actually) requires a server to run code. Where is it supposed to get this server from if it doesn't start it? It assumes it was already started? That sounds like the wrong dependency to me.

In reply to: 369689773 [](ancestors = 369689773)

Maybe the auto starting though. Just not the ensureServer calls.

In reply to: 369692802 [](ancestors = 369692802,369689773)

Actually that breaks the connection between the server and the UI. It doesn't know it got started. So I'll have to add other code to check if it's already started.

In reply to: 369693209 [](ancestors = 369693209,369692802,369689773)

Which means some sort of global event on the JupyterExecution class.

In reply to: 369695742 [](ancestors = 369695742,369693209,369692802,369689773)

DonJayamanne · 2020-01-22T16:37:05Z

src/client/datascience/interactive-ipynb/nativeEditor.ts

+                await this.fileSystem.createDirectory(path.dirname(filePath));
+                return this.fileSystem.writeFile(filePath, contents);
+            } else {
+                return this.fileSystem.deleteFile(filePath);


Do we care if there are any exceptions in here when deleting a backup file? #WontFix

If not, I'd prefer adding that and removing the catch. Feels like catch will bury issues instead of them getting reported. #WontFix

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

DonJayamanne · 2020-01-22T16:41:43Z

src/client/datascience/jupyter/serverPreload.ts

+    }
+
+    private onDidOpenOrCloseInteractive(interactive: IInteractiveWindow | undefined) {
+        if (interactive) {


Not sure why this is necessary?
Is it possible to open an interactive window without starting a server?
Also, why should we start a server when switching between tabs from interactive window to something else? #Resolved

There's no reason to start it on shutdown.

In reply to: 369673234 [](ancestors = 369673234)

Maybe I should rename startServer. it doesn't just start a server. It checks if one was succesfully started, and if so, writes the memento item to indicate it was succesful.

In reply to: 369690429 [](ancestors = 369690429,369673234)

src/test/datascience/nativeEditor.functional.test.tsx

DonJayamanne · 2020-01-22T16:49:48Z

src/client/datascience/jupyter/serverPreload.ts

+        // This is the list of things that should cause us to start a local server
+        // 1) Notebook is opened
+        // 2) Notebook was opened in the past 7 days
+        // 3) Interactive window was opened in the past 7 days


Lets discuss these rules/criteria.

src/client/datascience/jupyter/serverPreload.ts

DonJayamanne · 2020-01-22T16:58:47Z

src/test/datascience/nativeEditor.functional.test.tsx

+                // Make sure it has a server
+                assert.ok(editor.notebook, 'Notebook did not start with a server');
+
+                // Close down all servers and mock restarting


Do we need such a test, testing of restarting servers after disposing them?
I thought servers get disposed only when VS Code is closed. #Resolved

No it's so I can test the setting.

In reply to: 369683340 [](ancestors = 369683340)

Shouldn't that be a separate test? #Resolved

Yeah that would probably be better, especially if I only run these with mocking.

In reply to: 369692742 [](ancestors = 369692742)

codecov-io · 2020-01-22T17:12:28Z

Codecov Report

❗ No coverage uploaded for pull request base (master@417917c). Click here to learn what that means.
The diff coverage is 41.28%.

@@           Coverage Diff            @@
##             master   #9708   +/-   ##
========================================
  Coverage          ?   60.6%           
========================================
  Files             ?     552           
  Lines             ?   29833           
  Branches          ?    4514           
========================================
  Hits              ?   18079           
  Misses            ?   10743           
  Partials          ?    1011

Impacted Files	Coverage Δ
...terpreter/locators/services/condaEnvFileService.ts	`86.27% <ø> (ø)`
src/client/common/platform/types.ts	`100% <ø> (ø)`
src/client/activation/refCountedLanguageServer.ts	`37.2% <0%> (ø)`
.../datascience/interactive-common/interactiveBase.ts	`17.13% <0%> (ø)`
src/client/telemetry/index.ts	`85.58% <0%> (ø)`
src/client/activation/jedi.ts	`21.21% <0%> (ø)`
src/client/common/platform/fs-temp.ts	`100% <100%> (ø)`
src/client/common/platform/fileSystem.ts	`25.8% <100%> (ø)`
src/client/linters/serviceRegistry.ts	`100% <100%> (ø)`
src/client/common/variables/environment.ts	`92.68% <100%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 417917c...4414f75. Read the comment docs.

IanMatthewHuff · 2020-01-22T17:53:16Z

src/client/datascience/jupyter/serverPreload.ts

+        this.notebookProvider.onDidOpenNotebookEditor(this.onDidOpenNotebook.bind(this));
+        this.interactiveProvider.onDidChangeActiveInteractiveWindow(this.onDidOpenOrCloseInteractive.bind(this));
+    }
+    public activate(): Promise<void> {


This seems off in remote connection cases. Just making sure that I'm reading this right, but it looks like it would be starting up local servers if you opened a notebook and had just connected it remote. Is that correct? #Resolved

No. The serverstart only starts a local server if the options for starting don't include a server URI

In reply to: 369711529 [](ancestors = 369711529)

Maybe I should update the comment here to say that.

In reply to: 369713520 [](ancestors = 369713520,369711529)

IanMatthewHuff

DonJayamanne · 2020-01-22T22:11:14Z

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

        cancelToken?: CancellationToken,
        currentKernel?: IJupyterKernelSpec | LiveKernelModel
    ): Promise<KernelSpecInterpreter> {
        let suggestions = await this.selectionProvider.getKernelSelectionsForRemoteSession(session, cancelToken);
        suggestions = suggestions.filter(item => !this.kernelIdsToHide.has(item.selection.kernelModel?.id || ''));
-        return this.selectKernel(suggestions, session, cancelToken, currentKernel);
+        return this.selectKernel(suggestions, session, disableUI, cancelToken, currentKernel);


Why not just avoid calling these methods. After all, selectRemoteKernel will display a picker, thats what its mean to do.
We can avoid passing these flags by not calling the methods higher up. #Resolved

DonJayamanne · 2020-01-22T22:11:20Z

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

        cancelToken?: CancellationToken,
        currentKernel?: IJupyterKernelSpec | LiveKernelModel
    ): Promise<KernelSpecInterpreter> {
        let suggestions = await this.selectionProvider.getKernelSelectionsForLocalSession(session, cancelToken);
        suggestions = suggestions.filter(item => !this.kernelIdsToHide.has(item.selection.kernelModel?.id || ''));
-        return this.selectKernel(suggestions, session, cancelToken, currentKernel);
+        return this.selectKernel(suggestions, session, disableUI, cancelToken, currentKernel);


Why not just avoid calling these methods. After all, selectRemoteKernel will display a picker, thats what its mean to do.
We can avoid passing these flags by not calling the methods higher up. #Resolved

DonJayamanne · 2020-01-22T22:11:47Z

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

                } else {
                    telemetryProps.promptedToSelect = true;
-                    selection = await this.selectLocalKernel(sessionManager, cancelToken);
+                    selection = await this.selectLocalKernel(sessionManager, disableUI, cancelToken);


No need to call this method as user needs to select a kernel, hence the name selectLocalKernel. #Resolved

DonJayamanne · 2020-01-22T22:20:29Z

src/client/datascience/jupyter/jupyterExecution.ts

@@ -126,7 +132,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
                // Get hold of the kernelspec and corresponding (matching) interpreter that'll be used as the spec.
                // We can do this in parallel, while starting the server (faster).
                traceInfo(`Getting kernel specs for ${options ? options.purpose : 'unknown type of'} server`);
-                kernelSpecInterpreterPromise = this.kernelSelector.getKernelForLocalConnection(undefined, options?.metadata, kernelSpecCancelSource.token);
+                kernelSpecInterpreterPromise = this.kernelSelector.getKernelForLocalConnection(undefined, options?.metadata, !allowUI, kernelSpecCancelSource.token);


Why not just abort starting server if there's no kernel information.
I.e. in non-ui mode we cannot have a server without any kernels, hence we can just abort here (or just after the kernel promise has resolved).
I think that'll be more readable as well, indicating the fact that in non-ui mode it is mandatory. #Resolved

rchiodo · 2020-01-22T23:17:34Z

src/client/datascience/jupyter/liveshare/serverCache.ts

+
+        // See if the old options had the same UI setting. If not,
+        // cancel the old
+        if (data && data.options.disableUI !== fixedOptions.disableUI) {


I think maybe I should add a unit test for this stuff. It'd be rather hairy to try and get a functional tset with the right timing on this. #Resolved

rchiodo · 2020-01-23T17:43:25Z

@DonJayamanne did you have any more comments?

sonarqubecloud · 2020-01-23T20:48:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

rchiodo added 2 commits January 21, 2020 15:44

Preload server when appropriate

3605255

Fix problem with startup of the server preventing the notebook editor…

26fd0f0

… from opening

rchiodo self-assigned this Jan 22, 2020

rchiodo requested review from DavidKutu, DonJayamanne and IanMatthewHuff January 22, 2020 00:54

rchiodo added 2 commits January 22, 2020 08:22

Fix build errors

5f96102

Fix DS dependency error occuring in master

2e4c268

DonJayamanne requested changes Jan 22, 2020

View reviewed changes

IanMatthewHuff reviewed Jan 22, 2020

View reviewed changes

DonJayamanne approved these changes Jan 22, 2020

View reviewed changes

IanMatthewHuff approved these changes Jan 22, 2020

View reviewed changes

rchiodo added 3 commits January 22, 2020 11:23

Review feedback

f163fde

Fix news file to have right setting

4ad3a65

Fix unit and functional tests.

92e9fd5

DonJayamanne requested changes Jan 22, 2020

View reviewed changes

Address more code review comments

5d03166

rchiodo commented Jan 22, 2020

View reviewed changes

rchiodo added 2 commits January 22, 2020 16:17

Add unit test for server cache

96fcd34

Add some more tracing

3b3ef9a

DonJayamanne approved these changes Jan 23, 2020

View reviewed changes

rchiodo added 4 commits January 23, 2020 11:31

Submit change for the different output file

2074949

Fix smoke tests

417917c

Merge remote-tracking branch 'origin/master' into rchiodo/startup_server

11662d6

Fix linter

4414f75

rchiodo merged commit d5f153e into master Jan 23, 2020

rchiodo deleted the rchiodo/startup_server branch January 23, 2020 23:03

lock bot locked as resolved and limited conversation to collaborators Jan 31, 2020

Startup the jupyter server when appropriate #9708

Startup the jupyter server when appropriate #9708

Uh oh!

Conversation

rchiodo commented Jan 22, 2020

Uh oh!

rchiodo commented Jan 22, 2020

Uh oh!

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

IanMatthewHuff Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff left a comment

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Jan 22, 2020 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

codecov-io commented Jan 22, 2020 •

edited

Loading

IanMatthewHuff Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

DonJayamanne Jan 22, 2020 •

edited by rchiodo

Loading

rchiodo Jan 22, 2020 •

edited

Loading