Process: properly cleanup when exception in state transition
#5697
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
added
the
pr/blocked
|
Note this is blocked because it requires a fix in |
sphuber
force-pushed
the
fix/2861/exception-in-state-transition
branch
from
2668c0c
to
c9e9240
Compare
sphuber
force-pushed
the
fix/2861/exception-in-state-transition
branch
4 times, most recently
from
193dded
to
d837c97
Compare
sphuber
removed
the
pr/blocked
sphuber
force-pushed
the
fix/2861/exception-in-state-transition
branch
2 times, most recently
from
3086fb9
to
b5cda38
Compare
sphuber
force-pushed
the
fix/2861/exception-in-state-transition
branch
from
b5cda38
to
c9d6f0e
Compare
|
Anyone would like to review this? Otherwise I will merge this soon |
|
@sphuber I'll give it a look now. |
unkcpz
approved these changes
Nov 28, 2022
| self.node.set_process_state(state.LABEL) | ||
| self.update_outputs() |
There was a problem hiding this comment.
Not important, might be more clear move update_outputs to on_entered? I am quite surprised to realize that every state transfer will call update_outputs.
There was a problem hiding this comment.
we can see if we can move this to something like on_terminate so as to only do it once. Not sure if this would break anything but it should be more performant. Will have a look tomorrow.
If a process would hit an exception during a state transition, the node would not always be cleaned up properly. For example, when a process registers invalid outputs, the process would except in `update_outputs` which is called by `on_entered` which is called once the process has entered a new state. This exception would not be dealt with, skipping the code that is responsible for updating the process state on the node. This would lead to the node of the excepted node to still show the previous incorrect state. Typically, users would see processes as `Running` in the output of `verdi process list`. This is because that value is taken from the `process_state` attribute, but this simply contained the incorrect state. The solution requires a fix in `plumpy`, which was released with `v0.21.1` but it also requires an update in `update_node_state` of the `Process` class to make sure the `set_process_state` call is made before the `update_outputs` which is prone to excepting in the case of invalid outputs having been registered by the process implementation.
sphuber
force-pushed
the
fix/2861/exception-in-state-transition
branch
from
c9d6f0e
to
5b00a7d
Compare
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 #2861
Fixes #3334
If a process would hit an exception during a state transition, the node would not always be cleaned up properly. For example, when a process registers invalid outputs, the process would except in
update_outputswhich is called byon_enteredwhich is called once the process has entered a new state.This exception would not be dealt with, skipping the code that is responsible for updating the process state on the node. This would lead to the node of the excepted node to still show the previous incorrect state. Typically, users would see processes as
Runningin the output ofverdi process list. This is because that value is taken from theprocess_stateattribute, but this simply contained the incorrect state.The solution requires a fix in
plumpy, which was released withv0.21.but it also requires an update inupdate_node_stateof theProcessclass to make sure theset_process_statecall is made before theupdate_outputswhich is prone to excepting in the case of invalid outputs having been registered by the process implementation.