Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(robot-server,api): Add the skeleton for a new complete-recovery run action #14674

Merged
merged 10 commits into from
Mar 18, 2024
3 changes: 3 additions & 0 deletions api-client/src/runs/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,14 @@ export interface Runs {
export const RUN_ACTION_TYPE_PLAY: 'play' = 'play'
export const RUN_ACTION_TYPE_PAUSE: 'pause' = 'pause'
export const RUN_ACTION_TYPE_STOP: 'stop' = 'stop'
export const RUN_ACTION_TYPE_COMPLETE_RECOVERY: 'complete-recovery' =
'complete-recovery'

export type RunActionType =
| typeof RUN_ACTION_TYPE_PLAY
| typeof RUN_ACTION_TYPE_PAUSE
| typeof RUN_ACTION_TYPE_STOP
| typeof RUN_ACTION_TYPE_COMPLETE_RECOVERY

export interface RunAction {
id: string
Expand Down
3 changes: 3 additions & 0 deletions api/.flake8
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ extend-ignore =
ANN102
# do not require docstring for __init__, put them on the class
D107,
# Don't forbid the function signature from being mentioned in the first line of the
# docstring. It tends to raise false positives when referring to other functions.
D402,

# configure flake8-docstrings
# https://pypi.org/project/flake8-docstrings/
Expand Down
8 changes: 8 additions & 0 deletions api/src/opentrons/protocol_engine/actions/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ class StopAction:
from_estop: bool = False


@dataclass(frozen=True)
class CompleteRecoveryAction:
"""See `ProtocolEngine.complete_recovery()`."""

pass


@dataclass(frozen=True)
class FinishErrorDetails:
"""Error details for the payload of a FinishAction or HardwareStoppedAction."""
Expand Down Expand Up @@ -203,6 +210,7 @@ class SetPipetteMovementSpeedAction:
PlayAction,
PauseAction,
StopAction,
CompleteRecoveryAction,
FinishAction,
HardwareStoppedAction,
DoorChangeAction,
Expand Down
8 changes: 8 additions & 0 deletions api/src/opentrons/protocol_engine/protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from contextlib import AsyncExitStack
from logging import getLogger
from typing import Dict, Optional, Union
from opentrons.protocol_engine.actions.actions import CompleteRecoveryAction

from opentrons.protocols.models import LabwareDefinition
from opentrons.hardware_control import HardwareControlAPI
Expand Down Expand Up @@ -159,6 +160,13 @@ def pause(self) -> None:
self._action_dispatcher.dispatch(action)
self._hardware_api.pause(HardwarePauseType.PAUSE)

def complete_recovery(self) -> None:
"""Resume normal protocol execution after the engine was `AWAITING_RECOVERY`."""
action = self._state_store.commands.validate_action_allowed(
CompleteRecoveryAction()
)
self._action_dispatcher.dispatch(action)

def add_command(self, request: commands.CommandCreate) -> commands.Command:
"""Add a command to the `ProtocolEngine`'s queue.

Expand Down
18 changes: 15 additions & 3 deletions api/src/opentrons/protocol_engine/state/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from opentrons.ordered_set import OrderedSet

from opentrons.hardware_control.types import DoorState
from opentrons.protocol_engine.actions.actions import CompleteRecoveryAction

from ..actions import (
Action,
Expand Down Expand Up @@ -667,8 +668,16 @@ def get_is_terminal(self) -> bool:

def validate_action_allowed(
self,
action: Union[PlayAction, PauseAction, StopAction, QueueCommandAction],
) -> Union[PlayAction, PauseAction, StopAction, QueueCommandAction]:
action: Union[
PlayAction,
PauseAction,
StopAction,
CompleteRecoveryAction,
QueueCommandAction,
],
) -> Union[
PlayAction, PauseAction, StopAction, CompleteRecoveryAction, QueueCommandAction
]:
"""Validate whether a given control action is allowed.

Returns:
Expand Down Expand Up @@ -701,7 +710,10 @@ def validate_action_allowed(
"Setup commands are not allowed after run has started."
)

elif self.get_status() == EngineStatus.AWAITING_RECOVERY:
elif (
isinstance(action, CompleteRecoveryAction)
or self.get_status() == EngineStatus.AWAITING_RECOVERY
):
# While we're developing error recovery, we'll conservatively disallow
# all actions, to avoid putting the engine in weird undefined states.
# We'll allow specific actions here as we flesh things out and add support
Expand Down
4 changes: 4 additions & 0 deletions api/src/opentrons/protocol_runner/protocol_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ async def stop(self) -> None:
post_run_hardware_state=PostRunHardwareState.STAY_ENGAGED_IN_PLACE,
)

def complete_recovery(self) -> None:
"""See `ProtocolEngine.complete_recovery()`."""
self._protocol_engine.complete_recovery()

@abstractmethod
async def run(
self,
Expand Down
19 changes: 19 additions & 0 deletions api/tests/opentrons/protocol_engine/test_protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from opentrons_shared_data.robot.dev_types import RobotType
from opentrons.ordered_set import OrderedSet
from opentrons.protocol_engine.actions.actions import CompleteRecoveryAction

from opentrons.types import DeckSlotName
from opentrons.hardware_control import HardwareControlAPI, OT2HardwareControlAPI
Expand Down Expand Up @@ -427,6 +428,24 @@ def test_pause(
)


def test_complete_recovery(
decoy: Decoy,
state_store: StateStore,
action_dispatcher: ActionDispatcher,
subject: ProtocolEngine,
) -> None:
"""It should dispatch a CompleteRecoveryAction."""
expected_action = CompleteRecoveryAction()

decoy.when(
state_store.commands.validate_action_allowed(expected_action)
).then_return(expected_action)

subject.complete_recovery()

decoy.verify(action_dispatcher.dispatch(expected_action))


@pytest.mark.parametrize("drop_tips_after_run", [True, False])
@pytest.mark.parametrize("set_run_status", [True, False])
@pytest.mark.parametrize(
Expand Down
25 changes: 22 additions & 3 deletions api/tests/opentrons/protocol_runner/test_protocol_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def live_runner_subject(
(None, LiveRunner),
],
)
async def test_create_protocol_runner(
def test_create_protocol_runner(
protocol_engine: ProtocolEngine,
hardware_api: HardwareAPI,
task_queue: TaskQueue,
Expand Down Expand Up @@ -203,7 +203,7 @@ async def test_create_protocol_runner(
(lazy_fixture("live_runner_subject")),
],
)
async def test_play_starts_run(
def test_play_starts_run(
decoy: Decoy,
protocol_engine: ProtocolEngine,
task_queue: TaskQueue,
Expand All @@ -223,7 +223,7 @@ async def test_play_starts_run(
(lazy_fixture("live_runner_subject")),
],
)
async def test_pause(
def test_pause(
decoy: Decoy,
protocol_engine: ProtocolEngine,
subject: AnyRunner,
Expand Down Expand Up @@ -286,6 +286,25 @@ async def test_stop_when_run_never_started(
)


@pytest.mark.parametrize(
"subject",
[
(lazy_fixture("json_runner_subject")),
(lazy_fixture("legacy_python_runner_subject")),
(lazy_fixture("live_runner_subject")),
],
)
def test_complete_recovery(
decoy: Decoy,
protocol_engine: ProtocolEngine,
subject: AnyRunner,
) -> None:
"""It should call `complete_recovery()` on the underlying engine."""
subject.complete_recovery()

decoy.verify(protocol_engine.complete_recovery(), times=1)


async def test_run_json_runner(
decoy: Decoy,
hardware_api: HardwareAPI,
Expand Down
15 changes: 9 additions & 6 deletions robot-server/robot_server/runs/action_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,20 @@


class RunActionType(str, Enum):
"""Types of run control actions.

Args:
PLAY: Start or resume a protocol run.
PAUSE: Pause a run.
STOP: Stop (cancel) a run.
"""The type of the run control action.

* `"play"`: Start or resume a run.
* `"pause"`: Pause a run.
* `"stop"`: Stop (cancel) a run.
* `"complete-recovery"`: Resume normal protocol execution after a command failed,
the run was placed in `awaiting-recovery` mode, and manual recovery steps
were taken.
"""

PLAY = "play"
PAUSE = "pause"
STOP = "stop"
COMPLETE_RECOVERY = "complete-recovery"


class RunActionCreate(BaseModel):
Expand Down
1 change: 0 additions & 1 deletion robot-server/robot_server/runs/router/actions_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ async def get_run_controller(
async def create_run_action(
runId: str,
request_body: RequestModel[RunActionCreate],
engine_store: EngineStore = Depends(get_engine_store),
run_controller: RunController = Depends(get_run_controller),
action_id: str = Depends(get_unique_id),
created_at: datetime = Depends(get_current_time),
Expand Down
3 changes: 3 additions & 0 deletions robot-server/robot_server/runs/run_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ def create_action(
log.info(f'Stopping run "{self._run_id}".')
self._task_runner.run(self._engine_store.runner.stop)

elif action_type == RunActionType.COMPLETE_RECOVERY:
self._engine_store.runner.complete_recovery()

except ProtocolEngineError as e:
raise RunActionNotAllowedError(message=e.message, wrapping=[e]) from e

Expand Down
30 changes: 28 additions & 2 deletions robot-server/tests/runs/test_run_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ async def test_create_play_action_to_start(
)


async def test_create_pause_action(
def test_create_pause_action(
decoy: Decoy,
mock_engine_store: EngineStore,
mock_run_store: RunStore,
Expand All @@ -193,7 +193,7 @@ async def test_create_pause_action(
decoy.verify(mock_engine_store.runner.pause(), times=1)


async def test_create_stop_action(
def test_create_stop_action(
decoy: Decoy,
mock_engine_store: EngineStore,
mock_run_store: RunStore,
Expand All @@ -219,6 +219,32 @@ async def test_create_stop_action(
decoy.verify(mock_task_runner.run(mock_engine_store.runner.stop), times=1)


def test_create_complete_recovery_action(
decoy: Decoy,
mock_engine_store: EngineStore,
mock_run_store: RunStore,
mock_task_runner: TaskRunner,
run_id: str,
subject: RunController,
) -> None:
"""It should call `complete_recovery()` on the underlying engine store."""
result = subject.create_action(
action_id="some-action-id",
action_type=RunActionType.COMPLETE_RECOVERY,
created_at=datetime(year=2021, month=1, day=1),
action_payload=[],
)

assert result == RunAction(
id="some-action-id",
actionType=RunActionType.COMPLETE_RECOVERY,
createdAt=datetime(year=2021, month=1, day=1),
)

decoy.verify(mock_run_store.insert_action(run_id, result), times=1)
decoy.verify(mock_engine_store.runner.complete_recovery())


@pytest.mark.parametrize(
("action_type", "exception"),
[
Expand Down
Loading