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 resource_dir to KernelSpecHandler model #818

Closed

Conversation

mlhenderson
Copy link

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.

@welcome
Copy link

welcome bot commented Apr 29, 2022

Thanks for submitting your first pull request! You are awesome! 🤗

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.
welcome
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! 👋

Welcome to the Jupyter community! 🎉

@kevin-bates
Copy link
Member

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-commenter
Copy link

Codecov Report

Merging #818 (674cc0c) into main (6d84507) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ 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%) ⬆️
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 data
Powered by Codecov. Last update 6d84507...27a1067. Read the comment docs.

@mlhenderson
Copy link
Author

@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.

@kevin-bates
Copy link
Member

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.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

The addition of resource_dir should also be included in the KernelSpec definition of the Open API doc - presumably as a sibling to resources.

@Zsailer
Copy link
Member

Zsailer commented Sep 22, 2022

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.

@Zsailer Zsailer added the waiting for author Blocked, waiting for the author label Feb 16, 2023
@Zsailer
Copy link
Member

Zsailer commented Feb 16, 2023

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!

@Zsailer Zsailer closed this Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change enhancement waiting for author Blocked, waiting for the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants