-
Notifications
You must be signed in to change notification settings - Fork 214
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
fix: raise when detecting multiple entry points #5531
fix: raise when detecting multiple entry points #5531
Conversation
Due to a slight misunderstanding of the new importlib.metadata API, AiiDA did no longer raise when detecting multiple entries for a given entry point (pair of group + name), and instead simply loaded the first one. Example of previous (buggy) behavior: ``` In [8]: eps.select(group='aiida.workflows',name='mcscf.abcChain') Out[8]: [EntryPoint(name='mcscf.abcChain', value='aiida_mcscf.workflows:abcChain', group='aiida.workflows'), EntryPoint(name='mcscf.abcChain', value='aiida_mcscf.workflows:AbcDefChain', group='aiida.workflows')] In [9]: result=eps.select(group='aiida.workflows',name='mcscf.abcChain') In [10]: result.names Out[10]: {'mcscf.abcChain'} In [11]: len(result) Out[11]: 2 In [12]: len(result.names) Out[12]: 1 In [13]: result['mcscf.abcChain'] Out[13]: EntryPoint(name='mcscf.abcChain', value='aiida_mcscf.workflows:abcChain', group='aiida.workflows') ```
When encountering multiple entrypoints of the same name in a given group, still proceed if their values (where they are pointing) are identical. This makes it possible to e.g. install a version of AiiDA locally, over a system-level installation of AiiDA that cannot be uninstalled (as long as those versions do not change the value of an entry point).
Thanks @ltalirz . The fix looks fine to me. I have just taken the liberty to add a test. Hope you don't mind. Let me know if you are happy with the test and then I will accept and merge this. |
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
Thanks @sphuber, both for the review and for the test! |
Just rereading through your original description in the issue, are you sure that this actually fixes the problem you describe of AiiDAlab where a user installs another version of AiiDA in user space overriding one in global space? You gave the example exception:
But there the entry points are exactly identical. So even with this fix, this would still raise a |
Erm... didn't you just add a test that proves the opposite? As long as the entry points are exactly identical, it should not raise - which is what we want. |
Of course 🤦 scratch that, not sure what came over me. But, I am a bit surprised that those entry points would end up in the same environment. Is AiiDAlab using virtual envs? Or was |
P.S. And the fix did address the issue for me in the AiiDAlab. This was only part of the goal of this fix, however - the other was indeed to raise when there are multiple entries for a given entry point that are not identical. This is how I initially stumbled upon this bug - I was trying to change the value of an entry point but it was not reflected in the one loaded by AiiDA. It took me a while to figure out that apparently some other distribution was adding an entry point with the same name and a different value. |
The AiiDAlab uses a conda environment (managed by root), where aiida-core is installed.
I didn't look into it further, but I guess it makes sense that both are added to the same entry point map. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ltalirz
Due to a slight misunderstanding of the new importlib.metadata API,
AiiDA did no longer raise when detecting multiple entries for a given
entry point (pair of group + name), and instead simply loaded the first
one.
Example illustrating the behavior:
Fixes #5530