Adds output `verdi process [show|report|status|watch|call-root]` subcommands #6428

agoscinski · 2024-05-28T15:47:09Z

Also because it became apparent during the PR that verdi process watch does not have the --most-recent-node option, so I added it. There is no test testing the functionality (only the error), because before there were no test for process watch (they might be somewhere else, because this look like rabbitmq related logic).

codecov · 2024-05-28T16:53:22Z

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.83%. Comparing base (ef60b66) to head (edde64d).
Report is 112 commits behind head on main.

Files with missing lines	Patch %	Lines
src/aiida/cmdline/commands/cmd_process.py	81.82%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6428      +/-   ##
==========================================
+ Coverage   77.51%   77.83%   +0.33%     
==========================================
  Files         560      561       +1     
  Lines       41444    41789     +345     
==========================================
+ Hits        32120    32523     +403     
+ Misses       9324     9266      -58

Flag	Coverage Δ
presto	`73.13% <47.83%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

sphuber

Thanks @agoscinski fine with the concept but have some comments about the changes in the phrasing of docstrings

sphuber · 2024-05-28T21:16:12Z

docs/source/reference/command_line.rst

      dump       Dump process input and output files to disk.
-      kill       Kill running processes.
+      kill       Kill the given process(es).


I can see why you changed this. When looking at the help string of the actual command, this phrasing might make more sense. But in this overview, I feel it doesn't make much sense, because there are no actual process(es) given yet. The more abstract phrasing "{Action} processes" feels like a more natural description.

I think we could have both by reverting the first line of the docstring to what we had, but then add a line to the docstring that has your phrasing. Example:

def kill_processes(processes): """Kill running processes. Kill the running processes given by PROCESSES. """

In this way, we also include the metavar PROCESSES that references the automatically formatted usage line.

Are you okay with singular/plural consideration?

def kill_processes(processes): """Kill one or multiple running processes. Kill the running process(es) given by PROCESSES. """

Tried to be consistent with help messages of the other commands show, status and report

Are you okay with singular/plural consideration?

I am fine with specifying that it can be one or many, I just don't think it should be in the short help text. The way click works, with the first line of the docstring being used as the short help for the command group overview, and the rest of the docstring being shown for the extended help text of the command, I think it makes sense to keep the short help short and sweet. It doesn't really matter at that point what the specific of the implementation are. Sure you can use the explicit short_help argument in the command declaration as you are doing, but this just seems like unnecessary work and duplication.

What I think I would want to see as a user when I do verdi process --help is

call-root Show the root process of processes. dump Dump input and output files of processes to disk. kill Kill running processes. list Show a list of processes. pause Pause running processes. play Play (unpause) paused processes. repair Automatically repair all stuck processes. report Show the log report of processes. show Show details of processes. status Show the status of processes. watch Watch state transitions of processes.

In the extended help text (the docstring body) you can then use more elaborate and specific language such as your suggestion:

Kill the running process(es) given by PROCESSES.

This gives a short and clear overview of what commands do in the group help, and then in the command help text you provide details about the functionality and implementation.

What do you think?

implemented like in your suggestion

sphuber · 2024-05-28T21:17:53Z

src/aiida/cmdline/commands/cmd_process.py

 @arguments.PROCESSES()
 @options.MOST_RECENT_NODE()
 @decorators.with_dbenv()
 def process_show(processes, most_recent_node):
-    """Show details for one or multiple processes."""
+    """Show details for one or multiple processes given the primary key(s) (PK)."""


I wouldn't add primary key(s) (PK). The pk is just one way of identifying a node. All options/arguments that allow identifying an ORM entity (Computer, Node, User) they can all be identified by their PK, UUID or label. The pk might be the most common one, but I don't think we should write it like this as it may falsely suggest that it is the only possible identifier

Is it okay if we change it like in kill ?

def kill_processes(processes): """Show details for one or multiple processes. Show the details of the process(es) given by PROCESSES. """

(removing short help)

And secondly, I am not sure what valid identifiers are from the verdi command line. Can we maybe add a link to the doc?

def kill_processes(processes): """Show details for one or multiple processes. Show the details of the process(es) given by the identifier PROCESSES. See https://aiida.readthedocs.io/projects/aiida-core/en/stable/topics/cli.html#topics-cli-identifiers for valid identifiers. """

Or explicitly write them out

def kill_processes(processes): """Show details for one or multiple processes. Show the details of the process(es) given by the identifier PROCESSES. Valid identifiers are: PK, UUID or label. """

Can we maybe add a link to the doc?

Here you go: https://aiida.readthedocs.io/projects/aiida-core/en/stable/topics/cli.html#topics-cli-identifiers

But honestly, I don't want to have to start adding this to all the commands. This is a common feature that applies to all commands where ORM entities can be referenced. It would be way too repetitive to add this everywhere.

I see your point adding this information is very redundant. What do you think about changing the PROCESSES click.Parameter ?

PROCESSES = OverridableArgument('processes', nargs=-1, type=types.ProcessParamType(), metavar="[PK|UUID|Label]...")

resulting help page:

Usage: verdi process show [OPTIONS] [PK|UUID|Label]... Show details of processes. Show the details of the process(es) given by the identifier(s). Options: -M, --most-recent-node Select the most recently created node. -v, --verbosity [notset|debug|info|report|warning|error|critical] Set the verbosity of the output. -h, --help Show this message and exit.

I added ... to specify that a list of arguments can be given but that might not be clear.

Ah wow the ... is even specified in the doc. I see that this goes a bit beyond the PR. I would make maybe a separate discussion in an issue after looking at this in more depth

But I think [PROCESSES]... would be actually the correct way according to the documentation. Should I change it to this for now?

Ah wow the ... is even specified in the doc. I see that this goes a bit beyond the PR. I would make maybe a separate discussion in an issue after looking at this in more depth

I agree, this is getting way out of scope. The important part in this PR is to make sure all process commands properly error if no process is specified. I am fine with adding the missing most-recent-node option for the one command. The discussion on short help messages is already getting out of scope as well, but we could include it if we can come to an agreement, otherwise we should maybe keep it simple and leave that out.

But I think [PROCESSES]... would be actually the correct way according to the documentation. Should I change it to this for now?

Yes, this is the correct notation and it is automatically generated by click. I prefer not to change it manually unless we really have to. Having these things generated automatically guarantees a large degree of consistency across all commands

I see now it is working again. I probably disabled somehow unintentionally or it is getting too late. Removed any manual changes.

src/aiida/cmdline/commands/cmd_process.py

sphuber · 2024-05-28T21:22:55Z

src/aiida/cmdline/commands/cmd_process.py

    from aiida.cmdline.utils.common import get_node_info

+    if not (processes) and not (most_recent_node):
+        raise click.BadArgumentUsage('Please specify process primary key(s) (PK) or an option.')


could maybe be more explicit about the problem and what option could be used?

Suggested change

raise click.BadArgumentUsage('Please specify process primary key(s) (PK) or an option.')

raise click.BadArgumentUsage(

'No processes specified; specify one or more with arguments or use the `-M/--most-recent-node` flag.'

)

Agreed, I had the same comment

sphuber · 2024-05-28T21:25:24Z

src/aiida/cmdline/commands/cmd_process.py

    from aiida.cmdline.utils.common import get_node_info

+    if not (processes) and not (most_recent_node):
+        raise click.BadArgumentUsage('Please specify process primary key(s) (PK) or an option.')


Think UsageError might be more appropriate since it really involves the lack of arguments and an option

Suggested change

raise click.BadArgumentUsage('Please specify process primary key(s) (PK) or an option.')

raise click.UsageError('Please specify process primary key(s) (PK) or an option.')

Taking comment on the help message into account of show. What about this message?

Please specify process(es) identifier (PK, UUID or label) or an option.

Ok, this is fine for the exception

Can you actually retrieve processes with label? When I try it, it does not work

what did you use for a label, how did you set it on the process node, and what was the exact verdi command?

Had some processes from running the basic example (But I don't remember what I exactly have run)

(aiida-dev) alexgo@fw:~/code/aiida-core(fix/6391/verdi-process-show)$ verdi process list -a PK Created Process label ♻ Process State Process status ---- --------- ------------------------ --- --------------- ---------------- 5 7D ago MultiplyAddWorkChain ⏹ Finished [0] 6 7D ago multiply ⏹ Finished [0] 8 7D ago ArithmeticAddCalculation ⏹ Finished [0] 15 7D ago MultiplyAddWorkChain ⏹ Finished [0] 16 7D ago multiply ⏹ Finished [0] 18 7D ago ArithmeticAddCalculation ⏹ Finished [0]

then wanted to verify the label name, since in the list they are not unique so I thought Process label means something else

(aiida-dev) alexgo@fw:~/code/aiida-core(fix/6391/verdi-process-show)$ verdi process show 6 Property Value ----------- ------------------------------------ type multiply state Finished [0] pk 6 uuid 99217f69-1498-4619-906f-ef115522d6fc label multiply description ctime 2024-05-30 13:49:12.576837+00:00 mtime 2024-05-30 13:49:12.875801+00:00 Inputs PK Type -------- ---- ------ x 2 Int y 3 Int Outputs PK Type --------- ---- ------ result 7 Int Caller PK Type -------- ---- -------------------- CALL 5 MultiplyAddWorkChain

In this case the label is still multiply which is confusing as it is not unique, so I tried it nevertheless

Usage: verdi process show [OPTIONS] [PROCESSES]... Try 'verdi process show --help' for help. Error: Invalid value for '[PROCESSES]...': multiple ProcessNode entries found with LABEL<multiply>

I guess label is not what I think it is? Or maybe this is a bug because other processes dont have labels, only these multiply processes

The Process label in verdi process list is not the label that can be used to identify a node. The label is actually a column in the database schema and is exposed as a property, i.e., Node.label. The process label is stored as the process_label attribute and is only defined for ProcessNodes.

Regarding the error: this is not a bug, the error message is correct. The label is not guaranteed to be unique (unlike the PK and UUID) and this is intentional. So when users want to use labels to be able to identify nodes, it is up to them to guarantee they are unique.

The fact that the multiple node actually has multiple set as its label surprises me a bit tbh. It seems that the process label may also actually be set automatically as the label. I would say that this is not desirable. Will have to check in the code if and where this might be happening

sphuber

Thanks @agoscinski just left my final requests

sphuber · 2024-06-06T20:33:10Z

src/aiida/cmdline/commands/cmd_process.py

 @arguments.PROCESSES()
 @decorators.with_dbenv()
 def process_call_root(processes):
-    """Show root process of the call stack for the given processes."""
+    """Show root process(es) of the call stack for one or multiple processes."""
+    if not (processes):


Suggested change

if not (processes):

if not processes:

sphuber · 2024-06-06T20:33:20Z

src/aiida/cmdline/commands/cmd_process.py

@@ -244,6 +254,11 @@ def process_report(processes, most_recent_node, levelname, indent_size, max_dept
    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 not (processes) and not (most_recent_node):


Suggested change

if not (processes) and not (most_recent_node):

if not processes and not most_recent_node:

Please apply also in all other commands. These parentheses are redundant

sphuber · 2024-06-06T20:34:47Z

docs/source/reference/command_line.rst

@@ -348,7 +348,7 @@ Below is a list with all available subcommands.
    Options:
      --profile-name TEXT  Name of the profile. By default, a unique name starting with
                           `presto` is automatically generated.  [default: (dynamic)]
-      --email TEXT         Email of the default user.  [default: aiida@localhost]
+      --email TEXT         Email of the default user.  [default: [email protected]]


Need to revert this. This happens when you have the autofill.email option defined, which is automatically done when you run the verdi quicksetup command once. It is a bit unfortunate that this is automatically converted by the verdi-autodocs pre-commit hook. Will need to find a way somehow to ignore this change, but not sure how yet.

I defended it several times successful from being pushed, but it eventually it got in.

Removed.

sphuber · 2024-06-06T20:35:15Z

src/aiida/cmdline/commands/cmd_process.py

@@ -180,14 +180,19 @@ def process_list(
            echo.echo_report(f'Using {percent_load * 100:.0f}% of the available daemon worker slots.')


-@verdi_process.command('show')
+@verdi_process.command('show', short_help='Show details of processes.')


Please don't use the short_help and just use this as the first line of the docstring.

sphuber · 2024-06-06T20:36:06Z

src/aiida/cmdline/params/arguments/main.py

@@ -71,7 +71,7 @@

 PROCESS = OverridableArgument('process', type=types.ProcessParamType())

-PROCESSES = OverridableArgument('processes', nargs=-1, type=types.ProcessParamType())
+PROCESSES = OverridableArgument('processes', nargs=-1, type=types.ProcessParamType(), metavar='[PROCESSES]...')


This is already the autogenerated metavar

Suggested change

PROCESSES = OverridableArgument('processes', nargs=-1, type=types.ProcessParamType(), metavar='[PROCESSES]...')

PROCESSES = OverridableArgument('processes', nargs=-1, type=types.ProcessParamType())

sphuber · 2024-06-06T20:37:03Z

@agoscinski also please update the branch and address the conflict

agoscinski · 2024-06-06T21:46:43Z

Please don't squash, tried to separate the commits meaningful

The commands take one or more processes, but if no processes were provided no output was shown whatsoever which can be confusing to the user. Now an error is now shown if no processes are defined.

The `verdi process` commands pause, play, kill, wait, show, status, report, call-root had inconsistent help messages so we unified them.

sphuber · 2024-06-06T22:24:04Z

The tests-presto workflow failed. This is due to the test for verdi process watch which requires a broker, but the test profile for tests-presto intentionally doesn't define it. I have fixed it by marking that test with pytest.mark.requires_rmq.

agoscinski force-pushed the fix/6391/verdi-process-show branch 3 times, most recently from d362291 to c74ce2a Compare May 28, 2024 16:25

agoscinski marked this pull request as ready for review May 28, 2024 16:25

agoscinski requested a review from khsrali May 28, 2024 19:17

sphuber requested changes May 28, 2024

View reviewed changes

khsrali removed their request for review May 29, 2024 09:02

sphuber requested changes Jun 6, 2024

View reviewed changes

agoscinski force-pushed the fix/6391/verdi-process-show branch 3 times, most recently from 9adfb1f to 215c254 Compare June 6, 2024 21:43

agoscinski added 3 commits June 7, 2024 00:20

CLI: Fix verdi process [show|report|status|watch|call-root] no output

c4d95b0

The commands take one or more processes, but if no processes were provided no output was shown whatsoever which can be confusing to the user. Now an error is now shown if no processes are defined.

CLI: Add --most-recent-node option to verdi process watch

2665158

CLI: Unify help of verdi process commands

edde64d

The `verdi process` commands pause, play, kill, wait, show, status, report, call-root had inconsistent help messages so we unified them.

sphuber force-pushed the fix/6391/verdi-process-show branch from 215c254 to edde64d Compare June 6, 2024 22:22

sphuber merged commit d91e0a5 into aiidateam:main Jun 7, 2024
12 checks passed

agoscinski deleted the fix/6391/verdi-process-show branch June 7, 2024 09:55

	raise click.BadArgumentUsage('Please specify process primary key(s) (PK) or an option.')
	raise click.UsageError('Please specify process primary key(s) (PK) or an option.')

	if not (processes) and not (most_recent_node):
	if not processes and not most_recent_node:

	PROCESSES = OverridableArgument('processes', nargs=-1, type=types.ProcessParamType(), metavar='[PROCESSES]...')
	PROCESSES = OverridableArgument('processes', nargs=-1, type=types.ProcessParamType())

Adds output verdi process [show|report|status|watch|call-root] subcommands #6428

Adds output verdi process [show|report|status|watch|call-root] subcommands #6428

Conversation

agoscinski commented May 28, 2024 • edited by sphuber Loading

codecov bot commented May 28, 2024 • edited Loading

Codecov Report

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sphuber commented Jun 6, 2024

agoscinski commented Jun 6, 2024

sphuber commented Jun 6, 2024

Adds output `verdi process [show|report|status|watch|call-root]` subcommands #6428

Adds output `verdi process [show|report|status|watch|call-root]` subcommands #6428

agoscinski commented May 28, 2024 •

edited by sphuber

Loading

codecov bot commented May 28, 2024 •

edited

Loading

agoscinski Jun 6, 2024 •

edited

Loading