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

Select Kernel Source Per Document Experiment #11632

Merged
merged 7 commits into from
Oct 13, 2022

Conversation

IanMatthewHuff
Copy link
Member

@IanMatthewHuff IanMatthewHuff commented Oct 12, 2022

#11576

This PR completes shipping the basic prototype for selecting a kernel source per document. There are quite a few known hitches here still so I'll list a few.

  1. Right now it's a two step process, first you pick a kernel source with the command, then you open the picker again to pick it. My first follow up will be looking at adding a kernel selection step to the second step in picking a kernel source so that you only have to open the picker once.
  2. Suggestions are currently not wired up, I don't consider this a huge loss as I'd like to handle them differently in general, but when you select a kernel source everything is now just in a flat list.
  3. Kernel source selections are not persisted, when documents open they start in the fresh "no source selected" state
  4. Don't add the url as the default value for the server name, makes it hard to pick the name out
  5. Invalidated server entries in the list break the loading of the full set of kernel finders possibly resolved by: https://github.com/microsoft/vscode-jupyter/pull/11625/files
  6. Current selected kernel is not deselected if the notebook controller is hidden for that document

@IanMatthewHuff
Copy link
Member Author

Build is currently broken, but that's on an unrelated change currently in main, will update branch when main is fixed.

@@ -73,4 +77,9 @@ export class KernelFinder implements IKernelFinder {

return kernels;
}

// Give the info for what kernel finders are currently registered
Copy link
Member Author

Choose a reason for hiding this comment

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

Also added this so that the UI could see what kernel finders are registered. I didn't just go off the controllers that we see registered since if you add a kernel source with no controllers you would still want to see it as an option in the UI so you know that you connected to the server correctly.

@IanMatthewHuff IanMatthewHuff marked this pull request as ready for review October 12, 2022 17:38

// Only show this command if we are in our Insiders picker type
private updateVisibility() {
if (this.configService.getSettings().kernelPickerType === 'Insiders') {
Copy link
Member

Choose a reason for hiding this comment

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

It seems we are not picking up the context key change in the kernel picker so after changing the setting, a window reload is required for it to work. We might need a fix in the core.

@rebornix
Copy link
Member

The code looks good to me. Played around with it a bit and ran into this interesting scenario:

  • Select kernel source local
  • Pick a kernel
  • Run cells
  • Select another kernel source
  • Now the kernel picker will not show the local kernels anymore, but the kernel status bar and the notebook editor is still using the previous used local kernel, thus it should be hidden now.

image

src/kernels/types.ts Outdated Show resolved Hide resolved
src/kernels/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

LGTM, however please move the code to update kernelFinderInfo into the finder itself, as the finder owns creation of the KernelConnectionMetadata, this way we know that the KernelConnectionMeatadata is created and populated in one place as opposed to multiple different locations.

Copy link
Contributor

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

sorry but the connections should be ready only

@IanMatthewHuff IanMatthewHuff merged commit eaf40cb into main Oct 13, 2022
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/selectKernelPerDocument branch October 13, 2022 00:19
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.

3 participants