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

Replace global validate state with Figure level validate flag #2403

Merged
merged 9 commits into from
Apr 28, 2020

Conversation

jonmmease
Copy link
Contributor

This PR removes the global graph_objects.validate class. This class was introduce in #2368 as a way to globally disable figure property validation. While testing master, it seems that this global validation state can result in race conditions. Fortunately, this class was never included in a plotly.py release so we can still change the design.

This PR replaces that approach with Figure-level _validate property. Validation for a Figure (or other graph object) can be disabled by passing a validate=False kwarg.

cc @nicolaskruchten

@nicolaskruchten
Copy link
Contributor

@jonmmease do we lose the context manager approach in this case? how does _validate bubble through to the various sub-graph_objs in this case? Does it thread through update_* and add_* somehow?

@jonmmease
Copy link
Contributor Author

Yes, we do lose the context manager. It should propagate through update_*. I'm not sure about add_*. For the time being, I don't think I would recommend documenting/exposing this quite yet.

There's more I would want to do testing wise before advertising in. In particular, we should do all of the figure factory and px tests with validation disabled and make sure they all work properly.

@nicolaskruchten
Copy link
Contributor

I don't think I would recommend documenting/exposing this quite yet

Roger that.

There's something else I don't understand, about the bug that I hit... The symptom I saw was that PX was producing figures with invalid legend.titles (basically legend.title="string" rather than legend.title.text="string)... Somehow this has always been translated before rendering but due to this race condition it didn't happen. What else falls into this class of "won't happen when validation is off"? Can I still do .update_layout(legend_title_text="string") ?

@nicolaskruchten
Copy link
Contributor

BTW I plan to merge #2402 to ensure that PX isn't relying on magical title -> title.text translations but I'm not sure what else I'm missing :) I guess some of this is the kind of thing that would crop up if running the tests with no validation? In this particular instance, nothing would have failed at the Python layer, but at the render layer it would have: Plotly.js doesn't show a legend title if legend.title = "string"

@jonmmease
Copy link
Contributor Author

Somehow this has always been translated before rendering but due to this race condition it didn't happen. What else falls into this class of "won't happen when validation is off"? Can I still do .update_layout(legend_title_text="string") ?

This exact thing will only happen with the title* family of properties. The special logic to convert title->title.text and titlefont -> title.font. Right now, these are the only properties that we map for backward compatibility.

Basically, no coercions will take place. So things like pandas Series won't be converted to numpy arrays, they'll stay in the figure as Series. PIL images won't be converted to base64 encoded png strings. Colorscales won't be auto converted from names or lists into plotly.js form.

@nicolaskruchten
Copy link
Contributor

OK so turning validation off means we need to be a lot more careful, and for instance PX figures will not work all that well unless we produce only figures that don't need any kind of coercion, which is 100% not the case right now.

In light of this, do we even want to introduce validate=False right now? None of the performance improvements in 4.7 depend on it any more, or do we still actually use it now?

@jonmmease
Copy link
Contributor Author

In light of this, do we even want to introduce validate=False right now? None of the performance improvements in 4.7 depend on it any more, or do we still actually use it now?

We use it internally to handle loading/creating Templates without importing all of the layout and trace validators. This has a substantial performance benefit when creating the first figure of a session. Maybe we should rename the argument to _validate though.

@nicolaskruchten
Copy link
Contributor

Got it. Then I would favour _validate, yes :)

@nicolaskruchten
Copy link
Contributor

@jonmmease could you append a little changelog item for this whole performance thing BTW please ? :)

@jonmmease
Copy link
Contributor Author

Done and done @nicolaskruchten, thanks for the review. Go ahead and merge when you're ready!

@nicolaskruchten
Copy link
Contributor

Thanks very much!

@nicolaskruchten nicolaskruchten merged commit e882f85 into master Apr 28, 2020
@nicolaskruchten nicolaskruchten deleted the figure_level_validate branch June 19, 2020 16:17
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.

None yet

2 participants