Repository navigation
Conversation
camdecoster
changed the title
|
Hey @seshan18 Thanks again! |
|
Hey @KoolADE85 , when I tried to ran the ruff check it shows around 5000 error which is not from my piece of code . Would I want to fix that issue ? And Does I need to add what I changed in the code as a summary in the CHANGELOG.md or it can automatically been added when my pr merges . |
KoolADE85
left a comment
KoolADE85
left a comment
There was a problem hiding this comment.
Thanks @seshan18 - this does fix #5689 (and #5309!) as reported. We can't merge it in this form however since it introduces other regressions I've put inline.
Further, we will need some form of regression test, whatever shape the fix ends up taking. There's too much tricky code here to avoid tests.
For ruff, you probably need to ensure you're using the same version as the project uses:
uv sync --locked --extra dev_core
uv run ruff format --check .
| # Invalidate cache for changed compound array properties so that they are | ||
| # reconstructed from the underlying properties dictionary on the next access. | ||
| # We only pop from _compound_array_props because compound arrays are immutable | ||
| # tuples that must be rebuilt to reflect added or removed elements. | ||
| for path in changed_paths: | ||
| if len(path) > 0: | ||
| prop = path[0] | ||
| if prop in self._compound_array_props: | ||
| self._compound_array_props.pop(prop, None) |
There was a problem hiding this comment.
This dict isn't only a cache, it's also used for lookups by reference (as in here). Popping an entry here will break downstream:
fig = go.Figure()
fig.update_layout(shapes=[dict(type="rect", x0=0, x1=1, y0=0, y1=1)])
roi = fig.layout.shapes[0]
fig.plotly_relayout({"shapes[0].x0": 42}) # some update, e.g. from the frontend
roi.x0 # ValueError raised! roi is the object the dict was holding — and since you popped it out, roi can no longer find itself in the figure when asked for .x0
Another note: PR #5691 was recently merged to make concurrent reads of this dict safe. Modifying the dict here conflicts with that goal. And in any case, I'm not sure this is the right place to invalidate a cache as it's about notifying callbacks.
This branch has not been deployed
3 participants
Link to issue
Closes #5689
Description of change
This PR fixes a state synchronization bug in FigureWidget where updating compound array properties (like layout.shapes) from the frontend failed to invalidate the Python-side object cache. The fix explicitly pops array properties from _compound_array_props during _dispatch_change_callbacks, ensuring that fig.layout.shapes is accurately rebuilt from the underlying dictionary without surfacing raw Undefined () singletons.
Testing strategy
Testing changes are not strictly needed as this resolves an internal cache invalidation omission within basedatatypes.py. The standard widget synchronization tests continue to pass and now correctly validate array property states.
Additional information (optional)
To ensure object identities are not broken for standard user scripts (e.g., a user holding a reference to ax = fig.layout.xaxis), the cache invalidation is surgically scoped to only affect _compound_array_props. Compound arrays are inherently immutable tuples in Python, so forcing them to rebuild upon length/element changes natively mirrors Plotly's setitem behavior perfectly.
Guidelines
[x] I have reviewed the pull request guidelines and the Code of Conduct and confirm that this PR follows them.
[x] I have added an entry to the changelog if needed (not required for documentation PRs).