Choose kernel and reconnect to running kernel by hochshi · Pull Request #7790 · microsoft/vscode-python

hochshi · 2019-10-07T09:05:58Z

For #7014 , #3763

Fixes the requested changes on #7015 and adds the ability the select the remote kernel to connect to. This was done together since the kernel uuid is passed down using IJupyterKernelSpec instead of setting a different setting just for the kernel UUID.

…reconnect to remote kernel.

Shamelessly stole waitForStatus to allow the user to cancel a long server poll. Moved createConnectionInfo to jupyterUtils so calls to get running and available kernels wouldn't require creating a kernel. Selected KernelSpec is wrote to workspace settings and read from workspace settings when launching a new kernel.

Modified helper function defaultWaitForStatus to properly catch exception and display error message. Modified helper function getKernelSelection to throw CancellationError if user made no selection. As result modified selectRemoteJupyterKernel to be concise - no need to check if the user has made no selection this is handled by the catch clause.

…es.json

codecov-io · 2019-10-07T10:06:30Z

Codecov Report

Merging #7790 into master will decrease coverage by 0.42%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #7790      +/-   ##
==========================================
- Coverage   59.25%   58.83%   -0.43%     
==========================================
  Files         498      499       +1     
  Lines       22293    22961     +668     
  Branches     3580     3791     +211     
==========================================
+ Hits        13210    13509     +299     
- Misses       8261     8624     +363     
- Partials      822      828       +6

Impacted Files	Coverage Δ
...rc/client/datascience/jupyter/jupyterKernelSpec.ts	`80% <ø> (ø)`	⬆️
src/client/telemetry/index.ts	`86.13% <ø> (ø)`	⬆️
src/client/common/types.ts	`100% <ø> (ø)`	⬆️
...ce/jupyter/liveshare/guestJupyterSessionManager.ts	`20% <0%> (-2.23%)`	⬇️
src/client/datascience/constants.ts	`100% <100%> (ø)`	⬆️
src/client/common/utils/localize.ts	`94.2% <100%> (+0.19%)`	⬆️
src/client/datascience/types.ts	`100% <100%> (ø)`	⬆️
src/client/datascience/jupyter/jupyterExecution.ts	`53.47% <25%> (+0.79%)`	⬆️
src/client/datascience/jupyter/jupyterUtils.ts	`88.88% <77.77%> (-11.12%)`	⬇️
src/client/common/application/applicationShell.ts	`11.11% <0%> (-4.68%)`	⬇️
... and 15 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 4a2a0d5...025307c. Read the comment docs.

hochshi · 2019-10-07T12:45:18Z

Hi @rchiodo,

I did my best to resolve all validation errors. However some tests behavior is inconsistent.
Both PR Validation (Tests Test Linux-Py3.7 Unit) and PR Validation (Tests Test Mac-Py3.7 Unit) failure is resolved when running using npm run test:unittests -- --grep "Workspace Symbols Provider". This leads me to believe some other test creates a race condition or doesn't teardown properly.

Please let me know if there's something I should take care of, thanks.

DonJayamanne · 2019-10-07T14:24:40Z

Thanks for addressing the issues. I'll have a look at this today.

rchiodo · 2019-10-08T19:19:23Z

                    "scope": "resource"
                },
