Process: Update outputs before updating node process state #5813
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sphuber
force-pushed
the
fix/nightly
branch
8 times, most recently
from
424020b to
d671488
Compare
sphuber
force-pushed
the
fix/nightly
branch
2 times, most recently
from
e91e0c0 to
508970d
Compare
In `a7be795a26aea9fdff1d6f762582f1b77801ff3d` the order of the two method calls in `Process.update_node_state` was reversed. The idea was that the `update_outputs` call can except, for example if a `Process` implementation adds an invalid output by calling `Process.out`. In this case the process will go into the excepted state and the process state would sometimes not be updated on the node. By reversing the two actions the process state was hoped to be more accurate in these cases. However, since this change, the `nightly` workflow started failing where nested workchains would except because a subprocess didn't have an output that they expected. It is not clear at all what the relation is but it seems that reverting the change described above resolves the problem. Unfortunately, the bug does not seem to be deterministic and doesn't always manifest. Here, we reverse the order to the original state, updating the outputs first, but in order to guarantee that the process state is always updated, even if the output update excepts, it is called in the finally clause of the try-except in which the `update_outputs` is wrapped.
sphuber
changed the title
|
Hi @sphuber, I don't have clue why it failed. But since it happened on |
The problem is that the bug is not deterministic. In many of the runs that failed, a majority of the workchains worked just fine, and it was a random one that failed. That's why adding an explicit unit test for this would be pointless I think. |
chrisjsewell
reviewed
Dec 5, 2022
| @@ -406,7 +406,20 @@ def on_entered(self, from_state: Optional[plumpy.process_states.State]) -> None: | |||
| """After entering a new state, save a checkpoint and update the latest process state change timestamp.""" | |||
| # pylint: disable=cyclic-import | |||
| from aiida.engine.utils import set_process_state_change_timestamp | |||
| self.update_node_state(self._state) | |||
|
|
|||
| # For reasons unknown, it is important to update the outputs first, before doing anything else, otherwise there | |||
There was a problem hiding this comment.
Perhaps this should be turned into an issue, for someone to figure out later
chrisjsewell
approved these changes
Dec 5, 2022
This was referenced Dec 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5812
In
a7be795a26aea9fdff1d6f762582f1b77801ff3dthe order of the twomethod calls in
Process.update_node_statewas reversed. The idea wasthat the
update_outputscall can except, for example if aProcessimplementation adds an invalid output by calling
Process.out. In thiscase the process will go into the excepted state and the process state
would sometimes not be updated on the node. By reversing the two actions
the process state was hoped to be more accurate in these cases.
However, since this change, the
nightlyworkflow started failing wherenested workchains would except because a subprocess didn't have an
output that they expected. It is not clear at all what the relation is
but it seems that reverting the change described above resolves the
problem. Unfortunately, the bug does not seem to be deterministic and
doesn't always manifest.
Here, we reverse the order to the original state, updating the outputs
first, but in order to guarantee that the process state is always
updated, even if the output update excepts, it is called in the finally
clause of the try-except in which the
update_outputsis wrapped.