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

PluginRegistry persists unregistered plugins #3619

Open
dangotbanned opened this issue Oct 1, 2024 · 0 comments
Open

PluginRegistry persists unregistered plugins #3619

dangotbanned opened this issue Oct 1, 2024 · 0 comments
Labels

Comments

@dangotbanned
Copy link
Member

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

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

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

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)

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

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.
Returns
-------
plugin: PluginType or None
The plugin that was registered or unregistered.
"""
if value is None:
return self._plugins.pop(name, None)

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

dangotbanned added a commit that referenced this issue Oct 1, 2024
The commented out assertion may be added later after resolving #3619
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant