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

BaseRestartWorkChain: deal with namespaced outputs #4961

Merged

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented May 25, 2021

Fixes #4623

To also output the namespace of BaseRestartWorkChain. Instead of using out_many to dump all outputs from inner process, here I compare the outputs of sub process with the required port. The details are mentioned in #4623.

@unkcpz unkcpz requested a review from ramirezfranciscof May 25, 2021 07:04
@unkcpz

This comment has been minimized.

@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #4961 (2c380a1) into develop (2d51386) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4961      +/-   ##
===========================================
+ Coverage    80.12%   80.17%   +0.05%     
===========================================
  Files          515      515              
  Lines        36745    36746       +1     
===========================================
+ Hits         29438    29457      +19     
+ Misses        7307     7289      -18     
Flag Coverage Δ
django 74.65% <100.00%> (+0.05%) ⬆️
sqlalchemy 73.57% <100.00%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
aiida/engine/processes/workchains/restart.py 78.32% <100.00%> (+11.05%) ⬆️

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 2d51386...2c380a1. Read the comment docs.

@unkcpz unkcpz force-pushed the fix/4623/basewf-namespace branch from e3245d3 to 050ecf1 Compare July 13, 2021 07:10
@ramirezfranciscof ramirezfranciscof self-assigned this Jul 14, 2021
@ramirezfranciscof
Copy link
Member

@unkcpz hey do you know what the problem is with the pre-commits? Once that is settled I think this should be good to merge.

@unkcpz unkcpz force-pushed the fix/4623/basewf-namespace branch from 050ecf1 to 6bc67ee Compare July 22, 2021 06:50
@unkcpz
Copy link
Member Author

unkcpz commented Jul 22, 2021

@ramirezfranciscof thanks! It is now ready to be merged I hope.

@unkcpz unkcpz force-pushed the fix/4623/basewf-namespace branch from 6bc67ee to 10e2c28 Compare July 22, 2021 07:29
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.

All good now, thanks! Do you have merging privileges or do you need me to merge it? (in which case also let me know what description you want to leave in the commit message)

`BaseRestartWorkChain` does not return `output_namespaces` of
`_process_class` as describe in aiidateam#4623. It happend because in the
`results` method of `BaseRestartWorkChain`, only output keys go through by
`node.get_outgoing` are checked and returned by the parent WorkChain.

The exposed_outputs adapt to output the whole nested namespace by
`exposed_outputs`. The `out_many` method is not used here to make a
post-check for ports to remain the original exit code check and report.
@unkcpz unkcpz force-pushed the fix/4623/basewf-namespace branch from 10e2c28 to 2c380a1 Compare July 22, 2021 10:08
@unkcpz unkcpz changed the title fix #4623: BaseRestartWorkChain deal with namespaced outputs BaseRestartWorkChain: deal with namespaced outputs Jul 22, 2021
@unkcpz
Copy link
Member Author

unkcpz commented Jul 22, 2021

Thanks @ramirezfranciscof, I don't have the merging privileges, could you authorize me to do it (and direct me to where I can find the workflow to merge and things need to take care of)? As for this PR, please merge it for me and I updated the commit message. Correct it if you find any typos, must have some, I am not good at writing (you don't know what a writer I am in Chinese 🤣).

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.

Silly me last time I left a comment instead of approving.

@ramirezfranciscof
Copy link
Member

ramirezfranciscof commented Jul 22, 2021

Thanks @ramirezfranciscof, I don't have the merging privileges, could you authorize me to do it (and direct me to where I can find the workflow to merge and things need to take care of)?

I think this may be because I forgot to approve and instead just left a comment. Do you want to check again? If you still can't make it then I'll just merge it myself and then look into how to enable this for you.

As for this PR, please merge it for me and I updated the commit message. Correct it if you find any typos, must have some, I am not good at writing (you don't know what a writer I am in Chinese 🤣).

Haha, no worries, I am also not an English native speaker and I have my difficulties with the language as well =P. I adapted it a bit, could you check that the information is still correct and I didn't misinterpret anything?

FIX: namespaced outputs in `BaseRestartWorkChain` (#4961)

`BaseRestartWorkChain` did not return an `output_namespace` of the
`_process_class` as described in #4623. It happened because in its
`results` method, only the output keys are obtained from the call to
`node.get_outgoing` (checked and returned by the parent WorkChain).

This was changed for a call to `exposed_outputs`, which instead returns
the whole nested namespace. The `out_many` method is not used here 
in order to make a post-check for ports that allows to keep the original
exit code check and report.

@unkcpz
Copy link
Member Author

unkcpz commented Jul 22, 2021

@ramirezfranciscof I still can't merge.

The commit message looks great, thanks for merging it!

@ramirezfranciscof ramirezfranciscof merged commit e1abe0a into aiidateam:develop Jul 22, 2021
@unkcpz unkcpz deleted the fix/4623/basewf-namespace branch July 23, 2021 01:38
sphuber pushed a commit that referenced this pull request Aug 8, 2021
The `BaseRestartWorkChain` did not return an `output_namespace` of
its `_process_class` as described in #4623. It happened because in its
`results` method, only the output keys are obtained from the call to
`node.get_outgoing` (checked and returned by the parent WorkChain).

This was changed for a call to `exposed_outputs`, which instead returns
the whole nested namespace. The `out_many` method is not used here
in order to make a post-check for ports that allows to keep the original
exit code check and report.

Cherry-pick: e1abe0a
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.

BaseRestartWorkChain does not return output_namespaces of _process_class
2 participants