search

jupyter-server / enterprise_gateway

A lightweight, multi-tenant, scalable and secure gateway that enables Jupyter Notebooks to share resources across distributed clusters such as Apache Spark, Kubernetes and others.
https://jupyter-enterprise-gateway.readthedocs.io/en/latest/
Other
619 stars 223 forks source link

Making bootstrap script respect current working dir #1343

Closed allstrive closed 10 months ago

allstrive commented 11 months ago

Problem

The current usage of KERNEL_WORKING_DIR and EG_MIRROR_WORKING_DIRS in EG works fine when EG is in charge of launching kernel PODs. Some of the usecases, uses sparkoperator to launch kernel POD. While the environment variable KERNEL_WORKING_DIR is still passed on , there's no effect of this environment var on actual execution environment.

Solution

The bootstrap script change will ensure that KERNEL_WORKING_DIR is respected and that kernel is launched on right location.

welcome[bot] commented 11 months ago

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. 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! :wave:
Welcome to the Jupyter community! :tada:

kevin-bates commented 11 months ago

Hi @allstrive - thank for opening this pull request. It's really too bad, but it doesn't appear operators have the notion of a workingDir setting. (Have you confirmed that?)

Since these changes are in a shell script, I would like to see the same change applied to all kernel types. Here are some suggestions.

  1. Apply the change to all kernel types.
  2. Only change the directory to KERNEL_WORKING_DIR if the directory exists (otherwise echo a statement that it doesn't exist and operations will remain in cur_dir).
  3. Please use a lowercase variable name for CURR_DIR as we try to reserve all uppercase for env vars.
  4. Since the kernel's lifecycle should be that of the container, I'm not sure its that critical to restore the directory anyway - so I view the whole cur_dir stuff as optional.

Also, it would be fantastic if you could contribute an identical change to the Gateway Provisioners repo in the analogous file - since this repo will eventually replace the process-proxies (and their respective kernels) used in EG.

Thank you for your help and contribution!

allstrive commented 11 months ago

Thank you @kevin-bates , for your suggestions.

It's really too bad, but it doesn't appear operators have the notion of a workingDir setting. (Have you confirmed that?)

Yes, that was my conclusion after initial review of sparkoperator code base. I plan to do that again before making this back for review

Apply the change to all kernel types.

Yes, that's what I intend to do before I put back the PR for review again

Only change the directory to KERNEL_WORKING_DIR if the directory exists (otherwise echo a statement that it doesn't exist and operations will remain in cur_dir).

Great point, will add that check.

Please use a lowercase variable name for CURR_DIR as we try to reserve all uppercase for env vars.

Will follow that.

Since the kernel's lifecycle should be that of the container, I'm not sure its that critical to restore the directory anyway - so I view the whole cur_dir stuff as optional.

I kept it as a good practice for any shell script, my realization is most of the components in Jupyter's ecosystem past EG are used in different ways across various usage. This will minimize the side effects of my change.