-
Notifications
You must be signed in to change notification settings - Fork 146
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
[Tidy] Remove setting of pio.templates.default
on import
#615
Conversation
…igure in capture decorator instead
for more information, see https://pre-commit.ci
…igure in capture decorator instead
Your PR just reminded me of an issue. Plotly Express charts come with their own color palettes and do not adopt the default color palettes specified in your template, even if you assign the template to the charts as you do in this PR. Consequently, removing the global theme has caused us to lose our default color palettes. For example, if you run the dev example, you'll notice that it defaults to Plotly's standard color palette.. 🙈 The only way to resolve this seems to be by setting the global theme based on the issue discussion above..or we try to set the color palettes in the charts again. (EDIT: I just tried that out, but it doesn't seem to work) |
@huong-li-nguyen yes, I just realised that this morning as well 🤦 😱 😞 😬 I have some more thoughts on this now, let us discuss! I am keen for this to not drag on indefinitely since it's blocking other stuff and eating up lots of time. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
First of all, incredible work! 🥳 💯 🚀
I’m genuinely impressed that you found a solution that addresses the notebook case, the dashboard case, and all the different user groups we've discussed. 🫠
I hope the added complexity is justified because even after our multiple discussions, I still needed all your comments to fully grasp what’s happening and why we’re doing things a certain way. Thanks for documenting everything so thoroughly!
I’ve manually tested all our demo dashboards and the following test cases, and everything works as expected! 🏁 ✅
Test Case | Action | Expected Result | ✅ |
---|---|---|---|
Dashboard with vizro_dark as the default theme |
Set vizro_dark as the default theme and switch themes. |
The initial theme should be vizro_dark . Switching themes should update the dashboard accordingly. |
✅ |
Dashboard with vizro_light as the default theme |
Set vizro_light as the default theme and switch themes. |
The initial theme should be vizro_light . Switching themes should update the dashboard accordingly. |
✅ |
Notebook with pure Plotly chart with no template | Import vizro and create plots using pure plotly or plotly.express . |
These plots should not have the vizro theme applied. |
✅ |
Notebook with pure Plotly chart with vizro_light/vizro_dark template |
Create a pure Plotly chart using template="vizro_light" or template="vizro_dark" . |
The chart should adopt the specified vizro theme. |
✅ |
Notebook with plots from vizro |
Create plots using vizro.plotly.express and any custom plots written with @capture . Additionally check template argument. |
These plots should have the vizro theme applied. The default theme is vizro_dark , but it can be changed using: - pio.templates.default = "vizro_light" - Setting template="vizro_light" in a plot - Setting fig.layout.template = "vizro_light" in a plot |
✅ |
Theme persistence in mixed plotting | Create a vizro plot followed by a pure Plotly plot. |
The vizro plot should have the vizro theme. The subsequent pure Plotly plot should not have the vizro theme applied, as the theme is not changed globally. |
✅ |
vizro-core/changelog.d/20240802_160643_antony.milne_no_more_global_theme.md
Show resolved
Hide resolved
pio.templates.default
on import
Thank you for the amazing testing @huong-li-nguyen! I'm so pleased it worked and also that it made some sense from all my comments. I spent so long thinking about this I didn't want to forget about it if we look at this code again in future. I've just updated the PR description - please can you read through and let me know if it makes sense and is consistent with your expectations also? Then I will add it to our dev docs, and a much more user-consumable version will go into the main docs in a separate PR. |
I read through it several times 😄, and it all makes sense to me! There is one tricky case where it might not be immediately clear what's happening. The good news is that this example works consistently in both the notebook and the dashboard, thanks to your workaround! 🥳 From a user perspective, I would wonder which template takes precedence. Given that I use @capture, the global theme should be set to
I'm not sure if it's worth explaining as this is already such an edge case (basically if someone wants to use a distinct chart template inside our Vizro dashboard). But based on your description above, my understanding is that if someone does want to change it, they would need to modify pio.templates["vizro_dark/light"] directly. So, to get the unlikely case below working, one would have to do: And this indeed works ✅. So if I understood everything correctly, even this edge case does work as I expected it? |
Yes, you understand things correctly, and this is actually maybe not even such an edge case if someone wants to modify several figures in the same way. In due course it will be explained in the docs. What happens here is:
The confusing thing is the PARTIAL above: it's not actually fully overriding the template in the way that you might expect. This is the thing I was talking about yesterday on slack and is a confusing feature of how plotly express work. Completely independently of vizro, if you do this then the results will not be what you expect:
This will give a plot that's a weird hybrid where the template is One thing that is sort of possible is to do:
The problem with this is that you ned to specify the right stuff to not move to the template using Soooo basically if you want to style your plot in a way that's not
|
@antonymilne - that makes a lot of sense! And yes, I had in mind that this partial ambiguity comes from px and how they do the overrides. I think we are all clear from my side then 👍 |
for more information, see https://pre-commit.ci
Copying and pasting even more on this so it's all in one place for posterity...
|
for more information, see https://pre-commit.ci
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.
Wow what an analysis you two, and I think I mostly understood it although I didn't test it manually because Li has already done so.
Well done 👏 🚀 I am super happy we achieved this and think I get it mostly. I have this one question where I am not sure what the contextmanager is trying to achieve, but happy to approve!
for more information, see https://pre-commit.ci
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.
Awesome work @antonymilne and testing/investigation @huong-li-nguyen 🎉 💜
This solution is great! (and I think I understand all the changes 😄)
Thanks for the reviews all. @petar-qb your questions made me wonder something else which I'm going to document here since it didn't make sense to me (and still doesn't fully tbh, but it seems to work). When we do There's several ways you can update a figure's template:
What's clear from reading through the source code is that 1 and 2 are identical. It's not super clear whether 2, 3, and 4 are identical. There's maybe some subtle differences but not sure whether they make any difference in reality. The most important thing is that the
Method 3 that we're doing here seems to also act as if |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jo Stichbury <[email protected]>
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.
Same as before? LGTM! ⭐
for more information, see https://pre-commit.ci
Description
After a lot of work, everything should work almost exactly the same as before 😅
Dashboard: everything should be exactly the same as before.
Outside dashboard:
plotly.express
) should not have vizro themetemplate="vizro_light/vizro_dark"
if you wantvizro.plotly.express
and anything custom written in@capture
) should have vizro themevizro_dark
by default but can be changed usingpio.default.template = "vizro_light"
OR settingtemplate="vizro_light"
in a plot OR settingfig.layout.template = "vizro_light"
in a plottemplate="vizro_light"
and thentemplate="vizro_dark"
in the same notebook and it should respect those choicesHow does templating work?
On vizro import, we register plotly themes
vizro_dark
andvizro_light
, available aspio.templates["vizro_dark"]
andpio.templates["vizro_light"]
. The correct way to use these is:template="vizro_dark/light"
pio.templates["vizro_dark/light"]
Note the public API here is through
pio.templates
, NOT the underlyingvizro._themes
objects. If a user want to modify a template then the correct way to do it is to modifypio.templates["vizro_dark/light"]
, not the underlyingvizro._themes
objects. Whenever we consume the theme we take it frompio.templates
, notvizro._themes
. This ensures consistency with the plotly templating scheme and that user modifications are always applied correctly.On vizro import, nothing else happens to do with templates. We don't set a default plotly template, just register them.
In a dashboard
Dashboard.theme="vizro_dark/light"
, which then setspio.templates.default = dashboard.theme
inVizro.build
. This default template is never unset (even byVizro._reset
). Remember thisvizro_dark/light
template could have been modified by developer but it still has the same nameGraph.__call__
to execute the figure function to get ago.Figure
. Regardless of how this was produced, insideGraph.__call__
we take the user-specified templaterequired_template
and updatefig.layout.template = required_template
on the server before returning the graph. This is actually just a small optimisation to ensure there's no flickering on the frontend, which would happen if we relied on clientside callback for the initial graph to render in the correct themeupdate_graph_theme
runs to doPlotly.relayout
, which is basically equivalent tofig.layout.template = vizro_dark/light
. The Python graph function is not re-run (and shouldn't be, since doing so would be very slow just to switch some colours). Ideally this same clientside callback would also be used on initial graph rendering to set the theme, but that introduces a small flicker, hence setting it on the backend in advance alsoOutside a dashboard
Think "Jupyter notebook user" here, although there's nothing actually specific to Jupyter notebooks and we don't check to see if a Jupyter environment is active. The intention is that:
vizro_dark
unless the user is explicitly trying to dovizro_light
Plots that use
capture("graph)"
(could bevizro.plotly.express
, other builtinvizro
chart or custom user graph) work as follows.pio.templates.default = "vizro_dark"
unless it's already set tovizro_light
pio.templates.default
is reverted to whatever it was beforefig.layout.template = vizro_dark/light
to be exactly consistent with how the dashboard behaviour works. This ensures that if a user setstemplate=...
inside their custom function it gets overridden, just like it would on a dashboardTODO here:
+
- update ticketScreenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":