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

Making bootstrap script respect current working dir #1343

Closed
wants to merge 1 commit into from
Closed

Making bootstrap script respect current working dir #1343

wants to merge 1 commit into from

Conversation

allstrive
Copy link

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

welcome bot commented Oct 19, 2023

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! 🎉

@allstrive allstrive marked this pull request as draft October 20, 2023 04:57
@kevin-bates kevin-bates added bug gateway-provisioners Has application for gateway-provisioners (EG 4.0) labels Oct 22, 2023
@kevin-bates
Copy link
Member

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

allstrive commented Oct 22, 2023

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.

@allstrive allstrive closed this by deleting the head repository Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug gateway-provisioners Has application for gateway-provisioners (EG 4.0)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants