bokeh / ipywidgets_bokeh

Allows embedding of Jupyter widgets in Bokeh applications.
BSD 3-Clause "New" or "Revised" License
23 stars 11 forks source link

Embed full state on initial render #97

Closed philippjfr closed 1 year ago

philippjfr commented 1 year ago

@mattpap Not sure how much you remember about this stuff but basically here I'm including two separate state bundles. The problem that a few libraries I was looking into were not rendering correctly because message containing the state that was sent after the model was rendered was often missing a bunch of properties, e.g. the _view_name.

I'd also love to hear your thoughts @peytondmurray and @ndmlny-qs.

Fixes https://github.com/bokeh/ipywidgets_bokeh/issues/88 Fixes https://github.com/bokeh/ipywidgets_bokeh/issues/89

mattpap commented 1 year ago

I think I don't have anything to contribute here. If it works then I'm fine with merging this. Maybe other reviewers will have better feedback. One thing I would suggest though, is to explain the code in comments and perhaps think how we can test this in the longer term.

philippjfr commented 1 year ago

That's a good idea, @ndmlny-qs do you have any time to maybe set up some playwright tests for this repo?

philippjfr commented 1 year ago

Only other thing I'd want to check before merging is whether ipywidgets still sends the full state a second time.

ndmlny-qs commented 1 year ago

I can confirm that this change works for the anywidget button counter example. These issues are resolved with this change:

I do not know about the mapwidget issue https://github.com/bokeh/ipywidgets_bokeh/issues/88, as it requires an account.

@philippjfr can you clarify/give more info around what you mean by ...check before merging is whether ipywidgets still sends the full state a second time. so I can check it?

peytondmurray commented 1 year ago

@philippjfr I really don't have the context to understand this change, but I'm glad that it appears to fix a few of these outstanding issues. Instead I'll just defer to @ndmlny-qs about this.

philippjfr commented 1 year ago

This is looking great, thanks @ndmlny-qs!

ndmlny-qs commented 1 year ago

The windows tests are failing, and I have no idea how to overcome the issue. I ran the tests on a local windows 10 vm, and got the same failing results. Any clues about how to solve this would be greatly appreciated.

ndmlny-qs commented 1 year ago

I had to skip e2e tests for windows in the GH workflow. I thought skipping them in the anywidget module would be sufficient, but pytest will error with a code 5 (https://docs.pytest.org/en/latest/reference/exit-codes.html), i.e. no tests collected. In order to circumvent this error, I had to skip them in the CI

When we have unit tests, this restriction can be removed, and I think we can use the module level pytest.skip() method to skip e2e tests in windows. @mattpap if the tests look good to you, then I'll squash and merge.

philippjfr commented 1 year ago

Thank you @ndmlny-qs! This is a good start and we can iterate further from here. I have another follow-up PR that will allow dynamically adding new models so I'll merge this now.