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

verdi process show: order called by ctime and use process label #4407

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 26, 2020

Fixes #4406

The command was showing the called subprocesses in a random order and
used the node type, which is often uninformative. For example, all
workchains are always shown as WorkChainNode. By using the process
label instead, which is more specific, and ordering the called nodes by
creation time, the list gives a more natural overview of the order in
which the subprocesses were called.

@codecov
Copy link

codecov bot commented Sep 26, 2020

Codecov Report

Merging #4407 into develop will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4407      +/-   ##
===========================================
+ Coverage    79.22%   79.23%   +0.01%     
===========================================
  Files          475      475              
  Lines        34826    34826              
===========================================
+ Hits         27589    27591       +2     
+ Misses        7237     7235       -2     
Flag Coverage Δ
#django 73.07% <100.00%> (-<0.01%) ⬇️
#sqlalchemy 72.30% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
aiida/cmdline/utils/common.py 67.04% <100.00%> (+0.38%) ⬆️
aiida/engine/daemon/runner.py 82.76% <0.00%> (+3.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65ad067...9d29ccd. Read the comment docs.

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, and it will be much better to see the labels rather than the class name! I only wanted to discuss 1 comment before I approve.

Comment on lines +156 to +159
if nodes_caller:
result += '\n' + format_flat_links(
sorted(nodes_caller.all(), key=lambda x: x.node.ctime), headers=['Caller', 'PK', 'Type']
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(oops, I think I forgot to add this to the review, I'll just leave it here)

This "if-path" seems not to be covered by the tests; I think the easiest way to include it would be by either adding a cli-invocation on one of the calcjob nodes + respective checks, or by adding a call link between the workchains and just adding a check after the already existing cli-invoke (I woult think the second one is the most "economical one", but I'm not sure if you had a reason for creating those two workchains and leaving one of those disconected).

Also, tangential question: a process will either have one caller or none at all, right? Why do we write this here like there could be many callers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I woult think the second one is the most "economical one", but I'm not sure if you had a reason for creating those two workchains and leaving one of those disconected

No particular reason to leave it unconnected just that I only needed one of those two to do the test. I will add the second one as the caller of the first one and add the test.

Also, tangential question: a process will either have one caller or none at all, right? Why do we write this here like there could be many callers?

Absolutely correct. There is no specific reason other than that it is just easier to write and understand compared to the nodes_called.

The command was showing the called subprocesses in a random order and
used the node type, which is often uninformative. For example, all
workchains are always shown as `WorkChainNode`. By using the process
label instead, which is more specific, and ordering the called nodes by
creation time, the list gives a more natural overview of the order in
which the subprocesses were called.
@sphuber sphuber force-pushed the fix/4406/process-show-call-links branch from 3a45ff7 to 9d29ccd Compare September 29, 2020 12:05
Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ready to go now, thanks!

@sphuber sphuber merged commit 9144924 into aiidateam:develop Sep 29, 2020
@sphuber sphuber deleted the fix/4406/process-show-call-links branch September 29, 2020 12:30
chrisjsewell pushed a commit to chrisjsewell/aiida_core that referenced this pull request Oct 21, 2020
…idateam#4407)

The command was showing the called subprocesses in a random order and
used the node type, which is often uninformative. For example, all
workchains are always shown as `WorkChainNode`. By using the process
label instead, which is more specific, and ordering the called nodes by
creation time, the list gives a more natural overview of the order in
which the subprocesses were called.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use process type and sort by creation time for caller and called links in verdi process show
2 participants