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

Remove iframe when kernel is killed or restarted #22

Merged
merged 4 commits into from
Sep 20, 2023

Conversation

jnumainville
Copy link
Collaborator

@jnumainville jnumainville commented Sep 19, 2023

This will remove the iframe in three valid situations:

  1. The kernel is restarted
  2. The kernel is removed (such as by switching to 'No Kernel')
  3. The kernel is killed from the python side

Both 2 and 3 are doable from the python side, as they kill the interpreter, and can be caught via an atexit hook. 1 and 2 are doable from the javascript side of the widget by listening to changes to the session status, which is not officially supported for widgets as far as I can tell, but does work. See here for example of these listeners: https://github.com/jupyter-widgets/ipywidgets/blob/main/python/jupyterlab_widgets/src/manager.ts#L427

Only current downside of this approach is it will keep spawning listeners. I'm not sure if there's a good way to prune them or the widget itself completely (as even after the iframe is removed the widget is still around, with the exception of case 3).

I've tested with the following cells and verified the iframe is removed in all these cases:

from deephaven_server import Server
s = Server(port=8080)
s.start()
from deephaven import time_table
from deephaven_ipywidgets import DeephavenWidget
t = time_table("PT1S").update(["x=i", "y=Math.sin(i)"])
display(DeephavenWidget(t))

Also fixes #23
Checked by copying the cells from the iframe created above

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Need to apply prettier to the JS code as well

deephaven_ipywidgets/deephaven.py Show resolved Hide resolved
src/widget.ts Outdated Show resolved Hide resolved
src/widget.ts Outdated Show resolved Hide resolved
src/widget.ts Outdated
Comment on lines 134 to 135
// eslint-disable-next-line @typescript-eslint/no-explicit-any
onRestartOrTerminate = (sender: any, args: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

You could use unknown here instead and get rid of the warning, since you're not using sender:

Suggested change
// eslint-disable-next-line @typescript-eslint/no-explicit-any
onRestartOrTerminate = (sender: any, args: string) => {
onRestartOrTerminate = (sender: unknown, args: string) => {

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.

Copy does not work within Jupyter grid iframes
2 participants