Skip to content
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

Fix reopening a remote kernel #8510

Merged
merged 10 commits into from
Dec 10, 2021
Merged

Fix reopening a remote kernel #8510

merged 10 commits into from
Dec 10, 2021

Conversation

rchiodo
Copy link
Contributor

@rchiodo rchiodo commented Dec 9, 2021

Fixes #7610

Made a new event on the controller indicating when remote kernels are refreshed. This allows me to listen for that and update a memento setting for the kernel id.

Initially I had attempted to just do that for all kernel starts, but the notebookcontrollermanager doesn't have the kernel at that point so there's no way to tell which notebook it's for. I needed an event that fired AFTER the controller has updated because of kernel start.

@rchiodo rchiodo requested a review from a team as a code owner December 9, 2021 23:41
@rchiodo
Copy link
Contributor Author

rchiodo commented Dec 9, 2021

Oh and I also changed the UI a little bit.

Old UI shows this for remote kernels in the selector:

image

I changed it so that the notebook path was part of the real name:

image

The reason for this is because there's no way to tell you're connected to an already running kernel in the notebook without it.

All of the kernels look the same:

image

Whereas now the existing ones will look like this:

image

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2021

Codecov Report

Merging #8510 (636a7db) into main (a2579ab) will decrease coverage by 0%.
The diff coverage is 63%.

❗ Current head 636a7db differs from pull request most recent head b923a69. Consider uploading reports for the commit b923a69 to get more accurate results

@@          Coverage Diff          @@
##            main   #8510   +/-   ##
=====================================
- Coverage     71%     71%   -1%     
=====================================
  Files        377     378    +1     
  Lines      23920   23982   +62     
  Branches    3681    3691   +10     
=====================================
+ Hits       17204   17225   +21     
- Misses      5225    5270   +45     
+ Partials    1491    1487    -4     
Impacted Files Coverage Δ
src/client/datascience/baseJupyterSession.ts 76% <ø> (-1%) ⬇️
src/client/datascience/jupyter/jupyterSession.ts 67% <0%> (-5%) ⬇️
src/client/datascience/notebook/types.ts 100% <ø> (ø)
...client/datascience/raw-kernel/rawJupyterSession.ts 77% <0%> (+5%) ⬆️
src/client/datascience/types.ts 100% <ø> (ø)
...t/datascience/notebook/vscodeNotebookController.ts 75% <20%> (-6%) ⬇️
...ient/datascience/jupyter/kernels/kernelSelector.ts 68% <53%> (-11%) ⬇️
src/client/datascience/jupyter/kernels/helpers.ts 71% <66%> (-2%) ⬇️
.../client/datascience/notebook/liveKernelSwitcher.ts 68% <68%> (ø)
.../datascience/notebook/notebookControllerManager.ts 71% <100%> (-4%) ⬇️
... and 36 more

@rchiodo rchiodo merged commit 488474e into main Dec 10, 2021
@rchiodo rchiodo deleted the rchiodo/open_remote_notebook branch December 10, 2021 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opening a notebook on remote, closing it, and reopening it does not reuse the same kernel
4 participants