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

Passing request URI to kernel env #414

Merged

Conversation

derek-pyne
Copy link
Contributor

Issue: #410

POC of ability to pass request uri into kernel. This would enable all sorts of dynamic/linking dashboard setups.

Added a sample notebook that prints the env.

Concerns:
When the kernel manager starts a new kernel, it either uses the existing environment or an environment passed in as an argument. It does not merge these environments. This is a concern if there was anything in the previous os.environ that we needed.

https://github.com/jupyter/jupyter_client/blob/dc213de1b5cc7b5c28485ae799fecfef77589a85/jupyter_client/manager.py#L239

@derek-pyne
Copy link
Contributor Author

Now adding REQUEST_URI to env instead of replacing it.

voila/handler.py Outdated Show resolved Hide resolved
@maartenbreddels
Copy link
Member

https://tools.ietf.org/html/rfc3875 may be useful. But, I'm not sure everybody is on board with this idea. Maybe it should be an opt-in?

@jtpio
Copy link
Member

jtpio commented Oct 11, 2019

But, I'm not sure everybody is on board with this idea. Maybe it should be an opt-in?

For other variables? Having access to the request and query params would already be very useful to dashboard authors.

@maartenbreddels
Copy link
Member

Having SERVER_NAME and SERVER_SOFTWARE may also be useful.

@derek-pyne
Copy link
Contributor Author

Having SERVER_NAME and SERVER_SOFTWARE may also be useful.

Added SERVER_NAME. Not sure where to get SERVER_SOFTWARE though.

@maartenbreddels
Copy link
Member

SERVER_SOFTWARE

does "voila / 0.1.13" make sense ?

@derek-pyne
Copy link
Contributor Author

Having a lot of fun with this feature. It totally let us build connected/linked dashboards. Any thoughts about merging it in/next steps? @maartenbreddels @jtpio

@maartenbreddels
Copy link
Member

I think it's a good idea to discuss this at the next meeting: https://voila-dashboards.github.io/
Maybe you'd like to join?

@derek-pyne
Copy link
Contributor Author

derek-pyne commented Oct 17, 2019

Sounds good to me! I won't be able to join this Monday, but will join for the next meeting.

@jtpio
Copy link
Member

jtpio commented Oct 18, 2019

Thanks @derek-pyne for working on this.

Having access to the query parameters is indeed very useful, making it possible to recreate dashboards with a particular state. Especially in multi-page setups where one dashboard has a link to another dashboard and passes information via query parameters to initialize the state of the second dashboard.

@maartenbreddels
Copy link
Member

I think if we do this, we should coordinate for voila/lab/notebook/server. It makes kernels frontend aware.
Is Jupyter hub not also doing something like passing env variables around @jtpio ?

@jtpio
Copy link
Member

jtpio commented Oct 18, 2019

I think if we do this, we should coordinate for voila/lab/notebook/server. It makes kernels frontend aware.

Sure, but this is very specific to the voila handler. Kernels wouldn't be exposed to it in other environments.

Is Jupyter hub not also doing something like passing env variables around @jtpio ?

JupyterHub exposes a few environment variables yes. For example on mybinder.org:

image

@vidartf
Copy link
Contributor

vidartf commented Oct 18, 2019

Note that if jupyter_server switches to using kernel providers (jupyter-server/jupyter_server#90), this could potentially be passed in via this takluyver/jupyter_kernel_mgmt#22? CC @kevin-bates

This only exist in the future though, so I don't think that should block this, but more acts as a note to the future.

@kevin-bates
Copy link

@vidartf - thanks for the cc.

Yes. In fact, I'd like to support a well-known entry of launch_params to be env. "Well-known" in the sense that that would be one item that spans kernel providers. This is because we already convey env variables in the kernel start's json-body in the gateway projects (since we overwrite the handlers) - although that is a top-level 'env' entry - which I'd be okay keeping as well.

A couple things to keep in mind. Currently, (in EG), any env prefixed with KERNEL_ automatically "flows" (is made available in the launched kernel's env). Any others, however, are compared to an "env_whitelist" (configurable list of names) and only those variables whose name appears in the whitelist are transferred into the kernel's env. This prevents unwarranted entries from getting to the kernel.

@timkpaine
Copy link
Member

You can do this via the hook too #218 (comment)

@derek-pyne
Copy link
Contributor Author

Hey all, sorry for leaving this PR hanging for the team.

@jtpio @maartenbreddels Any suggested next steps? Doesn't necessarily need to be merged, just want to help getting to a conclusion :)

@timkpaine Prelaunch hooks looks like a REALLY nice more general solution to this.

If we are moving forward on prelaunch hooks, I'd suggest killing this PR and using that functionality.

@maartenbreddels
Copy link
Member

I've rebased this and removed REQUEST_URI which is not part of the CGI spec, I've added tests and a simple notebook example how to use this query parameters with the rest of the CGI parameters to create a link to the notebook itself.

self.kernel_env['SERVER_PROTOCOL'] = str(self.request.version)
host, port = split_host_and_port(self.request.host.lower())
self.kernel_env['SERVER_PORT'] = str(port) if port else ''
self.kernel_env['SERVER_NAME'] = host
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these environment variables be namespaced (prefixed with VOILA for example).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok then...

@SylvainCorlay SylvainCorlay merged commit cd743b1 into voila-dashboards:master Aug 18, 2020
@SylvainCorlay
Copy link
Member

🎉

@derek-pyne
Copy link
Contributor Author

Thanks all! We've been using this branch/feature for awhile now. Happy to see this feature get a path into master :) Changes look good.

@maartenbreddels
Copy link
Member

Thanks for your patience, and for starting this, I think it's gonna be a super useful feature. Note that we don't have REQUEST_URI anymore, but SCRIPT_NAME + QUERY_STRING should be the same

@cantagallo
Copy link

Hi all!
Could you please provide an example of how to use this cool feature in my voila-rendered notebook?

let's assume I render my notebook in my jupyterhub with:
https://..../jhub/user-redirect/voila/render/Notebook.ipynb?a=1

how can I access the variable a inside the notebook? i.e. where do I access SCRIPT_NAME and QUERY_STRING?

(Note: I don't find them in os.environ, I already tried)

@maartenbreddels
Copy link
Member

See https://github.com/voila-dashboards/voila/blob/master/notebooks/query-strings.ipynb, but this requires voila 0.2+

@cantagallo
Copy link

Thanks Maarten! It works, I was actually using an old version of Voilà.. thanks and congrats for all you’re doing! -Marco

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

Successfully merging this pull request may close these issues.

9 participants