plugins: ensure to log the offending plugin on instantiation failure #5739

wisp3rwind · 2025-04-17T16:18:06Z

Came up in #5701 (comment) where an error can't easily be traced back to a plugin.

(Draft because I want to avoid creating merge conflicts for #5701 before that is merged~~, and because I might a few additional improvements here~~.)

github-actions · 2025-04-17T16:18:24Z

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

beets/plugins.py

github-actions · 2025-04-20T09:01:40Z

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

wisp3rwind · 2025-04-20T09:02:40Z

This should be ready to go now; it's easiest to review commit-by-commit.

minimize how much code is wrapped in a given try...except statement

…roken error handling from a quick test, it appears that the old code shouldn't even be working anymore due to additional quotes in exc.args[0]

also remove a unnecessary check before adding to a set

without an argument (i.e. the default `names=()`), this was a no-op actual plugin loading is triggered by tests, or by the ui code -> Turns out it's not an actual no-op in our testing code due to patching with attaching some side-effects to load_plugins... This is fixed up in the next commit.

Instead of the first call to find_plugins(). This could change behavior slightly, since plugin load (and errors that happen in the process) could occur earlier now. But: - I'd argue this is better anyway, since plugin load errors now happen in the initial call to load_plugins() instead of somewhere in the middle of the first operation that uses plugins. - In our ui code, both happen immediately after each other anyway: ui._setup calls 1) ui._load_plugins -> plugins.load_plugins 2) plugins.types, plugins.named_queries, ...

snejus · 2025-06-16T19:42:10Z

beets/plugins.py

-                        _classes.add(obj)
-
-        except Exception:
+            mod = import_module(f".{name}", package=PLUGIN_NAMESPACE)


This may be slightly misleading in some cases, for example:

# beetsplug/discogs.py ... import beets.ibrary

$ beet import -LI sel ... ** plugin discogs not found

snejus · 2025-06-16T19:45:33Z

beets/plugins.py

-
-        except Exception:
+            mod = import_module(f".{name}", package=PLUGIN_NAMESPACE)
+        except ModuleNotFoundError:


Maybe we should catch a generic Exception here? Otherwise every other exception will make beets die, for example:

# beetsplug/discogs.py ... abc

$ beet import -LI sel ... Traceback (most recent call last): File "/home/sarunas/.local/bin/beet", line 5, in <module> main() File "/home/sarunas/repo/beets/beets/ui/__init__.py", line 1878, in main _raw_main(args) File "/home/sarunas/repo/beets/beets/ui/__init__.py", line 1853, in _raw_main subcommands, plugins, lib = _setup(options, lib) File "/home/sarunas/repo/beets/beets/ui/__init__.py", line 1680, in _setup plugins = _load_plugins(options, config) File "/home/sarunas/repo/beets/beets/ui/__init__.py", line 1669, in _load_plugins plugins.load_plugins(plugin_list) File "/home/sarunas/repo/beets/beets/plugins.py", line 358, in load_plugins mod = import_module(f".{name}", package=PLUGIN_NAMESPACE) File "/home/sarunas/.local/share/pyenv/versions/3.9.20/lib/python3.9/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "<frozen importlib._bootstrap>", line 1030, in _gcd_import File "<frozen importlib._bootstrap>", line 1007, in _find_and_load File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 680, in _load_unlocked File "<frozen importlib._bootstrap_external>", line 850, in exec_module File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed File "/home/sarunas/repo/beets/beetsplug/discogs.py", line 32, in <module> abc NameError: name 'abc' is not defined

snejus · 2025-06-16T19:46:58Z

beets/plugins.py

+                and cls != BeetsPlugin
+                and cls != MetadataSourcePlugin


Suggested change

and cls != BeetsPlugin

and cls != MetadataSourcePlugin

and cls not in {BeetsPlugin, MetadataSourcePlugin}

snejus · 2025-06-16T19:48:30Z

beets/plugins.py

+                issubclass(cls, BeetsPlugin)
+                and cls != BeetsPlugin
+                and cls != MetadataSourcePlugin
+                and cls not in _instances


Do we need to perform this check? It's unlikely we will come across dupes, and even if we do, classes is defined as a set, so no dupes are allowed.

snejus · 2025-06-16T19:50:04Z

beets/plugins.py

+    if cls in _instances:
+        return
+


I doubt this check is required

Suggested change

if cls in _instances:

return

snejus · 2025-06-16T19:51:23Z

beets/plugins.py

+                and cls != MetadataSourcePlugin
+                and cls not in _instances
+            ):
+                classes.add(cls)


