CLI: minor change on `verdi plugin list` logic to show description

[khsrali]

If a plugin that is not a Process but hasn't set plugin.is_process_function=False supports get_description() aiida-core should understand in this scenario, plugin.is_process_function is obviously False. Currently, AttributeError could get caught by not having is_process_function instead of get_description()

[sphuber]

I don't understand the problem from your description to be honest. Could you please provide an example where this failed? It would anyway be good to add that as a regression test.

[khsrali]

I don't understand the problem from your description to be honest. Could you please provide an example where this failed? It would anyway be good to add that as a regression test.

Alright, I try to improve the description:

Suppose you write a plugin named X.

Class X(Transport):
    ..
    @classmethod
    def get_description(cls) -> str:
         return "I'm  X"
    ..

Once the plugin is installed, try verdi plugin list aiida.transports X

What you get is: Error: No description available for X

Which means this AttributeError was handled only because is_process_function was not found in the plugin, despite the existence of get_description.

I agree, it would be great to write a test for this, but one has to know how to mock a plugin, which I don't. Do we have a fixture for that? I would appreciate any hint. Thank you,

[sphuber]

Which means this AttributeError was handled only because is_process_function was not found in the plugin, despite the existence of get_description.

Thanks, I understand now. I missed this because I thought the conditional had and instead of or and so I mistakenly thought that if the plugin was not a process, it would have been caught by the first part of the conditional and woulnd't even get to the plugin.is_process_function.

Now that we understand that though, a better solution is not to just broadly catch this AttributeError but more explicitly defend against this in the conditional. So I would propose instead

if (inspect.isclass(plugin) and issubclass(plugin, Process)) or hasattr(plugin, 'is_process_function'):

I agree, it would be great to write a test for this, but one has to know how to mock a plugin, which I don't. Do we have a fixture for that? I would appreciate any hint.

https://aiida.readthedocs.io/projects/aiida-core/en/stable/topics/plugins.html#topics-plugins-testfixtures-entry-points

[khsrali]

Now that we understand that though, a better solution is not to just broadly catch this AttributeError but more explicitly defend against this in the conditional. So I would propose instead

if (inspect.isclass(plugin) and issubclass(plugin, Process)) or hasattr(plugin, 'is_process_function'):

I would still check the value of is_process_function . It could be that the plugin has that attribute but the value is False So now, the latest commit should be fine.

https://aiida.readthedocs.io/projects/aiida-core/en/stable/topics/plugins.html#topics-plugins-testfixtures-entry-points

Unfortunately, that example doesn't seem to work. pytest returns:

src/aiida/tools/pytest_fixtures/entry_points.py:46: ValueError
ValueError: invalid `entry_point_string` format, should `group:name`.

And then if I do:

    def test_parser(entry_points):
        from aiida.parsers import Parser
        from aiida.plugins import ParserFactory

        class CustomParser(Parser):
                return ''

        entry_points.add(CustomParser, 'aiida.parsers:test')

>       assert ParserFactory('test', CustomParser)

still, returns aiida.common.exceptions.InvalidEntryPointTypeError: entry point `test` registered in group `aiida.parsers` is invalid because its type is not one of: Parser

I'm sorry to log the error here, but it's kind of time consuming to dig into this without further information.

[sphuber]

There are more problems with the example. The assert statement also doesn't make sense. It should be

assert ParserFactory('test') is CustomParser

and then the final problem is that the CustomParser should be defined on the module level. Like it is currently, defined in the local scope of the test function, the factory will not be possible to load it. So the correct version is

from aiida.parsers import Parser

class CustomParser(Parser):
    """Parser implementation."""

def test_parser(entry_points):
    """Test a custom ``Parser`` implementation."""
    from aiida.plugins import ParserFactory

    entry_points.add(CustomParser, 'aiida.parsers:custom.parser')

    assert ParserFactory('custom.parser') is CustomParser

I will submit a PR to fix the docs.

[sphuber]

https://github.com/aiidateam/aiida-core/pull/6412