Add type checks to all plugin factories

[sphuber]

Fixes #3455

It was possible to add CalcJob implementations and calculation
functions to the aiida.workflows entry point group, as well as
WorkChain and work functions to aiida.calculations. The respective
factories would load these entry points without complaining. However, to
keep consistency it is better to raise if an entry point is loaded from
the incorrect entry point group.

The various entry point factories, based on the base class
aiida.plugins.factories.BaseFactory would simply return the resource
that the given entry point corresponds to if it could be loaded. There
were no additional checks if the type of the loaded entry point
corresponds to what is expected for that particular entry point group.
It was possible, for example, to add CalcJob implementations and
calculation functions to the aiida.workflows entry point group, as well
as WorkChain and work functions to aiida.calculations. The respective
factories would load these entry points without complaining. However, to
keep consistency it is better to raise if an entry point is loaded from
the incorrect entry point group.

[sphuber]

Few questions to be discussed/answered:

• This makes the usage of entry points more strict and may break the use of these factories for plugins that have the wrong kind of entry points in the respective entry point groups. Do we want to be this strict in principle and if so, is it ok to introduce this now.
• Do we want the same type validation for the other factories?
• There is in fact some "active" validation on entry points, but this is done in a unit test aiida.backends.tests.test_plugin_loader. It will load all registered entry points and perform a type check. This will however only be run when a user runs this unit test obviously. If they don't they will never notice that they have an "illegal" entry point registered, until they load it through the factory. One drastic option was to load and validate all registered entry points when aiida is imported. However, this will probably have a non-negligible effect on load time and may be problematic for example verdi load time.

[ltalirz]

Do we want to be this strict in principle

I would say yes. A factory should spit out what it is designed to produce.

and if so, is it ok to introduce this now.

I would prefer to do it as a warning rather than an exception at this point (to be turned into an exception in version ...), exactly because it might break something.
If others feel really strongly in favor of an exception, we can discuss.

Do we want the same type validation for the other factories?

I would say yes.

aiida.backends.tests.test_plugin_loader

I would say it's good to keep the test if it still checks things that aren't checked by the factories.
You could move what the test does into a function of AiiDA that users can call on demand to check things.

I would strongly vote against entry point validation at import of aiida.

[sphuber]

I would say yes.

OK, I will add it for all factories.

I would say it's good to keep the test if it still checks things that aren't checked by the factories.
You could move what the test does into a function of AiiDA that users can call on demand to check things.

After adding type checking to all factories, the unit test should really just try and load all registered entry points. The type checking it is currently doing will then already be done by the factory.
Maybe I could abstract this to a function and also expose it as verdi plugin validate?

I would strongly vote against entry point validation at import.

I agree.

[ltalirz]

Maybe I could abstract this to a function and also expose it as verdi plugin validate?

Sounds like a good idea!

We might eventually want to add more validation functionalities, so perhaps think about whether you want to add a verdi command already now or just have a function in the python API.
Both are valid choices from my point of view.

P.S. Could e.g. be a "devel" command at this point. Up to you.

[sphuber]

@ltalirz this should be good for review. Tests are now running, although I had to add the mock backport as a dependency for python 2. I also added the CLI command verdi devel validate-plugins.

[sphuber]

Note that we decided to keep the behavior that the factories will raise instead of a warning. The reason is that even if a plugin has an entry point with an incorrect type (which is highly unlikely, probably only calcfcuntion in aiida.workflows or workfunction in aiida.calculations, if even that) only trying to load through the factory will raise. The resource itself can still be used through normal import. Also nodes in existing database corresponding to this will not be affected.

[giovannipizzi]

Good for me!
After merging, let's just ping developers here, to make sure they double check they didn't register things in the wrong place

[ltalirz]

Note that we decided to keep the behavior that the factories will raise instead of a warning. The reason is that even if a plugin has an entry point with an incorrect type (which is highly unlikely, probably only calcfcuntion in aiida.workflows or workfunction in aiida.calculations, if even that) only trying to load through the factory will raise. The resource itself can still be used through normal import.

Hm... I don't think this is a valid argument.
The fact of the matter is that if people are loading their data types/calculations in their plugins via a factory, and they're using the "wrong" factory then this change will break their code.
That is the definition of a breaking change.