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.

Sign up for GitHub

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

Jump to bottom

engine: implement sub-dicts for context in workchains #4871

Merged
merged 2 commits into from
Jun 4, 2021
Merged

engine: implement sub-dicts for context in workchains #4871

merged 2 commits into from
Jun 4, 2021

Conversation

dev-zero
Copy link
Contributor

@dev-zero dev-zero commented Apr 26, 2021 •
edited by sphuber

Fixes #4972

@dev-zero
Copy link
Contributor Author

dev-zero commented Apr 26, 2021 •
edited

@sphuber as discussed on Slack. Btw, there is still something fishy in the exception handling: I tried to add a test which triggers one of the AssertionError, but then the test always hang. Found the error: Awaitable are also AttributeDict, hence Duck typing failed and I was appending to an Awaitable, which then lead to a hang later on. The way I discovered it was:

  1. using py-spy to get a call stack for the hanging process and seeing that it wasn't hanging in an error handling path but waiting in nest-asyncio.
  2. using breakpoint() prior to _resolve_nested_context and then following the interpreter to discover that the assertion was not triggered but an Awaitable appended to an Awaitable

@dev-zero
Copy link
Contributor Author

also, I checked the codebase for other erroneous uses of assert, but didn't find any so far.

@codecov
Copy link

codecov bot commented Apr 26, 2021 •
edited

Codecov Report

Merging #4871 (38da950) into develop (6c159ca) will increase coverage by 0.04%.
The diff coverage is 88.47%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4871      +/-   ##
===========================================
+ Coverage    80.05%   80.09%   +0.04%     
===========================================
  Files          515      515              
  Lines        36611    36626      +15     
===========================================
+ Hits         29307    29333      +26     
+ Misses        7304     7293      -11
Flag Coverage Δ
django 74.55% <88.47%> (+0.07%) ⬆️
sqlalchemy 73.48% <88.47%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
aiida/engine/processes/workchains/workchain.py 92.99% <88.47%> (+7.09%) ⬆️
aiida/engine/processes/workchains/awaitable.py 95.00% <0.00%> (+5.00%) ⬆️

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 6c159ca...38da950. Read the comment docs.

@dev-zero dev-zero force-pushed the feature/nested_to_context branch 2 times, most recently from 93db3db to cf7a823 Compare April 27, 2021 08:18
@dev-zero dev-zero requested a review from sphuber April 27, 2021 08:18
sphuber
sphuber requested changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot @dev-zero . Overall I fully support the feature and implementation, just some minor comments. Also, it would be important to document this feature. I would suggest adding this to this section: https://aiida.readthedocs.io/projects/aiida-core/en/latest/topics/workflows/usage.html?highlight=append_#to-context

aiida/engine/processes/workchains/workchain.py Outdated Show resolved Hide resolved
aiida/engine/processes/workchains/workchain.py Outdated Show resolved Hide resolved
aiida/engine/processes/workchains/workchain.py Outdated Show resolved Hide resolved
aiida/engine/processes/workchains/workchain.py Outdated Show resolved Hide resolved
aiida/engine/processes/workchains/workchain.py Outdated Show resolved Hide resolved
tests/engine/test_work_chain.py Show resolved Hide resolved
@dev-zero dev-zero force-pushed the feature/nested_to_context branch 2 times, most recently from b60df9e to 874e018 Compare June 4, 2021 10:36
@dev-zero
engine: implement sub-dicts for context in workchains
678dd0b 
Incidentally, this also fixes an issue where no exceptions were raised
for either unknown awaitable actions or awaitable mismatches for lists
of awaitables since an `assert "some str"` is always True (hence never
triggers).
@dev-zero
docs: document the workchain context nesting
38da950
@dev-zero
Copy link
Contributor Author

dev-zero commented Jun 4, 2021

Thanks a lot @dev-zero . Overall I fully support the feature and implementation, just some minor comments. Also, it would be important to document this feature. I would suggest adding this to this section: https://aiida.readthedocs.io/projects/aiida-core/en/latest/topics/workflows/usage.html?highlight=append_#to-context

done

@dev-zero dev-zero requested a review from sphuber June 4, 2021 13:56
@sphuber sphuber merged commit 124fece into aiidateam:develop Jun 4, 2021
@sphuber
Copy link
Contributor

sphuber commented Jun 4, 2021

Thanks a lot @dev-zero

@dev-zero dev-zero deleted the feature/nested_to_context branch June 4, 2021 14:33
@chrisjsewell chrisjsewell mentioned this pull request Mar 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sphuber sphuber sphuber approved these changes

Assignees

@sphuber sphuber

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for implicit namespacing in the keys for WorkChain.to_context
2 participants
@dev-zero @sphuber