Skip to content

Commit

Permalink
Engine: Ensure node is sealed when process excepts (#6549)
Browse files Browse the repository at this point in the history
Processes that hit a certain exception were not being sealed. This would
cause problems when trying to export them, which only allows sealed
nodes. The problem occurs when another exception occurs while handling
the original exception.

An example is when `Process.update_outputs` would raise a `ValueError`
because an unstored node had be attached as an output. Since this method
is called in `on_entered`, which is called when the process entered a
new state, it would be called again when it entered the excepted state.
Since the process was already excepted, the rest of the state changes is
cut short by `plumpy`. This would cause the process to never go to the
final `TERMINATED` state and so the `on_terminated` method would not be
called, which is where the process' node is sealed.

The solution is to check the current state in `on_entered` and if it is
`EXCEPTED` to simply return and no longer perform any updates on the
node. This should prevent any other exceptions from being hit and ensure
the process transitions properly to the final terminated state. The only
update that is still performed is to update the process state on the
process' node, otherwise it would not properly be shown as excepted.

Cherry-pick: cbf672f
  • Loading branch information
sphuber committed Aug 7, 2024
1 parent 389fc48 commit e3ed9a2
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 1 deletion.
12 changes: 11 additions & 1 deletion src/aiida/engine/processes/process.py
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,19 @@ def on_create(self) -> None:
@override
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."""
from plumpy import ProcessState

from aiida.engine.utils import set_process_state_change_timestamp

super().on_entered(from_state)

if self._state.LABEL is ProcessState.EXCEPTED:
# The process is already excepted so simply update the process state on the node and let the process
# complete the state transition to the terminal state. If another exception is raised during this exception
# handling, the process transitioning is cut short and never makes it to the terminal state.
self.node.set_process_state(self._state.LABEL)
return

# For reasons unknown, it is important to update the outputs first, before doing anything else, otherwise there
# is the risk that certain outputs do not get attached before the process reaches a terminal state. Nevertheless
# we need to guarantee that the process state gets updated even if the ``update_outputs`` call excepts, for
Expand All @@ -431,7 +442,6 @@ def on_entered(self, from_state: Optional[plumpy.process_states.State]) -> None:

self._save_checkpoint()
set_process_state_change_timestamp(self.node)
super().on_entered(from_state)

@override
def on_terminated(self) -> None:
Expand Down
1 change: 1 addition & 0 deletions tests/engine/test_work_chain.py
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ def illegal(self):
orm.QueryBuilder().append(orm.ProcessNode, tag='node').order_by({'node': {'id': 'desc'}}).first(flat=True)
)
assert node.is_excepted
assert node.is_sealed
assert 'ValueError: Workflow<IllegalWorkChain> tried returning an unstored `Data` node.' in node.exception

def test_same_input_node(self):
Expand Down

0 comments on commit e3ed9a2

Please sign in to comment.