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

Make it easier for downstream libraries to *safely* contribute themes #3586

Open
dangotbanned opened this issue Sep 10, 2024 · 6 comments
Open

Comments

@dangotbanned
Copy link
Member

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

themes.register(
"default",
lambda: {"config": {"view": {"continuousWidth": 300, "continuousHeight": 300}}},
)
themes.register(
"opaque",
lambda: {
"config": {
"background": "white",
"view": {"continuousWidth": 300, "continuousHeight": 300},
}
},
)
themes.register("none", dict)
for theme in VEGA_THEMES:
themes.register(theme, VegaTheme(theme))
themes.enable("default")

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.

from enum import IntEnum

class ThemePriority(IntEnum):
    USER = 1
    THIRD_PARTY = 2
    DEFAULT = 3 # alternatives: `ALTAIR`, `STANDARD`, `BUILTIN`

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:

  • User does not enable a theme, no changes from existing behavior
  • User enables a theme with ThemePriority.DEFAULT, no changes from existing behavior
  • User enables a theme with ThemePriority.USER, no changes from existing behavior
  • User disables a theme with ThemePriority.USER, falls back to the last enabled ThemePriority.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:

  • Third-party wants to enable their theme for the lifetime of ChartType(s) they produce?
    • Either strictly or as a preference only
  • User wants to opt-out of third-party contributions?
  • Multiple third-parties?

Related

Note

Originally posted by @dangotbanned in #3519 (comment)

Splitting this into a separate issue for visibility

Have you considered any alternative solutions?

@binste
Copy link
Contributor

binste commented Sep 11, 2024

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:

  • the ability to attach theme information directly to a chart. This configuration would need to be pushed to the top in layered and multi-view charts (see this comment for why). This has two advantages:
    • allows multiple third-party libraries to be imported and to provide theme configurations without conflicting
    • unrelated to this, but it would help with making Altair thread-safe. Right now, you can get into race conditions (in multi-threading) if you're in a with alt.themes.enable(..) block as it modifies global state. I'll write down some thoughts on this in a separate issue soon
  • a priority system allowing for third-party libraries to "suggest" a theme but users can transparently overwrite it
    • maybe this is optional as a third-party library could also just check, when creating the chart, if alt.themes.active == 'default' and then they would attach their own theme information directly to the chart object, else they leave it

@dangotbanned
Copy link
Member Author

unrelated to this, but it would help with making Altair thread-safe. Right now, you can get into race conditions (in multi-threading) if you're in a with alt.themes.enable(..) block as it modifies global state. I'll write down some thoughts on this in a separate issue soon

@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)

@dangotbanned
Copy link
Member Author

dangotbanned commented Sep 11, 2024

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.

Definitely agree @binste, this is the exact situation I'm hoping we can avoid, when I said

However, I think this behavior itself should be pluggable - to support alternative resolution semantics like ...

  • User wants to opt-out of third-party contributions?

So maybe whatever the solution is, we make sure it is opt-in?
Third-parties using this feature can always mention this step in their docs; which is simple enough


The ability to attach theme information directly to a chart.

I'm interested!

This configuration would need to be pushed to the top in layered and multi-view charts (see this comment for why)

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.

usermeta: Optional metadata that will be passed to Vega. This object is completely ignored by Vega and Vega-Lite and can be used for custom metadata.

I imagine storing the registered name/callback here - rather than the config (dict) itself - would make this a simpler operation.
We could just defer a lot of the work/decisions until to_dict?

@binste
Copy link
Contributor

binste commented Sep 16, 2024

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 to_dict. Proposal:

  • A new theme attribute on a Chart which takes either:
    • registered name
    • callback function
    • a dictionary looking something like this:
class ThemeInfo(TypedDict):
   theme: str | Callback[[], dict]
   priority: Literal[1, 2, 3]
  • If Chart.theme is a string or callback, it's always used.
  • If it's a ThemeInfo, the prioritization happens as you described it further above. Themes enabled with alt.themes.enable() always have prio 1, so a library such as Polars would use prio 2. default theme has prio 3 except if it was enabled specifically via alt.themes.enable. -> theme="my_theme" and theme={"theme": "my_theme", "priority": 1} should be equivalent
  • This attribute needs to be consolidated and lifted to the top level whenever a layered or multi-view chart is created. The existing implementations for other attributes of LayerChart, HConcatChart, etc. can serve as templates.
  • Once to_dict is called, it assigns the theme based on the prioritization.

We could use usermeta instead of a new attribute and then remove again this information in to_dict but having a new attribute for this seems more explicit to me. Thoughts?

@dangotbanned
Copy link
Member Author

Appreciate you taking the time to put this all together @binste

@binste

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 to_dict. Proposal:

  • A new theme attribute on a Chart which takes either:
    • registered name
    • callback function
    • a dictionary looking something like this:

@dangotbanned

I imagine storing the registered name/callback here - rather than the config (dict) itself - would make this a simpler operation.

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 Chart.(theme|metadata) for a moment ...

I was thinking more along the lines of setting this via with alt.theme.enable(): ...

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 enabled

For 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 def

Just 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 polars could use

class DataFramePlot:
    """DataFrame.plot namespace."""

    def __init__(self, df: DataFrame) -> None:
        import altair as alt
		with alt.themes.enable("polars", ThemePriority.THIRD_PARTY):
			self._chart = alt.Chart(df)

    def point(
        self,
        x: X | None = None,
        y: Y | None = None,
        color: Color | None = None,
        size: Size | None = None,
        /,
        **kwargs: Unpack[EncodeKwds],
    ) -> alt.Chart:
		...
		return (
    		self.chart.mark_point()
        	.encode(*args, **{**encodings, **kwargs})
        	.interactive()
    	)

The behavior when a theme is enabled, would be the same as what you described for setting via the constructor; but this would apply for all charts defined within the with:... block.


class ThemeInfo(TypedDict):
   theme: str | Callback[[], dict]
   priority: Literal[1, 2, 3]
  • If Chart.theme is a string or callback, it's always used.

  • If it's a ThemeInfo, the prioritization happens as you described it further above. Themes enabled with alt.themes.enable() always have prio 1, so a library such as Polars would use prio 2. default theme has prio 3 except if it was enabled specifically via alt.themes.enable. -> theme="my_theme" and theme={"theme": "my_theme", "priority": 1} should be equivalent

I'm not sure if we'd need all of this if we just set during alt.themes.enable() - maybe we'd handle some of the logic here first?

Screenshot

image


  • This attribute needs to be consolidated and lifted to the top level whenever a layered or multi-view chart is created. The existing implementations for other attributes of LayerChart, HConcatChart, etc. can serve as templates.

  • Once to_dict is called, it assigns the theme based on the prioritization.

Sounds good to me 👍

We could use usermeta instead of a new attribute and then remove again this information in to_dict but having a new attribute for this seems more explicit to me. Thoughts?

A new attribute would definitely be more explicit.
The downside though (that I think you've hinted at?) is that we'd need to remove that attribute from the spec before validation.

If I've understood usermeta correctly - we could use a TypedDict here including the theme name and that could be rendered into the spec?

Not sure how many people are inspecting the resulting vega-lite, but having the name of the theme that was used seems helpful to persist to me


I'm happy with where this is going, seems like there are a few different ideas for us to test out!

Once #3536 is merged (to avoid conflicts) I'll open a draft PR so we can experiment further

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants