CLI: Add the -M/--most-recent-node option to verdi process commands
#6283
Conversation
sphuber
force-pushed
the
feature/cli-process-most-recent-node
branch
from
6673252 to
558fd8c
Compare
sphuber
changed the title
verdi process commands-M/--most-recent-node option to verdi process commands
|
@khsrali and @GeigerJ2 this is a small PR that might be interesting to have a look at. Not saying you should review it (although if you do have comments/questions, they are always welcome), but since it is nice and compact, it may be a nice entry point to familiarize yourself a bit with this part of the code. |
|
I am a little uncertain about the semantics here: what happens for more complex workchains that launch multiple processes in parallel? |
| """Show the log report for one or multiple processes.""" | ||
| from aiida.cmdline.utils.common import get_calcjob_report, get_process_function_report, get_workchain_report | ||
| from aiida.orm import CalcFunctionNode, CalcJobNode, WorkChainNode, WorkFunctionNode | ||
|
|
||
| if most_recent_node: |
There was a problem hiding this comment.
I am surprised that there was no check here that the user provided at least one process.
Right now, running verdi process report just silently does nothing. Same comment for other subcommands.
Compare with verdi process play
$ verdi process play
Report: no active processes selected.The log message however comes from engine/processes/control.py
(should this be an Error instead of Report?)
There was a problem hiding this comment.
I agree this choice could be debated. Guess the logic was that since it takes a list, if an empty list is provided it behaves as any other for-loop in Python over an empty list, it is essentially a no-op. Definitely open to changing this and making it a warning or even error, but that is probably another PR.
There was a problem hiding this comment.
Sure. To be clear, I am less worried about the API of the functions in control.py and more about the CLI experience.
To me this is a really surprising behaviour (especially because the command is taking some time even with empty argument, but then it doesn't print anything).
I would still propose a change in this PR. If the most recent node was requested, we should error out if the user provided the process(es) argument at the same time.
There the option won't be very useful. But when you are developing/testing simpler workflows or individual workchains, this shortcut may still be useful I think. Since it is an optional extra and doesn't require to be used, it made sense to me to add it. |
|
Hi @sphuber, thanks for the pointer! @khsrali and I just went through it. Everything is clear and makes sense. We agree, for the purposes of debugging individual workchains, the option seems very useful. The silent behavior of the |
| MOST_RECENT_NODE = OverridableOption( | ||
| '-M', | ||
| '--most-recent-node', | ||
| is_flag=True, |
There was a problem hiding this comment.
default=False here not required?
There was a problem hiding this comment.
For boolean flags without an off-switch, this is not necessary: https://click.palletsprojects.com/en/8.1.x/options/#boolean-flags
There was a problem hiding this comment.
Yeah, makes a lot of sense. We just got a bit confused as for example in the --all option below the default is given, but I guess this is redundant?
| '-M', | ||
| '--most-recent-node', | ||
| is_flag=True, | ||
| help='Select the most recently created node.', |
There was a problem hiding this comment.
Is there a plan to use this elsewhere for non-process nodes? If not, this help message could be more concrete. But I don't feel strongly, it's fine as is.
There was a problem hiding this comment.
I left it intentionally generic indeed. Maybe it could be used for verdi calcjob commands as well, like verdi calcjob report and verdi calcjob gotocomputer
sphuber
force-pushed
the
feature/cli-process-most-recent-node
branch
from
cd4a99e to
69a7eda
Compare
The `-M/--most-recent-node` option is added to the `status`, `show` and `report` subcommands of `verdi process`. When specified, these commands don't need specific process node identifiers but the most recent will automatically be selected. This is a useful feature when users are actively running processes one by one during testing and they usually want to look at the last created process node. Co-Authored-By: Sebastiaan Huber <[email protected]>
sphuber
force-pushed
the
feature/cli-process-most-recent-node
branch
from
69a7eda to
5aae874
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6283 +/- ##
==========================================
+ Coverage 77.70% 77.78% +0.08%
==========================================
Files 548 548
Lines 40243 40264 +21
==========================================
+ Hits 31267 31315 +48
+ Misses 8976 8949 -27 ☔ View full report in Codecov by Sentry. |
The
-M/--most-recent-nodeoption is added to thestatus,showandreportsubcommands ofverdi process. When specified, these commandsdon't need specific process node identifiers but the most recent will
automatically be selected. This is a useful feature when users are
actively running processes one by one during testing and they usually
want to look at the last created process node.