Add ability to reconnect to Jupyter kernel by kdkavanagh · Pull Request #7015 · microsoft/vscode-python

kdkavanagh · 2019-08-18T16:41:21Z

For #7014

Adds two new user-facing parameters presented after entering a remote jupyter URI which allow the user to reconnect to a kernel already running on the server and the option to leave that kernel running when the interactive window is closed

[X ] Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
[X ] Title summarizes what is changing
[X ] Has a news entry file (remember to thank yourself!)
[X ] Appropriate comments and documentation strings in the code
[X ] Has sufficient logging.
[X ] Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated
[ X] ~~Test plan is updated as appropriate~~
[ X] ~~package-lock.json has been regenerated by running npm install (if dependencies have changed)~~
The wiki is updated with any design decisions/details.

…d nasty jupyter session behavior

msftclas · 2019-08-18T16:41:36Z

All CLA requirements met.

janosh · 2019-08-19T08:18:07Z

Awesome suggestion and great work! Just starting to use remote kernels in VSCode myself and this would be really helpful.

gramster · 2019-08-20T22:39:13Z

This is great, thank you! We will review it ASAP.

IanMatthewHuff · 2019-08-20T23:21:35Z

@kdkavanagh . Sorry about the delay here. I've added myself on as a reviewer for this and will take a look first thing tomorrow.

kdkavanagh · 2019-08-21T13:02:44Z

One thing to note - If you choose to start your own kernel, VSCode ends up creating two sessions/kernels when you initially load the interactive window. I saw the code for this, but failed to grasp the reasoning behind it, so left it alone. This can be somewhat confusing when you come back to reconnect to the server and see two kernels when you wouldve only expected one

IanMatthewHuff · 2019-08-21T16:12:04Z

One thing to note - If you choose to start your own kernel, VSCode ends up creating two sessions/kernels when you initially load the interactive window. I saw the code for this, but failed to grasp the reasoning behind it, so left it alone. This can be somewhat confusing when you come back to reconnect to the server and see two kernels when you wouldve only expected one

The second session is used for the restart kernel command. We essentially always have a backup session running, and when user asks for a restart we just swap in the backup. Was a pretty big speed up for the restart operation.

IanMatthewHuff · 2019-08-21T16:29:51Z

