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

Support for Bokeh 3.4 #6072

Merged
merged 32 commits into from
Feb 17, 2024
Merged

Support for Bokeh 3.4 #6072

merged 32 commits into from
Feb 17, 2024

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Dec 18, 2023

I have chosen to hard-pin the build step. This can always be reversed.

panel/io/resources.py Outdated Show resolved Hide resolved
@@ -316,7 +316,7 @@ export class PlotlyPlotView extends HTMLBoxView {
}

_get_trace(index: number, update: boolean): any {
const trace = clone(this.model.data[index]);
const trace = clone(this.model.data[index]) as any;
Copy link
Member Author

@hoxbro hoxbro Jan 24, 2024

Choose a reason for hiding this comment

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

Else, it would give a type error.

panel/tests/io/test_reload.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (269b433) 84.19% compared to head (d0a03b1) 83.65%.
Report is 32 commits behind head on main.

Files Patch % Lines
panel/io/resources.py 70.58% 5 Missing ⚠️
panel/widgets/select.py 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6072      +/-   ##
==========================================
- Coverage   84.19%   83.65%   -0.55%     
==========================================
  Files         301      305       +4     
  Lines       45200    45518     +318     
==========================================
+ Hits        38056    38076      +20     
- Misses       7144     7442     +298     
Flag Coverage Δ
ui-tests 39.64% <63.63%> (-1.09%) ⬇️
unitexamples-tests 71.65% <66.66%> (-0.18%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoxbro hoxbro marked this pull request as ready for review January 24, 2024 18:47
panel/io/resources.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@hoxbro hoxbro marked this pull request as draft January 26, 2024 14:25
@hoxbro
Copy link
Member Author

hoxbro commented Jan 29, 2024

I can see the UI tests are starting to fail between Bokeh 3.4.0.dev4 and 3.4.0.dev5, this is likely to be because of bokeh/bokeh#13618, @mattpap do you agree? The PR is pretty big, and I can't see if it is backward compatible, with older versions of Bokeh.

A quick glance shows the following models are failing:

  • Tabulator
  • Plotly and Vega plots
  • ReactiveHTML

@philippjfr
Copy link
Member

Suspect this is due to the dict -> object/Map conversion. It used to be the case that most dicts would end up being converted to a JS Map but now you will end up with a standard JS object. Suspect those models will have to be updated.

@mattpap
Copy link
Collaborator

mattpap commented Jan 29, 2024

The PR is pretty big, and I can't see if it is backward compatible, with older versions of Bokeh.

It's not compatible on the implementation level. Previously if there was a dict in bokeh, then it was deserialized in bokehjs either as a plain object {}, if all keys were strings, or as Map in all other cases. Now dict is always deserialized as a Map. However, in all contexts, in particular all properties, that previously supported plain objects, now both plain objects and Map are supported. This is to keep API compatibility, while allowing preservation of object identity (no cloning) and performance (no proxying).

So what needs to happen, is that all cases where plain objects are used, instead of manipulate those objects directly, one should use APIs from core/util/object, like keys(), values(), entries(), dict(), to_object(), etc. Also don't ever use for .. in, but use for (const [key, vall] of entries(plain_object_or_map)).

This comes with some caveats. This approach breaks pseudo structs that are encoded as dictionaries, see e.g. bokeh/bokeh#13637. That issue is a critical regression to fix to make 3.4 viable. Otherwise I will need to revert PR bokeh/bokeh#13618. If you previously passed a plain object from a property to a third-party JS API, then now you have to use to_object() to make sure it's still a plain object.

@hoxbro
Copy link
Member Author

hoxbro commented Jan 29, 2024

Thank you, @mattpap, for the explanation.

Can you give an estimated time for when you look at the regression? (No pressure, just curious)

@philippjfr, I will wait with this PR until the regression or reversion has been made in Bokeh.

@mattpap
Copy link
Collaborator

mattpap commented Jan 29, 2024

Can you give an estimated time for when you look at the regression? (No pressure, just curious)

As soon as I'm done with UIs, so hopefully tomorrow.

@philippjfr
Copy link
Member

Note I don't think the regression affects Panel, unless I'm misunderstanding.

@hoxbro
Copy link
Member Author

hoxbro commented Jan 29, 2024

Note I don't think the regression affects Panel, unless I'm misunderstanding.

I agree, but if the PR is reverted, there is no need to update Panel to support it.

@mattpap
Copy link
Collaborator

mattpap commented Feb 15, 2024

@hoxbro, I reverted dict/Map related changes, upgraded to dev8, fixed tsconfig.json (as mentioned in the meeting) and renamed one bokeh API call (rebuild -> update_children).

@mattpap
Copy link
Collaborator

mattpap commented Feb 15, 2024

I will publish removed commits in an new PR when this one is merged.

@philippjfr philippjfr marked this pull request as ready for review February 16, 2024 17:42
@philippjfr philippjfr merged commit 7de0250 into main Feb 17, 2024
12 of 15 checks passed
@philippjfr philippjfr deleted the bokeh34 branch February 17, 2024 10:35
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.

4 participants