Entry points: add the core. prefix to all entry points
#5073
Conversation
sphuber
requested review from
chrisjsewell,
ltalirz,
giovannipizzi and
ramirezfranciscof
|
Thanks @sphuber!
|
Indeed, this is up to date so far with the state of |
|
Thanks @sphuber ! I have two main questions:
|
|
@ltalirz I agree with your 2nd point that it would be jarring for users to already start getting deprecation warnings with a brand new major release of the code, and it would be perhaps better if we wait a bit before deprecating and maybe join that with the deprecation of moving out data plugins. |
We kind of have to deprecate them because we cannot keep both at all levels. We can keep the old strings working in the factories (which is already implemented) but for some of the entry points (the calcjobs, workflows, parsers, schedulers and transports) the entry point name is stored on the relevant
I described the benefits in the OP of the PR. This is to be consistent, conformant with our own guidelines and it will make it more obvious where certain plugins come from. The ecosystem might be small enough now where that is not (yet) difficult to say, but in the future, things might not be so obvious. The explicitness of the
I don't agree. The whole point of deprecation warnings is to allow a grace period. The point is not to never have them show up. If that were the case, we wouldn't even need a deprecation period and just work with the developers to adapt their code straight away to the changes before they release the next version. The changes necessary would also be minimal. The entry points are not used that often, but where ever they are used, they can be updated with relatively simple search-and-replace.
The discussion of moving out the domain-specific data plugins has been brought up many times, even before 1.0 and we keep postponing it. At this point, we don't know when that is going to happen, so I'd prefer to do this now, so things are consistent and prepared. |
sphuber
force-pushed
the
fix/4914/core-entry-point-prefix
branch
7 times, most recently
from
9437797
to
0dac425
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5073 +/- ##
===========================================
- Coverage 80.63% 80.63% -0.00%
===========================================
Files 534 537 +3
Lines 37067 37179 +112
===========================================
+ Hits 29887 29975 +88
- Misses 7180 7204 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Here follows a summary of the discussion and discussion taken at the meeting of Aug 30 2021. Main argument against this change:
The counter arguments to this point:
It was decided that with these counter arguments, the main downside of this change of adding some work for plugin developers does not outweigh the long term benefit of having correct and explicit entry point names. Therefore we have decided to go ahead with this change. |
|
@chrisjsewell if we can get an official approval that would be great |
sphuber
force-pushed
the
fix/4914/core-entry-point-prefix
branch
2 times, most recently
from
7068f00
to
85275d3
Compare
The AiiDA plugin system allows the functionality provided by `aiida-core` to be extended through entry points. These are defined in specific entry point groups with a specific name. For these names to be uniquely resolvable, each name needs to be unique within an entry point group. That is why the convention is that each plugin package prefixes their entry points with the package name, effectively namespacing all entry points. Not only does this prevent entry point name clashes between different packages, it also makes it explicit what package any particular entry point comes from. Uptil now, however, `aiida-core` was not respecting its own community guideline and failed to prefix its entry points with `core.`. Not only does this make it less obvious that these entry points are defined by `aiida-core`, which may become more problematic as the ecosystem keeps growing, but it also effectively blocks certain namespaces for potential external plugin packages. For example, the namespace `array` is effectively hijacked in the `aiida.data` group, since our `ArrayData` plugin and its subclasses use it. This would make it difficult for a potential `aiida-array` package to choose the name for its entry points. Therefore, we opt to update `aiida-core`'s entry point names and properly prefix them with `core.`. The following entry point groups were updated: * `aiida.calculations` * `aiida.cmdline.computer.configure` * `aiida.cmdline.data` * `aiida.data` * `aiida.parsers` * `aiida.schedulers` * `aiida.tools.data.orbitals * `aiida.tools.dbimporters` * `aiida.transports` * `aiida.workflows` The missing entry point groups simply didn't have any entry points registered by `aiida-core`. The exception is `aiida.node`, but this entry point group is not actually extensible by plugin packages and is merely there for internal use. As such, the entry points are also never used by external packages. The classes are imported directly from the `aiida.orm` package.
The `get_entry_point_from_string` method of the `PluginParamType` parameter type, tries to determine the entry point group and name from the value passed to the parameter and when matched, attempts to load the corresponding entry point. It was doing so by directly calling the `get_entry_point` method from the `aiida.plugins` module. Since the plugins that ship with `aiida-core` were recently updated to have them properly prefixed with `core.`, the old unprefixed entry point names are now deprecated. When used through the factories, the legacy entry point names are automatically detected and converted to the new one, with a deprecation warning being printed. However, the command line didn't have this functionality, since the `PluginParamType` was not going through the factories. Here, for the entry point groups that have a factory, the plugin param type is updated to use the factories, hence also automatically profiting from the deprecation pathway, allowing users to keep using the old entry point names for a while. The factories had to be modified slightly in order to make this work. Since the `PluginParamType` has the argument `load` which determines whether the matched entry point should be loaded or not, it should be able to pass this through to the factories, since this was always loading the entry point by default. For this reason, the factories now also have the `load` keyword argument. When set to false, the entry point itself is returned, instead of the resource that it points to. It is set to `True` by default to maintain backwards compatibility.
sphuber
force-pushed
the
fix/4914/core-entry-point-prefix
branch
from
85275d3
to
7efd1a2
Compare
well better late than never |
Fixes #4914
This PR updates all* entry points that ship with
aiida-coreand prepends thecore.entry point namespace, as to conform with our own convention. The benefit is not just to be consistent but it will also make it easy to determine exactly where all entry points come from, which may not always have been trivial without the prefix.The PR includes full database migrations and archive migrations to update all the relevant versions of the entry point names as they are stored on
ComputerandNodeinstances. This makes sure that existing data is fully backwards compatible. As for backwards compatibility of code: all factories include a check for the old entry points and will emit a deprecation warning and will then load the corresponding entry point from the new entry point string. This means that code using the factories is fully backwards compatible. There are a few places in the CLI that are not backwards compatible unfortunately. For example when using-S slurmwhen setting up a computer, one now has to update this to use the new entry point, i.e.,-S core.slurm. The same goes for REST API queries where query parameters that usedtransport_type=localnow should usetransport_type=core.local. Ultimately, I think this is worth it in the long run.