vega / altair

Declarative statistical visualization library for Python
https://altair-viz.github.io/
BSD 3-Clause "New" or "Revised" License
9.39k stars 795 forks source link

`PluginRegistry` persists unregistered plugins #3619

Open dangotbanned opened 1 month ago

dangotbanned commented 1 month ago

What happened?

Discovered this while writing a test for alt.theme.unregister in #3618

Test code

Haven't pushed this yet on #3618, but I was surprised the assertion didn't hold ```py def test_theme_unregister() -> None: @theme.register("big square", enable=True) def custom_theme() -> ThemeConfig: return {"height": 1000, "width": 1000} assert theme.active == "big square" fn = theme.unregister("big square") assert fn is not None assert fn() == custom_theme() assert theme.active == theme.themes.active assert theme.active != "big square" FAILED tests/vegalite/v5/test_theme.py::test_theme_unregister - AssertionError: assert 'big square' != 'big square' ```

General repro

I'm using ThemeRegistry in this example, but this would apply to all other PluginRegistry subclasses.

All expected behavior

```py import altair as alt alt.themes.active # 'default' def theme_1() -> Any: return {"height": 1000, "width": 1000} alt.themes.register("theme 1", theme_1) alt.themes.register("theme 2", lambda: {"height": 500, "width": 500}) {"theme 1", "theme 2"} < set(alt.themes.names()) # True alt.themes.enable("theme 1") alt.themes.active # 'theme 1' unregistered = alt.themes.register("theme 1", None) unregistered is not None # True unregistered() # {'height': 1000, 'width': 1000} ```

Questionable behavior

```py alt.themes.active # 'theme 1' "theme 1" not in alt.themes.names() # True alt.Chart("data.csv").mark_bar().encode(x="x:Q").to_dict() {'height': 1000, 'width': 1000, 'data': {'url': 'data.csv'}, 'mark': {'type': 'bar'}, 'encoding': {'x': {'field': 'x', 'type': 'quantitative'}}, '$schema': 'https://vega.github.io/schema/vega-lite/v5.20.1.json'} active_theme = alt.themes.get() active_theme() == unregistered() # True ```

I guess it could be argued that un/register is not the same as enable/disable.

However the following assumes that the active plugin is registered.

alt.themes.enable()
Traceback (very long)

```py --------------------------------------------------------------------------- ValueError Traceback (most recent call last) File c:/Users/danie/Documents/GitHub/altair/altair/utils/plugin_registry.py:198, in PluginRegistry._enable(self, name, **options) 197 try: --> 198 (ep,) = ( 199 ep 200 for ep in importlib_metadata_get(self.entry_point_group) 201 if ep.name == name 202 ) 203 except ValueError as err: ValueError: not enough values to unpack (expected 1, got 0) The above exception was the direct cause of the following exception: NoSuchEntryPoint Traceback (most recent call last) Cell In[21], line 5 3 with warnings.catch_warnings(): 4 warnings.filterwarnings("ignore", category=alt.AltairDeprecationWarning) ----> 5 alt.themes.enable() File c:/Users/danie/Documents/GitHub/altair/altair/vegalite/v5/theme.py:61, in ThemeRegistry.enable(self, name, **options) 33 def enable( 34 self, 35 name: LiteralString | AltairThemes | VegaThemes | None = None, 36 **options: Any, 37 ) -> PluginEnabler[Plugin[ThemeConfig], ThemeConfig]: 38 """ 39 Enable a theme by name. 40 (...) 59 Default `vega` themes can be previewed at https://vega.github.io/vega-themes/ 60 """ ---> 61 return super().enable(name, **options) File c:/Users/danie/Documents/GitHub/altair/altair/utils/plugin_registry.py:240, in PluginRegistry.enable(self, name, **options) 238 if name is None: 239 name = self.active --> 240 return PluginEnabler(self, name, **options) File c:/Users/danie/Documents/GitHub/altair/altair/utils/plugin_registry.py:61, in PluginEnabler.__init__(self, registry, name, **options) 59 self.options: dict[str, Any] = options 60 self.original_state: dict[str, Any] = registry._get_state() ---> 61 self.registry._enable(name, **options) File c:/Users/danie/Documents/GitHub/altair/altair/utils/plugin_registry.py:207, in PluginRegistry._enable(self, name, **options) 205 raise ValueError(self.entrypoint_err_messages[name]) from err 206 else: --> 207 raise NoSuchEntryPoint(self.entry_point_group, name) from err 208 value = cast(PluginT, ep.load()) 209 self.register(name, value) NoSuchEntryPoint: No 'theme 1' entry point found in group 'altair.vegalite.v5.theme' ```

This error isn't useful, and would point the user in the wrong direction of solving the bug.

What would you like to happen instead?

I think the most intuitive option would be to reset the active plugin to the default.

I expected this would already happen as part of the call to PluginRegistry.register

https://github.com/vega/altair/blob/cabf1e6428bab107a26adc6c51993d3b2c5bf6f0/altair/utils/plugin_registry.py#L135-L155

Is there a use case I'm not seeing; where this behavior would be desirable?

Which version of Altair are you using?

5.5.0dev