diff --git a/api/src/opentrons/protocol_engine/actions/__init__.py b/api/src/opentrons/protocol_engine/actions/__init__.py index 2155b67190d..9d7c20c1ccf 100644 --- a/api/src/opentrons/protocol_engine/actions/__init__.py +++ b/api/src/opentrons/protocol_engine/actions/__init__.py @@ -10,11 +10,13 @@ PlayAction, PauseAction, StopAction, + FinishAction, + HardwareStoppedAction, QueueCommandAction, UpdateCommandAction, FailCommandAction, AddLabwareOffsetAction, - StopErrorDetails, + FinishErrorDetails, ) __all__ = [ @@ -27,10 +29,12 @@ "PlayAction", "PauseAction", "StopAction", + "FinishAction", + "HardwareStoppedAction", "QueueCommandAction", "UpdateCommandAction", "FailCommandAction", "AddLabwareOffsetAction", # action payload values - "StopErrorDetails", + "FinishErrorDetails", ] diff --git a/api/src/opentrons/protocol_engine/actions/actions.py b/api/src/opentrons/protocol_engine/actions/actions.py index 4baf0c371e6..2622ec08761 100644 --- a/api/src/opentrons/protocol_engine/actions/actions.py +++ b/api/src/opentrons/protocol_engine/actions/actions.py @@ -23,8 +23,16 @@ class PauseAction: @dataclass(frozen=True) -class StopErrorDetails: - """Error details for the payload of a StopAction.""" +class StopAction: + """Stop the current engine execution. + + After a StopAction, the engine status will be marked as stopped. + """ + + +@dataclass(frozen=True) +class FinishErrorDetails: + """Error details for the payload of a FinishAction.""" error: Exception error_id: str @@ -32,10 +40,18 @@ class StopErrorDetails: @dataclass(frozen=True) -class StopAction: - """Stop processing commands in the engine, marking the engine status as done.""" +class FinishAction: + """Gracefully stop processing commands in the engine. + + After a FinishAction, the engine status will be marked as succeeded or failed. + """ + + error_details: Optional[FinishErrorDetails] = None - error_details: Optional[StopErrorDetails] = None + +@dataclass(frozen=True) +class HardwareStoppedAction: + """An action dispatched after hardware has successfully been stopped.""" @dataclass(frozen=True) @@ -79,6 +95,8 @@ class AddLabwareOffsetAction: PlayAction, PauseAction, StopAction, + FinishAction, + HardwareStoppedAction, QueueCommandAction, UpdateCommandAction, FailCommandAction, diff --git a/api/src/opentrons/protocol_engine/protocol_engine.py b/api/src/opentrons/protocol_engine/protocol_engine.py index 757c7c6a7c5..9e84b4ba666 100644 --- a/api/src/opentrons/protocol_engine/protocol_engine.py +++ b/api/src/opentrons/protocol_engine/protocol_engine.py @@ -13,9 +13,11 @@ PlayAction, PauseAction, StopAction, - StopErrorDetails, + FinishAction, + FinishErrorDetails, QueueCommandAction, AddLabwareOffsetAction, + HardwareStoppedAction, ) @@ -126,16 +128,16 @@ async def add_and_execute_command(self, request: CommandCreate) -> Command: return self._state_store.commands.get(command.id) - async def halt(self) -> None: - """Halt execution, stopping all motion and cancelling future commands. + async def stop(self) -> None: + """Stop execution immediately, halting all motion and cancelling future commands. - You should call `stop` after calling `halt` for cleanup and to allow - the engine to settle and recover. + After an engine has been `stop`'ed, it cannot be restarted. """ self._action_dispatcher.dispatch(StopAction()) self._queue_worker.cancel() await self._hardware_api.halt() await self._hardware_api.stop(home_after=False) + self._action_dispatcher.dispatch(HardwareStoppedAction()) async def wait_until_complete(self) -> None: """Wait until there are no more commands to execute. @@ -146,11 +148,11 @@ async def wait_until_complete(self) -> None: condition=self._state_store.commands.get_all_complete ) - async def stop(self, error: Optional[Exception] = None) -> None: - """Gracefully stop the ProtocolEngine, waiting for it to become idle. + async def finish(self, error: Optional[Exception] = None) -> None: + """Gracefully finish using the ProtocolEngine, waiting for it to become idle. The engine will finish executing its current command (if any), - and then shut down. After an engine has been `stop`'ed, it cannot + and then shut down. After an engine has been `finished`'ed, it cannot be restarted. This method should not raise, but if any exceptions happen during @@ -161,7 +163,7 @@ async def stop(self, error: Optional[Exception] = None) -> None: error: An error that caused the stop, if applicable. """ if error: - error_details: Optional[StopErrorDetails] = StopErrorDetails( + error_details: Optional[FinishErrorDetails] = FinishErrorDetails( error_id=self._model_utils.generate_id(), created_at=self._model_utils.get_timestamp(), error=error, @@ -169,13 +171,14 @@ async def stop(self, error: Optional[Exception] = None) -> None: else: error_details = None - self._action_dispatcher.dispatch(StopAction(error_details=error_details)) + self._action_dispatcher.dispatch(FinishAction(error_details=error_details)) try: await self._queue_worker.join() finally: await self._hardware_api.stop(home_after=False) + self._action_dispatcher.dispatch(HardwareStoppedAction()) self._plugin_starter.stop() def add_labware_offset(self, request: LabwareOffsetCreate) -> LabwareOffset: diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index f0dfca1e53d..922c0f46018 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -12,6 +12,8 @@ PlayAction, PauseAction, StopAction, + FinishAction, + HardwareStoppedAction, ) from ..commands import Command, CommandStatus @@ -27,10 +29,28 @@ @dataclass(frozen=True) class CommandState: - """State of all protocol engine command resources.""" - - is_running: bool - stop_requested: bool + """State of all protocol engine command resources. + + Attributes: + is_running_queue: Whether the engine is currently pulling new + commands off the queue to execute. A command may still be + executing, and the robot may still be in motion, even if False. + is_hardware_stopped: Whether the engine's hardware has ceased + motion. Once set, this flag cannot be unset. + should_stop: Whether a graceful finish or an ungraceful stop has + been requested. Once set, this flag cannot be unset. + should_report_result: Whether the engine should report a success or + failure status once stopped. If unset, the engine will simply + report "stopped." Once set, this flag cannot be unset. + commands_by_id: All command resources, in insertion order, mapped + by their unique IDs. + errors_by_id: All error occurrences, mapped by their unique IDs. + """ + + is_running_queue: bool + is_hardware_stopped: bool + should_stop: bool + should_report_result: bool # TODO(mc, 2021-06-16): OrderedDict is mutable. Switch to Sequence + Mapping commands_by_id: OrderedDict[str, Command] errors_by_id: Mapping[str, ErrorOccurrence] @@ -44,13 +64,15 @@ class CommandStore(HasState[CommandState], HandlesActions): def __init__(self) -> None: """Initialize a CommandStore and its state.""" self._state = CommandState( - is_running=True, - stop_requested=False, + is_running_queue=True, + is_hardware_stopped=False, + should_stop=False, + should_report_result=False, commands_by_id=OrderedDict(), errors_by_id={}, ) - def handle_action(self, action: Action) -> None: + def handle_action(self, action: Action) -> None: # noqa: C901 """Modify state in reaction to an action.""" errors_by_id: Mapping[str, ErrorOccurrence] @@ -103,38 +125,57 @@ def handle_action(self, action: Action) -> None: ) elif isinstance(action, PlayAction): - if not self._state.stop_requested: - self._state = replace(self._state, is_running=True) + if not self._state.should_stop: + self._state = replace(self._state, is_running_queue=True) elif isinstance(action, PauseAction): - self._state = replace(self._state, is_running=False) + self._state = replace(self._state, is_running_queue=False) elif isinstance(action, StopAction): - # any `ProtocolEngineError`'s will be captured by `FailCommandAction`, - # so only capture unknown errors here - if action.error_details and not isinstance( - action.error_details.error, - ProtocolEngineError, - ): - errors_by_id = dict(self._state.errors_by_id) - error_id = action.error_details.error_id - created_at = action.error_details.created_at - error = action.error_details.error - - errors_by_id[error_id] = ErrorOccurrence( - id=error_id, - createdAt=created_at, - errorType=type(error).__name__, - detail=str(error), + if not self._state.should_stop: + self._state = replace( + self._state, + is_running_queue=False, + should_report_result=False, + should_stop=True, + ) + + elif isinstance(action, FinishAction): + if not self._state.should_stop: + # any `ProtocolEngineError`'s will be captured by `FailCommandAction`, + # so only capture unknown errors here + if action.error_details and not isinstance( + action.error_details.error, + ProtocolEngineError, + ): + errors_by_id = dict(self._state.errors_by_id) + error_id = action.error_details.error_id + created_at = action.error_details.created_at + error = action.error_details.error + + errors_by_id[error_id] = ErrorOccurrence( + id=error_id, + createdAt=created_at, + errorType=type(error).__name__, + detail=str(error), + ) + else: + errors_by_id = self._state.errors_by_id + + self._state = replace( + self._state, + is_running_queue=False, + should_report_result=True, + should_stop=True, + errors_by_id=errors_by_id, ) - else: - errors_by_id = self._state.errors_by_id + elif isinstance(action, HardwareStoppedAction): self._state = replace( self._state, - is_running=False, - stop_requested=True, - errors_by_id=errors_by_id, + is_running_queue=False, + is_hardware_stopped=True, + should_stop=True, ) @@ -176,10 +217,10 @@ def get_next_queued(self) -> Optional[str]: Raises: EngineStoppedError: """ - if self._state.stop_requested: + if self._state.should_stop: raise ProtocolEngineStoppedError("Engine was stopped") - if not self._state.is_running: + if not self._state.is_running_queue: return None for command_id, command in self._state.commands_by_id.items(): @@ -192,7 +233,7 @@ def get_next_queued(self) -> Optional[str]: def get_is_running(self) -> bool: """Get whether the engine is running and queued commands should be executed.""" - return self._state.is_running + return self._state.is_running_queue def get_is_complete(self, command_id: str) -> bool: """Get whether a given command is completed. @@ -236,15 +277,16 @@ def get_stop_requested(self) -> bool: A command may still be executing while the engine is stopping. """ - return self._state.stop_requested + return self._state.should_stop def get_is_stopped(self) -> bool: """Get whether an engine stop has completed.""" - return self._state.stop_requested and not any( + return self._state.should_stop and not any( c.status == CommandStatus.RUNNING for c in self._state.commands_by_id.values() ) + # TODO(mc, 2021-12-07): reject adding commands to a stopped engine def validate_action_allowed(self, action: Union[PlayAction, PauseAction]) -> None: """Validate if a PlayAction or PauseAction is allowed, raising if not. @@ -253,7 +295,7 @@ def validate_action_allowed(self, action: Union[PlayAction, PauseAction]) -> Non Raises: ProtocolEngineStoppedError: the engine has been stopped. """ - if self._state.stop_requested: + if self._state.should_stop: action_desc = "play" if isinstance(action, PlayAction) else "pause" raise ProtocolEngineStoppedError(f"Cannot {action_desc} a stopped engine.") @@ -263,20 +305,21 @@ def get_status(self) -> EngineStatus: all_errors = self._state.errors_by_id.values() all_statuses = [c.status for c in all_commands] - if self._state.stop_requested: - if any(all_errors): + if self._state.should_report_result: + if not self._state.is_hardware_stopped: + return EngineStatus.RUNNING + elif any(all_errors): return EngineStatus.FAILED - - if all(s == CommandStatus.SUCCEEDED for s in all_statuses): + else: return EngineStatus.SUCCEEDED - elif any(s == CommandStatus.RUNNING for s in all_statuses): + elif self._state.should_stop: + if not self._state.is_hardware_stopped: return EngineStatus.STOP_REQUESTED - else: return EngineStatus.STOPPED - elif self._state.is_running: + elif self._state.is_running_queue: any_running = any(s == CommandStatus.RUNNING for s in all_statuses) any_queued = any(s == CommandStatus.QUEUED for s in all_statuses) @@ -287,8 +330,4 @@ def get_status(self) -> EngineStatus: return EngineStatus.IDLE else: - if any(s == CommandStatus.RUNNING for s in all_statuses): - return EngineStatus.PAUSE_REQUESTED - - else: - return EngineStatus.PAUSED + return EngineStatus.PAUSED diff --git a/api/src/opentrons/protocol_engine/types.py b/api/src/opentrons/protocol_engine/types.py index cda6fb93d38..e4452a3945d 100644 --- a/api/src/opentrons/protocol_engine/types.py +++ b/api/src/opentrons/protocol_engine/types.py @@ -14,7 +14,6 @@ class EngineStatus(str, Enum): IDLE = "idle" RUNNING = "running" - PAUSE_REQUESTED = "pause-requested" PAUSED = "paused" STOP_REQUESTED = "stop-requested" STOPPED = "stopped" diff --git a/api/src/opentrons/protocol_runner/protocol_runner.py b/api/src/opentrons/protocol_runner/protocol_runner.py index b641bc13cf2..5686a8fd8b2 100644 --- a/api/src/opentrons/protocol_runner/protocol_runner.py +++ b/api/src/opentrons/protocol_runner/protocol_runner.py @@ -115,9 +115,7 @@ def load(self, protocol_source: ProtocolSource) -> None: # ensure the engine is stopped gracefully once the # protocol file stops issuing commands - self._task_queue.set_cleanup_func( - func=self._protocol_engine.stop, - ) + self._task_queue.set_cleanup_func(self._protocol_engine.finish) def play(self) -> None: """Start or resume the run.""" @@ -131,7 +129,7 @@ def pause(self) -> None: async def stop(self) -> None: """Stop (cancel) the run.""" self._task_queue.stop() - await self._protocol_engine.halt() + await self._protocol_engine.stop() async def join(self) -> None: """Wait for the run to complete, propagating any errors. diff --git a/api/src/opentrons/protocol_runner/task_queue.py b/api/src/opentrons/protocol_runner/task_queue.py index e2e68735831..b1a501c70b7 100644 --- a/api/src/opentrons/protocol_runner/task_queue.py +++ b/api/src/opentrons/protocol_runner/task_queue.py @@ -79,14 +79,17 @@ async def _run(self) -> None: try: if self._run_func is not None: await self._run_func() + except asyncio.CancelledError: + log.debug("Run task was cancelled") + raise except Exception as e: log.debug( "Exception raised during protocol run", exc_info=error, ) error = e - finally: - if self._cleanup_func is not None: - await self._cleanup_func(error=error) - elif error: - raise error + + if self._cleanup_func is not None: + await self._cleanup_func(error=error) + elif error: + raise error diff --git a/api/tests/opentrons/protocol_engine/state/test_command_store.py b/api/tests/opentrons/protocol_engine/state/test_command_store.py index 81feda9270f..8a60290bf8a 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_store.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_store.py @@ -16,8 +16,10 @@ FailCommandAction, PlayAction, PauseAction, + FinishAction, + FinishErrorDetails, StopAction, - StopErrorDetails, + HardwareStoppedAction, ) from .command_fixtures import ( @@ -33,8 +35,10 @@ def test_initial_state() -> None: subject = CommandStore() assert subject.state == CommandState( - is_running=True, - stop_requested=False, + is_running_queue=True, + should_report_result=False, + is_hardware_stopped=False, + should_stop=False, commands_by_id=OrderedDict(), errors_by_id={}, ) @@ -201,8 +205,10 @@ def test_command_store_handles_pause_action() -> None: subject.handle_action(PauseAction()) assert subject.state == CommandState( - is_running=False, - stop_requested=False, + is_running_queue=False, + should_report_result=False, + is_hardware_stopped=False, + should_stop=False, commands_by_id=OrderedDict(), errors_by_id={}, ) @@ -215,75 +221,102 @@ def test_command_store_handles_play_action() -> None: subject.handle_action(PlayAction()) assert subject.state == CommandState( - is_running=True, - stop_requested=False, + is_running_queue=True, + should_report_result=False, + is_hardware_stopped=False, + should_stop=False, + commands_by_id=OrderedDict(), + errors_by_id={}, + ) + + +def test_command_store_handles_finish_action() -> None: + """It should clear the running flag and set the done flag on FinishAction.""" + subject = CommandStore() + + subject.handle_action(PlayAction()) + subject.handle_action(FinishAction()) + + assert subject.state == CommandState( + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=False, + should_stop=True, commands_by_id=OrderedDict(), errors_by_id={}, ) def test_command_store_handles_stop_action() -> None: - """It should clear the running flag and set the done flag on stop.""" + """It should mark the engine as non-gracefully stopped on StopAction.""" subject = CommandStore() subject.handle_action(PlayAction()) subject.handle_action(StopAction()) assert subject.state == CommandState( - is_running=False, - stop_requested=True, + is_running_queue=False, + should_report_result=False, + is_hardware_stopped=False, + should_stop=True, commands_by_id=OrderedDict(), errors_by_id={}, ) -def test_command_store_cannot_restart_after_stop() -> None: +def test_command_store_cannot_restart_after_should_stop() -> None: """It should reject a play action after a stop action.""" subject = CommandStore() - subject.handle_action(StopAction()) + subject.handle_action(FinishAction()) subject.handle_action(PlayAction()) assert subject.state == CommandState( - is_running=False, - stop_requested=True, + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=False, + should_stop=True, commands_by_id=OrderedDict(), errors_by_id={}, ) -def test_command_store_ignores_known_stop_error() -> None: +def test_command_store_ignores_known_finish_error() -> None: """It not store a ProtocolEngineError that comes in with the stop action.""" subject = CommandStore() - error_details = StopErrorDetails( + error_details = FinishErrorDetails( error=errors.ProtocolEngineError("oh no"), error_id="error-id", created_at=datetime(year=2021, month=1, day=1), ) - subject.handle_action(StopAction(error_details=error_details)) + subject.handle_action(FinishAction(error_details=error_details)) assert subject.state == CommandState( - is_running=False, - stop_requested=True, + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=False, + should_stop=True, commands_by_id=OrderedDict(), errors_by_id={}, ) -def test_command_store_saves_unknown_stop_error() -> None: +def test_command_store_saves_unknown_finish_error() -> None: """It not store a ProtocolEngineError that comes in with the stop action.""" subject = CommandStore() - error_details = StopErrorDetails( + error_details = FinishErrorDetails( error=RuntimeError("oh no"), error_id="error-id", created_at=datetime(year=2021, month=1, day=1), ) - subject.handle_action(StopAction(error_details=error_details)) + subject.handle_action(FinishAction(error_details=error_details)) assert subject.state == CommandState( - is_running=False, - stop_requested=True, + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=False, + should_stop=True, commands_by_id=OrderedDict(), errors_by_id={ "error-id": errors.ErrorOccurrence( @@ -296,6 +329,42 @@ def test_command_store_saves_unknown_stop_error() -> None: ) +def test_command_store_ignores_stop_after_graceful_finish() -> None: + """It should no-op on stop if already gracefully finished.""" + subject = CommandStore() + + subject.handle_action(PlayAction()) + subject.handle_action(FinishAction()) + subject.handle_action(StopAction()) + + assert subject.state == CommandState( + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=False, + should_stop=True, + commands_by_id=OrderedDict(), + errors_by_id={}, + ) + + +def test_command_store_ignores_finish_after_non_graceful_stop() -> None: + """It should no-op on finish if already ungracefully stopped.""" + subject = CommandStore() + + subject.handle_action(PlayAction()) + subject.handle_action(StopAction()) + subject.handle_action(FinishAction()) + + assert subject.state == CommandState( + is_running_queue=False, + should_report_result=False, + is_hardware_stopped=False, + should_stop=True, + commands_by_id=OrderedDict(), + errors_by_id={}, + ) + + def test_command_store_handles_command_failed() -> None: """It should store an error and mark the command if it fails.""" command = create_running_command(command_id="command-id") @@ -317,8 +386,10 @@ def test_command_store_handles_command_failed() -> None: ) assert subject.state == CommandState( - is_running=True, - stop_requested=False, + is_running_queue=True, + should_report_result=False, + is_hardware_stopped=False, + should_stop=False, commands_by_id=OrderedDict([("command-id", expected_failed_command)]), errors_by_id={ "error-id": errors.ErrorOccurrence( @@ -329,3 +400,18 @@ def test_command_store_handles_command_failed() -> None: ) }, ) + + +def test_handles_hardware_stopped() -> None: + """It should mark the hardware as stopped on HardwareStoppedAction.""" + subject = CommandStore() + subject.handle_action(HardwareStoppedAction()) + + assert subject.state == CommandState( + is_running_queue=False, + should_report_result=False, + is_hardware_stopped=True, + should_stop=True, + commands_by_id=OrderedDict(), + errors_by_id={}, + ) diff --git a/api/tests/opentrons/protocol_engine/state/test_command_view.py b/api/tests/opentrons/protocol_engine/state/test_command_view.py index d0dfee76831..a7ec5ccdad6 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_view.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_view.py @@ -18,15 +18,19 @@ def get_command_view( - is_running: bool = False, - stop_requested: bool = False, + is_running_queue: bool = False, + should_report_result: bool = False, + is_hardware_stopped: bool = False, + should_stop: bool = False, commands_by_id: Sequence[Tuple[str, cmd.Command]] = (), errors_by_id: Optional[Mapping[str, errors.ErrorOccurrence]] = None, ) -> CommandView: """Get a command view test subject.""" state = CommandState( - is_running=is_running, - stop_requested=stop_requested, + is_running_queue=is_running_queue, + should_report_result=should_report_result, + is_hardware_stopped=is_hardware_stopped, + should_stop=should_stop, commands_by_id=OrderedDict(commands_by_id), errors_by_id=errors_by_id or {}, ) @@ -75,7 +79,7 @@ def test_get_next_queued_returns_first_pending() -> None: completed_command = create_completed_command() subject = get_command_view( - is_running=True, + is_running_queue=True, commands_by_id=[ ("command-id-1", running_command), ("command-id-2", completed_command), @@ -92,12 +96,12 @@ def test_get_next_queued_returns_none_when_no_pending() -> None: running_command = create_running_command(command_id="command-id-1") completed_command = create_completed_command(command_id="command-id-2") - subject = get_command_view(is_running=True) + subject = get_command_view(is_running_queue=True) assert subject.get_next_queued() is None subject = get_command_view( - is_running=True, + is_running_queue=True, commands_by_id=[ ("command-id-1", running_command), ("command-id-2", completed_command), @@ -112,7 +116,7 @@ def test_get_next_queued_returns_none_if_not_running() -> None: pending_command = create_pending_command() subject = get_command_view( - is_running=False, + is_running_queue=False, commands_by_id=[("command-id", pending_command)], ) result = subject.get_next_queued() @@ -128,7 +132,7 @@ def test_get_next_queued_raises_when_earlier_command_failed() -> None: pending_command = create_pending_command(command_id="command-id-4") subject = get_command_view( - is_running=True, + is_running_queue=True, commands_by_id=[ ("command-id-1", running_command), ("command-id-2", completed_command), @@ -143,18 +147,18 @@ def test_get_next_queued_raises_when_earlier_command_failed() -> None: def test_get_next_queued_raises_if_stopped() -> None: """It should raise if an engine stop has been requested.""" - subject = get_command_view(stop_requested=True) + subject = get_command_view(should_stop=True) with pytest.raises(errors.ProtocolEngineStoppedError): subject.get_next_queued() -def test_get_is_running() -> None: +def test_get_is_running_queue() -> None: """It should be able to get if the engine is running.""" - subject = get_command_view(is_running=False) + subject = get_command_view(is_running_queue=False) assert subject.get_is_running() is False - subject = get_command_view(is_running=True) + subject = get_command_view(is_running_queue=True) assert subject.get_is_running() is True @@ -229,12 +233,12 @@ def test_get_all_complete() -> None: assert subject.get_all_complete() is True -def test_get_stop_requested() -> None: - """It should return true if the stop_requested flag is set.""" - subject = get_command_view(stop_requested=True) +def test_get_should_stop() -> None: + """It should return true if the should_stop flag is set.""" + subject = get_command_view(should_stop=True) assert subject.get_stop_requested() is True - subject = get_command_view(stop_requested=False) + subject = get_command_view(should_stop=False) assert subject.get_stop_requested() is False @@ -246,19 +250,19 @@ def test_get_is_stopped() -> None: failed_command = create_failed_command(command_id="command-id-4") subject = get_command_view( - stop_requested=False, + should_stop=False, commands_by_id=(), ) assert subject.get_is_stopped() is False subject = get_command_view( - stop_requested=True, + should_stop=True, commands_by_id=[("command-id-2", running_command)], ) assert subject.get_is_stopped() is False subject = get_command_view( - stop_requested=True, + should_stop=True, commands_by_id=[ ("command-id-1", completed_command), ("command-id-3", pending_command), @@ -278,32 +282,32 @@ class ActionAllowedSpec(NamedTuple): action_allowed_specs: List[ActionAllowedSpec] = [ ActionAllowedSpec( - subject=get_command_view(stop_requested=False, is_running=False), + subject=get_command_view(should_stop=False, is_running_queue=False), action=PlayAction(), expected_error=None, ), ActionAllowedSpec( - subject=get_command_view(stop_requested=False, is_running=True), + subject=get_command_view(should_stop=False, is_running_queue=True), action=PlayAction(), expected_error=None, ), ActionAllowedSpec( - subject=get_command_view(stop_requested=True, is_running=False), + subject=get_command_view(should_stop=True, is_running_queue=False), action=PlayAction(), expected_error=errors.ProtocolEngineStoppedError, ), ActionAllowedSpec( - subject=get_command_view(stop_requested=False, is_running=False), + subject=get_command_view(should_stop=False, is_running_queue=False), action=PauseAction(), expected_error=None, ), ActionAllowedSpec( - subject=get_command_view(stop_requested=False, is_running=True), + subject=get_command_view(should_stop=False, is_running_queue=True), action=PauseAction(), expected_error=None, ), ActionAllowedSpec( - subject=get_command_view(stop_requested=True, is_running=False), + subject=get_command_view(should_stop=True, is_running_queue=False), action=PauseAction(), expected_error=errors.ProtocolEngineStoppedError, ), @@ -339,7 +343,7 @@ def test_get_errors() -> None: ) subject = get_command_view( - stop_requested=False, + should_stop=False, commands_by_id=(), errors_by_id={"error-1": error_1, "error-2": error_2}, ) @@ -357,40 +361,54 @@ class GetStatusSpec(NamedTuple): get_status_specs: List[GetStatusSpec] = [ GetStatusSpec( subject=get_command_view( - is_running=True, - stop_requested=False, + is_running_queue=True, + should_report_result=False, + should_stop=False, commands_by_id=[], ), expected_status=EngineStatus.IDLE, ), GetStatusSpec( subject=get_command_view( - is_running=True, - stop_requested=False, + is_running_queue=True, + should_report_result=False, + should_stop=False, commands_by_id=[("command-id", create_pending_command())], ), expected_status=EngineStatus.RUNNING, ), GetStatusSpec( subject=get_command_view( - is_running=True, - stop_requested=False, + is_running_queue=True, + should_report_result=False, + should_stop=False, commands_by_id=[("command-id", create_running_command())], ), expected_status=EngineStatus.RUNNING, ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=False, + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=False, + should_stop=True, + commands_by_id=[], + ), + expected_status=EngineStatus.RUNNING, + ), + GetStatusSpec( + subject=get_command_view( + is_running_queue=False, + should_report_result=False, + should_stop=False, commands_by_id=[("command-id", create_running_command())], ), - expected_status=EngineStatus.PAUSE_REQUESTED, + expected_status=EngineStatus.PAUSED, ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=False, + is_running_queue=False, + should_stop=False, commands_by_id=[ ("command-id-1", create_completed_command()), ("command-id-2", create_pending_command()), @@ -400,32 +418,37 @@ class GetStatusSpec(NamedTuple): ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=False, + is_running_queue=False, + should_report_result=False, + should_stop=False, commands_by_id=[], ), expected_status=EngineStatus.PAUSED, ), GetStatusSpec( subject=get_command_view( - is_running=True, - stop_requested=False, + is_running_queue=True, + should_report_result=False, + should_stop=False, commands_by_id=[("command-id", create_failed_command())], ), expected_status=EngineStatus.IDLE, ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=False, + is_running_queue=False, + should_report_result=False, + should_stop=False, commands_by_id=[("command-id", create_failed_command())], ), expected_status=EngineStatus.PAUSED, ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=True, + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=True, + should_stop=True, errors_by_id={ "error-id": errors.ErrorOccurrence( id="error-id", @@ -439,36 +462,41 @@ class GetStatusSpec(NamedTuple): ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=True, + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=True, + should_stop=True, commands_by_id=[], ), expected_status=EngineStatus.SUCCEEDED, ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=True, + is_running_queue=False, + should_report_result=True, + is_hardware_stopped=True, + should_stop=True, commands_by_id=[("command-id", create_completed_command())], ), expected_status=EngineStatus.SUCCEEDED, ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=True, - commands_by_id=[("command-id", create_running_command())], + is_running_queue=False, + should_report_result=False, + is_hardware_stopped=False, + should_stop=True, + commands_by_id=[], ), expected_status=EngineStatus.STOP_REQUESTED, ), GetStatusSpec( subject=get_command_view( - is_running=False, - stop_requested=True, - commands_by_id=[ - ("command-id", create_completed_command()), - ("command-id", create_pending_command()), - ], + is_running_queue=False, + should_report_result=False, + is_hardware_stopped=True, + should_stop=True, + commands_by_id=[], ), expected_status=EngineStatus.STOPPED, ), diff --git a/api/tests/opentrons/protocol_engine/test_protocol_engine.py b/api/tests/opentrons/protocol_engine/test_protocol_engine.py index d1f1d24a067..323c358e3ab 100644 --- a/api/tests/opentrons/protocol_engine/test_protocol_engine.py +++ b/api/tests/opentrons/protocol_engine/test_protocol_engine.py @@ -24,8 +24,10 @@ PlayAction, PauseAction, StopAction, - StopErrorDetails, + FinishAction, + FinishErrorDetails, QueueCommandAction, + HardwareStoppedAction, ) @@ -228,7 +230,7 @@ def test_pause( ) -async def test_stop( +async def test_finish( decoy: Decoy, action_dispatcher: ActionDispatcher, plugin_starter: PluginStarter, @@ -236,18 +238,19 @@ async def test_stop( hardware_api: HardwareAPI, subject: ProtocolEngine, ) -> None: - """It should be able to stop the engine.""" - await subject.stop() + """It should be able to gracefully tell the engine it's done.""" + await subject.finish() decoy.verify( - action_dispatcher.dispatch(StopAction()), + action_dispatcher.dispatch(FinishAction()), await queue_worker.join(), await hardware_api.stop(home_after=False), + action_dispatcher.dispatch(HardwareStoppedAction()), plugin_starter.stop(), ) -async def test_stop_with_error( +async def test_finish_with_error( decoy: Decoy, action_dispatcher: ActionDispatcher, queue_worker: QueueWorker, @@ -255,9 +258,9 @@ async def test_stop_with_error( model_utils: ModelUtils, subject: ProtocolEngine, ) -> None: - """It should be able to stop the engine with an error.""" + """It should be able to tell the engine it's finished because of an error.""" error = RuntimeError("oh no") - expected_error_details = StopErrorDetails( + expected_error_details = FinishErrorDetails( error_id="error-id", created_at=datetime(year=2021, month=1, day=1), error=error, @@ -268,16 +271,17 @@ async def test_stop_with_error( datetime(year=2021, month=1, day=1) ) - await subject.stop(error=error) + await subject.finish(error=error) decoy.verify( - action_dispatcher.dispatch(StopAction(error_details=expected_error_details)), + action_dispatcher.dispatch(FinishAction(error_details=expected_error_details)), await queue_worker.join(), await hardware_api.stop(home_after=False), + action_dispatcher.dispatch(HardwareStoppedAction()), ) -async def test_stop_stops_hardware_if_queue_worker_join_fails( +async def test_finish_stops_hardware_if_queue_worker_join_fails( decoy: Decoy, queue_worker: QueueWorker, hardware_api: HardwareAPI, @@ -289,7 +293,7 @@ async def test_stop_stops_hardware_if_queue_worker_join_fails( ).then_raise(RuntimeError("oh no")) with pytest.raises(RuntimeError, match="oh no"): - await subject.stop() + await subject.finish() decoy.verify( await hardware_api.stop(home_after=False), @@ -310,21 +314,22 @@ async def test_wait_until_complete( ) -async def test_halt( +async def test_stop( decoy: Decoy, action_dispatcher: ActionDispatcher, queue_worker: QueueWorker, hardware_api: HardwareAPI, subject: ProtocolEngine, ) -> None: - """It should be able to halt the engine.""" - await subject.halt() + """It should be able to stop the engine.""" + await subject.stop() decoy.verify( action_dispatcher.dispatch(StopAction()), queue_worker.cancel(), await hardware_api.halt(), await hardware_api.stop(home_after=False), + action_dispatcher.dispatch(HardwareStoppedAction()), ) diff --git a/api/tests/opentrons/protocol_runner/test_python_context_creator.py b/api/tests/opentrons/protocol_runner/smoke_tests/test_python_context_creator.py similarity index 98% rename from api/tests/opentrons/protocol_runner/test_python_context_creator.py rename to api/tests/opentrons/protocol_runner/smoke_tests/test_python_context_creator.py index a049574ff40..2d308d37837 100644 --- a/api/tests/opentrons/protocol_runner/test_python_context_creator.py +++ b/api/tests/opentrons/protocol_runner/smoke_tests/test_python_context_creator.py @@ -20,7 +20,7 @@ async def protocol_engine(hardware: HardwareAPI) -> AsyncIterable[ProtocolEngine engine = await create_protocol_engine(hardware_api=hardware) engine.play() yield engine - await engine.stop() + await engine.finish() async def test_creates_protocol_context(protocol_engine: ProtocolEngine) -> None: diff --git a/api/tests/opentrons/protocol_runner/test_protocol_runner.py b/api/tests/opentrons/protocol_runner/test_protocol_runner.py index 6d618674557..a011360badf 100644 --- a/api/tests/opentrons/protocol_runner/test_protocol_runner.py +++ b/api/tests/opentrons/protocol_runner/test_protocol_runner.py @@ -164,7 +164,7 @@ async def test_stop( """It should halt a protocol run with stop.""" await subject.stop() - decoy.verify(task_queue.stop(), await protocol_engine.halt()) + decoy.verify(task_queue.stop(), await protocol_engine.stop()) async def test_join( @@ -217,7 +217,7 @@ def test_load_json( ) ), task_queue.set_run_func(func=protocol_engine.wait_until_complete), - task_queue.set_cleanup_func(func=protocol_engine.stop), + task_queue.set_cleanup_func(func=protocol_engine.finish), ) @@ -254,7 +254,7 @@ def test_load_python( protocol=python_protocol, context=protocol_context, ), - task_queue.set_cleanup_func(func=protocol_engine.stop), + task_queue.set_cleanup_func(func=protocol_engine.finish), ) @@ -303,7 +303,7 @@ def test_load_legacy_python( protocol=legacy_protocol, context=legacy_context, ), - task_queue.set_cleanup_func(func=protocol_engine.stop), + task_queue.set_cleanup_func(func=protocol_engine.finish), ) @@ -349,5 +349,5 @@ def test_load_legacy_json( protocol=legacy_protocol, context=legacy_context, ), - task_queue.set_cleanup_func(func=protocol_engine.stop), + task_queue.set_cleanup_func(func=protocol_engine.finish), ) diff --git a/api/tests/opentrons/protocol_runner/test_task_queue.py b/api/tests/opentrons/protocol_runner/test_task_queue.py index c6d8f93dbb1..24a0af6189e 100644 --- a/api/tests/opentrons/protocol_runner/test_task_queue.py +++ b/api/tests/opentrons/protocol_runner/test_task_queue.py @@ -75,6 +75,25 @@ async def test_cleanup_gets_run_error(decoy: Decoy) -> None: decoy.verify(await cleanup_func(error=error)) +async def test_cleanup_does_not_run_if_cancelled(decoy: Decoy) -> None: + """It should not run cleanup func if task is cancelled.""" + run_func = decoy.mock(is_async=True) + cleanup_func = decoy.mock(is_async=True) + error = asyncio.CancelledError() + + decoy.when(await run_func()).then_raise(error) + + subject = TaskQueue() + subject.set_run_func(func=run_func) + subject.set_cleanup_func(func=cleanup_func) + subject.start() + + with pytest.raises(asyncio.CancelledError): + await subject.join() + + decoy.verify(await cleanup_func(error=error), times=0) + + async def test_join_waits_for_start() -> None: """It should wait until the queue is started when join is called.""" subject = TaskQueue()