+                "python.dataScience.jupyterServerKernelSpec": {
+                    "type": "object",


object [](start = 29, length = 6)

I believe this should be a string? Or is this the entire kernel.json?

No and no.
This is a IJupyterKernelSpec object.
This is the object used by IJupyterSessionManager to start a new session.

rchiodo · 2019-10-08T19:24:34Z

+        };
+        try {
+            const retVal = await this.waitForStatus(promise, message, undefined, canceled);
+            return Promise.resolve(retVal);


Promise.resolve(retVal) [](start = 19, length = 23)

This is unnecessary, you can just return the return value. You still need the await so that the catch happens though

As DonJayamanne requested, I've changed the way errors are handled and so there is no need for defaultwaitForStatus.

rchiodo · 2019-10-08T19:30:37Z

+    private async getKernelSpecSelection(kernelSpecs: IJupyterKernelSpec[]): Promise<IKernelQuickPickItem> {
+        const availArr: IKernelQuickPickItem[] = kernelSpecs.map(availableKernel => {
+            return {
+                label: `Kernel ${availableKernel.name}`,


Kernel ${availableKernel.name} [](start = 24, length = 30)

All of these labels have to be localized. They should use a format string from the localize.ts/package.nls.json

rchiodo · 2019-10-08T19:32:48Z


        if (userURI) {
            await this.configuration.updateSetting('dataScience.jupyterServerURI', userURI, undefined, vscode.ConfigurationTarget.Workspace);
+            await this.selectRemoteJupyterKernel();


selectRemoteJupyterKernel [](start = 23, length = 25)

This forcing people to pick a remote jupyter kernel and forcing a connection to the remote host at this point. This means the server has to be up already. We didn't have this requirement before. I think it's okay, but I want to ask @ronglums and @jmew what they think

I think at the least we should have a flag enabling this functionality.

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

If the server is not ready then the remote kernel spec and auto shutdown properties remain unchanged.
At this point this functionality is only implemented in the remote selection stage and not when starting the interactive or native windows.
If you think it would be more appropriate to move this functionality - let me know. It might assist in allowing the interactive or native interpreters to use different kernels for different notebooks simultaneously.

We'll likely change/add to the UI around this in the future (we're probably going to have a kernel picking UI in each notebook/interactive window), but your solution is a good stopgap. We're trying to decide if your quickpick way will interfere with the new UI later or not.

Would people be upset if the quickpick UI and settings went away (or maybe we just use them as the default settings for a kernel in the new UI)

If you think it should be implemented in another fashion - let me know I'll get it done.

rchiodo · 2019-10-08T19:33:46Z

@hochshi can you include a screenshot of the different quick picks? It's hard to tell what they're going to look like

rchiodo · 2019-10-08T19:35:07Z

+        return arr;
+    }
+
+    private async getKernelSelection(kernelOptions: IKernelQuickPickItem[]): Promise<IKernelQuickPickItem> {


getKernelSelection [](start = 18, length = 18)

I think i'd change the name of the functions that are showing a quick pick to something like pickKernelSelection instead of getKernelSelection. At least some indication that the function is going to display some UI

I've added "QuickPick" prefix to functions which display UI elements.

hochshi · 2019-10-09T19:53:33Z

@DonJayamanne, @rchiodo Thank you for taking the time to review the PR.
I was on a short vacation and will apply the requested changes tomorrow.

1. Localized strings. 2. Moved error handling to selectRemoteJupyterKernel - the very root. 3. Fixed a typo in ShutdownOptions constants.

hochshi · 2019-10-10T16:38:23Z

@rchiodo, as you've requested here are screencast of the kernel setup process.

First local server:

Followed by using a remote server - showing two running kernels:

Quick remark:
I've grew tired of retyping the server URI each time and so I've added a undocumented setting:
"python.dataScience.jupyterServers".

…nto microsoft-master

rchiodo

Looks good to me. We're going to discuss today if we want to go forward with the UI as is, or maybe put the whole thing behind a flag.

Thanks for doing this 👍

hochshi · 2019-10-10T20:42:48Z

Looks good to me. We're going to discuss today if we want to go forward with the UI as is, or maybe put the whole thing behind a flag.

Thanks for doing this 👍

My pleasure.

jmew · 2019-10-10T22:11:33Z

LGTM

rchiodo · 2019-10-10T22:52:24Z

Thanks @hochshi , we're cool with the current design. We're going to do something similar in the future but this will work great as a first step for allowing remote kernel selection.

Once @DonJayamanne signs off, we'll submit.

rchiodo · 2019-10-14T21:48:48Z

@don.jayamanne@yahoo.com do you have any more input?

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

rchiodo · 2019-10-17T17:37:00Z

@DonJayamanne I believe we agreed to submit this, did you have any other comments?

DonJayamanne · 2019-10-17T18:34:51Z

@rchiodo I'll kick off our nightly and CI pipeline against this PR to ensure things are ok before merging.

DonJayamanne

The Startup and shutdown test is failing
So far its failing on Mac & Linux. Fairly certain it'll fail on Windows as well.

Please could you look into the failing test.
You can run the functional tests by running the command npm run test:functional
If you want to run just the failing test suite, you can run the command npm run test:functional -- --grep="Editor tests"

@rchiodo /cc

hochshi · 2019-10-24T05:03:52Z

Hi @DonJayamanne @rchiodo,
Anything I can do to push this forward?

greazer · 2019-12-03T23:33:21Z

Hi @hochsi, so sorry for the delay. While it seemed clear that we were ready and able to accept your PR, we did some further discussion and investigation of the overall kernel management problem. We discovered that pushing this particular PR was going to be painting us into a corner with regard to how we would be able to implement other kernel management behaviors and/or make changes further down the line. Since providing better kernel management was high on our backlog priority, we decided it would be best to not to push this particular change through. We are highly grateful for the interest, insight, and work you provided to us about this problem and its potential solutions!

The overall issue that's tracking all of the changes being made to support kernel management can be seen by looking at this issue: #8207 . Note that it's best to view it through the lens of zenhub, but without that you can view linked issues by scanning through 8207's issue activity, and view each issue that has been "added [issue] to this epic".

shayho and others added 3 commits October 4, 2019 13:53

Reimplemented: choose remote kernel, leave remote kernel running and …

0522f4f

…reconnect to remote kernel.

hochshi mentioned this pull request Oct 7, 2019

Choose kernel and reconnect to running kernel #7191

Closed

10 tasks

Added required package node-libs-browser to datascience-ui.dependenci…

6bca6e7

…es.json

Added missing localization keys.

a7ce57c

kimadeline added external contributor labels Oct 7, 2019

rchiodo requested review from DavidKutu, DonJayamanne, IanMatthewHuff and rchiodo October 8, 2019 19:12

rchiodo reviewed Oct 8, 2019

View reviewed changes

DonJayamanne requested changes Oct 8, 2019

View reviewed changes

Applied requested changes.

a75025a

1. Localized strings. 2. Moved error handling to selectRemoteJupyterKernel - the very root. 3. Fixed a typo in ShutdownOptions constants.

hochshi added 4 commits October 10, 2019 19:39

Fixed a typo in kernel quick pick item detail.

fcf8964

Fixed conflicting files.

9c6dbc5

Merge branch 'master' of https://github.com/microsoft/vscode-python i…

b78123c

…nto microsoft-master

Merge branch 'microsoft-master'

025307c

hochshi requested a review from DonJayamanne October 10, 2019 17:48

hochshi requested a review from rchiodo October 10, 2019 17:48

rchiodo requested changes Oct 10, 2019

View reviewed changes

rchiodo approved these changes Oct 10, 2019

View reviewed changes

DonJayamanne approved these changes Oct 17, 2019

View reviewed changes

DonJayamanne requested changes Oct 17, 2019

View reviewed changes

gramster added this to the DS Bucket milestone Oct 23, 2019

gramster modified the milestone: DS Bucket Oct 30, 2019

greazer closed this Dec 3, 2019

lock Bot locked as resolved and limited conversation to collaborators Dec 10, 2019

Conversation

hochshi commented Oct 7, 2019

Uh oh!

codecov-io commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hochshi commented Oct 7, 2019

Uh oh!

DonJayamanne commented Oct 7, 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!

hochshi Oct 10, 2019 • edited 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!

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!

rchiodo commented Oct 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hochshi commented Oct 9, 2019

Uh oh!

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

Uh oh!

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

hochshi commented Oct 10, 2019

Uh oh!

jmew commented Oct 10, 2019

Uh oh!

rchiodo commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchiodo commented Oct 14, 2019

Uh oh!

rchiodo commented Oct 17, 2019

Uh oh!

DonJayamanne commented Oct 17, 2019

Uh oh!

DonJayamanne left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hochshi commented Oct 24, 2019

Uh oh!

greazer commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov-io commented Oct 7, 2019 •

edited

Loading

hochshi Oct 10, 2019 •

edited

Loading

hochshi commented Oct 10, 2019 •

edited

Loading

rchiodo commented Oct 10, 2019 •

edited

Loading

DonJayamanne left a comment •

edited

Loading