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

Load kernels from cache and kernel for active interpreter #6754

Merged
merged 7 commits into from
Jul 22, 2021
Merged

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Jul 21, 2021

Part of #5861

  • Load controller ASAP for Active Interpreter
  • Load controllers from cache (whilst fetching real controllers & updating the list after we get them)

First time users

  • We load active interpreter as kernel fast
  • We load all other kernels (as we do today, which is a little slow)

Users coming back

  • We load kernels from cache
  • We load kernels without cache in the background, and as its made available, we add those.
    • I.e. if a user as a new environment, and that gets picked up by the kernel discovery it will be added subsequently to the list of kernels (but that's a little slower).

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2021

Codecov Report

Merging #6754 (b63fd12) into main (c3fb26d) will decrease coverage by 1%.
The diff coverage is 82%.

@@          Coverage Diff           @@
##            main   #6754    +/-   ##
======================================
- Coverage     69%     68%    -2%     
======================================
  Files        412     411     -1     
  Lines      28621   28641    +20     
  Branches    4272    4283    +11     
======================================
- Hits       19849   19569   -280     
- Misses      7079    7401   +322     
+ Partials    1693    1671    -22     
Impacted Files Coverage Δ
src/client/datascience/kernel-launcher/types.ts 56% <ø> (ø)
.../datascience/notebook/notebookControllerManager.ts 86% <78%> (+<1%) ⬆️
...t/datascience/kernel-launcher/localKernelFinder.ts 83% <84%> (-2%) ⬇️
...eractive-window/nativeInteractiveWindowProvider.ts 35% <0%> (-32%) ⬇️
...ence/interactive-window/nativeInteractiveWindow.ts 5% <0%> (-29%) ⬇️
...nt/datascience/editor-integration/hoverProvider.ts 48% <0%> (-28%) ⬇️
...cience/interactive-ipynb/nativeEditorOldWebView.ts 59% <0%> (-27%) ⬇️
...t/datascience/interactive-ipynb/autoSaveService.ts 66% <0%> (-25%) ⬇️
src/client/datascience/jupyter/kernels/kernel.ts 62% <0%> (-13%) ⬇️
...ience/interactive-ipynb/nativeEditorProviderOld.ts 65% <0%> (-12%) ⬇️
... and 37 more

@DonJayamanne DonJayamanne changed the title WIP Load controllers from cache and load controller for active interpreter asap Jul 22, 2021
listKernels(resource: Resource, cancelToken?: CancellationToken): Promise<LocalKernelConnectionMetadata[]>;
listNonPythonKernels(
cancelToken?: CancellationToken,
useCache?: 'useCache' | 'ignoreCache'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't want to use a boolean, i feel this is more descriptive

@DonJayamanne DonJayamanne marked this pull request as ready for review July 22, 2021 21:35
@DonJayamanne DonJayamanne requested a review from a team as a code owner July 22, 2021 21:35
// Possible the kernelspec file no longer exists, in such cases, exclude this cached kernel from the list.
promises.push(
this.fs
.localFileExists(item.kernelSpec.specFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is true? Do we ever create 'startUsingKernelSpec' without a specfile?

Copy link
Contributor

Choose a reason for hiding this comment

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

As in I don't see this condition from before.

Copy link
Contributor

Choose a reason for hiding this comment

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

The code in the jupyterKernelService seems to rely on the fact that the specFile doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its possible the user deleted the kernelspec file.

As in I don't see this condition from before.

Thats correct, thats because we always serach and return only kernelspecs that exist on disc.
However now that I'm caching it, its possibel the user subsequently uninstalls/deletes that kernelspec from disc. In which case, the item in the cache is no longer valid (as it doesn't exist).

Copy link
Contributor

Choose a reason for hiding this comment

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

So yes we only search for ones on disk, but I thought we generate ones too, where the file may not have ever existed. So this would destroy those (as well as removing the ones you intend).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here the file may not have ever existed. So this would destroy those (as well as removing the ones you intend).

In that case, aren't we starting from PythonInterpreter & not from kernel spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I.e. if we're starting from a kernel spec, then the spec file must exist & this code ensures we only deal with those kernels where kind= startUsingKernelSpec

Copy link
Contributor

Choose a reason for hiding this comment

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

aren't we starting from PythonInterpreter & not from kernel spec.

I can't say what the type would end up as. I mean you can test it and find out. Or maybe just look at all the spots where it's 'startUsingKernelSpec' and verify they always need a kernelspec. I thought we always create a 'default' one that may or may not have an actual kernelspec on disk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where it's 'startUsingKernelSpec' and verify they always need a kernelspec.

Yup, i checked, and can confirm that it must exist.

thought we always create a 'default' one that may or may not have an actual kernelspec on disk.

Yes, in that case the kind=DefaultKernelConnectionMetadata
Hence the code i have should work correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought we always create a 'default' one that may or may not have an actual kernelspec on disk.

Yes, thats for when we start Python interpreters (we have a bogus kernelspec, which will get created only when we register the kernel), Again, i'm not interested in those (not interested when we start using Python interpreter).

Copy link
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

⏲️

@DonJayamanne DonJayamanne requested a review from rchiodo July 22, 2021 21:50
@DonJayamanne DonJayamanne changed the title Load controllers from cache and load controller for active interpreter asap Load kernels from cache and kernel for active interpreter Jul 22, 2021
@DonJayamanne DonJayamanne merged commit bf5cef7 into main Jul 22, 2021
@DonJayamanne DonJayamanne deleted the perfK branch July 22, 2021 22:55
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.

3 participants