Kernel selection UI #8866
Kernel selection UI #8866
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8866 +/- ##
==========================================
- Coverage 57.64% 56.89% -0.75%
==========================================
Files 530 550 +20
Lines 27664 28957 +1293
Branches 4185 4376 +191
==========================================
+ Hits 15946 16476 +530
- Misses 10824 11581 +757
- Partials 894 900 +6
Continue to review full report at Codecov.
|
to change server and kernel
added state to jupyter server, still missing states busy and idle
- moved the session event handling to jupyterSession.ts - moved the server and kernel select dropdown to interactiveBase.ts - added the kernel select UI to the interactive window
- added telemetry - moved kernel selection UI to its own react component - call the kernelSelector when pressing the UI
jupyterSession once
| }; | ||
|
|
||
| export interface IFont { | ||
| size: number; | ||
| family: string; | ||
| } | ||
|
|
||
| export interface IKernelState { | ||
| jupyterServerStatus: ServerStatus; | ||
| uri: string; |
There was a problem hiding this comment.
Jupyter server status can't be a subset of kernel state.
It needs to be the other way around.
Id rename this type to IServerState
| @@ -283,6 +283,9 @@ | |||
| "DataScience.loadingMessage": "loading ...", | |||
| "DataScience.fetchingDataViewer": "Fetching data ...", | |||
| "DataScience.noRowsInDataViewer": "No rows match current filter", | |||
| "DataScience.jupyterServer": "Jupyter Server", | |||
| "DataScience.noKernel": "No Kernel", | |||
| "DataScience.localJupyterServer": "local", | |||
There was a problem hiding this comment.
This is a dup, we need to use the existing setting Settings.JupyterServerLocalLaunch.
There was a problem hiding this comment.
I'm going to keep this here, but use it only for display
| private async selectKernel() { | ||
| const settings = this.configuration.getSettings(); | ||
|
|
||
| if (settings.datascience.jupyterServerURI === localize.DataScience.localJupyterServer()) { |
There was a problem hiding this comment.
I'd prefer if this was in a central location. Here we added a new localizable setting to match this value against.
Matching against localized settings in such a way to determine whether something is local or not seems flaky,
| } | ||
|
|
||
| if (kernel && kernel.kernelSpec && this.notebook) { | ||
| let name = kernel.kernelSpec?.display_name; |
There was a problem hiding this comment.
kernel.kernelSpec? isn't necessary, as the if condition has ensured kernelSpec isn't undefined.
| }).ignoreErrors(); | ||
|
|
||
| // Also actually tell the kernel. | ||
| await this.notebook.setKernelSpec(kernel.kernelSpec); |
There was a problem hiding this comment.
Should't this be done first, before we update the UI?
What if this fails.
There was a problem hiding this comment.
Yeah that's probably better. Let me try that. I think I can eliminate the status update then too.
In reply to: 356777545 [](ancestors = 356777545)
For #8866 When selecting a remote session, connect to that session instead of connecting to that kernel. If we connect to a kernel, the API shutdowns the previously connected kernel. With the new changes, when selecting a remote kernel, we're in-fact selecting a remote session that we connect to. However the API is still indicates we're connecting to a kernel (however under the hoods we connect to a session).
For #8866 When selecting a remote session, connect to that session instead of connecting to that kernel. If we connect to a kernel, the API shutdowns the previously connected kernel. With the new changes, when selecting a remote kernel, we're in-fact selecting a remote session that we connect to. However the API is still indicates we're connecting to a kernel (however under the hoods we connect to a session).
For #8611
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed)