Register kernelspecs in private directory #9249
Conversation
Codecov Report
@@ Coverage Diff @@
## main #9249 +/- ##
=====================================
- Coverage 71% 71% -1%
=====================================
Files 383 383
Lines 24803 24833 +30
Branches 3986 3989 +3
=====================================
+ Hits 17753 17754 +1
- Misses 5456 5476 +20
- Partials 1594 1603 +9
|
| jupyterDataPaths.push(path.dirname(await this.jupyterPaths.getKernelSpecTempRegistrationFolder())); | ||
| spawnOptions.env = { | ||
| ...envVars, | ||
| JUPYTER_PATH: jupyterDataPaths.join(path.delimiter) |
There was a problem hiding this comment.
Magic sauce, this ensures Jupyter finds our kernelspecs (registered in a temporary directory)
| @@ -234,7 +233,7 @@ export class JupyterKernelService { | |||
| cancelToken?: CancellationToken, | |||
| forceWrite?: boolean | |||
| ) { | |||
| const kernelSpecRootPath = await this.kernelFinder.getKernelSpecRootPath(); | |||
| const kernelSpecRootPath = await this.jupyterPaths.getKernelSpecTempRegistrationFolder(); | |||
There was a problem hiding this comment.
Change directory where we register kernelspecs
| * (this way we don't register kernels in global path). | ||
| */ | ||
| public async getKernelSpecTempRegistrationFolder() { | ||
| const dir = path.join(this.context.extensionUri.fsPath, 'temp', 'jupyter', 'kernels'); |
There was a problem hiding this comment.
All kernelspecs are always in extension folder.
This way when ever we update the extension, we always end up creating these.
I kind of like this, if users delete environments or the like, the kernels will not live around unnecessarily.
Note: If you upgrade to a new version of the extension, the exact same kernel (with the same name is created)
I.e. the algorithm for geneating kernel names hasn't changed.
| import { loadKernelSpec } from '../../../client/datascience/kernel-launcher/localKernelSpecFinderBase'; | ||
| import { traceInfoIfCI } from '../../../client/common/logger'; | ||
|
|
||
| [false, true].forEach((isWindows) => { |
There was a problem hiding this comment.
Removed all of the old tests
@rchiodo You might recall this is something you asked for (& obviously I agreed with that suggestion).
Basically migrate old tests to the new style.
There was a problem hiding this comment.
I'm confused. Why wouldn't most of these tests still be valid?
There was a problem hiding this comment.
They are, they are duplicates and written a different way.
You suggested that I just migrate them to the new way.
Basically I wrote tests in a different way, which I believe was easier, in the test:
- You can define all global kernle specs, all interpreters & kernlespecs that exist in each interpreter
- In the previous test this wasn't straightforward, it was slightly complicated.
- In that PR you suggested I migrate all the old tests to the new style (if the new tests covered the same scenarios)
| assert.strictEqual(kernel?.kind, 'startUsingPythonInterpreter'); | ||
| assert.deepEqual(kernel?.interpreter, activePythonEnv); | ||
| }); | ||
| test('Return active interpreter for interactive window (metadata only has language)', async function () { |
There was a problem hiding this comment.
Added some of the missing tests (as part of deleting old tests)
| return kernels; | ||
| } | ||
|
|
||
| // This should return a WRITABLE place that jupyter will look for a kernel as documented | ||
| // here: https://jupyter-client.readthedocs.io/en/stable/kernels.html#kernel-specs | ||
| public async getKernelSpecRootPath(): Promise<string | undefined> { |
There was a problem hiding this comment.
I'm kinda surprised we don't need this anymore. We should still be looking for global kernels too?
There was a problem hiding this comment.
I think we're still looking for globals (because juptyerPaths.getKernelSpecRootPath() is still used), so you likely just didn't need this to be public anymore.
There was a problem hiding this comment.
Yes, we have a list of more paths and the root path was one of them.
| if (kernelSpec?.specFile && shouldDeleteKernelSpec) { | ||
| // If this kernelSpec was registered by us and is in the global kernels folder, | ||
| // then remove it. | ||
| this.deleteOldKernelSpec(kernelSpec.specFile).catch(noop); |
There was a problem hiding this comment.
This seems like unexpected behavior - the result of asking for a kernelSpec that exists is that it possibly gets deleted
There was a problem hiding this comment.
Yes, its a side effect, we need to delete the old kernelspecs & this is a one time task.
I don't want to create a whole new method to iterate through kernels all over again (which is disc io & unnecessary complication).
Also I need a place to filter out these old kernlespecs (hence this is the spot).
In the future this code can be removed.
Fixes #9141