refactor(api): Split UpdateCommandAction (#14726)

api/src/opentrons/protocol_engine/actions/__init__.py

@@ -14,7 +14,8 @@
     FinishAction,
     HardwareStoppedAction,
     QueueCommandAction,
-    UpdateCommandAction,
+    RunCommandAction,
+    SucceedCommandAction,
     FailCommandAction,
     AddLabwareOffsetAction,
     AddLabwareDefinitionAction,
@@ -40,7 +41,8 @@
     "FinishAction",
     "HardwareStoppedAction",
     "QueueCommandAction",
-    "UpdateCommandAction",
+    "RunCommandAction",
+    "SucceedCommandAction",
     "FailCommandAction",
     "AddLabwareOffsetAction",
     "AddLabwareDefinitionAction",

api/src/opentrons/protocol_engine/actions/actions.py

@@ -6,8 +6,7 @@
 from dataclasses import dataclass
 from datetime import datetime
 from enum import Enum
-from typing import Optional, Union
-from opentrons.protocol_engine.error_recovery_policy import ErrorRecoveryType
+from typing import List, Optional, Union
 
 from opentrons.protocols.models import LabwareDefinition
 from opentrons.hardware_control.types import DoorState
@@ -16,6 +15,8 @@
 from opentrons_shared_data.errors import EnumeratedError
 
 from ..commands import Command, CommandCreate, CommandPrivateResult
+from ..error_recovery_policy import ErrorRecoveryType
+from ..notes.notes import CommandNote
 from ..types import (
     LabwareOffsetCreate,
     ModuleDefinition,
@@ -121,21 +122,42 @@ class QueueCommandAction:
 
 
 @dataclass(frozen=True)
-class UpdateCommandAction:
-    """Update a given command."""
+class RunCommandAction:
+    """Mark a given command as running.
+
+    At the time of dispatching this action, the command must be queued,
+    and no other command may be running.
+    """
+
+    command_id: str
+    started_at: datetime
+
+
+@dataclass(frozen=True)
+class SucceedCommandAction:
+    """Mark a given command as succeeded.
+
+    At the time of dispatching this action, the command must be running.
+    """
 
     command: Command
+    """The command in its new succeeded state."""
+
     private_result: CommandPrivateResult
 
 
 @dataclass(frozen=True)
 class FailCommandAction:
-    """Mark a given command as failed."""
+    """Mark a given command as failed.
+
+    At the time of dispatching this action, the command must be running.
+    """
 
     command_id: str
     error_id: str
     failed_at: datetime
     error: EnumeratedError
+    notes: List[CommandNote]
     type: ErrorRecoveryType
 
 
@@ -211,7 +233,8 @@ class SetPipetteMovementSpeedAction:
     HardwareStoppedAction,
     DoorChangeAction,
     QueueCommandAction,
-    UpdateCommandAction,
+    RunCommandAction,
+    SucceedCommandAction,
     FailCommandAction,
     AddLabwareOffsetAction,
     AddLabwareDefinitionAction,

api/src/opentrons/protocol_engine/execution/command_executor.py

@@ -1,7 +1,7 @@
 """Command side-effect execution logic container."""
 import asyncio
 from logging import getLogger
-from typing import Optional, List, Dict, Any, Protocol
+from typing import Optional, List, Protocol
 
 from opentrons.hardware_control import HardwareControlAPI
 
@@ -20,9 +20,13 @@
     AbstractCommandImpl,
     CommandResult,
     CommandPrivateResult,
-    Command,
 )
-from ..actions import ActionDispatcher, UpdateCommandAction, FailCommandAction
+from ..actions import (
+    ActionDispatcher,
+    RunCommandAction,
+    SucceedCommandAction,
+    FailCommandAction,
+)
 from ..errors import RunStoppedError
 from ..errors.exceptions import EStopActivatedError as PE_EStopActivatedError
 from ..notes import CommandNote, CommandNoteTracker
@@ -114,9 +118,9 @@ async def execute(self, command_id: str) -> None:
             command_id: The identifier of the command to execute. The
                 command itself will be looked up from state.
         """
-        command = self._state_store.commands.get(command_id=command_id)
+        queued_command = self._state_store.commands.get(command_id=command_id)
         note_tracker = self._command_note_tracker_provider()
-        command_impl = command._ImplementationCls(
+        command_impl = queued_command._ImplementationCls(
             state_view=self._state_store,
             hardware_api=self._hardware_api,
             equipment=self._equipment,
@@ -132,29 +136,24 @@ async def execute(self, command_id: str) -> None:
         )
 
         started_at = self._model_utils.get_timestamp()
-        running_command = command.copy(
-            update={
-                "status": CommandStatus.RUNNING,
-                "startedAt": started_at,
-            }
-        )
 
         self._action_dispatcher.dispatch(
-            UpdateCommandAction(command=running_command, private_result=None)
+            RunCommandAction(command_id=queued_command.id, started_at=started_at)
         )
+        running_command = self._state_store.commands.get(queued_command.id)
 
         try:
             log.debug(
-                f"Executing {command.id}, {command.commandType}, {command.params}"
+                f"Executing {running_command.id}, {running_command.commandType}, {running_command.params}"
             )
             if isinstance(command_impl, AbstractCommandImpl):
-                result: CommandResult = await command_impl.execute(command.params)  # type: ignore[arg-type]
+                result: CommandResult = await command_impl.execute(running_command.params)  # type: ignore[arg-type]
                 private_result: Optional[CommandPrivateResult] = None
             else:
-                result, private_result = await command_impl.execute(command.params)  # type: ignore[arg-type]
+                result, private_result = await command_impl.execute(running_command.params)  # type: ignore[arg-type]
 
         except (Exception, asyncio.CancelledError) as error:
-            log.warning(f"Execution of {command.id} failed", exc_info=error)
+            log.warning(f"Execution of {running_command.id} failed", exc_info=error)
             # TODO(mc, 2022-11-14): mark command as stopped rather than failed
             # https://opentrons.atlassian.net/browse/RCORE-390
             if isinstance(error, asyncio.CancelledError):
@@ -163,24 +162,14 @@ async def execute(self, command_id: str) -> None:
                 error = PE_EStopActivatedError(message=str(error), wrapping=[error])
             elif not isinstance(error, EnumeratedError):
                 error = PythonException(error)
-            notes_update = _append_notes_if_notes(
-                running_command, note_tracker.get_notes()
-            )
-
-            if notes_update:
-                command_with_new_notes = running_command.copy(update=notes_update)
-                self._action_dispatcher.dispatch(
-                    UpdateCommandAction(
-                        command=command_with_new_notes, private_result=None
-                    )
-                )
 
             self._action_dispatcher.dispatch(
                 FailCommandAction(
                     error=error,
-                    command_id=command_id,
+                    command_id=running_command.id,
                     error_id=self._model_utils.generate_id(),
                     failed_at=self._model_utils.get_timestamp(),
+                    notes=note_tracker.get_notes(),
                     # todo(mm, 2024-03-13):
                     # When a command fails recoverably, and we handle it with
                     # WAIT_FOR_RECOVERY or CONTINUE, we want to update our logical
@@ -199,21 +188,11 @@ async def execute(self, command_id: str) -> None:
                 "result": result,
                 "status": CommandStatus.SUCCEEDED,
                 "completedAt": self._model_utils.get_timestamp(),
-                **_append_notes_if_notes(running_command, note_tracker.get_notes()),
+                "notes": note_tracker.get_notes(),
             }
-            completed_command = running_command.copy(update=update)
+            succeeded_command = running_command.copy(update=update)
             self._action_dispatcher.dispatch(
-                UpdateCommandAction(
-                    command=completed_command, private_result=private_result
+                SucceedCommandAction(
+                    command=succeeded_command, private_result=private_result
                 ),
             )
-
-
-def _append_notes_if_notes(
-    running_command: Command, notes: List[CommandNote]
-) -> Dict[str, Any]:
-    if not notes:
-        return {}
-    if running_command.notes is None:
-        return {"notes": notes}
-    return {"notes": running_command.notes + notes}

api/src/opentrons/protocol_engine/protocol_engine.py

@@ -242,7 +242,13 @@ async def add_and_execute_command(
         await self.wait_for_command(command.id)
         return self._state_store.commands.get(command.id)
 
-    def estop(self, maintenance_run: bool) -> None:
+    def estop(
+        self,
+        # TODO(mm, 2024-03-26): Maintenance runs are a robot-server concept that
+        # ProtocolEngine should not have to know about. Can this be simplified or
+        # defined in other terms?
+        maintenance_run: bool,
+    ) -> None:
         """Signal to the engine that an estop event occurred.
 
         If there are any queued commands for the engine, they will be marked
@@ -266,6 +272,7 @@ def estop(self, maintenance_run: bool) -> None:
                 error_id=self._model_utils.generate_id(),
                 failed_at=self._model_utils.get_timestamp(),
                 error=EStopActivatedError(message="Estop Activated"),
+                notes=[],
                 type=ErrorRecoveryType.FAIL_RUN,
             )
             self._action_dispatcher.dispatch(fail_action)
@@ -279,6 +286,7 @@ def estop(self, maintenance_run: bool) -> None:
                     error_id=self._model_utils.generate_id(),
                     failed_at=self._model_utils.get_timestamp(),
                     error=EStopActivatedError(message="Estop Activated"),
+                    notes=[],
                     type=ErrorRecoveryType.FAIL_RUN,
                 )
                 self._action_dispatcher.dispatch(fail_action)

api/src/opentrons/protocol_engine/state/addressable_areas.py

@@ -35,7 +35,7 @@
     DeckConfigurationType,
     Dimensions,
 )
-from ..actions import Action, UpdateCommandAction, PlayAction, AddAddressableAreaAction
+from ..actions import Action, SucceedCommandAction, PlayAction, AddAddressableAreaAction
 from .config import Config
 from .abstract_store import HasState, HandlesActions
 
@@ -182,7 +182,7 @@ def __init__(
 
     def handle_action(self, action: Action) -> None:
         """Modify state in reaction to an action."""
-        if isinstance(action, UpdateCommandAction):
+        if isinstance(action, SucceedCommandAction):
             self._handle_command(action.command)
         elif isinstance(action, AddAddressableAreaAction):
             self._check_location_is_addressable_area(action.addressable_area)