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

using voila specific kernel manager #41

Conversation

maartenbreddels
Copy link
Member

Now voila and the server extension both use a voila_kernel_manager instead of kernel_manager. This means that voila used as server extension also does not allow arbitrary code execution.

@maartenbreddels
Copy link
Member Author

Also allow the custom_message for voila standalone and server extension.

@@ -125,6 +138,7 @@ def start(self):
'comm_msg',
'comm_info_request',
'kernel_info_request',
'custom_message',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need custom_message here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was confused about this, and now even more. I remember seeing warnings in the console from voila that custom_message was send and ignored. I assumed that it was a custom message like the Button widget uses to communicate the click event. But that should be a regular comm_msg right? So what is custom_message, and who would send that?

Copy link
Member

Choose a reason for hiding this comment

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

It is not part of the kernel protocol. I think it is an ad-hoc message that was used by the ipywidgets_server to trigger initial execution.

Copy link
Member Author

Choose a reason for hiding this comment

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

That must have been it, so it can be removed.

@jtpio
Copy link
Member

jtpio commented May 28, 2020

Thinking about this a bit more, having a separate kernel manager for voila could also be very useful to:

  • show the "voila only" kernels (Show the list of voila kernels in the running tabs #611), as clients could send a GET /voila/api/kernels when voila is used as a server extension, and will only get the voila kernels without the regular kernel sessions started via the notebook frontend
  • configure the idle culler to only cull the voila kernels when voila is used as a server extension (this was suggested by @maartenbreddels).

@jtpio
Copy link
Member

jtpio commented May 28, 2020

I can rebase this PR onto the latest master tomorrow.

(cc @maartenbreddels let me know if you have any objections)

@maartenbreddels
Copy link
Member Author

Yes, please go ahead, I think we should have a different manager, with different defaults, different permissions, and different authentication.

@maartenbreddels
Copy link
Member Author

Something that crossed my mind: Would it be a different pool of kernels? Like would a kernel spawned by notebook or lab be visible to voila?
I'm thinking about a future use case where you have a running notebook, and want to share that existing kernel via voila.

@jtpio
Copy link
Member

jtpio commented May 29, 2020

I'm thinking about a future use case where you have a running notebook, and want to share that existing kernel via voila.

It looks like this should be possible at the moment with voila running as a server extension sharing the same kernel manager as the notebook server, by passing an existing kernel_id to the start_kernel?

kernel_id = await self.kernel_manager.start_kernel(kernel_name=self.notebook.metadata.kernelspec.name, path=self.cwd)

Although there will probably be some issues with the widgets (multiple clients for the same kernel)

Would it be a different pool of kernels? Like would a kernel spawned by notebook or lab be visible to voila?

If we isolate the voila kernel manager, then this becomes a bit trickier. Unless the voila kernel manager is able to proxy some of the messages to the other kernel manager (not sure yet if that's even possible right now).

@maartenbreddels
Copy link
Member Author

Although there will probably be some issues with the widgets (multiple clients for the same kernel)

Currently yes, but there are ways around that, and for sure it's going to be supported in the future :)

@jtpio jtpio force-pushed the voila_kernel_manager branch from 5fdbd49 to 00336b7 Compare May 29, 2020 09:14
@SylvainCorlay
Copy link
Member

I don't think that this resolves the arbitrary code execution issue though, because the raw jupyter server always exposes the base kernel API endpoint.

@jtpio
Copy link
Member

jtpio commented Jun 2, 2020

A couple of things to cover before this change can be considered as "ready":

  • Fix failing test in tests/server/cwd_subdir_test.py (for reference: https://github.com/voila-dashboards/voila/runs/720801386?check_suite_focus=true)
  • Shutdown all voila kernels on Ctrl-C / SIGTERM when voila is used as server extension (they are not shut down automatically anymore now that there is a separate kernel manager instance)
  • Add test for the kernel namespacing when voila is used as a server extension (/api/kernels and /voila/api/kernels should return different results)

@jtpio
Copy link
Member

jtpio commented Jun 2, 2020

Shutdown all voila kernels on Ctrl-C / SIGTERM when voila is used as server extension (they are not shut down automatically anymore now that there is a separate kernel manager instance)

The ExtensionApp.stop() method might be relevant here (although it seems to be specific to standalone mode?):

https://github.com/jupyter/jupyter_server/blob/f1df254e17854aecb0ef025a5891c6910c62fd70/jupyter_server/extension/application.py#L387-L391

@trungleduc
Copy link
Member

Closing as the proposed changes are not applicable anymore to the next release of Voila

@trungleduc trungleduc closed this Aug 1, 2023
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.

4 participants