Fixed issue with plugins not loaded if multiple plugins are exported in a plugins file #6034
Conversation
Reviewer's GuideEnhance plugin loading by honoring a module’s all for class discovery with fallback to full module, introduce a clear fail-fast error when multiple BeetsPlugin subclasses are exported, add explicit all definitions to chroma and bpsync plugins, and update the changelog. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression in plugin loading where plugins with multiple BeetsPlugin classes could not be loaded properly. The fix introduces support for the __all__ module attribute to explicitly control which plugin classes are exposed, preventing conflicts from helper or internal classes.
Key changes:
- Enhanced plugin loading logic to respect
__all__exports when present - Added validation to prevent multiple plugin classes from being loaded from a single module
- Updated affected plugins (
chromaandbpsync) to explicitly export their main plugin classes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| beets/plugins.py | Modified _get_plugin function to check __all__ exports and validate single plugin class |
| beetsplug/chroma.py | Added __all__ to export only AcoustidPlugin |
| beetsplug/bpsync.py | Added __all__ to export only BPSyncPlugin |
| docs/changelog.rst | Added changelog entry documenting the bug fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
beets/plugins.py
Outdated
| exports = getattr(module, "__all__", module.__dict__) | ||
| members = {key: getattr(module, key) for key in exports} |
There was a problem hiding this comment.
When __all__ is a list of strings (the typical case), this code will fail because it tries to use the strings as dictionary keys against module.__dict__. The code should handle __all__ as an iterable of attribute names, not as a dictionary. Consider: members = {key: getattr(module, key) for key in (exports if hasattr(module, '__all__') else module.__dict__)}
| exports = getattr(module, "__all__", module.__dict__) | |
| members = {key: getattr(module, key) for key in exports} | |
| if hasattr(module, "__all__"): | |
| members = {key: getattr(module, key) for key in module.__all__} | |
| else: | |
| members = {key: getattr(module, key) for key in module.__dict__} |
There was a problem hiding this comment.
Seems to work like this for me... AI might be wrong here.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Before building the members dict from
__all__, validate that each entry is a string and actually exists on the module to avoid KeyError or AttributeError at runtime. - Consider replacing the
filter+lambdawith a concise list comprehension for collectingplugin_classesto improve readability and maintainability. - When falling back to scanning the full module (no
__all__), filter out private/internal names (e.g., starting with_) to reduce the risk of accidentally picking up helper classes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Before building the members dict from `__all__`, validate that each entry is a string and actually exists on the module to avoid KeyError or AttributeError at runtime.
- Consider replacing the `filter`+`lambda` with a concise list comprehension for collecting `plugin_classes` to improve readability and maintainability.
- When falling back to scanning the full module (no `__all__`), filter out private/internal names (e.g., starting with `_`) to reduce the risk of accidentally picking up helper classes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
semohr
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6034 +/- ##
==========================================
- Coverage 66.92% 66.73% -0.19%
==========================================
Files 117 119 +2
Lines 18133 18148 +15
Branches 3076 3076
==========================================
- Hits 12135 12111 -24
- Misses 5338 5378 +40
+ Partials 660 659 -1
🚀 New features to boost your workflow:
|
|
Does this not suffice? and issubclass(obj, BeetsPlugin)
and obj != BeetsPlugin
and not inspect.isabstract(obj)
+ and obj.__module__ == namespace.__name__ |
I was thinking about this too but I don't think this will work. Plugins which are spread across different files and are only exposed in an Haven't tested it tho, just thought about it quickly and disregarded it. I might be wrong here. |
>>> namespace = import_module("beetsplug.lastgenre")
>>> namespace.__name__
'beetsplug.lastgenre'
>>> namespace.LastGenrePlugin.__module__
'beetsplug.lastgenre'Notably, The full diff diff --git a/beets/plugins.py b/beets/plugins.py
index d8d465183..4751ac7f3 100644
--- a/beets/plugins.py
+++ b/beets/plugins.py
@@ -23,4 +23,5 @@
from collections import defaultdict
from functools import wraps
+from importlib import import_module
from pathlib import Path
from types import GenericAlias
@@ -366,9 +367,9 @@ def _get_plugin
try:
try:
- namespace = __import__(f"{PLUGIN_NAMESPACE}.{name}", None, None)
+ namespace = import_module(f"{PLUGIN_NAMESPACE}.{name}")
except Exception as exc:
raise PluginImportError(name) from exc
- for obj in getattr(namespace, name).__dict__.values():
+ for obj in namespace.__dict__.values():
if (
inspect.isclass(obj)
@@ -379,4 +380,5 @@ def _get_plugin
and obj != BeetsPlugin
and not inspect.isabstract(obj)
+ and namespace.__name__ == obj.__module__
):
return obj() |
|
In this specific case it works, but the more general case does not. If the plugin class is not defined directly in the For example: # __init__.py
from .foo import MyPlugin
__all__ = ["MyPlugin"]# foo.py
class MyPlugin:
passWhen loaded: namespace = importlib.import_module("beetsplug.test")
namespace.__name__
>>> 'beetsplug.test'
namespace.MyPlugin.__module__
>>> 'beetsplug.test.foo'This means that relying on A key issue is that there may be multiple plugin classes defined in different files or with inheritance relationships, and only a one should be exposed. For this reason, we should explicitly support and prioritize |
I don't think we need to be overly defensive here and account for such a use case, since it adds a fair bit of complexity. I haven't seen such an import in the wild, and our docs clearly specify that the plugin needs to be defined either in |
I'm doing this in the aisauce plugin 🥲 Edit:
|
I kinda think we do have to be a bit defensive here. Raising an error here is the proper way to do it. Some developer will run into this issue down the line and we can spare them some time debugging this. Also we will break some plugin otherwise 😨 |
|
Thanks, I stand corrected! I do still think the solution can be simpler: diff --git a/beetsplug/bpsync.py b/beetsplug/bpsync.py
index 9ae6d47d5..e49986798 100644
--- a/beetsplug/bpsync.py
+++ b/beetsplug/bpsync.py
@@ -20 +20 @@
-from .beatport import BeatportPlugin
+from . import beatport
@@ -26 +26 @@ def __init__
- self.beatport_plugin = BeatportPlugin()
+ self.beatport_plugin = beatport.BeatportPlugin()
@@ -100 +100 @@ def is_beatport_track
- item.get("data_source") == BeatportPlugin.data_source
+ item.get("data_source") == beatport.BeatportPlugin.data_source
diff --git a/beetsplug/chroma.py b/beetsplug/chroma.py
index 192310fb8..c15985b58 100644
--- a/beetsplug/chroma.py
+++ b/beetsplug/chroma.py
@@ -31 +31 @@
-from beetsplug.musicbrainz import MusicBrainzPlugin
+from beetsplug import musicbrainz
@@ -191,2 +191,2 @@ def __init__
- def mb(self) -> MusicBrainzPlugin:
- return MusicBrainzPlugin()
+ def mb(self) -> musicbrainz.MusicBrainzPlugin:
+ return musicbrainz.MusicBrainzPlugin() |
You are not a fan of We still need some kind of fix for the actual core plugin loading tho, right? Earlier this wasn't an issue since we had a dictionary of plugin instances, this was changed to a list in 2.4.0 and now we run into the plugin dupe issue 🤔 |
I am a fan of simple solutions, rather 😅
I doubt this would be helpful, since |
|
Fair! Although if we go for this approach, there is the possibility for it to happen again in the future without tests catching it. I'm more in favor of raising some kind of error if two Also we can only fix it for internal plugins. External ones might still be broken. Was hard enough to triag this. |
|
What about a note in the docs that says 'if your plugin depends on an internal plugin, import the plugin's module' with an example? |
Seems a bit hacky to me since we already have the fix here 🙃 Also this is a regression in 2.4.0 so we should probably restore the original behavior. You that much against the a bit more verbose solution? The only change in the logic is that we now checks the |
Yes I am, when it fixes an issue which does not exist (once the imports are adjusted as specified in the message above). |
|
Sorry for being an ass - I'm just mindful that we have more than enough complexity in the code base, so unless we have no other option, let's keep it simple and stupid. |
|
Lol the issue still exists it is just not visible anymore. Just sweeped under the carpet. E.g. external plugins could still show the same pattern... Why don't you want to fix the underlying issue and make transparent what is happening? This feels like such strange and unmaintainable decicion to me... |
My problem with the solution is just that the acctual underlying issue is not fixed at all. With the "easier" solution we are just treating symptoms imo. Maybe a bit of outside perspective could help? @beetbox/maintainers someone else wants to have a look here? |
Look, a couple of messages above I changed my mind once you've given me evidence regarding
On a similar note, can you find an external plugin that is now broken because of this issue? So far, this issue has been limited to two internal plugins, and we have a simple fix for it. In order to consider a more bulletproof/reliable (but more complex) solution, I want to see a real-world example that it's applicable to, rather than a theoretical one. |
There was a problem hiding this comment.
The fix to this regression should restore the previous behavior, rather than requiring the ecosystem to adapt to this issue. This shifts the burden to plugin developers and requires them to adjust their source code.
We have previously been able to load a plugin regardless of whether any other plugin was imported into the same module. We want to continue being able to do so, and we do not want to depend on plugin developers' due diligence to ensure it.
This check should be a good candidate
obj.__module__ == namespace.__path__ or obj.__module__.startswith(f"{namespace.__path__}.")
Im fine with this instead. This is basically what I meant in #6034 (comment). This is a way better solution than changing imports in the plugins and hoping for the best. I hope we can do more here tho, please read on! After thinking about this more, I believe we’re dealing with two closely related (but not identical) issues. That might explain why we haven’t been converging so far. I would like to address both issues if possible with this PR. a. A single plugin namespace should not export more than one plugin class (bijective mapping). Fixing It also strengthens our public-facing API: if we only fix Option 1:
Option 2:
Proposal: Maybe we can combine both approaches: use |
Two reasons why we should not do this:
Our plugin loading mechanism relies on finding a single plugin class definition (our docs make this pretty clear). Someone attempting to export multiple plugins from the same namespace is misusing the API, thus it's their responsibility to redesign their plugin, once they notice that only the first plugin is loaded.
|
I just looked at the dev docs and I dont think we mention that, atleast for me it is not clear 🤷 A concrete example of the problem: class Base(BeetsPlugin):
pass
class RealPlugin(Mixin):
passThere is nothing preventing a developer from writing code like this. Depending on the order of class definitions,
I disagree! We should have a friendly plugin architecture that allows devs of any level to contribute and write plugins. If a developer tries to write a plugin without fully understanding the underlying architecture, as is often the case, we can’t really blame them if the plugin fails to load or if they decide not to continue contributing. Personally, I would find that frustrating too. I don't say we need to check |
Feel free to clarify this in the docs then. Though, I don't think there's a need to do so - after all, it seems like no one has encountered this issue in the last 15 years. I think this is a textbook case of YAGNI violation. And ultimately, the point of this PR is to restore the previous behaviour for all plugins, regardless of how many classes they provide. |
I will try once again: We could have prevented this in the first place i.e. the regression. It is not primarily about the external plugin developer for me. A simple assertion or a raise would have shown this issue early. It will just prevent people (also us) doing stupid things down the line. Also it aligns expectations with the actual implementation.
This makes no sense in this context... We are not talking about a new feature here which we need to continuously support. It is just a tiny improvement. You are interpreting this wrong! Also, the 600 open issues paint a different picture. More robustness will help us in the long run.
Introducing both fixes as proposed restores the behaviour.
Allowing multiple plugin classes per namespace would require some changes to the core. While possible it is not what beets internally expects and I don't think it should. This would be an actual example of YAGNI. |
Guarding against purely a theoretical possibility without a known example in the wild is also an example of YAGNI. I don't think I can contribute more to this discussion: the fix I proposed ensures the previous behaviour for all plugins. On the other hand, your suggestion breaks plugins that define multiple plugin classes which makes it unsuitable. Are you happy to adjust this PR or shall I open another one? We do need to fix this. |
|
The sad reality is, both proposed fixes could break some plugins. But I believe checking the namespace name has a bigger limitations, namely re-export a plugin from an external (primary) namespace. I’d expect that to keep working, as I'm working on a tidal plugin which uses that approach. On the other hand, exporting multiple plugins in the same namespace feels like something that should never be supported in the first place, and with the proposed solution, the developer actually sees an error instead of the issue being hidden. The discussion is getting stuck on blocking rather than weighing the tradeoffs. There’s no perfect solution here, and I’ve acknowledged the limitations of both approaches. My view is that going forward we should explicitly enforce only one plugin per namespace, regardless of where it’s originally defined. That strikes me as the cleanest and most predictable solution. I’ve really tried to find a compromise here so we could both be happy, but at this point I feel I need to stand my ground on this. |
|
@wisp3rwind help us |
Guys sometimes a compromise needs to be made in life. I do not see why this solution of @semohr's is not good. Clearly errorying and pointing to usage of Even though there is not thousands of plugins that run into this it is not bad to clearly raise something to help developers that run into it in the future. This solution is not really much more code than that other solution over there: #6039 @wisp3rwind what is your opinion on this? It seems like we need tiebreakers here to be able to move on. Thanks in advance! |
|
So, I didn't follow this conversation closely and have only very quickly read through it, so there's definitely a chance that I missed a point or even completely misunderstood something. I don't have a very clear opinion, but a few thoughts: While this issue hasn't showed up through beets existence before, I do have the impression that the previous implementation (where finding re-"exported" plugin classes in unrelated plugin modules was possible and just happened to work due to how plugin classes were stored in a dict) wasn't well thought out and probably worked rather by chance than conscious design. I think it makes sense to clean this up and have a better documented and more explicit way of detecting/declaring the intended plugin class. For backwards compatibility, I think we should try to get as close to the old behavior as possible, without requiring changes to plugins. In that sense, Option 1, i.e. the obj.__module__ == namespace.__path__ or obj.__module__.startswith(f"{namespace.__path__}.")check (which is essentially #6039 if I didn't miss anything) seems sensible. From the above, it also appears that everybody would be fine with that? Then, the remaining question is whether we should also add the additional features of Option 2,
Regarding (a), relying on any implicit ordering to determine the class seems brittle at a first glance. It's probably not much of an issue in reality as pointed out above: First, if it happens, it's figured out once by plugin authors when testing their plugin and won't be seen by users. Second, because dict ordering is deterministic these days by preserving insertion order IIRC. Nevertheless, I'm in favor of checking for this, it just doesn't seem like a great "API" to rely on definition order in a plugin module 1. Giving a hint to developers here seems like a nice gesture without a big increase in complexity. Regarding (b), I'm less sure about the necessity, but I'm fine with doing this as well. It probably does not improve plugin development ergonomics by a huge margin (considering the beatport diff given above), but I do like that it is rather explicit, and So, in summary, I tend to say: Let's merge (something like) @snejus PR quickly2, to leave room for any discussion around how to do any further changes, but do take the latter seriously. Footnotes
|
|
Thanks @wisp3rwind!. I've just approved #6039 and want to ask if someone wants to find a way to remember the "making this future proof parts":
Change this PR? Or open a new PR right away? Convert an issue out of @wisp3rwind's post? Not sure what's best. And of course follow this advice :-)
|
Maybe just continue in this PR, if @semohr is still interested in polishing it? I think it would be interesting to see what it looks like with some of the above suggestions for better backwards compatibility applied. |
Beyond helping plugin developers, it also gives us a way to verify the intended behavior. While I don’t expect the internal plugin import logic to change often, a recent change here (#5887) was the original cause of this issue. Having a check in place, one that we are actually able to test will be a improvement. For me this is quite significant portion of the fix and one if not the main reason for me to push this hard here.
I agree while it is not necessary, it seems like the obvious way how imports should be handled. As mentioned earlier I'm also fine with dropping this. |
semohr
force-pushed
the
dupe_plugin_fix
branch
from
2206597 to
8cbf633
Compare
I just rebased.
Added a deprecation warning for python 3.0.0. We can change it to an exception once we do a version bump. |
semohr
force-pushed
the
dupe_plugin_fix
branch
2 times, most recently
from
c5d9917 to
b45ce34
Compare
semohr
force-pushed
the
dupe_plugin_fix
branch
from
b45ce34 to
f7ab1da
Compare
|
As a plugin dev working on a real-world example of (a) (see the above-mentioned issue), I think it should be allowed to have the class that's intended to be registered by beets in |
5 participants
It seems like when we simplified the plugin loading, we introduced a regression (#6033).
This PR introduces a fix:
We now consider publicly exposed classes listed in a plugin module’s
__all__.__all__is defined, only those members are scanned forBeetsPluginsubclasses.__all__is defined, we fall back to scanning the full module (preserving backward compatibility).This makes plugin loading predictable and avoids cases where helper/internal classes are picked up by mistake.
It seems like the
chromaandbpsyncplugins exposed an additionalBeetsPluginclass. Luckily, the TestImportPlugin test already catches this error if it is raised 👍@snejus I think this takes precedence and should be prioritized. Maybe we also do a 2.4.1 release here?
closes #6033
closes #6023