jupyter_server
jupyter_server copied to clipboard
Add resource_dir to KernelSpecHandler model
For the case where a user has the template "{resource_dir}" in their kernel.json argv, the server api for api/kernelspecs returns the raw kernel.json spec contents (with the template string in place) but does not also include the resource_dir path that could be used to substitute the template string.
The use case for this PR is locating a script co-located with the kernel.json that is used to launch the kernel and has some custom setup that the user needs that is hard to pack into kernel.json argv directly and wants to be able to request remote tasks that utilize the same script. Using the same launch script would help to make sure that the task environment can be compatible with (libraries/dependencies, etc) the kernel environment. You can of course use the resource_dir for any resource co-located with the kernel, but in particular, my use case is a launch script. Using resource_dir instead of an absolute path is less error-prone for users, it makes the kernel files easier to copy for sharing or just replication, and there are other potential benefits. It's a nice feature, but unfortunately, when you use it, you do run into the problem of not being able to find that kernel easily on disk from the server api.
The change here (which should be straightforward) is to add a key to the KernelSpecHandler model for resource_dir, and then include that as a required key to validate that a dict represents a kernelspec model. In practice, this would mean that for a "standard" deployment with no subclassing, the KernelSpecManager would return the specs to the KernelSpecHandler, and then each spec dict would be instantiated as a new model that also includes the resource_dir key and value. Any KernelSpecManager subclass that already included resource_dir with the model dict would not have this model conversion take place.
The test_gateway sample kernelspec was modified slightly to adopt the added key, and I created a sample kernel that has a launch script colocated with the kernel.json and referred to with "{resource_dir}/" in the kernel.json.
If there are any issues with this PR, happy to discuss and or iterate as needed.
Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:
Hi @mlhenderson - thanks for opening the PR and the great description.
I believe this would constitute an API change since it's altering the model returned from the server. That said, we're on the cusp of a major release, so if such a change were to be made, the timing of this isn't bad. I do have some difficulty seeing general utility in this since the value of resource_dir may not be local to the user-facing web server at all (e.g., when a Gateway is configured).
Given you'd like this available via the REST API, are you planning on developing a front-end extension to launch the other applications that rely on the co-located scripts? If so, there would likely be code necessary to launch those applications and I wonder if it would be better for that code to use the KernelSpecManager directly (where resource_dir is available) or leverage a server extension that exposes a handler to do what you propose?
Codecov Report
Merging #818 (674cc0c) into main (6d84507) will increase coverage by
0.02%. The diff coverage is100.00%.
:exclamation: Current head 674cc0c differs from pull request most recent head 27a1067. Consider uploading reports for the commit 27a1067 to get more accurate results
@@ Coverage Diff @@
## main #818 +/- ##
==========================================
+ Coverage 70.16% 70.18% +0.02%
==========================================
Files 63 63
Lines 7480 7486 +6
Branches 1253 1254 +1
==========================================
+ Hits 5248 5254 +6
Misses 1851 1851
Partials 381 381
| Impacted Files | Coverage Δ | |
|---|---|---|
| jupyter_server/pytest_plugin.py | 88.39% <100.00%> (+0.31%) |
:arrow_up: |
| jupyter_server/services/kernelspecs/handlers.py | 91.93% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 6d84507...27a1067. Read the comment docs.
@kevin-bates I am coordinating with @rcthomas to be able to explain the use case more and discuss this further on a jupyter-server call. Happy to get feedback on different/better ways to approach this.
hi @mlhenderson - thank you for raising this PR and #932 in today's Server meeting. Since the general feeling was that introducing resource_dir into the kernelspec model would not necessarily be considered "API-breaking" and given that the pending 2.0 release would be the time to introduce such a change anyway, I'm (now) thinking this PR is preferred over #932 (which performs the substitution of {resource_dir} on kernelspec retrieval). This way, any possible customizations that may rely on its late (just prior to kernel launch) substitution would not be side-affected.
So, in essence, we'd merely be including resource_dir in the /api/kernelspecs response and let consumers use it as they need - similar to the base kernelspec behavior in the first place.
Thanks for opening this PR, @mlhenderson—and apologies for the slow response here.
My share the concerns that @vidartf raised at last week's Server meeting. The resource_dir returned in the model's response is an absolute path from the server's filesystem—which could be deemed sensitive information.
We shouldn't require an item in our core REST API that includes sensitive data; I suspect this is the historical reason behind why this item is stripped out of the kernel spec response model in the first place.
To get around this limitation of the core APIs, you could build a server extension that returns the resource dir.
We're triaging PRs in the Server Meeting today. Because there hasn't been activity on this PR in a little while, I'm going to close this PR for now. If this work is still in-progress, feel free to re-open anytime. Thank you!