Do we need to store classes in this intermediate set? Can't we just _register_plugin here?

snejus · 2025-06-16T19:52:09Z

beets/plugins.py

-    """Returns a list of BeetsPlugin subclass instances from all
-    currently loaded beets plugins. Loads the default plugin set
-    first.
+def find_plugins() -> Iterator[BeetsPlugin]:


Why are we returning an iterator here instead of the list?

wisp3rwind mentioned this pull request Apr 17, 2025

Added typehints to the plugins file. #5701

Merged

1 task

snejus reviewed Apr 18, 2025

View reviewed changes

beets/plugins.py Outdated Show resolved Hide resolved

beets/plugins.py Outdated Show resolved Hide resolved

beets/plugins.py Outdated Show resolved Hide resolved

beets/plugins.py Outdated Show resolved Hide resolved

wisp3rwind force-pushed the plugin-traceback branch from 3a4a4b7 to db32fa1 Compare April 18, 2025 11:28

wisp3rwind mentioned this pull request Apr 18, 2025

plugins: Should beets exit if plugins are not found or fail to load? #5740

Open

wisp3rwind force-pushed the plugin-traceback branch 5 times, most recently from bde4397 to 2129eb4 Compare April 20, 2025 08:43

wisp3rwind marked this pull request as ready for review April 20, 2025 09:01

wisp3rwind force-pushed the plugin-traceback branch from 2129eb4 to cc877e5 Compare April 20, 2025 09:06

wisp3rwind added the refactor label May 20, 2025

wisp3rwind added 6 commits May 20, 2025 11:09

plugins: ensure to log the offending plugin on instantiation failure

e4749d3

plugins: flatten loading code

d977b6c

minimize how much code is wrapped in a given try...except statement

plugins: catch the "new" ModuleNotFoundError instead of old hacky & b…

37e2821

…roken error handling from a quick test, it appears that the old code shouldn't even be working anymore due to additional quotes in exc.args[0]

plugins: use importlib and inspect instead __import__ and __dict__

371f6a8

also remove a unnecessary check before adding to a set

wisp3rwind force-pushed the plugin-traceback branch from cc877e5 to 8a862ba Compare May 20, 2025 09:14

snejus reviewed Jun 16, 2025

View reviewed changes

snejus mentioned this pull request Jun 18, 2025

Refactor of metadata plugin and opt in all metadata plugins to new baseclass #5787

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

plugins: ensure to log the offending plugin on instantiation failure #5739

plugins: ensure to log the offending plugin on instantiation failure #5739

Uh oh!

wisp3rwind commented Apr 17, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 20, 2025

Uh oh!

wisp3rwind commented Apr 20, 2025

Uh oh!

snejus Jun 16, 2025

Uh oh!

snejus Jun 16, 2025

Uh oh!

snejus Jun 16, 2025

Uh oh!

snejus Jun 16, 2025

Uh oh!

snejus Jun 16, 2025

Uh oh!

snejus Jun 16, 2025

Uh oh!

snejus Jun 16, 2025

Uh oh!

Uh oh!

	and cls != BeetsPlugin
	and cls != MetadataSourcePlugin
	and cls not in {BeetsPlugin, MetadataSourcePlugin}

plugins: ensure to log the offending plugin on instantiation failure #5739

Are you sure you want to change the base?

plugins: ensure to log the offending plugin on instantiation failure #5739

Uh oh!

Conversation

wisp3rwind commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 20, 2025

Uh oh!

wisp3rwind commented Apr 20, 2025

Uh oh!

snejus Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

snejus Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

snejus Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

snejus Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

snejus Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

snejus Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

snejus Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wisp3rwind commented Apr 17, 2025 •

edited

Loading