-
Notifications
You must be signed in to change notification settings - Fork 793
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
Make it easier for downstream libraries to *safely* contribute themes #3586
Comments
The priority system is an interesting idea! I think it gets tricky once, as you mentioned, there are multiple third-party libraries. Also, if for example I'd import polars, I don't want as a side-effect to have all my other charts change their theme. How about a combination of:
|
@binste thanks for the thoughts! Will follow up with a proper (on topic) response, but this made me realise the link I gave didn't mention thread safety 🤦♂️ but it is there at the top of the page https://docs.python.org/3/library/queue.html#module-queue I was thinking about this in relation to #3416 (comment) |
Definitely agree @binste, this is the exact situation I'm hoping we can avoid, when I said
So maybe whatever the solution is, we make sure it is opt-in?
I'm interested!
This was my final link in the description! 😉 usermeta seems like the obvious place to me since all charts have it and it is ignored.
I imagine storing the registered name/callback here - rather than the config ( |
I think we're on a good track here with adding the information to a chart, implementing a prioritization system, and making the final decision on which theme is used in
class ThemeInfo(TypedDict):
theme: str | Callback[[], dict]
priority: Literal[1, 2, 3]
We could use |
Appreciate you taking the time to put this all together @binste
I feel I've muddied things here with my spitballing. This all becomes much simpler if we only used registered name to get the callable from the registry when needed; without adding a new place that a user can declare a theme. So if we just set aside the I was thinking more along the lines of setting this via I did link comment, but IMO the current behavior of this context manager is surprising. Both uses here are effectively no-ops at the moment: No theme enabledFor the reasons described in pola-rs/polars#17995 (comment) with alt.themes.enable('ggplot2'):
return (
self.chart.mark_point()
.encode(*args, **{**encodings, **kwargs})
.interactive()
) with alt.themes.enable('ggplot2'):
chart = self.chart.mark_point().encode(*args, **{**encodings, **kwargs}).interactive()
return chart To me, the context manager seems like the more natural place for this - since we're setting global config. ThemePriority defJust adding here to save the scroll back to description from enum import IntEnum
class ThemePriority(IntEnum):
USER = 1
THIRD_PARTY = 2
DEFAULT = 3 # alternatives: `ALTAIR`, `STANDARD`, `BUILTIN` How
|
So I've just discovered Very new to this but thought the usage of
|
What is your suggestion?
Originally posted by @dangotbanned in discussion w/ @MarcoGorelli
As I understand, the
alt.Chart.configure_
calls are being used to avoid registering + enabling a theme - which could override a user's custom theme.These work fine in isolation, but AFAIK would have issues if a user were to layer/concat/facet the result - since
config
is only valid at the top-level.You might want to add tests to see if these ops would still be possible
Using a theme would have the benefit of deferring these config settings until the
Chart
is rendered - placing them in the top-level only.It might be worth seeing if we can come to a good solution to this as part of #3519 since we have already discussed issues with the theme route
Problem
A library like
polars
may wish to provide a default theme, but not override a user-defined or user-enabled theme.AFAIK, the "best" solution for this right now would be to override our
"default"
theme.However, this would be a destructive action and wouldn't scale well to multiple 3rd-parties each doing so:
Code block
altair/altair/vegalite/v5/theme.py
Lines 56 to 74 in df14929
Solution(s)
We could extend
ThemeRegistry
to support priority levels.Either when registering/enabling a theme a level will be set corresponding to the party.
For backwards-compatibility, this must default to
ThemePriority.USER
in any signatures the argument can be passed in from.All themes defined/registered in https://github.com/vega/altair/blob/df14929075b45233126f4cfe579c139e0b7f0559/altair/vegalite/v5/theme.py will be assigned
ThemePriority.DEFAULT
.The semantics of which theme should be enabled for
ThemePriority.(USER|DEFAULT)
are quite simple.The highest priority (lowest-valued) enabled theme is selected:
ThemePriority.DEFAULT
, no changes from existing behaviorThemePriority.USER
, no changes from existing behaviorThemePriority.USER
, falls back to the last enabledThemePriority.DEFAULT
The basic resolution implementation for
ThemePriority.THIRD_PARTY
would be identical to the above.Simply a way for 3rd-parties to opt-in for a way to safely be used instead of the defaults - but not over user themes.
However, I think this behavior itself should be pluggable - to support alternative resolution semantics like:
ChartType
(s) they produce?Related
Note
Originally posted by @dangotbanned in #3519 (comment)
Splitting this into a separate issue for visibility
Have you considered any alternative solutions?
The text was updated successfully, but these errors were encountered: