Consolidate: alt.theme(s), alt.typing.theme, alt.vegalite.v5.theme, alt.utils.theme

[dangotbanned]

What is your suggestion?

The following related PRs have been merged since v5.4.1 release:

• feat: Adds @register_theme decorator #3526
• feat: Adds ThemeConfig (TypedDict) #3536

This issue is the continuation of the discussion below:

Originally posted by @dangotbanned in post review comment

...
In #3536 (comment) I mentioned some related issues, but can now see I forgot to elaborate on what I would prefer for v6.

I would like to have used altair.theme as the namespace instead of altair.typing.theme.
However, this would be very easy to mistake for altair.themes - which is a variable storing a registry of themes.

For v6 I would propose the following api changes - and if there is any support I will open a new issue:

• altair.typing.theme -> altair.theme
  • Including dropping altair.typing.ThemeConfig, which is already included in the above
• @altair.register_theme -> @altair.theme.register
• New function: altair.theme.enable
• Remove altair.themes top-level export

This would consolidate all theme related functionality behind a single namespace:

from altair import theme

@theme.register("custom 1", enable=False)
def custom_theme() -> theme.ThemeConfig:
    return {
        "autosize": {"contains": "content", "resize": True}, 
        "config": theme.ConfigKwds(
            circle=theme.MarkConfigKwds(fill="purple")
        ),
    }

...

theme.enable("custom 1")
...
theme.enable("default")

Response by @mattijn in comment

...
I like the suggestion you provide in #3536 (comment) to consolidate everything under a single namespace, including the proposed ThemeConfig.
I’m fine with doing that now, rather than releasing a version first and requiring deprecation immediately after.

However, instead of phasing out the plural altair.themes in favour of the singleton altair.theme can we consolidate this PR's work into the existing altair.themes?
For example, can we have altair.themes.ThemeConfig (and altair.themes.AxisConfigKwds, etc.) aligning with the existing altair.themes.register() and altair.themes.enable().

Aesthetically I like the singleton altair.theme, but I think sticking with altair.themes is more practical, if that is also an option. Can you reflect on this?

Proposing this issue itself

---

Also tagging @binste, as our discussion in another issue probably needs consideration

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

---

Notes

• alt.vegalite.v5.theme, alt.utils.theme were not mentioned in the discussion - stating them here for completeness

Edit

@mattijn apologies for the delay, but see #3610 (comment) for my thoughts

[dangotbanned]

alt.themes

Aesthetically I like the singleton altair.theme, but I think sticking with altair.themes is more practical, if that is also an option. Can you reflect on this?

@mattijn putting aesthetics aside, I see two key issues with using alt.themes.

Instance vs module

As I mentioned in #3536 (comment) and #3536 (comment) alt.themes is an instance of ThemeRegistry and PluginRegistry.
We could support usage like below, in a few different ways:

How?

• Adding a ThemeRegistry.__getattr__
• Adding the classes as properties of ThemeRegistry
• Patching the classes onto the instance themes

from altair import themes

themes.register
themes.ThemeConfig
themes.MarkConfigKwds
...

But unless we get particularly creative with importlib, we couldn't support:

from altair.themes import ThemeConfig, MarkConfigKwds, ...

I imagine if we treated themes like it is a module, people would try this to reduce verbosity and be surprised by the import error.

Making alt.themes a module?

Would solve the import issues.

I think we could recreate most of the existing behavior, using a mixture of functions and a module-level __getattr__.

The two things I can't see a solution for are:

• __repr__
• isinstance() or other builtins

alt.themes.register

The inherited signature uses None to unregister a plugin:

PluginRegistry.register()

altair/altair/utils/plugin_registry.py

