Refactor KernelSelection type to include a `kind` #13407

DonJayamanne · 2020-08-12T23:41:31Z

For #13406

First refactor existing code to ensure we do not need the interpreter property along with kernelspecs

src/client/datascience/jupyter/jupyterNotebook.ts

src/client/datascience/jupyter/liveshare/hostJupyterServer.ts

src/client/datascience/types.ts

codecov-commenter · 2020-08-12T23:50:11Z

Codecov Report

Merging #13407 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@ Coverage Diff @@ ## master #13407 +/- ## ========================================== - Coverage 59.92% 59.90% -0.03%  ========================================== Files 670 670 Lines 37203 37207 +4 Branches 5312 5311 -1 ========================================== - Hits 22294 22287 -7  - Misses 13772 13780 +8  - Partials 1137 1140 +3

Impacted Files	Coverage Δ
...nt/datascience/jupyter/kernels/kernelSelections.ts	`84.21% <100.00%> (ø)`
...ient/datascience/jupyter/kernels/kernelSelector.ts	`67.93% <100.00%> (-0.18%)`	⬇️
src/client/common/utils/platform.ts	`64.70% <0.00%> (-11.77%)`	⬇️
src/client/datascience/crossProcessLock.ts	`79.41% <0.00%> (-11.77%)`	⬇️
src/client/linters/pydocstyle.ts	`86.66% <0.00%> (-2.23%)`	⬇️
src/client/datascience/debugLocationTracker.ts	`76.56% <0.00%> (-1.57%)`	⬇️
src/client/common/process/proc.ts	`14.49% <0.00%> (-0.73%)`	⬇️
src/client/datascience/jupyter/jupyterUtils.ts	`53.12% <0.00%> (-0.45%)`	⬇️
...datascience/jupyter/liveshare/hostJupyterServer.ts	`12.59% <0.00%> (ø)`
...atascience/kernel-launcher/kernelLauncherDaemon.ts	`75.00% <0.00%> (+0.92%)`	⬆️

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 66a4ffb...6e4813b. Read the comment docs.

rchiodo · 2020-08-13T00:10:30Z

src/client/datascience/jupyter/jupyterNotebook.ts

 // If our new kernelspec has an interpreter, set that as our interpreter too
 if (interpreter) {
- this._executionInfo.interpreter = interpreter;
+ const metadata = { ...this._executionInfo.kernelSpec.metadata } || {};


Not super happy about how this is implicit now. Although I guess it's no different than having it as a separate member.

I agree.
We are already storing interpreter in the metadata in kernel spec.
Will ensure this is not written to disc
We just need a place to store the selection for the lifetime of the notebook.
The problem is trying to find a property where this can be stored. I cannot just add a property to keep track of this as we have a few different classes/layers and not sure where it needs to go. I feel its easier to stick it in here as this is tied directly to the notebook.

Anyways, a test is failing.

I feel like this is a big change. I'm going to look, but I think that we have some code that assumes the kernelspecs with interpreter metadata in them are kernelspecs that we created off of an interpreter. This looks like it's now saving interpreter info in any kernelspec that we use.

Something like this is now going to display the path of the interpreter not the path of the kernelspec, which I don't think we want.

vscode-python/src/client/datascience/jupyter/kernels/kernelSelections.ts

Line 38 in 0a0493a

detail: pathUtils.getDisplayName(kernelSpec.metadata?.interpreter?.path || kernelSpec.path),

This should not have an effect on that.
We're only updating the metadata in the private copy.
Though I can see how this leads to a confusion.
As we're trying to clean up the whole KernelSelection stuff, I think this approach might not be the best.

However, this kernelspec is what is something that is shared across multiple classes and layers. And we need the corresponding interpreter information to be passed on.

Here's the explanation

When we select a kernel, we also try to identify the best matching interpreter

This is required to start a kernel from a Python interpreter

The kernel spec is merely used to provide a name for the kernel and provide additional information such as env variables, startup CLI args.

In light of the above, a kernel connection is NOT limited just to the following three:

1). raw (start from executable)

2). kernelSpec

3). Live Kernel

Instead we have a 4rd option

4). raw + kernelSpec
Based on our previous conversation this is no different from a kernelspec that has the python interpreter defined in the argv as the executable.

I'll see what I can do to fix this up, moving back to draft.
@rchiodo @IanMatthewHuff

sonarqubecloud · 2020-08-13T15:40:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

DonJayamanne · 2020-08-13T15:46:19Z

src/client/datascience/jupyter/kernels/types.ts

- | { kernelModel: undefined; kernelSpec: undefined; interpreter: PythonInterpreter };
+ | LiveKernelConnectionMetadata
+ | KernelSpecConnectionMetadata
+ | PythonKernelConnectionMetadata;


Here's the new type (3 of them for now):

We have Kernel specs (starting kernels from a kernel spec JSON)

Applies to both local and remote connections

We have kernel models (live kernels) - connecting to existing kernels from Kernel Model

Only applicable for remote connections

We have Python Kernels

We are either going to register the python interpreter as a kernel and start it (old jupyter)

Or we start this using raw kernel approach.

I did not use the words raw as that is an implementation detail on how we start the kernel.
I.e. we could have a kernelSpec and we start that using Raw Kernels, e.g. python interpreter or .NET Executable could be defined as the first argument in argv of the kernelspec JSON file.

