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

Improve kernel finder caching #11156

Merged
merged 36 commits into from
Sep 6, 2022
Merged

Improve kernel finder caching #11156

merged 36 commits into from
Sep 6, 2022

Conversation

rebornix
Copy link
Member

@rebornix rebornix commented Aug 18, 2022

Re #10832. This is still on-going and require offline discussion with Don.

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for feature-requests.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).

@rebornix
Copy link
Member Author

Updated the branch to reflect the discussions @DonJayamanne and I had offline about moving the kernel detection/finder to push model. The new workflow of kernel detection would be

  • On extension activation, we initialize KernelFinder
    • RemoteKernelFinder start loading cache from global memento.
      • Once cache loaded, store in in memory
      • Kick off a kernelspec search without cache
    • LocalKernelFinder start loading cache from global memento.
      • Once cache loaded, store in in memory
      • Kick off a kernelspec search without cache
  • Controller Loader activation
    • Request kernels from KernelFinder
    • KernelFinder waits for all contributed kernel finder to finish initialization (loading cache)
    • KernelFinder asks all contributed kernel finder for kernels, this step is synchronous.
  • When interpreters, python extension, conda environement changes
    • Contributed kernel finders perform another round of kernel detection
    • Once get the latest kernels, update cache
    • Broadcast kernels change event
  • Controller Loader receives the event
    • Request kernels from KernelFinder
    • KernelFinder asks all contributed kernel finder for kernels, this step is synchronous.

With this new workflow, the only asynchronous part is the initial cache loading. Once we have the cache in memory, it's each contributed kernel finder's responsibility to update the cache per environment change. The main KernelFinder and ControllerLoader are just reading their cache synchronously.

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2022

Codecov Report

Merging #11156 (9ccb275) into main (b164a7e) will increase coverage by 0%.
The diff coverage is 86%.

@@           Coverage Diff           @@
##            main   #11156    +/-   ##
=======================================
  Coverage     63%      63%            
=======================================
  Files        476      476            
  Lines      33880    33915    +35     
  Branches    5488     5495     +7     
=======================================
+ Hits       21360    21558   +198     
+ Misses     10474    10295   -179     
- Partials    2046     2062    +16     
Impacted Files Coverage Δ
src/kernels/jupyter/types.ts 100% <ø> (ø)
src/kernels/types.ts 100% <ø> (ø)
...otebooks/controllers/controllerPreferredService.ts 85% <ø> (ø)
src/notebooks/controllers/types.ts 100% <ø> (ø)
src/notebooks/serviceRegistry.node.ts 100% <ø> (ø)
src/kernels/raw/finder/localKernelFinder.node.ts 74% <79%> (-4%) ⬇️
src/kernels/jupyter/finder/remoteKernelFinder.ts 88% <86%> (+2%) ⬆️
src/notebooks/controllers/controllerLoader.ts 87% <92%> (-1%) ⬇️
src/kernels/kernelFinder.ts 100% <100%> (ø)
src/kernels/raw/finder/jupyterPaths.node.ts 79% <100%> (+<1%) ⬆️
... and 29 more

@rebornix rebornix marked this pull request as ready for review August 19, 2022 19:15
@rebornix rebornix requested a review from a team as a code owner August 19, 2022 19:15
@rebornix
Copy link
Member Author

rebornix commented Aug 19, 2022

@DonJayamanne with the changes in this PR, we now have push model / event based IContributedKernelFinders which can

  • Load cache on activation
  • Respond to relevant events and update cache
  • On cache update, broadcast cache update event

KernelFinder still behave the same as before, whose listKernels method is still asynchronous. Whether we want to change the semantics of this method or what's the best shape of it is worth of a discussion with @IanMatthewHuff (i.e., whether we want to differeciate Remote/Local, KernelSpec/Session).

This PR reduces redundant kernel research for irrelevant events, but it didn't try to make the cache update in each individual contributed kernel finder search operations minimal. For example, the Local Kernel Finder will trigger research when conda/python interpreter changes, but they might not be necessary for local kernel specs. Besides, we didn't do a diff between newly found kernels and old ones to reduce the events we broadcast. These can be improved in the coming debt week or along with the adoption of the new Python API.

I'll leave the the decision to you @DonJayamanne when we should merge this PR, and feel free to push changes directly to the branch.

Copy link
Contributor

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Looks good, just got on issue with a possible race condition causing the kernel list to be empty

@rebornix rebornix merged commit f6a0b6e into main Sep 6, 2022
@rebornix rebornix deleted the debt/kernel-finder-cache branch September 6, 2022 19:51
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.

4 participants