+                "python.dataScience.jupyterServerKernelId": {
+                    "type": "string",
+                    "default": "",
+                    "description": "Select the Jupyter server Kernel UUID to connect to. Leave blank to start a new kernel",


The kernel here in this string should be lower case.

IanMatthewHuff · 2019-08-21T16:30:13Z

+                "python.dataScience.jupyterServerAllowKernelShutdown": {
+                    "type": "boolean",
+                    "default": true,
+                    "description": "Shutdown the Jupyter kernel when finished",


"." at the end of this string to make it a full sentence.

IanMatthewHuff · 2019-08-21T16:30:42Z

    "DataScience.jupyterSelectURISpecifyURI": "Type in the URI to connect to a running Jupyter server",
    "DataScience.jupyterSelectURIPrompt": "Enter the URI of a Jupyter server",
    "DataScience.jupyterSelectURIInvalidURI": "Invalid URI specified",
+    "DataScience.jupyterServerReconnectKernelLocal": "Select Jupyer Kernel",


Lower case kernel in a couple of instances here.

Also the Local on the end isn't needed. We don't use that for our localized strings and it could cause some confusion with actual Local / Remote server cases.

IanMatthewHuff · 2019-08-21T16:31:19Z

+import { JupyterExecutionBase } from './jupyter/jupyterExecution';
+import { ICodeWatcher, IDataScience, IDataScienceCodeLensProvider, IDataScienceCommandListener, IJupyterSessionManager } from './types';
+
+interface IKernelQuickPickItem extends vscode.QuickPickItem {


If you could bump this over into the ./types file that is where we keep our vscode extension types.

IanMatthewHuff · 2019-08-21T16:57:03Z

+                allowShutdown = false;
+            }
+            sendTelemetryEvent(Telemetry.JupyterKernelAutoShutdown, undefined, { autoShutdownEnabled: allowShutdown });
+            await this.configuration.updateSetting('dataScience.jupyterServerAllowKernelShutdown', allowShutdown, undefined, vscode.ConfigurationTarget.Workspace);


This looks solid, with one small change needed here. Currently these settings are specified when selecting a remote server URI, but they also are applying for the local launch case. Given that the ID / Shutdown are specified after the remote URI I think that users would be surprised if they move back to working locally and all of a sudden their session are not shutting down on close. These setting should be cleared out in the SetJupyterURIToLocal case.

IanMatthewHuff · 2019-08-21T17:01:36Z

        // Create a new session and attempt to connect to it
-        const session = new JupyterSession(connInfo, kernelSpec, this.jupyterPasswordConnect);
+        const settings = this.configurationService.getSettings();
+        const allowShutdown = settings.datascience.jupyterServerAllowKernelShutdown;


These settings will need to be passed down here via the IConnection object and via the INotebookServerOptions at the JupyterExecution startOrConnect level not pulled directly from the configuration. We use those INotebookServerOption to determine if we can resuse an existing connection in serverCache.ts. With this implemented as is our code will see a connection to server A kernel B as the exact same as a connection to server A kernel C and will try to reuse the old connection, even if the user has specified a new kernel to connect to.

hochshi · 2019-09-03T05:58:18Z

Hi,
I've applied the requested changes with some extras.
Mainly, the user can now specify which kernel to start instead of using the base kernel every time.

One thing to note, when leaving the kernel running, it is not clear how should the user kill that kernel - perhaps a kill button should be added to the interactive python window.

How would you like me to proceed? Create a new PR?

rchiodo · 2019-09-03T18:20:06Z


 export class JupyterExecutionBase implements IJupyterExecution {

+    public get sessionChanged(): Event<void> {


sessionChanged [](start = 15, length = 14)

I believe the linter will complain here. This should be where it was before.

rchiodo

🕐

rchiodo · 2019-09-03T19:22:44Z

        // Create a new session and attempt to connect to it
-        const session = new JupyterSession(connInfo, kernelSpec, this.jupyterPasswordConnect);
+        const settings = this.configurationService.getSettings();
+        const allowShutdown = settings.datascience.jupyterServerAllowKernelShutdown;


jupyterServerAllowKernelShutdown [](start = 51, length = 32)

Related to what Ian said in datascience.ts, I think the allowKernelShutdown should only be used if were in a remote situation. You could check that here by looking at the localLaunch field of the connInfo

rchiodo · 2019-10-10T18:25:55Z

Since @hochshi has taken this over, closing this PR

kdkavanagh added 6 commits August 17, 2019 13:28

Working kernel selection when session created

f44ae6a

Select jupyter kernel once up front

509e50b

Rename temp file created when initially connecting to jupyter to avoi…

f915eb5

…d nasty jupyter session behavior

update news

bad2140

fix typo

17f8f61

Add localization

ea2ec27

Linting

8059106

gramster added data science feature-request Request for new features or functionality labels Aug 20, 2019

IanMatthewHuff self-requested a review August 20, 2019 23:21

IanMatthewHuff requested changes Aug 22, 2019

View reviewed changes

rchiodo reviewed Sep 3, 2019

View reviewed changes

rchiodo requested changes Sep 3, 2019

View reviewed changes

rchiodo reviewed Sep 3, 2019

View reviewed changes

hochshi mentioned this pull request Sep 4, 2019

Choose kernel and reconnect to running kernel #7191

Closed

10 tasks

hochshi mentioned this pull request Oct 7, 2019

Choose kernel and reconnect to running kernel #7790

Closed

10 tasks

rchiodo closed this Oct 10, 2019

lock Bot locked as resolved and limited conversation to collaborators Oct 17, 2019


		export class JupyterExecutionBase implements IJupyterExecution {

		public get sessionChanged(): Event<void> {

Conversation

kdkavanagh commented Aug 18, 2019

Uh oh!

msftclas commented Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janosh commented Aug 19, 2019

Uh oh!

gramster commented Aug 20, 2019

Uh oh!

IanMatthewHuff commented Aug 20, 2019

Uh oh!

kdkavanagh commented Aug 21, 2019

Uh oh!

IanMatthewHuff commented Aug 21, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hochshi commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

msftclas commented Aug 18, 2019 •

edited

Loading

hochshi commented Sep 3, 2019 •

edited

Loading