-
-
Notifications
You must be signed in to change notification settings - Fork 538
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
compat: Bokeh 3.7 #7724
compat: Bokeh 3.7 #7724
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7724 +/- ##
==========================================
+ Coverage 86.09% 87.09% +1.00%
==========================================
Files 346 346
Lines 52790 52793 +3
==========================================
+ Hits 45449 45982 +533
+ Misses 7341 6811 -530 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -123,7 +123,7 @@ export class HTMLView extends PanelMarkupView { | |||
return [...super.stylesheets(), html_css] | |||
} | |||
|
|||
protected rerender() { |
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 one scares me and is probably not backward compatible. rerender
used to be a panel only thing, seems like there's now a method in Bokeh that probably does something slightly different:
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.
Yeah, was not sure about this one. If we still want to be backward compatible, can't we just rename the function?
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 was added in #3616 and released in 0.14.0a2. Locally, the test still passes when running the test added in the PR and removing this function. Let's see what the CI says.
@@ -127,7 +127,7 @@ | |||
"p2 = figure(height=150, sizing_mode='stretch_width')\n", | |||
"\n", | |||
"p1.line([1, 2, 3], [1, 2, 3])\n", | |||
"p2.circle([1, 2, 3], [1, 2, 3])\n", | |||
"p2.scatter([1, 2, 3], [1, 2, 3])\n", |
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.
Made radius required in bokeh/bokeh#14370
@@ -386,6 +386,7 @@ def _update_object( | |||
parent.children[node] = new_models # type: ignore | |||
break | |||
elif isinstance(parent, _BkTabs): | |||
parent.tabs = cast(list[_BkTabPanel], parent.tabs) |
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.
@mattpap, This shouldn't be needed, in my opinion. This conversion always happens on the Bokeh site of things:
Maybe default factory functions could be a way to reduce this overhead.
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.
Currently for simplicity .accepts()
clauses where merged into property types in *.pyi
files. In the future, relevant declarations will have be split into setters and getters with asymmetric types, if possible at all.
@@ -234,7 +234,7 @@ def _get_column_definitions(self, col_names: list[str], df: pd.DataFrame) -> lis | |||
if isinstance(data, pd.DataFrame): | |||
raise ValueError("DataFrame contains duplicate column names.") | |||
|
|||
col_kwargs = {} | |||
col_kwargs: dict[str, Any] = {} |
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.
In an ideal world this should be replaced with a TypedDict
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.
In the ideal world you could use TableColumn
as-is declared in bokeh. But, for many reasons, we had to declare models as dataclasses and you can't get TypedDict
from a dataclass. Same problem with Unpack[]
which was supposed to help with **kwargs
, but we can't use it.
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 comment was more meant on Panels site of things.
@@ -162,7 +162,8 @@ def _update_sources(cls, json_data, sources: list[ColumnDataSource]): | |||
# Create index of sources by columns | |||
source_columns = defaultdict(list) | |||
for i, source in enumerate(sources): | |||
key = tuple(sorted(source.data.keys())) | |||
# source.data = cast("DataDict", source.data) |
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 didn't work, failed with the error: ValueError: ColumnDataSource.data properties may only be set from plain Python dicts, not other ColumnDataSource.data values.
Maybe cast
could be supported in Bokeh?
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.
An idea would be to make source.data = source.data
(which is what cast is) a no-op by doing an identity check. I haven't looked at the Bokeh code to see if this is feasible.
[skip ci]
Bokeh 3.7 RC is soon gonna be released.
This tests the latest dev release to see how much work we need.
EDIT: It is very backward compatible.