-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Choose kernel and reconnect to running kernel #7790
Conversation
…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.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Hi @rchiodo, I did my best to resolve all validation errors. However some tests behavior is inconsistent. Please let me know if there's something I should take care of, thanks. |
Thanks for addressing the issues. I'll have a look at this today. |
"python.dataScience.jupyterServerURI": { | ||
"type": "string", | ||
"default": "local", | ||
"description": "Select the Jupyter server URI to connect to. Select 'local' to launch a new Juypter server on the local machine.", | ||
"scope": "resource" | ||
}, | ||
"python.dataScience.jupyterServerKernelSpec": { | ||
"type": "object", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object [](start = 29, length = 6)
I believe this should be a string? Or is this the entire kernel.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No and no.
This is a IJupyterKernelSpec object.
This is the object used by IJupyterSessionManager to start a new session.
}; | ||
try { | ||
const retVal = await this.waitForStatus(promise, message, undefined, canceled); | ||
return Promise.resolve(retVal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As DonJayamanne requested, I've changed the way errors are handled and so there is no need for defaultwaitForStatus.
private async getKernelSpecSelection(kernelSpecs: IJupyterKernelSpec[]): Promise<IKernelQuickPickItem> { | ||
const availArr: IKernelQuickPickItem[] = kernelSpecs.map(availableKernel => { | ||
return { | ||
label: `Kernel ${availableKernel.name}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -289,6 +303,159 @@ export class DataScience implements IDataScience { | |||
|
|||
if (userURI) { | |||
await this.configuration.updateSetting('dataScience.jupyterServerURI', userURI, undefined, vscode.ConfigurationTarget.Workspace); | |||
await this.selectRemoteJupyterKernel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at the least we should have a flag enabling this functionality.
In reply to: 332693871 [](ancestors = 332693871)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it should be implemented in another fashion - let me know I'll get it done.
@hochshi can you include a screenshot of the different quick picks? It's hard to tell what they're going to look like |
return arr; | ||
} | ||
|
||
private async getKernelSelection(kernelOptions: IKernelQuickPickItem[]): Promise<IKernelQuickPickItem> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added "QuickPick" prefix to functions which display UI elements.
@DonJayamanne, @rchiodo Thank you for taking the time to review the PR. |
1. Localized strings. 2. Moved error handling to selectRemoteJupyterKernel - the very root. 3. Fixed a typo in ShutdownOptions constants.
@rchiodo, as you've requested here are screencast of the kernel setup process. Followed by using a remote server - showing two running kernels: Quick remark: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
LGTM |
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. |
@don.jayamanne@yahoo.com do you have any more input? In reply to: 540830886 [](ancestors = 540830886) |
@DonJayamanne I believe we agreed to submit this, did you have any other comments? |
@rchiodo I'll kick off our nightly and CI pipeline against this PR to ensure things are ok before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Hi @DonJayamanne @rchiodo, |
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". |
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.
Test plan is updated as appropriatepackage-lock.json
has been regenerated by runningnpm install
(if dependencies have changed)