-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 legacy _raise and get_ordered use cases #1158
Comments
@scjody We can definitely bring back That said, every interface between plotly.py and Plotly.js ( |
@jonmmease It sounds like the missing What we're doing is roughly:
We then call If you have access to the streambed repo you can see the whole thing here: https://github.com/plotly/streambed/blob/570fe40e83c393e3f751f142651e341f6d0f4e1c/shelly/code_translator/translator.py#L40-L52 . This is part of the code translator, which provides code to create plotly.js figures in various languages. This is an important Chart Studio feature and we need to support existing plots in our system, many of which are full of errors; how hard would it be to bring back the plotly.py capabilities we're using? |
We can definitely add something back to handle this case, but I don't think it can exactly match the In v3 a We could add a It looks like How does that sound? |
Implementation outlineIn Add a The The val = validator.validate_coerce(val, skip_invalid=self._skip_invalid)
Other potential usesContext managerAdd on, a with fig.skip_invalid():
# No error raised inside context manager
fig.data[0].bogus = 23 Read from fileThis import plotly.io as pio
fig = pio.read_json('/path/to/fig.json', skip_invalid=True) |
@jonmmease Your proposed changes sound good to me - it sounds like the streambed code will be more straightforward with these changes even. I don't know enough about plotly.py internals to comment on the design details though. |
Great, I'll add these changes soon and I'll ping you on the PR. It would be great if someone from your side could try out these changes in streambed (probably early next week) and see if there's anything else you all need from plotly.py before upgrading. |
Adds a new BaseFigure method, to_ordered_dict. This builds and returns a representation of the figure as a nested structure of OrderedDict and list instances. The OrderedDict keys are sorted alphabetically. This makes it possible for library users to iterate over the nested structure of a figure in a deterministic order. This method takes the place of the Figure.get_ordered method in plotly.py < 3.0.0. See #1158 for some discussion * Added to_ordered_dict * Use setitem rather than setattr syntax in graph object constructors This has less indirection, and seems to work around a strange nose testing bug with frames.
@scjody I just merged #1162 and #1167. These will be released as part of 3.2.1. With these updates, the use-case of constructing a fig = go.Figure(json_fig, skip_invalid=True) Then, to convert a ordered_fig = fig.to_ordered_dict() Would someone from your team have time to test out a release candidate early next week? Due to a couple of other bug fixes, I'd like to publish the final 3.2.1 by mid-week if possible. Thanks! |
@scjody I just published 3.2.1rc1 to PyPI. If you all have time, give it a try and let me know if there's anything else that streambed needs. |
@jonmmease Thanks for the changes! I tested plotly.py master in streambed and most code translation cases work, but unfortunately the JSON case is broken. With the new code, https://github.com/plotly/streambed/blob/master/shelly/code_translator/translator.py#L67 fails as follows:
Would it be possible to return something JSON-serializable from |
Hi @scjody , it looks like something in streambed is trying to JSON serialize a To serialize a figure you should pass json.dumps(fig, cls=plotly.utils.PlotlyJSONEncoder) This was also the recommended way to serialize Figure in versions < 3, but it wasn't required because |
@jonmmease Good catch - I didn't notice we were operating on the original Everything looks fine to me in manual testing. Some automated tests fail because they expect exceptions to be raised due to invalid keys, so we'll need to update those. I opened a WIP PR at https://github.com/plotly/streambed/pull/11460. |
Great! I'm hoping to do the final release Friday morning, but let me know if you run into anything else. I'll leave this issue open until you give the 👍 . |
@scjody I'm about ready to release 3.2.1 final (which looks like it will be identical to 3.2.1rc1). Do things still look alright on your end? |
@jonmmease Unfortunately I don't have time to re-test this, but if nothing has changed I don't anticipate any issues. |
For what it's worth, I tested |
Hi @scjody , I was only referring to whether anything else had come up with the version that you were testing last time. Nothing has changed since then, and now 3.2.1 has been released to the wild. |
plotly.py raises a
ValueError
when a blank color property is found in the legend font (and possibly other places), rather than aPlotlyGraphObjectError
. We depend onPlotlyGraphObjectError
being raised in the streambed backend so that we can retry with_raise=False
if the user tries to operate on an invalid figure.Minimal code to reproduce the issue (Python 2.7.6, plotly.py 3.2.0):
Expected results:
PlotlyGraphObjectError
is raised.Actual results:
@jonmmease FYI
The text was updated successfully, but these errors were encountered: