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

CLI: verdi plugin list fails for data plugins #6557

Closed
t-reents opened this issue Aug 14, 2024 · 3 comments · Fixed by #6560
Closed

CLI: verdi plugin list fails for data plugins #6557

t-reents opened this issue Aug 14, 2024 · 3 comments · Fixed by #6560
Labels

Comments

@t-reents
Copy link

@edan-bainglass encountered the following issue and I've quickly investigated it.

Running e.g. verdi plugin list aiida.data core.array causes an exception:

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

This error was introduced in #6411. In previous versions, e.g. aiida-core 2.5, the command would simply print the error:
Error: No description available for core.array

Due to the changes in the aforementioned PR #6411, the try except is not triggered. As no AttributeError is raised anymore after adding the hasattr(plugin, 'is_process_function'):

try:
if (inspect.isclass(plugin) and issubclass(plugin, Process)) or (
hasattr(plugin, 'is_process_function') and plugin.is_process_function
):
print_process_info(plugin)
else:
echo.echo(str(plugin.get_description()))
except AttributeError:
echo.echo_error(f'No description available for {entry_point}')

Since the hasattr simply returns False in case of the ArrayData, the else statement is executed. Here, the error shown above is raised, as plugin.get_description() tries to call get_description on the class and not on an object. However, get_description is not a class method in orm.Node, leading to the exception.

Without thinking too much about it, one possible fix would be to create an object, i.e. calling plugin().get_description() (this would work for methods and class methods). However, I'm not sure if this will cause additional problems.
Furthermore, one could catch the TypeError in the except statement as well. Again, these were just some ideas that came to my mind. There might be more things to consider that could lead to different solutions.

Pinging @sphuber and @khsrali who have worked on the previous PR.

@sphuber
Copy link
Contributor

sphuber commented Aug 14, 2024

Thanks for the analysis @t-reents . Coincidentally, I had just responded to @edan-bainglass comment on the related PR. I will copy the comment here:

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

As to your suggestion:

Without thinking too much about it, one possible fix would be to create an object, i.e. calling plugin().get_description() (this would work for methods and class methods).

This would be vulnerable still, because it requires constructing the class and that might require arguments. An example would be a Parser plugin which raises if you construct it without arguments.

What we really need to have is something that would work for all "plugins". And plugins here really can be any class, or even function really. So I think our best bet would be to use the docstring. It is guaranteed to exist and at worst returns None when not defined. We can check for this case and just print the default message that no help is available.

@t-reents
Copy link
Author

t-reents commented Aug 14, 2024

Ah, I missed that Edan has already commented, I thought he stopped after our discussion :D sorry for the kind of duplicated issue

True, the Parser is one of the examples that I was assuming without having a specific example, thanks for the clarification.

The doc-string approach makes sense!

@khsrali
Copy link
Contributor

khsrali commented Aug 15, 2024

Thanks @t-reents and @edan-bainglass for finding this issue.
For some reason, I always assumed get_description() was defined as a class method across the interface.
Now, by doing a simple search, just realized that's not always the case.

So for backward compatibility, I would keep the if.. else clause, and in case get_description() exists and is not a classmethod, handle the TypeError, and as @sphuber suggests print the docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants