fix: DH-18653: migrate to plotly >= 6.0.0 #1179
Conversation
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the plotting functionality to use plotly versions ≥6.0.0 by updating dependencies, replacing deprecated mapbox‐based functions with their new map counterparts, and updating test constants and parameter names.
- Bump plotly.py to 6.0.0 and plotly.js to 3.0.0 in package configurations
- Replace all usages of mapbox functions (e.g. scatter_mapbox, line_mapbox, density_mapbox) with new functions (scatter_map, line_map, density_map) and add deprecation warnings
- Update test files to use new binary data constants (PLOTLY_NULL_INT and PLOTLY_NULL_DOUBLE) and adjust expected layouts accordingly
Reviewed Changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| plugins/plotly-express/test/deephaven/plot/express/plots/* | Replace deprecated NULL_* values with new PLOTLY_NULL_* constants and update test names for map-based functions |
| plugins/plotly-express/src/js/* | Update JS utility and model files to work with the new plotly.js version and reframe map detection logic |
| plugins/plotly-express/src/deephaven/plot/express/plots/maps.py | Deprecate mapbox functions in favor of the new map functions; update parameter names (mapbox_style → map_style) and docstrings accordingly |
| plugins/plotly-express/setup.cfg & package.json | Bump plotly dependency constraints to support plotly version ≥6.0.0 |
Comments suppressed due to low confidence (3)
plugins/plotly-express/test/deephaven/plot/express/plots/test_indicator.py:236
- For consistency across tests, consider replacing NULL_INT with PLOTLY_NULL_INT as done in other test files.
from deephaven.constants import NULL_INT
plugins/plotly-express/src/deephaven/plot/express/plots/maps.py:152
- The deprecation functions now call the new scatter_map; ensure that the docstrings and return descriptions are updated to consistently reflect the new naming and API changes.
def scatter_mapbox(*args, **kwargs) -> DeephavenFigure:
plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts:794
- Verify that checking for the substring 'map' in trace types (via hasMap()) does not unintentionally match trace types other than the intended map traces; a stricter condition may be needed.
return this.hasScene() || this.hasGeo() || this.hasMap() || this.hasPolar();
|
plotly-express docs preview (Available for 14 days) |
| "nanoid": "^5.0.7", | ||
| "plotly.js": "^2.29.1", | ||
| "plotly.js-dist-min": "^2.29.1", | ||
| "plotly.js": "^3.0.0", |
There was a problem hiding this comment.
We should update this in web-client-ui as well: https://github.com/deephaven/web-client-ui/blob/75fe9c11ee23e5fc59a1160750a28ae6c3f047b5/package.json#L96
There was a problem hiding this comment.
| hasMap(): boolean { | ||
| return this.plotlyData.some(({ type }) => type?.includes('map')); | ||
| } |
There was a problem hiding this comment.
We should make all these has* methods private as well, since they're just used internally.
| "close": PLOTLY_NULL_INT, | ||
| "high": PLOTLY_NULL_INT, | ||
| "low": PLOTLY_NULL_INT, | ||
| "open": PLOTLY_NULL_INT, |
There was a problem hiding this comment.
This expects null now, when it expected a 2D array before?
There was a problem hiding this comment.
The 2D array was a subtle bug I noticed when going through and doing this PLOTLY_NULL_INT work. See the changes to the function in custom_draw. It should have been a null int array in the first place.
| "mapbox": { | ||
| "center": {"lat": NULL_INT, "lon": NULL_INT}, | ||
| "map": { | ||
| "center": {"lat": -2147483648.0, "lon": -2147483648.0}, |
There was a problem hiding this comment.
Does this actually make sense? How come it's getting these values?
There was a problem hiding this comment.
This should have been NULL_INT still, fixed.
As to why it is these values, it is because plotly sets the center to the mean of the values by default (see here)
Since the NULL_INT is the only data point passed to plotly for lat and lon, it become the center. It's then not converted to the base64 spec because it is not considered a data array.
| "showlegend": False, | ||
| "textposition": "auto", | ||
| "x": [0.0], | ||
| "x": {"bdata": "AA==", "dtype": "i1"}, |
There was a problem hiding this comment.
Where'd this magic value come from?
There was a problem hiding this comment.
This is the [0.0] value encoded in their base64 spec. I didn't create a variable for it because it is not reused.
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
Update as requested in deephaven/deephaven-plugins#1179
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
|
Some notes about packaging changes in my latest commits:
|
|
plotly-express docs preview (Available for 14 days) |
|
plotly-express docs preview (Available for 14 days) |
2 participants
Fixes #946
alignmentgroupandoffsetgroupin tests where they now do not need to be settitle=<str>withtitle_text=<str>as the former is deprecated