-
Notifications
You must be signed in to change notification settings - Fork 945
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
Handle KernelWidgetManager
in the JupyterLab OutputModel
#3561
Handle KernelWidgetManager
in the JupyterLab OutputModel
#3561
Conversation
@@ -114,7 +117,7 @@ export class OutputModel extends outputBase.OutputModel { | |||
} | |||
} | |||
|
|||
widget_manager: WidgetManager; | |||
widget_manager: LabWidgetManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately changing from a more specialized to a more generic type here looks like a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking that another approach could be for those defining their own widget manager based on a KernelWidgetManager
to also define their own OutputModel
and register it.
Although this means duplicating quite a bit of code from ipywidgets and change the handling of the context / kernel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking that another approach could be for those defining their own widget manager based on a
KernelWidgetManager
to also define their ownOutputModel
and register it
Linking to voila-dashboards/voila@3840999 which implements this approach (may be temporary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately changing from a more specialized to a more generic type here looks like a breaking change.
Having breaking changes and therefore a major version bump on the lab manager isn't necessarily that problematic though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but will probably require a major version bump of the jupyterlab extension (at least on the JS side), since the typing on the OutputModel changes. That should hopefully not be a big problem though.
Thanks! |
This looks like this could have been released in |
So a quick way to check could be to upgrade to the latest version with |
Thanks for the orientation @jtpio - i've just confirmed i'm running that version in jupyterlab and I can see the change in |
Investigating an issue in voila-dashboards/voila#846 where using an
interactive_output
would trigger the following error:voila-dashboards/voila#846 uses the JupyterLab packages but with a custom
KernelWidgetManager
, not aWidgetManager
like in the JupyterLab extension. This is because it only has access to a kernel connection, not a document context.However in its present state, the
OutputModel
assumes it can access thecontext
from itswidget_manager
here:ipywidgets/python/jupyterlab_widgets/src/output.ts
Line 36 in 58adde2
Ideally it should be able to handle both a
WidgetManager
andKernelWidgetManager
or even aLabWidgetManager
directly.For reference the
widget_manager
assigned to aWidgetModel
is declared asany
here:ipywidgets/packages/base/src/widget.ts
Line 72 in 58adde2
And assigned here:
ipywidgets/packages/base/src/widget.ts
Line 124 in 58adde2