Hook up kernel selection to the kernel finder (part 1)#11505
Conversation
| } | ||
|
|
||
| // Provider for searching for installed kernelspecs on disk without using jupyter to search | ||
| export class InstalledRawKernelSelectionListProvider implements IKernelSelectionListProvider { |
There was a problem hiding this comment.
KernelSelections was structured pretty nicely for making a new SelectionListProvider without having to change the higher level services. So kudos @DonJayamanne .
|
Kudos, SonarCloud Quality Gate passed!
|
| } | ||
|
|
||
| // Search all our local file system locations for installed kernel specs and return them | ||
| public async listKernelSpecs(_cancelToken?: CancellationToken): Promise<IJupyterKernelSpec[]> { |
There was a problem hiding this comment.
This was the part 2 of this work. @DavidKutu was still working on some finder changes. So I didn't want to collide here hence just doing the routing up to here in this PR. Also there is a question of who should actually create the kernelspec. I believe that the kernelFinder is now caching just file paths, so since the kernelSelection code does it's own caching it might make sense to return back the paths here an have the json read on the selector side.
| @@ -356,6 +360,7 @@ suite('Data Science - Kernel Switcher', () => { | |||
| anything(), | |||
There was a problem hiding this comment.
The all anything() verify check are a bit painful since they both need to updated along with the when, and due to optional parameters they only break at test run time. Would be nice if we could combine the when and verify into one call definition, but not sure if that can be done.
| // Lets pre-warm the list of local kernels. | ||
| this.selectionProvider.getKernelSelectionsForLocalSession(resource, sessionManager, cancelToken).ignoreErrors(); | ||
| this.selectionProvider | ||
| .getKernelSelectionsForLocalSession(resource, 'jupyter', sessionManager, cancelToken) |
There was a problem hiding this comment.
jupyter [](start = 59, length = 7)
Seems like it would be better to have the calling code 'know' that an IJupyterSessionManager is a 'jupyter' kernel instead of having that knowledge explicit to this function.
There was a problem hiding this comment.
@rchiodo the tricky bit here, which I didn't actually realize until I was tracing this out, is that there are jupyter only paths currently that don't pass down a session manager. JupyterExecution does it in connectToNotebookServer. Only the kernel switching paths have session managers, so we can't rely on seeing if we have one to know if we are in a jupyter or non jupyter scenario.
There was a problem hiding this comment.
I think you're misunderstanding. This function should have an argument of 'type'. the caller of that function should make a determination of whether or not the kernels should come from jupyter or raw.
This function is only called from the jupyter code, so it would always be 'jupyter'
There was a problem hiding this comment.
Hmm, I think I did misunderstand. So instead of the explicit hardcode to 'jupyter' here this just takes a type and type gets pushed up another level to getKernelForLocalConnection. Then were getKernelForLocalConnection is called (jupyterExecution and jupyterServer) put the 'jupyter' hardcode there since those are jupyter specific classes? @rchiodo was that what you were saying?
There was a problem hiding this comment.
I think this is what you mean, but I'll add this on part 2 so I can keep it flowing.
| cancelToken?: CancellationToken | ||
| ): Promise<IKernelSpecQuickPickItem[]> { | ||
| const items = await this.kernelFinder.listKernelSpecs(cancelToken); | ||
| return items |
There was a problem hiding this comment.
I think its best to sort based on some algorithm.
There was a problem hiding this comment.
@DonJayamanne after the classes return their ipykernels they get sorted at line 287 regardless of the source. That seems the right spot so that both jupyter and raw kernel specs get the same sorting applied.
DonJayamanne
left a comment
There was a problem hiding this comment.
Happy to discuss unknonw/liveShare.
@DonJayamanne . I see your point. 'unknown' is never a great option. My only slight counter point would be that while live share is the one exception that I know about, there could be something else in the future. All that we know is that the given notebook does not have connection information. "liveshare" sounds like a specific type of connection when what we have it the absence of a connection. How do you feel about 'raw' | 'jupyter' | 'noConnection'? It think that might express it better. Either you are connected via raw kernel connection, connected via jupyter connection, or not connected. |
👍 |
Codecov Report
@@ Coverage Diff @@
## master #11505 +/- ##
==========================================
- Coverage 60.63% 60.35% -0.29%
==========================================
Files 613 614 +1
Lines 33507 33553 +46
Branches 4727 4734 +7
==========================================
- Hits 20318 20250 -68
+ Misses 12739 12303 -436
- Partials 450 1000 +550
Continue to review full report at Codecov.
|
|
windows tests were just a timeout |
This is the first part of making kernel selection independent from jupyter.
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).