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

ipywidgets expects Comm, not BaseComm #1090

Closed
matthewturk opened this issue Feb 8, 2023 · 6 comments · Fixed by #1097 · May be fixed by #1091
Closed

ipywidgets expects Comm, not BaseComm #1090

matthewturk opened this issue Feb 8, 2023 · 6 comments · Fixed by #1097 · May be fixed by #1091

Comments

@matthewturk
Copy link

Hi! I've been trying to track down an issue with my usage of ipywidgets, described on discourse. The basic gist is:

  • I'm attempting to create a widget from the client by using a model.widget_manager's method new_widget. In previous versions, this worked, but now, it doesn't.
  • The issue seems to stem from the kernel attempting to create a new comm instance for a widget, but supplying to that widget a BaseComm rather than a Comm, which throws a validation error.

After digging through this, it looks to me like ipywidgets declares comm = Instance('ipykernel.comm.Comm', allow_none=True), but ipykernel.ipkernel.create_comm explicitly creates a BaseComm, so the traitlet doesn't validate, and the comm fails to properly initialize. This results in the widgets not being synced back to the kernel from the frontend.

I confess that while I've spent time going through the code, I'm not entirely sure if this could potentially be a version mismatch, or if this is an oversight for a use case that may not come up terribly frequently.

My versions:

  • ipywidgets - 8.0.4
  • ipykernel - 6.21.1
  • comm - 0.1.2

I was able to address the problem I was having by changing the import and usage of BaseComm to Comm in ipkernel.py, and so if this is an acceptable solution I'd be happy to issue a pull request. (My hesitance is that there may be unexpected downstream effects of a change to something so deep in the kernel code, and so I wanted to open the issue as possible first step.)

@blink1073
Copy link
Contributor

Hi @matthewturk, please do open a PR. We test most known downstream consumers in Jupyter and spyder, seeing if those downstream tests fail would be a good indicator.

@maartenbreddels
Copy link
Contributor

I think this is an ipywidget issue.

@matthewturk In jupyter-widgets/ipywidgets#3602 (comment) I removed the comm being an Instance of this class. I think this will fix it, but we never merged it.

@matthewturk
Copy link
Author

@maartenbreddels That PR is pretty big though -- does it need to be a part of that, or would a smaller PR that simply loosened restrictions work, so that we don't have to pin all our dependencies to a lower version?

@maartenbreddels
Copy link
Contributor

I think if we just cherry-pick, jupyter-widgets/ipywidgets@c1c34f9 we should be able to get this in sooner.

An even saver option, it to change it to traitlets.Any. I don't think we need the type safety for the comm.

@martinRenou
Copy link
Contributor

I will work on finishing jupyter-widgets/ipywidgets#3533 this morning. This PR uses traitlets.Any and we should backport it to ipywidgets 7

@maartenbreddels
Copy link
Contributor

I think we should backport this indeed

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