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 Kernelspec caching #908

Merged
merged 10 commits into from
Dec 22, 2020

Conversation

kevin-bates
Copy link
Member

@kevin-bates kevin-bates commented Dec 7, 2020

This pull request adds support for Kernel Specification caching. Since various applications tend to poll for kernelspecs, and EG needs to support multiple such applications, it makes sense to route kernelspec requests through a caching facility, which defers to the configured KernelSpecManager if the kernelspec is not found.

This implementation uses watchdog to monitor the directory hierarchies containing kernelspecs. If new kernelspecs are added that exist in non-monitored directories a cache miss will ensue - triggering the monitoring of the newly discovered directory.

In current trials, this appears to have about a 70% performance increase in kernelspec retrievals.

For now, the cache is disabled by default. In EG 3.0 (or sooner?) we will enable caching to be on by default. Until then, it can be enabled via CLI option --KernelSpecCache.cache_enabled=True or env EG_KERNELSPEC_CACHE_ENABLED=True.

  • Update configuration section of docs
  • Copy resource handler to EG and use cache to handle those requests (though static file handling will still work as today)

@kevin-bates kevin-bates marked this pull request as draft December 7, 2020 20:12
@kevin-bates kevin-bates marked this pull request as ready for review December 8, 2020 16:41
@lresende
Copy link
Member

lresende commented Dec 8, 2020

Overall it looks good, I had a couple of question around some corner cases.

Having said that, I believe this solves the overhead load on the server-side, but the contents of the kernelspec still go over the network, and maybe we could add support for ETAGS on a subsequent PR.

@kevin-bates
Copy link
Member Author

Correct, this only addresses the redundant gathering of kernelspecs on the server. I wasn't aware of ETags, but that makes total sense - good idea. I'm assuming the browser would handle the 304 response, etc. and that client applications wouldn't need to change.

@kevin-bates
Copy link
Member Author

kevin-bates commented Dec 8, 2020

So my last commit removed f strings thinking that was preventing python 3.5 support, but it looks like PosixPath handling is an issue as well. Sigh. I think we should look at dropping 3.5 and perhaps moving to jupyter_server as our base implementation (and away from notebook). This probably implies setting up a 2.x branch and this functionality would be part of a 3.0 release.

What do you think?

@kevin-bates
Copy link
Member Author

Here's a gist to add code to create missing kernel_dir entries (rather than issue a warning) if we want that functionality: https://gist.github.com/kevin-bates/45ddbc94f49f5415bf2985394dbf6925

@kevin-bates kevin-bates merged commit 452e69c into jupyter-server:master Dec 22, 2020
@kevin-bates kevin-bates deleted the kernelspec-cache branch December 22, 2020 17:19
kevin-bates added a commit that referenced this pull request Dec 23, 2020
Looks like the merge for the kernelspec cache (#908) - which followed this file's removal via #913 - instead persisted an empty file.  This commit deletes it.
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.

2 participants