aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
431 stars 187 forks source link

CLI: `verdi node graph generate` root nodes as arguments #6427

Closed sphuber closed 4 months ago

sphuber commented 4 months ago

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[bot] commented 4 months ago

Codecov Report

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

Project coverage is 77.69%. Comparing base (ef60b66) to head (7ed3b15). Report is 112 commits behind head on main.

Files Patch % Lines
src/aiida/cmdline/commands/cmd_node.py 83.34% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6427 +/- ## ========================================== + Coverage 77.51% 77.69% +0.19% ========================================== Files 560 562 +2 Lines 41444 41687 +243 ========================================== + Hits 32120 32384 +264 + Misses 9324 9303 -21 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sphuber commented 4 months ago

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