CLI: verdi node graph generate root nodes as arguments

[sphuber]

Fixes #6397

Recently, the command was changed to support specifying multiple root nodes. To keep backwards compatibility, the -N/--nodes option was added. This led to some pretty awkward behavior though with also the output file being defined as an argument being deprecated in favor of the -O/--output-file option.

If backwards compatibility wasn't a concern, a better interface would be to take root nodes as positional arguments, which is the standard across verdi commands. Since this is a more intuitive and consistent design, it is adopted here despite it breaking backwards compatibility.

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 77.69%. Comparing base (ef60b66) to head (f7a7500).
Report is 14 commits behind head on main.

Files	Patch %	Lines
src/aiida/cmdline/commands/cmd_node.py	83.34%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6427      +/-   ##
==========================================
+ Coverage   77.51%   77.69%   +0.18%     
==========================================
  Files         560      562       +2     
  Lines       41444    41687     +243     
==========================================
+ Hits        32120    32383     +263     
+ Misses       9324     9304      -20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[agoscinski]

Tried locally, works for me well. I find this design much clearer and I think keeping backwards compatibility for the graph generation is not crucial.

One could try to capture old user behavior by looking at the last command and checking if it is a filename like (not an int and acceptable by pathlib.Path) to give a bit extra information to the current error message

$ verdi node graph generate 5 out.pdf
Usage: verdi node graph generate [OPTIONS] [ROOT_NODES]...
Try 'verdi node graph generate --help' for help.

Error: Invalid value for '[ROOT_NODES]...': no Node found with LABEL<out.pdf>: No result was found

The last error could be changed to

Error: Invalid value for '[ROOT_NODES]...': no Node found with LABEL<out.pdf>: No result was found. You might specify the output file at the end. This is not supported anymore. Please use -O/--output-file to specify the file.

But this is just a suggestion, it could end up making the code much more complicated to capture something the user will get anyway.

[agoscinski]

        echo.echo_critical('No root node(s) specified.')

[sphuber]

Thanks a lot for the review @agoscinski

One could try to capture old user behavior by looking at the last command and checking if it is a filename like (not an int and acceptable by pathlib.Path) to give a bit extra information to the current error message
But this is just a suggestion, it could end up making the code much more complicated to capture something the user will get anyway.

I thought about this as well, but it would indeed not be straightforward and would make the code more complex and would introduce other edge-cases. First, the validation and error message is performed by the generic arugments.NODES reusable argument, and it is not directly possible to customize the logic and error message. We would have to essentially copy it and write a one-off solution just for this case.

Second, the logic would be tricky. The nodes can not only be specified by their PK, but also their UUID and label. This is the case for all the "identifier" options and arguments in verdi. So if a user labeled a node my_output but then added a typo in verdi node graph generate my_otput the code would think "ah, they probably meant the output file my_otput", whereas really they meant a node. Similarly, if they pass a pk, but it is incorrect (typo again) should the code think it was a typo or think they wanted to write to file with an integer as filename.

sure, these are extreme edge-cases perhaps, but as you already mentioned, the added complexity is probably not worth it for the added ambiguity. So let's not go down that road