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

fix: raise when detecting multiple entry points #5531

Merged
merged 4 commits into from
May 20, 2022

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented May 20, 2022

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:


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

Fixes #5530

ltalirz and others added 3 commits May 20, 2022 16:55
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).
@sphuber
Copy link
Contributor

sphuber commented May 20, 2022

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.

@sphuber sphuber self-requested a review May 20, 2022 19:57
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
@ltalirz
Copy link
Member Author

ltalirz commented May 20, 2022

Thanks @sphuber, both for the review and for the test!

@sphuber
Copy link
Contributor

sphuber commented May 20, 2022

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:

ValueError: aiida.groups:core is not a valid entry point string: Multiple entry points 'core' found in group 'aiida.groups': [EntryPoint(name='core', value='aiida.orm.groups:Group', group='aiida.groups'), EntryPoint(name='core', value='aiida.orm.groups:Group', group='aiida.groups')]

But there the entry points are exactly identical. So even with this fix, this would still raise a MultipleEntryPointError right? You said yourself though that there is nothing we can do here, which I agree with. But would that mean it still poses problems for AiiDAlab?

@ltalirz
Copy link
Member Author

ltalirz commented May 20, 2022

But there the entry points are exactly identical. So even with this fix, this would still raise a MultipleEntryPointError right?

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.

@sphuber
Copy link
Contributor

sphuber commented May 20, 2022

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 aiida-core installed twice, once in system space and one in user space? Not sure how and where the entry points are stored and read from in that case.

@ltalirz
Copy link
Member Author

ltalirz commented May 20, 2022

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.

@ltalirz
Copy link
Member Author

ltalirz commented May 20, 2022

But, I am a bit surprised that those entry points would end up in the same environment. Is AiiDAlab using virtual envs? Or was aiida-core installed twice, once in system space and one in user space?

The AiiDAlab uses a conda environment (managed by root), where aiida-core is installed.
This environment cannot be modified by the user - but they can still install their own version (which pip will end up installing with --user automatically), which is often useful for development.

Not sure how and where the entry points are stored and read from in that case.

I didn't look into it further, but I guess it makes sense that both are added to the same entry point map.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ltalirz

@sphuber sphuber merged commit a8ea6b7 into aiidateam:main May 20, 2022
@sphuber sphuber deleted the issue_5530_multiple_entry_points branch May 20, 2022 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise when detecting multiple matching entry points
2 participants