Lines 135 to 147 in 02e8c34

	def register(self, name: str, value: PluginT | None) -> PluginT | None:
	"""
	Register a plugin by name and value.
	This method is used for explicit registration of a plugin and shouldn't be
	used to manage entry point managed plugins, which are auto-loaded.
	Parameters
	----------
	name: str
	The name of the plugin.
	value: PluginType or None
	The actual plugin object to register or None to unregister that plugin.

Since this is not a default, we could probably work around this by adding some sentinel as a default for @alt.themes.register().
The source for @dataclasses.dataclass() has an example of dual usage (function and decorator).

I'm fairly sure the issues around this wouldn't be the technical aspect, mostly just a bit of extra complexity for the user and more explanation in the docs.

---

alt.theme

I really do think it would be simpler to start fresh with a new namespace, without any additional baggage.

• Decide what we want to expose under alt.theme.___
• Populate alt.theme
• Update docs to use alt.theme
• Deprecate alt.themes and then remove in v6
  • We could add a alt.__init__.__getattr__ to hide from autocomplete

We could also be sure that this wouldn't change the meaning of existing user code.
A deprecation warning can always be ignored for those determined to still use alt.themes

[mattijn]

Thanks for the detailed description! I’m good with your suggestion to start with alt.theme. While it may not become or serve as a theme or plugin registry, I’m still hopeful we can achieve a similar user experience to the current alt.themes (even if it’s just wishful thinking😄)

[dangotbanned]

Thanks for the detailed description! I’m good with your suggestion to start with alt.theme. While it may not become or serve as a theme or plugin registry, I’m still hopeful we can achieve a similar user experience to the current alt.themes (even if it’s just wishful thinking😄)

No problem @mattijn, thanks I'll try to work on this soon.

We can always have themes accessible via alt.theme.themes - if you'd see a benefit?

[mattijn]

Based on your work in PR #3618 (approaching from a fresh perspective, without considering legacy code) and your findings in issue #3586, could PR #3618 potentially serve as a drop-in replacement for the current alt.themes? What functionality would we lose, and would that actually impact detected real-world use cases (as found by sourcegraph)?

[dangotbanned]

Based on your work in PR #3618 (approaching from a fresh perspective, without considering legacy code) and your findings in issue #3586, could PR #3618 potentially serve as a drop-in replacement for the current alt.themes? What functionality would we lose, and would that actually impact detected real-world use cases (as found by sourcegraph)?

Thanks @mattijn

New search for all usage of alt.themes. sourcegraph got 45 results.

Note

I'll try to put together a short analysis/visualization of this usage soon

#3618 Functional Changes

• alt.themes works exactly the same, but provides a warning when used
• themes is accessible without a warning via alt.theme.themes and existing use of alt.themes points you there
• alt.themes.(enable|get|names) methods have drop-in replacements via alt.theme.___
• alt.themes.register
  • Drop-in usage would be alt.theme.themes.register
  • Recommended for new code
    • To register @alt.theme.register
    • To unregister alt.theme.unregister

Currently no direct alt.theme.___ replacement

These can still be accessed via alt.theme.themes.___.

• alt.themes.(active|options)

It would probably be possible to add these, but since they use @property it may be more difficult to document them.

Update

By the end of writing this I'd talked myself into adding support for alt.themes.(active|options)
See #3618 (comment) for updated preview of API Reference

Diffs showing these changes applied internally

• https://github.com/vega/altair/pull/3618/files#diff-788dae7ce1ea103a8d4c7d0e4f4af13a3e3df447ed1d01f2a9ea52a11bfab417
• https://github.com/vega/altair/pull/3618/files#diff-e52efec94b8b7f0f1b27eac85385173dacfaca37d65afd0df9839e09e58bf377
• https://github.com/vega/altair/pull/3618/files#diff-a0ccf73ccb98eab8693e5522b707705def8cc5bbb5f73f77aab30e2497794b40

[dangotbanned]

@mattijn I'm thinking about making alt.theme.themes -> alt.theme._themes.

As of #3618 (commits) everything has a replacement in alt.theme already.

With the original registry still accessible (w/ deprecation alt.themes) - I can't see having it available here as well being anything more than a point of confusion