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

khsrali · 2024-05-22T10:32:38Z

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 · 2024-05-22T12:33:57Z

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 · 2024-05-23T07:53:23Z

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 · 2024-05-23T09:54:13Z

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 · 2024-05-23T14:46:26Z

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 · 2024-05-23T14:56:01Z

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 · 2024-05-23T15:02:10Z

#6412

sphuber

Thanks @khsrali just a few minor corrections

sphuber · 2024-05-23T16:54:35Z

tests/cmdline/commands/test_plugin.py

+    assert ParserFactory('custom.parser') is CustomParser
+
+    result = run_cli_command(cmd_plugin.plugin_list, ['aiida.parsers', 'custom.parser'])
+    assert result.stdout_bytes.decode('utf-8').strip() == 'str69'


Suggested change

assert result.stdout_bytes.decode('utf-8').strip() == 'str69'

assert result.output.strip() == 'str69'

sphuber · 2024-05-23T16:55:08Z

tests/cmdline/commands/test_plugin.py

+
+
+def test_plugin_description(run_cli_command, entry_points):
+    """Test if `verdi plugin list` would show description of an external plugin."""


Suggested change

"""Test if `verdi plugin list` would show description of an external plugin."""

"""Test that ``verdi plugin list`` uses ``get_description`` if defined."""

sphuber · 2024-05-23T16:57:29Z

src/aiida/cmdline/commands/cmd_plugin.py

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


This phrasing with a nested if feels a bit awkward. I think a more typical formulation would be

Suggested change

if (inspect.isclass(plugin) and issubclass(plugin, Process)) or (

False if not hasattr(plugin, 'is_process_function') else plugin.is_process_function

):

if (inspect.isclass(plugin) and issubclass(plugin, Process)) or (

hasattr(plugin, 'is_process_function') and plugin.is_process_function

):

sphuber

Thanks a lot @khsrali

edan-bainglass · 2024-08-14T14:15:14Z

Hmm, not sure if related to this PR, but I now get the following on verdi plugin list aiida.data core.array

Traceback (most recent call last):
  File "/opt/conda/bin/verdi", line 10, in <module>
    sys.exit(verdi())
  File "/opt/conda/lib/python3.9/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
  File "/opt/conda/lib/python3.9/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
  File "/opt/conda/lib/python3.9/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/opt/conda/lib/python3.9/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/opt/conda/lib/python3.9/site-packages/aiida/cmdline/groups/verdi.py", line 117, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/opt/conda/lib/python3.9/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
  File "/opt/conda/lib/python3.9/site-packages/aiida/cmdline/commands/cmd_plugin.py", line 56, in plugin_list
    echo.echo(str(plugin.get_description()))
TypeError: get_description() missing 1 required positional argument: 'self'

Happens with a few (if not all) data entry points.

v2.6.2

sphuber · 2024-08-14T20:30:54Z

It is in fact related to this PR. The PR fixed a bug where the conditional would always raise an AttributeError for non-process plugins and so would always skip and simply print that no description was available. After this fix, the code now actually goes to the else-clause and calls plugin.get_description, but this is also a bug that simply was always hidden by the first bug. The get_description method is an instance method and the plugin is a class, so that call fails. There is no get_description classmethod though that provides a description of the plugin. The best we can do I think is to simply display the docstring of the plugin

…iidateam#6411) The implementation would call `get_description` on the plugin to retrieve a verbose description and would catch the `AttributeError` in case the method was not defined. However, the conditional determining whether the plugin is a process or process function was flawed and it would always call `plugin.is_process_function` for plugins that were not processes. This would therefore raise an `AttributeError` for any other plugins besides process functions and so the command would always print that no additional information was available.

logic of get_description in plugin_list

c7f7bd8

use hasattr()

ddaf774

tests added

7a8a06b

sphuber requested changes May 23, 2024

View reviewed changes

Apply review

5cf9027

sphuber approved these changes May 23, 2024

View reviewed changes

sphuber merged commit e952d77 into aiidateam:main May 23, 2024
18 of 19 checks passed

t-reents mentioned this pull request Aug 14, 2024

CLI: verdi plugin list fails for data plugins #6557

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

khsrali commented May 22, 2024

sphuber commented May 22, 2024

khsrali commented May 23, 2024

sphuber commented May 23, 2024

khsrali commented May 23, 2024

sphuber commented May 23, 2024

sphuber commented May 23, 2024

sphuber left a comment

sphuber May 23, 2024

sphuber May 23, 2024

sphuber May 23, 2024

sphuber left a comment

edan-bainglass commented Aug 14, 2024

sphuber commented Aug 14, 2024

	assert result.stdout_bytes.decode('utf-8').strip() == 'str69'
	assert result.output.strip() == 'str69'



		def test_plugin_description(run_cli_command, entry_points):
		"""Test if `verdi plugin list` would show description of an external plugin."""

	"""Test if `verdi plugin list` would show description of an external plugin."""
	"""Test that ``verdi plugin list`` uses ``get_description`` if defined."""

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

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

Conversation

khsrali commented May 22, 2024

sphuber commented May 22, 2024

khsrali commented May 23, 2024

sphuber commented May 23, 2024

khsrali commented May 23, 2024

sphuber commented May 23, 2024

sphuber commented May 23, 2024

sphuber left a comment

Choose a reason for hiding this comment

sphuber May 23, 2024

Choose a reason for hiding this comment

sphuber May 23, 2024

Choose a reason for hiding this comment

sphuber May 23, 2024

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

edan-bainglass commented Aug 14, 2024

sphuber commented Aug 14, 2024

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

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