Hence we shouldn't really use the terms raw in the connection information.

I have left the commented code, as I will need to resolve this in the next PR
Basically we have other ways to connect to kernels

E.g. we have a kernel spec and we don't have the python interpreter and we find one and use that

Or we have a remote jupyter kernel spec and we need a python interpreter for that too (for intellisense, etc)

Tomorrow we should be able to remove this easily

Todo:

Rename KernelSelection to KernelConnectionMetadata (its a big change, I didn't want that change to come in here else you might miss these subtle changes that are more important than a name change)

Refactor other parts to use this

The last stage would be to clean up this type and use a single property if at all possible and remove the 3 properties (kernelModel, kernelSpec and interpreter) - though i fear we might end up with at most 2 in some cases)

Either way, clean up slowly then the changes are clearer

@rchiodo @IanMatthewHuff /cc

Thanks for removing raw. Seemed confusing as I knew there was a jupyter case that just had an interpreter in this type.

DonJayamanne · 2020-08-13T15:47:08Z

src/client/datascience/jupyter/kernels/types.ts

-
-export interface IKernelSelectionListProvider {
- getKernelSelections(resource: Resource, cancelToken?: CancellationToken): Promise<IKernelSpecQuickPickItem[]>;
+export interface IKernelSelectionListProvider<T extends KernelSelection = KernelSelection> {


Using generics here, as that made the code simpler and obviously it is now strongly typed, hence found a few issues (deleted code that was not necessary).

DonJayamanne · 2020-08-13T15:47:39Z

src/test/datascience/jupyter/kernels/kernelSelector.unit.test.ts

 );
 });
- test('Should hide kernel from local sessions', async () => {
- const kernelModels: LiveKernelModel[] = [


When connecting to a local session, we cannot select a live kernel. Live kernels can only be picked when using remote sessions (even if the remote session is running on the local box)

DonJayamanne · 2020-08-13T15:48:33Z

src/client/datascience/jupyter/kernels/kernelSelector.ts

 session,
 cancelToken
 );
- suggestions = suggestions.filter((item) => !this.kernelIdsToHide.has(item.selection.kernelModel?.id || ''));


This was never going to happen, thanks to the strong typing found dead code.

DonJayamanne · 2020-08-13T15:49:14Z

@IanMatthewHuff please re-review

DonJayamanne commented Aug 12, 2020

View reviewed changes

src/client/datascience/jupyter/jupyterNotebook.ts Outdated Show resolved Hide resolved

DonJayamanne commented Aug 12, 2020

View reviewed changes

src/client/datascience/jupyter/liveshare/hostJupyterServer.ts Outdated Show resolved Hide resolved

DonJayamanne commented Aug 12, 2020

View reviewed changes

src/client/datascience/types.ts Show resolved Hide resolved

DonJayamanne requested review from DavidKutu, IanMatthewHuff, greazer, joyceerhl and rchiodo August 12, 2020 23:58

DonJayamanne added the no-changelog No news entry required label Aug 12, 2020

rchiodo approved these changes Aug 13, 2020

View reviewed changes

rchiodo reviewed Aug 13, 2020

View reviewed changes

DonJayamanne changed the title ~~Remove interpreter info from types~~ DRAFT - Remove interpreter info from types Aug 13, 2020

Clean up kernelSelection types

6e4813b

DonJayamanne force-pushed the unifyTypes branch from f691a49 to 6e4813b Compare August 13, 2020 15:38

DonJayamanne changed the title ~~DRAFT - Remove interpreter info from types~~ DRAFT - Refactor KernelSelection type Aug 13, 2020

DonJayamanne commented Aug 13, 2020

View reviewed changes

DonJayamanne requested a review from rchiodo August 13, 2020 15:48

DonJayamanne changed the title ~~DRAFT - Refactor KernelSelection type~~ Refactor KernelSelection type to include a kind Aug 13, 2020

IanMatthewHuff approved these changes Aug 13, 2020

View reviewed changes

rchiodo approved these changes Aug 13, 2020

View reviewed changes

DonJayamanne merged commit 2ba5380 into microsoft:master Aug 13, 2020

DonJayamanne deleted the unifyTypes branch August 13, 2020 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor KernelSelection type to include a `kind` #13407

Refactor KernelSelection type to include a `kind` #13407

Uh oh!

DonJayamanne commented Aug 12, 2020

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 12, 2020 •

edited

Loading

rchiodo Aug 13, 2020

DonJayamanne Aug 13, 2020

IanMatthewHuff Aug 13, 2020

IanMatthewHuff Aug 13, 2020

DonJayamanne Aug 13, 2020

sonarqubecloud bot commented Aug 13, 2020

DonJayamanne Aug 13, 2020

IanMatthewHuff Aug 13, 2020

DonJayamanne Aug 13, 2020

DonJayamanne Aug 13, 2020

DonJayamanne Aug 13, 2020

DonJayamanne commented Aug 13, 2020

Labels

4 participants

Refactor KernelSelection type to include a kind #13407

Refactor KernelSelection type to include a kind #13407

Uh oh!

Conversation

DonJayamanne commented Aug 12, 2020

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne commented Aug 13, 2020

Labels

4 participants

Refactor KernelSelection type to include a `kind` #13407

Refactor KernelSelection type to include a `kind` #13407

codecov-commenter commented Aug 12, 2020 •

edited

Loading