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

Regression handling callback output #1353

Merged
merged 7 commits into from May 16, 2017

Conversation

Projects
None yet
3 participants
@pbugnion
Member

pbugnion commented May 14, 2017

In ipywidgets 6.0.0 (and all the way through to the merge of PR #1274), the outputs of widget callbacks would be printed to the widget output area:

screen shot 2017-05-14 at 07 50 50

This stops working in PR 1274. I presume this is accidental? (if not, the rest of this PR is irrelevant).

As far as I can tell, the issue arises because callback messages do not have the correct iopub.output and iopub.handlers handlers any more. This is because we don't explicitly bind the handlers of the view to the handlers of the output area.

This PR creates a temporary fix by re-enabling the WidgetManager$callbacks(view) method which defines the iopub callbacks. Prior to PR #1274, this looked for the cell in which the view is defined. We don't have access to this now, but we do have access to the output area when the widget is defined, so this PR just injects a reference to the output area into the view's options.

This fix is just a proof of concept, it's not really meant to be the ultimate solution, but some guidance here would be great:

  • Is this even a regression? Or was it intentional?
  • Where should we re-bind the output callbacks? We could do it directly when the widget is rendered, rather than relying on a method on the manager, unless the
  • Am I missing places where views are defined?

pbugnion added some commits May 14, 2017

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 15, 2017

It wasn't an accident, but I viewed it as an unfortunate consequence of #1274. For example, what if a widget observe callback called clear_output? That would clear the view that sent the message. Also, now that we can have output and widgets intermixed, it seemed confusing to have output being appended below all of the other widgets and output. However, especially given how complicated #1321 turned out to be, perhaps this can be reconsidered. I still think the proper behavior is now to create an Output widget, but this workaround at least allows errors to appear to the user instead of being hidden.

@maartenbreddels

This comment has been minimized.

Member

maartenbreddels commented May 15, 2017

Yes, I'd say better have 'odd' behaviour/output that can be fixed with using Output, than no msg at all.

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 15, 2017

To reproduce:

from ipywidgets import IntSlider
x = IntSlider()
def onchange(change):
    print(change)
x.observe(onchange, 'value')
x

@jasongrout jasongrout force-pushed the pbugnion:fix-regression-output-area branch from aee85e4 to be4e918 May 15, 2017

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 15, 2017

@pbugnion - can you review my changes to this PR? Basically, I simplified the callback construction, and used the display_model options argument.

@jasongrout jasongrout added this to the 7.0 milestone May 15, 2017

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 15, 2017

Where should we re-bind the output callbacks? We could do it directly when the widget is rendered, rather than relying on a method on the manager, unless the

We could set the iopub callback dictionary in the view options when we create the view options. That would save creating the bound function each time a message is sent. Thoughts? I'm not sure it's worth it.

@pbugnion

This comment has been minimized.

Member

pbugnion commented May 16, 2017

Thanks -- your changes make the code a lot cleaner. I hadn't realised the options passed into display_model also get propagated to the views.

The output still wasn't appearing if the callback was bound to a child model. I've added a fix for passing the output area to children of the outermost widget (I copied the right method from pre PR 1274). To reproduce:

import ipywidgets as widgets

slider = widgets.IntSlider()
box = widgets.HBox([slider])
outer_box = widgets.VBox([box])
def on_change(change):
    print('hello')
slider.observe(on_change, names='value')
outer_box
Bind iopub callbacks when setting view options
Prior to this commits, the view’s iopub callbacks were re-bound to the output area’s iopub callbacks every time the callbacks were called. They are now just bound once when the view options are instantiated.
@pbugnion

This comment has been minimized.

Member

pbugnion commented May 16, 2017

We could set the iopub callback dictionary in the view options when we create the view options. That would save creating the bound function each time a message is sent. Thoughts? I'm not sure it's worth it.

I've had a go at doing this. Let me know what you think. It seems cleaner to bind the iopub callbacks once, but I could see benefits either way.

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 16, 2017

The output still wasn't appearing if the callback was bound to a child model. I've added a fix for passing the output area to children of the outermost widget (I copied the right method from pre PR 1274).

Oh right, good catch. I'm pushing an update to the comment + small simplification.

@jasongrout

This comment has been minimized.

Member

jasongrout commented May 16, 2017

I'll wait for tests to pass before merging this. Thanks!

@jasongrout jasongrout merged commit 9742b29 into jupyter-widgets:master May 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasongrout jasongrout referenced this pull request May 16, 2017

Closed

Update Changelog #1279

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment