Added add_to_path option for jupyter_environments
#48
Conversation
| @@ -16,5 +16,5 @@ | |||
| set -euo pipefail | |||
|
|
|||
| trap 'echo SIGTERM received' TERM | |||
|
|
|||
| {{prologue}} | |||
There was a problem hiding this comment.
I used prologue here rather than a more specific implementation to keep close to batchspawner which uses the same.
jupyterhub_moss/spawner.py
Outdated
| @@ -55,6 +55,7 @@ class MOSlurmSpawner(SlurmSpawner): | |||
| per_key_traits={ | |||
| "path": traitlets.Unicode(), | |||
| "description": traitlets.Unicode(), | |||
| "add_to_path": traitlets.Bool(default_value=True), | |||
There was a problem hiding this comment.
Keeping it optional to avoid a breaking change in the config.
jupyterhub_moss/spawner.py
Outdated
| @@ -220,6 +221,13 @@ def options_from_form(self, formdata: Dict[str, List[str]]) -> Dict[str, str]: | |||
| )[0] | |||
| options["environment_path"] = default_venv["path"] | |||
|
|
|||
| for env in self.partitions[partition]["jupyter_environments"].values(): | |||
There was a problem hiding this comment.
I don't understand why you need to cycle though the jupyter_environments there. Also, I find the for, else syntax a bit obscure.
There was a problem hiding this comment.
It's checking whether the path corresponds to a default jupyter_environments or to a custom one.
As it is done now, only the environment_path is passed through the form, so it checks whether this path corresponds to one of the default envs, thus the loop.
If it's a custom one, then it is added to the PATH, if it's one of the defaults it checks add_to_path.
The for else syntax is bit uncommon but it is meant for this case. I'll propose an alternate implementation.
jupyterhub_moss/spawner.py
Outdated
| matching_envs_without_add_to_path = [ | ||
| env | ||
| for env in partition_environments | ||
| if env["path"] == options["environment_path"] | ||
| and not env.get("add_to_path", True) | ||
| ] | ||
| if not matching_envs_without_add_to_path: |
There was a problem hiding this comment.
I replaced the for .. else pattern with a list filter and a test of list content, hope that's simpler to read.
There was a problem hiding this comment.
BTW the alternative to looping through the list is to use the path as the key for the jupyter envs and put the key as an id item of the dict.
jupyterhub_moss/spawner.py
Outdated
| options["environment_path"] = default_venv["path"] | ||
| options["environment_path"] = partition_environments[0]["path"] | ||
|
|
||
| # Add environment_path to PATH unless it is in the settings with add_to_path=False |
There was a problem hiding this comment.
I feel the comment should be more meaningful. We should explain the fact that custom envs are always added to the path and because of this, we must first check if the env is a default env.
And then, if the env is a default env, we check if it should be added to the path or not.
For example. I would rename corresponding_env in corresponding_default_env or matching_default_env.
And then above the condition if matching_default_env is None, I would add the comment Custom env are always added to the PATH
jupyterhub_moss/spawner.py
Outdated
| lambda env: env["path"] == options["environment_path"], | ||
| partition_environments, | ||
| ) | ||
| if corresponding_env is None or corresponding_env.get("add_to_path", True): |
There was a problem hiding this comment.
Given you set a default value for the trait add_to_path, is the get(..., True) needed ?
There was a problem hiding this comment.
I add an issue with that, I didn't investigate where it comes from.
There was a problem hiding this comment.
I think default_value does not work in this case because it specifies a value inside a dict rather than a trait.
| @traitlets.validate("partitions") | ||
| def _validate_partitions(self, proposal): | ||
| # Set add_to_path if missing in jupyter_environments | ||
| partitions = deepcopy(proposal["value"]) | ||
| for partition in partitions.values(): | ||
| for env in partition["jupyter_environments"].values(): | ||
| env.setdefault("add_to_path", True) | ||
| return partitions |
There was a problem hiding this comment.
This is the documented way to handle validation of traitlets.
| lambda env: env["path"] == options["environment_path"], | ||
| partition_environments, | ||
| ) | ||
| # custom envs are always added to PATH, defaults ones only if add_to_path is True |
There was a problem hiding this comment.
Hope this is clearer, else feel free to provide suggestion
| lambda env: env["path"] == options["environment_path"], | ||
| partition_environments, | ||
| ) | ||
| # custom envs are always added to PATH, defaults ones only if add_to_path is True |
This option allows to toggle whether or not the jupyter environment path is prepended to
PATH.For instance, when giving access to Python installed on the system, the virtual environment used for jupyter server should not be added to the
PATH.Custom environments are always added to
PATH.