-
Notifications
You must be signed in to change notification settings - Fork 301
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
Conversation
if (kernelRegistrationKind === 'registeredByNewVersionOfExtForCustomKernelSpec') { | ||
const originalSpecFile = spec.metadata?.jupyter?.originalSpecFile || spec.metadata?.originalSpecFile; | ||
if (originalSpecFile) { | ||
specName = `${specName}#${path.basename(path.dirname(originalSpecFile))}`; |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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)
* @param {CancellationToken} [token] | ||
* @returns | ||
*/ | ||
export async function parseKernelSpecs(stdout: string, token?: CancellationToken) { |
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
? specFile | ||
: path.join(kernelSpecRootPath, kernel.name, 'kernel.json'); | ||
const kernelSpecFilePath = | ||
path.basename(specFile).toLowerCase() === kernel.name.toLowerCase() |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?: { |
There was a problem hiding this comment.
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[]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
Codecov Report
@@ 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
|
'python3811.json' | ||
].map((name) => | ||
// Prefix with some character, else a folder of `python` is deemed a default kernelspec. | ||
['_' + path.basename(name, '.json'), name].join('/') |
There was a problem hiding this comment.
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.
@@ -461,7 +515,6 @@ export function getInterpreterHashInMetadata( | |||
export function findPreferredKernel( | |||
kernels: KernelConnectionMetadata[], | |||
resource: Resource, | |||
languages: string[], |
There was a problem hiding this comment.
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
6b0f166
to
b6980e2
Compare
b6980e2
to
d8008fa
Compare
For #8481
Add support for: