-
Notifications
You must be signed in to change notification settings - Fork 308
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 of using custom env variables #1448
base: main
Are you sure you want to change the base?
Add support of using custom env variables #1448
Conversation
jupyter_server/serverapp.py
Outdated
def page_config_hook(self, handler, page_config): | ||
page_config["allow_custom_env_variables"] = self.allow_custom_env_variables | ||
return page_config | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
page_config is a concept specific to JupyterLab. Up until this point, Jupyter Server has been agnostic to it. This hook might need to land in jupyterlab_server instead, which extends jupyter_server and brings the page_config concept to the server.
jupyter_server/serverapp.py
Outdated
allow_custom_env_variables = Bool( | ||
False, | ||
config=True, | ||
help="""Allow to use custom env variables""", | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this trait name and description is too vague.
Since this is specifically about passing custom environment variables to the kernel, I think we need to clarify in this trait and help message that we mean kernel env vars here.
Thanks for working on this @AnastasiaSliusar! Great stuff! I glanced at the diff and left some minor comments in the PR. I know this is still in draft state, so I didn't do a deeper review. Feel free to ping me here anytime you're ready for a review. |
Hello, @Zsailer Thank you for your time and for your comments. I have a question. At the moment, custom env variables are not saved for a certain kernel. A user can just setup them for a kernel which has not be launched yet and then run a kernel. But if a user wants to run a notebook from a filebrowser a kernel won't run with its custom env variables if they have been setup before.
Thank you for your attention |
That's right. Notebook documents can leave the server they originated from, e.g. someone downloads their notebook and emails it to a friend. The environment variables (at best) include irrelevant information or (at worst) might contain sensitive information specific to the original server. Aside from the security concerns, though, I just don't think these custom environment variables are a notebook-driven concept anyways, and thus, don't belong in the notebook document. They are essentially parameterized "instances" of the kernel. If we really want to cache previous launch configurations, we should do it through a separate service/extension IMO and store in a memory cache or (if persistence is needed across server lifetimes) store in a DB. If you went that route, we'd need to extend JupyterLab's open document command, triggered by the filebrowser, to check this cache for previous launches and prompt the user to confirm if they'd like to use the same environment as previous launch. |
Thank you for detailed explanation |
for more information, see https://pre-commit.ci
…iusar/jupyter_server into custom-env-variables
for more information, see https://pre-commit.ci
…iusar/jupyter_server into custom-env-variables
for more information, see https://pre-commit.ci
…iusar/jupyter_server into custom-env-variables
Dear @Zsailer I separated logic of using page_config between jupyter_server and jupyterlab_server (jupyterlab/jupyterlab_server#459) and you can review this PR now. Thank you for your attention:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AnastasiaSliusar.
I've left a few comments throughout the code. :) In general, I think we should use the new trait
as a switch to accept/ignore these variables in the server REST API. Right now, this trait is only used to inform JupyterLab server clients about this feature via the page_config in your upstream jupyterlab_server PR. But that doesn't prevent clients from ignoring this config and still POSTing custom variables that affect the kernel. Ideally, if the trait is False
, the server will ignore these variables even if a client tries to submit them.
One additional question. This PR adds the notion of custom kernel environment variables to the session REST API, but shouldn't we also extend this to the kernel's REST API too? Why special case it here? I think we should add this functionality to both APIs, since they are doing the same thing under the hood with kernels.
@@ -77,6 +77,7 @@ async def post(self): | |||
kernel = model.get("kernel", {}) | |||
kernel_name = kernel.get("name", None) | |||
kernel_id = kernel.get("id", None) | |||
custom_env_vars = kernel.get("env", {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be conditional based on the value of allow_setup_custom_env_variables
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @Zsailer I think it is a good idea. Sure, I agree. Thank you for suggestion.
@@ -152,6 +154,8 @@ async def patch(self, session_id): | |||
changes["name"] = model["name"] | |||
if "type" in model: | |||
changes["type"] = model["type"] | |||
if "env" in model: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here, shouldn't this be conditional based on the value of allow_setup_custom_env_variables
?
jupyter_server/serverapp.py
Outdated
@@ -1427,6 +1427,12 @@ def _default_allow_remote(self) -> bool: | |||
""", | |||
) | |||
|
|||
allow_setup_custom_env_variables = Bool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments on this new trait:
- I think we should shorten the name if we can 😅
- Even though I said we should shorten it, I think we need the word
kernel
in the name somewhere 😆 . How aboutaccept_kernel_env_vars
? It's slightly shorter and more clear (IMO). - I think this trait needs to be passed as a tornado setting so that the request handlers can use this trait too.
for more information, see https://pre-commit.ci
References
This PR is the part of solution of the feature where users can set up custom env variables for a kernel which has not been run yet.
UI is on AnastasiaSliusar/jupyterlab#2
Code changes
accept_kernel_env_var
flag which can be used during launching the application. If it is true then a user can see UI for configuring custom env variables when they select another not used kernel from the kernel select dialog or when they call a context menu for a certain kernel icon on Launchercustom_env_vars
variable which has user selection and it is used to setup custom env variables for a certain kernel