Kernel selection UI by DavidKutu · Pull Request #8866 · microsoft/vscode-python

David Kutugata (DavidKutu) · 2019-12-02T21:46:16Z

For #8611

part of the kernel data

Codecov (codecov-io) · 2019-12-03T01:08:55Z

Codecov Report

Merging #8866 into master will decrease coverage by 0.74%.
The diff coverage is 33.12%.

@@            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

Impacted Files	Coverage Δ
...tascience-ui/native-editor/redux/reducers/index.ts	`100% <ø> (+100%)`	⬆️
...atascience/jupyter/liveshare/guestJupyterServer.ts	`11.94% <ø> (ø)`	⬆️
src/datascience-ui/react-common/locReactSide.ts	`50% <ø> (+50%)`	⬆️
src/client/datascience/types.ts	`100% <ø> (ø)`	⬆️
...tascience-ui/history-react/redux/reducers/index.ts	`100% <ø> (+100%)`	⬆️
...ascience/interactive-ipynb/nativeEditorProvider.ts	`55.24% <0%> (ø)`	⬆️
src/datascience-ui/native-editor/redux/actions.ts	`11.62% <0%> (+11.62%)`	⬆️
...ce/interactive-window/interactiveWindowProvider.ts	`15.11% <0%> (ø)`	⬆️
src/datascience-ui/history-react/redux/actions.ts	`9.67% <0%> (+9.67%)`	⬆️
...ence-ui/interactive-common/redux/reducers/types.ts	`100% <100%> (ø)`
... and 66 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 a1dc04c...f2caf13. Read the comment docs.

to change server and kernel

added state to jupyter server, still missing states busy and idle

Rich Chiodo (rchiodo) · 2019-12-04T23:47:13Z

});

We should probably have a test for changing the current kernel and making sure the UI updates. Ideally also for restart/interrupt as well.

Refers to: src/test/datascience/interactive-ipynb/nativeEditor.unit.test.ts:464 in 5cc2346. [](commit_id = 5cc2346, deletion_comment = False)

Rich Chiodo (rchiodo)

🕐

- 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

Rich Chiodo (rchiodo)

jupyterSession once

Don Jayamanne (DonJayamanne)

Please use existing Settings.JupyterServerLocalLaunch

Don Jayamanne (DonJayamanne) · 2019-12-04T02:09:00Z


+export interface IKernelState {
+    jupyterServerStatus: ServerStatus;
+    uri: string;


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

Don Jayamanne (DonJayamanne) · 2019-12-11T17:05:25Z

    "DataScience.noRowsInDataViewer": "No rows match current filter",
+    "DataScience.jupyterServer": "Jupyter Server",
+    "DataScience.noKernel": "No Kernel",
+    "DataScience.localJupyterServer": "local",


This is a dup, we need to use the existing setting Settings.JupyterServerLocalLaunch.

I'm going to keep this here, but use it only for display

Don Jayamanne (DonJayamanne) · 2019-12-11T17:06:27Z

+    private async selectKernel() {
+        const settings = this.configuration.getSettings();
+
+        if (settings.datascience.jupyterServerURI === localize.DataScience.localJupyterServer()) {


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,

Optional change.

…lection_UI

Don Jayamanne (DonJayamanne)

Shoudnt we update UI after the kernel switch has been successfully made?

Don Jayamanne (DonJayamanne) · 2019-12-11T18:59:03Z

+        }
+
+        if (kernel && kernel.kernelSpec && this.notebook) {
+            let name = kernel.kernelSpec?.display_name;


kernel.kernelSpec? isn't necessary, as the if condition has ensured kernelSpec isn't undefined.

Don Jayamanne (DonJayamanne) · 2019-12-11T19:01:34Z

+            }).ignoreErrors();
+
+            // Also actually tell the kernel.
+            await this.notebook.setKernelSpec(kernel.kernelSpec);


Should't this be done first, before we update the UI?
What if this fails.

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 For #9166

David Kutugata added 4 commits November 27, 2019 13:34

added the kernel selection UI

55bf0f4

Merge branch 'master' into davidkutu/kernel_selection_UI

3ef83f8

Send message back to react side with

5e22dc5

part of the kernel data

Merge branch 'master' into davidkutu/kernel_selection_UI

a89eb8d

Don Jayamanne (DonJayamanne) requested changes Dec 3, 2019

View reviewed changes

David Kutugata added 5 commits December 3, 2019 10:09

Merge branch 'master' into davidkutu/kernel_selection_UI

6293ef1

adressed PR comments

5a4a6aa

Merge branch 'master' into davidkutu/kernel_selection_UI

7aee062

added news file and created a separate message

bc0d9f7

to change server and kernel

changed kernel version to display name

5cc2346

added state to jupyter server, still missing states busy and idle

David Kutugata (DavidKutu) commented Dec 4, 2019

View reviewed changes

Comment thread src/client/datascience/interactive-ipynb/nativeEditor.ts Outdated

David Kutugata (DavidKutu) commented Dec 4, 2019

View reviewed changes

Comment thread src/client/datascience/jupyter/jupyterServer.ts Outdated

David Kutugata (DavidKutu) commented Dec 4, 2019

View reviewed changes

Comment thread src/client/datascience/jupyter/liveshare/guestJupyterServer.ts Outdated

David Kutugata (DavidKutu) commented Dec 4, 2019

View reviewed changes

Comment thread src/datascience-ui/react-common/locReactSide.ts

David Kutugata (DavidKutu) requested review from Ian Huff (IanMatthewHuff) and Rich Chiodo (rchiodo) December 4, 2019 22:56

Rich Chiodo (rchiodo) reviewed Dec 4, 2019

View reviewed changes

Comment thread src/datascience-ui/history-react/redux/mapping.ts Outdated

Rich Chiodo (rchiodo) reviewed Dec 4, 2019

View reviewed changes

Comment thread src/datascience-ui/interactive-common/redux/store.ts Outdated

Rich Chiodo (rchiodo) reviewed Dec 4, 2019

View reviewed changes

Comment thread src/datascience-ui/native-editor/nativeEditor.tsx Outdated

Rich Chiodo (rchiodo) requested changes Dec 4, 2019

View reviewed changes

Ian Huff (IanMatthewHuff) reviewed Dec 4, 2019

View reviewed changes

Comment thread src/client/common/utils/localize.ts Outdated

Ian Huff (IanMatthewHuff) reviewed Dec 5, 2019

View reviewed changes

Comment thread src/datascience-ui/native-editor/nativeEditor.less Outdated

David Kutugata added 6 commits December 6, 2019 17:40

- localized a hardcoded string

ee1169a

- 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

Merge branch 'master' into davidkutu/kernel_selection_UI

a86da06

- added an icon to the kernel selection UI

2f1238b

- added telemetry - moved kernel selection UI to its own react component - call the kernelSelector when pressing the UI

Merge branch 'master' into davidkutu/kernel_selection_UI

02cbba0

added tests

0b5dac3

fixed tests

e25eb72

Rich Chiodo (rchiodo) approved these changes Dec 10, 2019

View reviewed changes

David Kutugata added 2 commits December 10, 2019 16:02

addressed some comments

22efccd

only bind statusChanged event on the

bbbae0a

jupyterSession once

Don Jayamanne (DonJayamanne) requested changes Dec 11, 2019

View reviewed changes

Rich Chiodo (rchiodo) added 3 commits December 11, 2019 09:22

Merge remote-tracking branch 'origin/master' into davidkutu/kernel_se…

23accfc

…lection_UI

Code review comments

5f0bb15

Fix changing kernels to actually change the kernel

25e2d19

Don Jayamanne (DonJayamanne) requested changes Dec 11, 2019

View reviewed changes

Make sure notebook has kernel before ui does.

2f758f4

Don Jayamanne (DonJayamanne) approved these changes Dec 11, 2019

View reviewed changes

Fix linter and functional test

f2caf13

Rich Chiodo (rchiodo) merged commit 6cadda5 into master Dec 11, 2019

Rich Chiodo (rchiodo) deleted the davidkutu/kernel_selection_UI branch December 11, 2019 21:16

This was referenced Dec 17, 2019

Restart live kernel instead of starting a new session #9150

Merged

Add telemetry for failed activation and searching kernel #9158

Merged

Install ipykernel into prescribed conda environment #9167

Merged

Don Jayamanne (DonJayamanne) added a commit that referenced this pull request Dec 17, 2019

Install ipykernel into prescribed conda environment (#9167)

5cba610

For #8866 For #9166

Don Jayamanne (DonJayamanne) added a commit that referenced this pull request Dec 18, 2019

Install ipykernel into prescribed conda environment (#9167)

ef4b66a

For #8866 For #9166

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

Uh oh!

Conversation

David Kutugata (DavidKutu) commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Codecov (codecov-io) commented Dec 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rich Chiodo (rchiodo) commented Dec 4, 2019

Uh oh!

Rich Chiodo (rchiodo) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Rich Chiodo (rchiodo) left a comment

Choose a reason for hiding this comment

Uh oh!

Don Jayamanne (DonJayamanne) left a comment

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!

Don Jayamanne (DonJayamanne) left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

David Kutugata (DavidKutu) commented Dec 2, 2019 •

edited

Loading

Codecov (codecov-io) commented Dec 3, 2019 •

edited

Loading