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

Add support for custom kernelspecs #8950

Merged
merged 22 commits into from
Feb 11, 2022
Merged

Add support for custom kernelspecs #8950

merged 22 commits into from
Feb 11, 2022

Conversation

DonJayamanne
Copy link
Contributor

For #8481

Add support for:

  • If users create a custom kernelspec, display that in the list
  • If users modify the default kernelspecs (to add new env variables), then display that in the list

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
if (kernelRegistrationKind === 'registeredByNewVersionOfExtForCustomKernelSpec') {
const originalSpecFile = spec.metadata?.jupyter?.originalSpecFile || spec.metadata?.originalSpecFile;
if (originalSpecFile) {
specName = `${specName}#${path.basename(path.dirname(originalSpecFile))}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds support for custom kernelspecs that users create (we'll give them a unique id, Id will contain the original name of the kernel the user created).

*/
export function isKernelRegisteredByUs(kernelSpec: IJupyterKernelSpec): 'oldVersion' | 'newVersion' | undefined {
export function getKernelRegistrationInfo(
Copy link
Contributor Author

@DonJayamanne DonJayamanne Feb 7, 2022

Choose a reason for hiding this comment

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

Personally I think we're going to have to refactor this to make it a bit easier.
I believe the problem is with the kernelspecs we create.

Assume we load a kernelspec from disc, we have two problems:

  • We change the name of this (after all we need it to be unique so that it will work with non-raw jupyter)
  • We change the display name of this as well

Thus, the information in the kernelspec on disc is not the same as the kernelspec we have in our code.
This makes it difficult to determine whether the user created this or not. fortunately we have code to keep track of the original kernelspec file name (from that we can determine the original name).

Either way, this is confusing.

Assume a user creates a kernelspec names ultraviolet, and we load this, now we give this a name of pvsc310.......
From this name we have no idea whether this is a real kernelspec or one that we created for non-raw Jupyter or the like.
This function tries to differentiate between these different flavours.

I think adding better metadata into the kernelspec is the way to go (if we modify it) & storing these kernlespecs in NON-global locations (then we know that these need never be discovered & can be excluded).

For now this PR is a work around, I would like to refactor this to simplify this part (how we load and manage kernelspsecs)

Unverified

This user has not yet uploaded their public signing key.
* @param {CancellationToken} [token]
* @returns
*/
export async function parseKernelSpecs(stdout: string, token?: CancellationToken) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused

@@ -148,66 +147,7 @@ export abstract class LocalKernelSpecFinderBase {
interpreter?: PythonEnvironment,
cancelToken?: CancellationToken
): Promise<IJupyterKernelSpec | undefined> {
// This is a backup folder for old kernels created by us.
if (specPath.includes(oldKernelsSpecFolderName)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved out so as to use this in tests.

kernelJson.display_name =
kernelJson.language === PYTHON_LANGUAGE && isDefaultPythonName
? interpreter?.displayName || kernelJson.display_name
: kernelJson.display_name;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I removed this code, we're no longer updating the display_name of the kernelspec.
We don't need this (we load kernelspecs from disc and give them our own display names in the controllers).

Also, this preserves the display names the users may have added for kernelspecs (instead of us blowing it away)

kernelJson.metadata = kernelJson.metadata || {};
kernelJson.metadata.jupyter = kernelJson.metadata.jupyter || {};
if (!kernelJson.metadata.jupyter.originalSpecFile) {
kernelJson.metadata.jupyter.originalSpecFile = specPath;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move originalSpecFile in metadata.vscode

Unverified

This user has not yet uploaded their public signing key.
? specFile
: path.join(kernelSpecRootPath, kernel.name, 'kernel.json');
const kernelSpecFilePath =
path.basename(specFile).toLowerCase() === kernel.name.toLowerCase()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more accurate
Kernelspec name is the folder that contains the kernelspec file.

getKernelRegistrationInfo(kernel) === 'registeredByNewVersionOfExtForCustomKernelSpec'
? specModel.env || {}
: {};
specModel.env = Object.assign(envInKernelSpecJson, specModel.env);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When re-generating env variables, we need to preserve the env variables the user may have created in their custom kernelspec files.

results.push(kernelspec);
}
} catch (ex) {
traceError(`Failed to load kernelspec for ${resultPath.kernelSpecFile}`, ex);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a try catch, just because one fails all should not crash and burn.

@@ -314,7 +314,27 @@ export interface IJupyterKernelSpec {
* Optionally storing the interpreter information in the metadata (helping extension search for kernels that match an interpereter).
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
readonly metadata?: Record<string, any> & { interpreter?: Partial<PythonEnvironment>; originalSpecFile?: string };
readonly metadata?: Record<string, any> & {
vscode?: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Store metadata under metadata.vscode (based on jupyter specs, custom metadata must be scoped per application).

/**
* All of the globally installed KernelSpecs
*/
globalKernelSpecs?: KernelSpec.ISpecModel[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrote some of the tests using a new way, wanted to be very explicit about the kernelspecs and the environments.
With the previous test (above this file), its very difficult to determine what kernlespec belongs where..

This type TestData is explicit in defining the global kernelspecs and where each kernel is located.
Need something as explicit as this for the tests, else its difficult to reason out the results and the failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this way, we can mix and match kernelspecs now,
E.g. i can create 2 default kernelspecs for 3 interpreters using the same objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just remove the old tests? if these are easier to write, why keep the other tests around?

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2022

Codecov Report

Merging #8950 (1906f97) into main (dd2022c) will decrease coverage by 0%.
The diff coverage is 55%.

@@          Coverage Diff           @@
##            main   #8950    +/-   ##
======================================
- Coverage     70%     70%    -1%     
======================================
  Files        381     382     +1     
  Lines      24268   24660   +392     
  Branches    3830    3997   +167     
======================================
+ Hits       17214   17353   +139     
- Misses      5525    5716   +191     
- Partials    1529    1591    +62     
Impacted Files Coverage Δ
.../datascience/kernel-launcher/remoteKernelFinder.ts 91% <ø> (ø)
...t/datascience/notebook/vscodeNotebookController.ts 77% <0%> (-2%) ⬇️
src/client/datascience/types.ts 100% <ø> (ø)
...rc/client/datascience/notebookStorage/baseModel.ts 64% <29%> (-5%) ⬇️
src/client/datascience/jupyter/kernels/helpers.ts 50% <53%> (-24%) ⬇️
...ience/kernel-launcher/localKernelSpecFinderBase.ts 76% <62%> (+<1%) ⬆️
.../kernel-launcher/localKnownPathKernelSpecFinder.ts 89% <66%> (+2%) ⬆️
...atascience/jupyter/kernels/jupyterKernelService.ts 79% <87%> (+1%) ⬆️
.../localPythonAndRelatedNonPythonKernelSpecFinder.ts 83% <90%> (+1%) ⬆️
...t/datascience/jupyter/kernels/jupyterKernelSpec.ts 92% <100%> (+35%) ⬆️
... and 38 more

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
'python3811.json'
].map((name) =>
// Prefix with some character, else a folder of `python` is deemed a default kernelspec.
['_' + path.basename(name, '.json'), name].join('/')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the kind of things i wish to avoid with the new tests.

@DonJayamanne DonJayamanne requested a review from amunger February 8, 2022 03:40
@DonJayamanne DonJayamanne marked this pull request as ready for review February 8, 2022 03:40
@DonJayamanne DonJayamanne requested a review from a team as a code owner February 8, 2022 03:40

Unverified

This user has not yet uploaded their public signing key.
@@ -461,7 +515,6 @@ export function getInterpreterHashInMetadata(
export function findPreferredKernel(
kernels: KernelConnectionMetadata[],
resource: Resource,
languages: string[],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No value is ever passed for this, hence removing it

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
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.

None yet

3 participants