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

Make it easier to pass custom env variables to kernel #981

Merged

Conversation

divyansshhh
Copy link
Contributor

This is done to get the dir of the notebook where notebook initialising the kernel belongs.

#980

@kevin-bates
Copy link
Member

Hi @divyansshhh - This seems very similar to the JPY_SESSION_NAME value introduced in #679 and, AFAICT, both kernel_path and notebook_path would be identical in the default case.

Since you're using a custom ContentsManager, might it make sense to override its get_kernel_path() method to produce the result based on your virtual filesystem as I assume kernel_path and notebook_path are different in your scenario?

When JPY_SESSION_NAME was added, its original intent was to convey the notebook's path but was changed to session name because the notebook path could change within a session, but, in essence, it's the notebook path.

On a somewhat unrelated note, does anyone know what the UUID (placeholder?) is for, and does it correlate to something on the Lab side of things?

@divyansshhh
Copy link
Contributor Author

Thanks for the recommendations, I'll try making a change to the get_kernel_path method and see if it helps my usecase. I'll leave this PR open till I've confirmed it from my end.

FWIW, it seems the UUID was added to solve a race condition (as per this comment)

@maartenbreddels
Copy link
Contributor

In voila we used CGI variables, see e.g. https://github.com/voila-dashboards/voila/blob/main/voila/handler.py#L88

We could also add PATH_TRANSLATED for this, which would be the notebook path on disk. See https://datatracker.ietf.org/doc/html/rfc3875#section-4.1.6

@divyansshhh
Copy link
Contributor Author

I tried the get_kernel_path recommendation and it doesn't work for my use case. So, I propose making it easier to pass env variables to kernels in a way that makes it easier to extend the environment variables as per their need.

Thoughts on something like this?

def get_kernel_env(self, path):
    return {**os.environ, "JP_SESSION_NAME": path}

To add extra env variabes (and make it easier to keep it in sync with upstream code) we can extend it's behavior in a custom session manager like this -

# in a custom session manager
def get_kernel_env(self, path):
    upstream_env = super().get_kernel_env()
    return {**upstream_env, "EXTRA_ENV_VARIABLE": path_modified}

@kevin-bates
Copy link
Member

Using a custom session manager, you're free to do anything you want, and this is probably the best approach for now.

I think what is really necessary here is the ability to express parameters in general, and a formal kernel parameterization feature should include the ability to set arbitrary(?) envs (or perhaps those that are specified in the kernel parameter schema) along with whatever other parameters are published by the underlying kernel provisioner and kernel implementations. For example, remote_ikernel could publish a schema that lists EXTRA_ENV_VARIABLE as one of the envs that it will use internally.

@divyansshhh divyansshhh changed the title Set JPY_NOTEBOOK_DIR env variable in the kernel Make it easier to pass custom env variables to kenre; Sep 22, 2022
@divyansshhh divyansshhh changed the title Make it easier to pass custom env variables to kenre; Make it easier to pass custom env variables to kernel Sep 22, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022

Codecov Report

Base: 72.59% // Head: 72.60% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (75ed036) compared to base (a4266be).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #981   +/-   ##
=======================================
  Coverage   72.59%   72.60%           
=======================================
  Files          64       64           
  Lines        8239     8242    +3     
  Branches     1378     1378           
=======================================
+ Hits         5981     5984    +3     
  Misses       1844     1844           
  Partials      414      414           
Impacted Files Coverage Δ
jupyter_server/services/sessions/sessionmanager.py 88.47% <100.00%> (+0.16%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Thank you for submitting this PR @divyansshhh. This seems like a reasonable step forward allowing custom session manager implementations to extend the env stanza.

@blink1073 blink1073 merged commit 98b4285 into jupyter-server:main Sep 22, 2022
@divyansshhh
Copy link
Contributor Author

@meeseeksdev please backport to 1.x

@lumberbot-app
Copy link

lumberbot-app bot commented Sep 23, 2022

Awww, sorry divyansshhh you do not seem to be allowed to do that, please ask a repository maintainer.

@divyansshhh
Copy link
Contributor Author

Seems I can't use meeseeksdev. @kevin-bates can you please backport this to 1.x?

Also, when is the next scheduled release of jupyter_server 1.x? It'd be nice to have a 1.x release with this commit by next Friday.

@blink1073
Copy link
Collaborator

@meeseeksdev please backport to 1.x

@lumberbot-app
Copy link

lumberbot-app bot commented Sep 23, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 1.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 98b428548bbadb007133effb6750a7fd6ed2cde5
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #981: Make it easier to pass custom env variables to kernel'
  1. Push to a named branch:
git push YOURFORK 1.x:auto-backport-of-pr-981-on-1.x
  1. Create a PR against branch 1.x, I would have named this PR:

"Backport PR #981 on branch 1.x (Make it easier to pass custom env variables to kernel)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@blink1073
Copy link
Collaborator

@divyansshhh, if you want to handle the manual backport I can make a 1.x release on Monday. We try not to release on Fridays in case we break things. 😄

divyansshhh added a commit to divyansshhh/jupyter_server that referenced this pull request Sep 26, 2022
@divyansshhh
Copy link
Contributor Author

I've raised #994 for the backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants