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

Display local & remote kernels together #8490

Merged
merged 20 commits into from
Dec 9, 2021
Merged

Display local & remote kernels together #8490

merged 20 commits into from
Dec 9, 2021

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Dec 8, 2021

For #8489

Need to add some tests

  • Run a cell against a local kernel, connect to remote Jupyter and verify the local kernel state is not lost,
  • Run cell in same notebook with local kernel and remote kernel
  • Ensure local and remote kernels are listed in controllers when connecting to remote
  • Ensure only local kernels are listed in controllers when switching to local
  • Keep track of the controllers (base url) and verify it is different when connecting to a different remote Jupyter (with a different port)

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2021

Codecov Report

Merging #8490 (fc22759) into main (ee85faf) will increase coverage by 0%.
The diff coverage is 71%.

❗ Current head fc22759 differs from pull request most recent head 1c9201a. Consider uploading reports for the commit 1c9201a to get more accurate results

@@          Coverage Diff          @@
##            main   #8490   +/-   ##
=====================================
  Coverage     71%     71%           
=====================================
  Files        377     377           
  Lines      23928   23951   +23     
  Branches    3679    3686    +7     
=====================================
+ Hits       17189   17226   +37     
+ Misses      5246    5235   -11     
+ Partials    1493    1490    -3     
Impacted Files Coverage Δ
.../datascience/kernel-launcher/remoteKernelFinder.ts 95% <ø> (+4%) ⬆️
src/client/datascience/kernel-launcher/types.ts 100% <ø> (ø)
src/client/datascience/types.ts 100% <ø> (ø)
src/client/datascience/jupyter/serverUriStorage.ts 83% <33%> (-4%) ⬇️
src/client/common/utils/multiStepInput.ts 73% <50%> (-3%) ⬇️
src/client/datascience/jupyter/serverSelector.ts 71% <63%> (-3%) ⬇️
.../datascience/notebook/notebookControllerManager.ts 74% <77%> (+2%) ⬆️
src/client/common/utils/localize.ts 95% <100%> (+<1%) ⬆️
src/client/datascience/jupyter/kernels/helpers.ts 72% <100%> (+<1%) ⬆️
...t/datascience/kernel-launcher/localKernelFinder.ts 82% <100%> (ø)
... and 21 more

@@ -106,6 +117,19 @@ export class MultiStepInput<S> implements IMultiStepInput<S> {
input.placeholder = placeholder;
input.ignoreFocusOut = true;
input.items = items;
if (onDidChangeItems) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per demo, allow one to remove individual items

: DataScience.jupyterSelectURIQuickPickTitleRemoteOnly(),
onDidTriggerItemButton: (e) => {
const url = e.item.url;
if (url && e.button.tooltip === DataScience.removeRemoteJupyterServerEntryInQuickPick()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abiliy to remove an item instead of clearing everything in the list.

@DonJayamanne DonJayamanne marked this pull request as ready for review December 9, 2021 19:32
@DonJayamanne DonJayamanne requested a review from a team as a code owner December 9, 2021 19:32
@DonJayamanne DonJayamanne merged commit 4b37682 into main Dec 9, 2021
@DonJayamanne DonJayamanne deleted the localAndRemove branch December 9, 2021 22: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.

4 participants