diff --git a/robot-server/robot_server/runs/engine_store.py b/robot-server/robot_server/runs/engine_store.py index 04b4b91a0f3..036515eedf3 100644 --- a/robot-server/robot_server/runs/engine_store.py +++ b/robot-server/robot_server/runs/engine_store.py @@ -1,24 +1,24 @@ """In-memory storage of ProtocolEngine instances.""" -from typing import Optional, NamedTuple +from typing import Dict, NamedTuple, Optional from opentrons.hardware_control import API as HardwareAPI -from opentrons.protocol_engine import ProtocolEngine, create_protocol_engine +from opentrons.protocol_engine import ProtocolEngine, StateView, create_protocol_engine from opentrons.protocol_runner import ProtocolRunner -class EngineConflictError(RuntimeError): - """An error raised if the runner already has an engine initialized.""" - - pass - - class EngineMissingError(RuntimeError): """An error raised if the engine somehow hasn't been initialized. If this error is raised, it's almost certainly due to a software bug. """ - pass + +class EngineConflictError(RuntimeError): + """An error raised if an active engine is already initialized. + + The store will not create a new engine unless the "current" runner/engine + pair is idle. + """ class RunnerEnginePair(NamedTuple): @@ -41,10 +41,11 @@ def __init__(self, hardware_api: HardwareAPI) -> None: """ self._hardware_api = hardware_api self._runner_engine_pair: Optional[RunnerEnginePair] = None + self._engines_by_run_id: Dict[str, ProtocolEngine] = {} @property def engine(self) -> ProtocolEngine: - """Get the persisted ProtocolEngine. + """Get the "current" persisted ProtocolEngine. Raises: EngineMissingError: Engine has not yet been created and persisted. @@ -56,7 +57,7 @@ def engine(self) -> ProtocolEngine: @property def runner(self) -> ProtocolRunner: - """Get the persisted ProtocolRunner. + """Get the "current" persisted ProtocolRunner. Raises: EngineMissingError: Runner has not yet been created and persisted. @@ -66,28 +67,44 @@ def runner(self) -> ProtocolRunner: return self._runner_engine_pair.runner - async def create(self) -> RunnerEnginePair: - """Create and store a ProtocolRunner and ProtocolEngine. + async def create(self, run_id: str) -> StateView: + """Create and store a ProtocolRunner and ProtocolEngine for a given Run. + + Args: + run_id: The run resource the engine is assigned to. Returns: The created and stored ProtocolRunner / ProtocolEngine pair. Raises: - EngineConflictError: a ProtocolEngine is already present. + EngineConflictError: The current runner/engine pair is not idle, so + a new set may not be created. """ - # NOTE: this async. creation happens before the `self._engine` - # check intentionally to avoid a race condition where `self._engine` is - # set after the check but before the engine has finished getting created, - # at the expense of having to potentially throw away an engine instance engine = await create_protocol_engine(hardware_api=self._hardware_api) runner = ProtocolRunner(protocol_engine=engine, hardware_api=self._hardware_api) if self._runner_engine_pair is not None: - raise EngineConflictError("Cannot load multiple runs simultaneously.") + if not self.engine.state_view.commands.get_is_stopped(): + raise EngineConflictError("Current engine is not stopped.") self._runner_engine_pair = RunnerEnginePair(runner=runner, engine=engine) + self._engines_by_run_id[run_id] = engine + + return engine.state_view + + def get_state(self, run_id: str) -> StateView: + """Get a run's ProtocolEngine state. - return self._runner_engine_pair + Args: + run_id: The run resource to retrieve engine state from. + + Raises: + EngineMissingError: No engine found for the given run ID. + """ + try: + return self._engines_by_run_id[run_id].state_view + except KeyError: + raise EngineMissingError(f"No engine state found for run {run_id}") def clear(self) -> None: """Remove the persisted ProtocolEngine, if present, no-op otherwise.""" diff --git a/robot-server/robot_server/runs/router/actions_router.py b/robot-server/robot_server/runs/router/actions_router.py index 79f21516bb9..c8355668fff 100644 --- a/robot-server/robot_server/runs/router/actions_router.py +++ b/robot-server/robot_server/runs/router/actions_router.py @@ -1,8 +1,11 @@ """Router for /runs actions endpoints.""" from fastapi import APIRouter, Depends, status from datetime import datetime +from typing import Union from typing_extensions import Literal +from opentrons.protocol_engine.errors import ProtocolEngineStoppedError + from robot_server.errors import ErrorDetails, ErrorResponse from robot_server.service.dependencies import get_current_time, get_unique_id from robot_server.service.json_api import RequestModel, ResponseModel @@ -10,9 +13,9 @@ from ..run_store import RunStore, RunNotFoundError from ..run_view import RunView from ..action_models import RunAction, RunActionType, RunActionCreateData -from ..engine_store import EngineStore, EngineMissingError +from ..engine_store import EngineStore from ..dependencies import get_run_store, get_engine_store -from .base_router import RunNotFound +from .base_router import RunNotFound, RunStopped actions_router = APIRouter() @@ -31,7 +34,9 @@ class RunActionNotAllowed(ErrorDetails): status_code=status.HTTP_201_CREATED, response_model=ResponseModel[RunAction], responses={ - status.HTTP_400_BAD_REQUEST: {"model": ErrorResponse[RunActionNotAllowed]}, + status.HTTP_409_CONFLICT: { + "model": ErrorResponse[Union[RunActionNotAllowed, RunStopped]], + }, status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[RunNotFound]}, }, ) @@ -57,16 +62,22 @@ async def create_run_action( """ try: prev_run = run_store.get(run_id=runId) + except RunNotFoundError as e: + raise RunNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) - action, next_run = run_view.with_action( - run=prev_run, - action_id=action_id, - action_data=request_body.data, - created_at=created_at, + if not prev_run.is_current: + raise RunStopped(detail=f"Run {runId} is not the current run").as_error( + status.HTTP_409_CONFLICT ) - # TODO(mc, 2021-07-06): add a dependency to verify that a given - # action is allowed + action, next_run = run_view.with_action( + run=prev_run, + action_id=action_id, + action_data=request_body.data, + created_at=created_at, + ) + + try: if action.actionType == RunActionType.PLAY: engine_store.runner.play() elif action.actionType == RunActionType.PAUSE: @@ -74,10 +85,8 @@ async def create_run_action( if action.actionType == RunActionType.STOP: await engine_store.runner.stop() - except RunNotFoundError as e: - raise RunNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) - except EngineMissingError as e: - raise RunActionNotAllowed(detail=str(e)).as_error(status.HTTP_400_BAD_REQUEST) + except ProtocolEngineStoppedError as e: + raise RunActionNotAllowed(detail=str(e)).as_error(status.HTTP_409_CONFLICT) run_store.upsert(run=next_run) diff --git a/robot-server/robot_server/runs/router/base_router.py b/robot-server/robot_server/runs/router/base_router.py index 2556703de6c..a9525835729 100644 --- a/robot-server/robot_server/runs/router/base_router.py +++ b/robot-server/robot_server/runs/router/base_router.py @@ -40,7 +40,6 @@ class RunNotFound(ErrorDetails): title: str = "Run Not Found" -# TODO(mc, 2021-05-28): evaluate multi-run logic class RunAlreadyActive(ErrorDetails): """An error if one tries to create a new run while one is already active.""" @@ -59,6 +58,13 @@ class RunNotIdle(ErrorDetails): ) +class RunStopped(ErrorDetails): + """An error if one tries to modify a stopped run.""" + + id: Literal["RunStopped"] = "RunStopped" + title: str = "Run Stopped" + + @base_router.post( path="/runs", summary="Create a run", @@ -96,7 +102,9 @@ async def create_run( """ create_data = request_body.data if request_body is not None else None run = run_view.as_resource( - run_id=run_id, created_at=created_at, create_data=create_data + run_id=run_id, + created_at=created_at, + create_data=create_data, ) protocol_id = None @@ -104,7 +112,7 @@ async def create_run( protocol_id = create_data.createParams.protocolId try: - await engine_store.create() + engine_state = await engine_store.create(run_id=run_id) if protocol_id is not None: protocol_resource = protocol_store.get(protocol_id=protocol_id) @@ -124,10 +132,10 @@ async def create_run( data = run_view.as_response( run=run, - commands=engine_store.engine.state_view.commands.get_all(), - pipettes=engine_store.engine.state_view.pipettes.get_all(), - labware=engine_store.engine.state_view.labware.get_all(), - engine_status=engine_store.engine.state_view.commands.get_status(), + commands=engine_state.commands.get_all(), + pipettes=engine_state.pipettes.get_all(), + labware=engine_state.labware.get_all(), + engine_status=engine_state.commands.get_status(), ) return ResponseModel(data=data) @@ -155,13 +163,14 @@ async def get_runs( data = [] for run in run_store.get_all(): - # TODO(mc, 2021-06-23): add multi-engine support + run_id = run.run_id + engine_state = engine_store.get_state(run_id) run_data = run_view.as_response( run=run, - commands=engine_store.engine.state_view.commands.get_all(), - pipettes=engine_store.engine.state_view.pipettes.get_all(), - labware=engine_store.engine.state_view.labware.get_all(), - engine_status=engine_store.engine.state_view.commands.get_status(), + commands=engine_state.commands.get_all(), + pipettes=engine_state.pipettes.get_all(), + labware=engine_state.labware.get_all(), + engine_status=engine_state.commands.get_status(), ) data.append(run_data) @@ -198,12 +207,14 @@ async def get_run( except RunNotFoundError as e: raise RunNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) + engine_state = engine_store.get_state(run.run_id) + data = run_view.as_response( run=run, - commands=engine_store.engine.state_view.commands.get_all(), - pipettes=engine_store.engine.state_view.pipettes.get_all(), - labware=engine_store.engine.state_view.labware.get_all(), - engine_status=engine_store.engine.state_view.commands.get_status(), + commands=engine_state.commands.get_all(), + pipettes=engine_state.pipettes.get_all(), + labware=engine_state.labware.get_all(), + engine_status=engine_state.commands.get_status(), ) return ResponseModel(data=data) @@ -230,10 +241,12 @@ async def remove_run_by_id( engine_store: ProtocolEngine storage and control. """ try: - if not engine_store.engine.state_view.commands.get_is_stopped(): - raise RunNotIdle().as_error(status.HTTP_409_CONFLICT) + engine_state = engine_store.get_state(runId) except EngineMissingError: pass + else: + if not engine_state.commands.get_is_stopped(): + raise RunNotIdle().as_error(status.HTTP_409_CONFLICT) try: engine_store.clear() diff --git a/robot-server/robot_server/runs/router/commands_router.py b/robot-server/robot_server/runs/router/commands_router.py index 00737ba0c0a..f4a2fb7d0ce 100644 --- a/robot-server/robot_server/runs/router/commands_router.py +++ b/robot-server/robot_server/runs/router/commands_router.py @@ -15,7 +15,7 @@ from ..run_models import Run, RunCommandSummary from ..engine_store import EngineStore from ..dependencies import get_engine_store -from .base_router import RunNotFound, get_run +from .base_router import RunNotFound, RunStopped, get_run commands_router = APIRouter() @@ -38,6 +38,7 @@ class CommandNotFound(ErrorDetails): status_code=status.HTTP_200_OK, response_model=ResponseModel[pe_commands.Command], responses={ + status.HTTP_400_BAD_REQUEST: {"model": ErrorResponse[RunStopped]}, status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[RunNotFound]}, }, ) @@ -57,6 +58,11 @@ async def post_run_command( `GET /runs/{runId}`. Present to ensure 404 if run not found. """ + if not run.data.current: + raise RunStopped(detail=f"Run {run.data.id} is not the current run").as_error( + status.HTTP_400_BAD_REQUEST + ) + command = engine_store.engine.add_command(request_body.data) return ResponseModel[pe_commands.Command](data=command) @@ -120,8 +126,9 @@ async def get_run_command( `GET /run/{runId}`. Present to ensure 404 if run not found. """ + engine_state = engine_store.get_state(run.data.id) try: - command = engine_store.engine.state_view.commands.get(commandId) + command = engine_state.commands.get(commandId) except pe_errors.CommandDoesNotExistError as e: raise CommandNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) diff --git a/robot-server/robot_server/runs/run_models.py b/robot-server/robot_server/runs/run_models.py index 98a4b3cf477..15f8ae71404 100644 --- a/robot-server/robot_server/runs/run_models.py +++ b/robot-server/robot_server/runs/run_models.py @@ -39,6 +39,13 @@ class _AbstractRun(ResourceModel): runType: RunType = Field(..., description="Specific run type.") createdAt: datetime = Field(..., description="When the run was created") status: RunStatus = Field(..., description="Execution status of the run") + current: bool = Field( + ..., + description=( + "Whether this run is currently controlling the robot." + " There can be, at most, one current run." + ), + ) actions: List[RunAction] = Field( ..., description="Client-initiated run control actions.", diff --git a/robot-server/robot_server/runs/run_store.py b/robot-server/robot_server/runs/run_store.py index 2e5b45c77f4..256396f5ded 100644 --- a/robot-server/robot_server/runs/run_store.py +++ b/robot-server/robot_server/runs/run_store.py @@ -1,5 +1,5 @@ """Runs' in-memory store.""" -from dataclasses import dataclass +from dataclasses import dataclass, replace from datetime import datetime from typing import Dict, List @@ -19,6 +19,7 @@ class RunResource: create_data: RunCreateData created_at: datetime actions: List[RunAction] + is_current: bool class RunNotFoundError(ValueError): @@ -46,6 +47,11 @@ def upsert(self, run: RunResource) -> RunResource: Returns: The resource that was added to the store. """ + if run.is_current is True: + for target_id, target in self._runs_by_id.items(): + if target.is_current and target_id != run.run_id: + self._runs_by_id[target_id] = replace(target, is_current=False) + self._runs_by_id[run.run_id] = run return run diff --git a/robot-server/robot_server/runs/run_view.py b/robot-server/robot_server/runs/run_view.py index 3e2c0837868..0dd82f12579 100644 --- a/robot-server/robot_server/runs/run_view.py +++ b/robot-server/robot_server/runs/run_view.py @@ -52,6 +52,7 @@ def as_resource( created_at=created_at, create_data=create_data or BasicRunCreateData(), actions=[], + is_current=True, ) @staticmethod @@ -97,8 +98,12 @@ def as_response( ) -> Run: """Transform a run resource into its public response model. - Arguments: + Args: run: Internal resource representation of the run. + commands: Commands from ProtocolEngine state. + pipettes: Pipettes from ProtocolEngine state. + labware: Labware from ProtocolEngine state. + engine_status: Status from ProtocolEngine state. Returns: Run response model representing the same resource. @@ -114,6 +119,7 @@ def as_response( id=run.run_id, createParams=create_data.createParams, createdAt=run.created_at, + current=run.is_current, actions=run.actions, commands=command_summaries, pipettes=pipettes, @@ -126,6 +132,7 @@ def as_response( id=run.run_id, createParams=create_data.createParams, createdAt=run.created_at, + current=run.is_current, actions=run.actions, commands=command_summaries, pipettes=pipettes, diff --git a/robot-server/tests/runs/router/test_actions_router.py b/robot-server/tests/runs/router/test_actions_router.py index ee394558584..490537526dd 100644 --- a/robot-server/tests/runs/router/test_actions_router.py +++ b/robot-server/tests/runs/router/test_actions_router.py @@ -7,9 +7,10 @@ from httpx import AsyncClient from tests.helpers import verify_response +from opentrons.protocol_engine.errors import ProtocolEngineStoppedError from robot_server.runs.run_models import BasicRunCreateData from robot_server.runs.run_view import RunView -from robot_server.runs.engine_store import EngineStore, EngineMissingError +from robot_server.runs.engine_store import EngineStore from robot_server.runs.run_store import ( RunStore, RunNotFoundError, @@ -22,7 +23,7 @@ RunActionCreateData, ) -from robot_server.runs.router.base_router import RunNotFound +from robot_server.runs.router.base_router import RunNotFound, RunStopped from robot_server.runs.router.actions_router import ( actions_router, @@ -35,6 +36,7 @@ create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1), actions=[], + is_current=True, ) @@ -53,6 +55,7 @@ def setup_run_store(decoy: Decoy, run_store: RunStore) -> None: def test_create_play_action( decoy: Decoy, run_view: RunView, + run_store: RunStore, engine_store: EngineStore, unique_id: str, current_time: datetime, @@ -70,6 +73,7 @@ def test_create_play_action( create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1), actions=[action], + is_current=True, ) decoy.when( @@ -117,12 +121,13 @@ def test_create_run_action_with_missing_id( def test_create_run_action_without_runner( decoy: Decoy, run_view: RunView, + run_store: RunStore, engine_store: EngineStore, unique_id: str, current_time: datetime, client: TestClient, ) -> None: - """It should 400 if the runner is not able to handle the action.""" + """It should 409 if the runner is not able to handle the action.""" actions = RunAction( actionType=RunActionType.PLAY, createdAt=current_time, @@ -130,10 +135,11 @@ def test_create_run_action_without_runner( ) next_run = RunResource( - run_id="unique-id", + run_id="run-id", create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1), actions=[actions], + is_current=True, ) decoy.when( @@ -145,7 +151,9 @@ def test_create_run_action_without_runner( ), ).then_return((actions, next_run)) - decoy.when(engine_store.runner.play()).then_raise(EngineMissingError("oh no")) + decoy.when(engine_store.runner.play()).then_raise( + ProtocolEngineStoppedError("oh no") + ) response = client.post( "/runs/run-id/actions", @@ -154,14 +162,47 @@ def test_create_run_action_without_runner( verify_response( response, - expected_status=400, + expected_status=409, expected_errors=RunActionNotAllowed(detail="oh no"), ) +def test_create_run_action_not_current( + decoy: Decoy, + run_view: RunView, + run_store: RunStore, + engine_store: EngineStore, + unique_id: str, + current_time: datetime, + client: TestClient, +) -> None: + """It should 409 if the run is not current.""" + prev_run = RunResource( + run_id="run-id", + create_data=BasicRunCreateData(), + created_at=datetime(year=2021, month=1, day=1), + actions=[], + is_current=False, + ) + + decoy.when(run_store.get(run_id="run-id")).then_return(prev_run) + + response = client.post( + "/runs/run-id/actions", + json={"data": {"actionType": "play"}}, + ) + + verify_response( + response, + expected_status=409, + expected_errors=RunStopped(detail="Run run-id is not the current run"), + ) + + def test_create_pause_action( decoy: Decoy, run_view: RunView, + run_store: RunStore, engine_store: EngineStore, unique_id: str, current_time: datetime, @@ -175,10 +216,11 @@ def test_create_pause_action( ) next_run = RunResource( - run_id="unique-id", + run_id="run-id", create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1), actions=[action], + is_current=True, ) decoy.when( @@ -202,6 +244,7 @@ def test_create_pause_action( async def test_create_stop_action( decoy: Decoy, run_view: RunView, + run_store: RunStore, engine_store: EngineStore, unique_id: str, current_time: datetime, @@ -215,10 +258,11 @@ async def test_create_stop_action( ) next_run = RunResource( - run_id="unique-id", + run_id="run-id", create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1), actions=[action], + is_current=True, ) decoy.when( diff --git a/robot-server/tests/runs/router/test_base_router.py b/robot-server/tests/runs/router/test_base_router.py index 7f8a03db1ec..63588048eff 100644 --- a/robot-server/tests/runs/router/test_base_router.py +++ b/robot-server/tests/runs/router/test_base_router.py @@ -1,13 +1,17 @@ """Tests for base /runs routes.""" import pytest from datetime import datetime -from decoy import Decoy +from decoy import Decoy, matchers from fastapi import FastAPI from fastapi.testclient import TestClient from httpx import AsyncClient from opentrons.types import DeckSlotName, MountType -from opentrons.protocol_engine import commands as pe_commands, types as pe_types +from opentrons.protocol_engine import ( + StateView, + commands as pe_commands, + types as pe_types, +) from opentrons.protocol_runner import JsonPreAnalysis from robot_server.service.task_runner import TaskRunner @@ -74,28 +78,35 @@ async def test_create_run( created_at=current_time, create_data=BasicRunCreateData(), actions=[], + is_current=True, ) expected_response = BasicRun( id=unique_id, createdAt=current_time, createParams=BasicRunCreateParams(), status=pe_types.EngineStatus.READY_TO_RUN, + current=True, actions=[], commands=[], pipettes=[], labware=[], ) - decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.pipettes.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.labware.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.commands.get_status()).then_return( + engine_state = decoy.mock(cls=StateView) + decoy.when(await engine_store.create(run_id=unique_id)).then_return(engine_state) + + decoy.when(engine_state.commands.get_all()).then_return([]) + decoy.when(engine_state.pipettes.get_all()).then_return([]) + decoy.when(engine_state.labware.get_all()).then_return([]) + decoy.when(engine_state.commands.get_status()).then_return( pe_types.EngineStatus.READY_TO_RUN ) decoy.when( run_view.as_resource( - run_id=unique_id, created_at=current_time, create_data=BasicRunCreateData() + run_id=unique_id, + created_at=current_time, + create_data=BasicRunCreateData(), ) ).then_return(run) @@ -117,7 +128,6 @@ async def test_create_run( verify_response(response, expected_status=201, expected_data=expected_response) decoy.verify( - await engine_store.create(), task_runner.run(engine_store.runner.join), run_store.upsert(run=run), ) @@ -141,6 +151,7 @@ async def test_create_protocol_run( createParams=ProtocolRunCreateParams(protocolId="protocol-id") ), actions=[], + is_current=True, ) protocol_resource = ProtocolResource( protocol_id="protocol-id", @@ -152,6 +163,7 @@ async def test_create_protocol_run( id=unique_id, createdAt=current_time, status=pe_types.EngineStatus.READY_TO_RUN, + current=True, createParams=ProtocolRunCreateParams(protocolId="protocol-id"), actions=[], commands=[], @@ -173,10 +185,13 @@ async def test_create_protocol_run( ) ).then_return(run) - decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.pipettes.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.labware.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.commands.get_status()).then_return( + engine_state = decoy.mock(cls=StateView) + decoy.when(await engine_store.create(run_id=unique_id)).then_return(engine_state) + + decoy.when(engine_state.commands.get_all()).then_return([]) + decoy.when(engine_state.pipettes.get_all()).then_return([]) + decoy.when(engine_state.labware.get_all()).then_return([]) + decoy.when(engine_state.commands.get_status()).then_return( pe_types.EngineStatus.READY_TO_RUN ) @@ -203,7 +218,6 @@ async def test_create_protocol_run( verify_response(response, expected_status=201, expected_data=expected_response) decoy.verify( - await engine_store.create(), engine_store.runner.load(protocol_resource), run_store.upsert(run=run), ) @@ -256,15 +270,20 @@ async def test_create_run_conflict( create_data=BasicRunCreateData(), created_at=current_time, actions=[], + is_current=True, ) decoy.when( run_view.as_resource( - run_id=unique_id, created_at=current_time, create_data=None + run_id=unique_id, + created_at=current_time, + create_data=None, ) ).then_return(run) - decoy.when(await engine_store.create()).then_raise(EngineConflictError("oh no")) + decoy.when(await engine_store.create(run_id=matchers.Anything())).then_raise( + EngineConflictError("oh no") + ) response = await async_client.post("/runs") @@ -290,6 +309,7 @@ def test_get_run( create_data=create_data, created_at=created_at, actions=[], + is_current=False, ) command = pe_commands.Pause( @@ -317,6 +337,7 @@ def test_get_run( createParams=BasicRunCreateParams(), createdAt=created_at, status=pe_types.EngineStatus.READY_TO_RUN, + current=False, actions=[], commands=[ RunCommandSummary( @@ -331,10 +352,13 @@ def test_get_run( decoy.when(run_store.get(run_id="run-id")).then_return(run) - decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([command]) - decoy.when(engine_store.engine.state_view.pipettes.get_all()).then_return([pipette]) - decoy.when(engine_store.engine.state_view.labware.get_all()).then_return([labware]) - decoy.when(engine_store.engine.state_view.commands.get_status()).then_return( + engine_state = decoy.mock(cls=StateView) + + decoy.when(engine_store.get_state("run-id")).then_return(engine_state) + decoy.when(engine_state.commands.get_all()).then_return([command]) + decoy.when(engine_state.pipettes.get_all()).then_return([pipette]) + decoy.when(engine_state.labware.get_all()).then_return([labware]) + decoy.when(engine_state.commands.get_status()).then_return( pe_types.EngineStatus.READY_TO_RUN ) @@ -393,14 +417,23 @@ def test_get_runs_not_empty( client: TestClient, ) -> None: """It should return a collection response when a run exists.""" - # TODO(mc, 2021-06-23): add actual multi-run support - created_at_1 = datetime.now() + created_at_1 = datetime(year=2021, month=1, day=1) + created_at_2 = datetime(year=2022, month=2, day=2) run_1 = RunResource( run_id="unique-id-1", create_data=BasicRunCreateData(), created_at=created_at_1, actions=[], + is_current=False, + ) + + run_2 = RunResource( + run_id="unique-id-2", + create_data=BasicRunCreateData(), + created_at=created_at_2, + actions=[], + is_current=True, ) response_1 = BasicRun( @@ -408,21 +441,47 @@ def test_get_runs_not_empty( createParams=BasicRunCreateParams(), createdAt=created_at_1, status=pe_types.EngineStatus.SUCCEEDED, + current=False, + actions=[], + commands=[], + pipettes=[], + labware=[], + ) + + response_2 = BasicRun( + id="unique-id-2", + createParams=BasicRunCreateParams(), + createdAt=created_at_2, + status=pe_types.EngineStatus.READY_TO_RUN, + current=True, actions=[], commands=[], pipettes=[], labware=[], ) - decoy.when(run_store.get_all()).then_return([run_1]) + decoy.when(run_store.get_all()).then_return([run_1, run_2]) - decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.pipettes.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.labware.get_all()).then_return([]) - decoy.when(engine_store.engine.state_view.commands.get_status()).then_return( + engine_state_1 = decoy.mock(cls=StateView) + engine_state_2 = decoy.mock(cls=StateView) + + decoy.when(engine_store.get_state("unique-id-1")).then_return(engine_state_1) + decoy.when(engine_store.get_state("unique-id-2")).then_return(engine_state_2) + + decoy.when(engine_state_1.commands.get_all()).then_return([]) + decoy.when(engine_state_1.pipettes.get_all()).then_return([]) + decoy.when(engine_state_1.labware.get_all()).then_return([]) + decoy.when(engine_state_1.commands.get_status()).then_return( pe_types.EngineStatus.SUCCEEDED ) + decoy.when(engine_state_2.commands.get_all()).then_return([]) + decoy.when(engine_state_2.pipettes.get_all()).then_return([]) + decoy.when(engine_state_2.labware.get_all()).then_return([]) + decoy.when(engine_state_2.commands.get_status()).then_return( + pe_types.EngineStatus.READY_TO_RUN + ) + decoy.when( run_view.as_response( run=run_1, @@ -433,9 +492,21 @@ def test_get_runs_not_empty( ), ).then_return(response_1) + decoy.when( + run_view.as_response( + run=run_2, + commands=[], + pipettes=[], + labware=[], + engine_status=pe_types.EngineStatus.READY_TO_RUN, + ), + ).then_return(response_2) + response = client.get("/runs") - verify_response(response, expected_status=200, expected_data=[response_1]) + verify_response( + response, expected_status=200, expected_data=[response_1, response_2] + ) def test_delete_run_by_id( @@ -445,15 +516,15 @@ def test_delete_run_by_id( client: TestClient, ) -> None: """It should be able to remove a run by ID.""" - decoy.when(engine_store.engine.state_view.commands.get_is_stopped()).then_return( - True - ) + engine_state = decoy.mock(cls=StateView) + decoy.when(engine_store.get_state("run-id")).then_return(engine_state) + decoy.when(engine_state.commands.get_is_stopped()).then_return(True) - response = client.delete("/runs/unique-id") + response = client.delete("/runs/run-id") decoy.verify( engine_store.clear(), - run_store.remove(run_id="unique-id"), + run_store.remove(run_id="run-id"), ) assert response.status_code == 200 @@ -469,9 +540,7 @@ def test_delete_run_with_bad_id( """It should 404 if the run ID does not exist.""" key_error = RunNotFoundError(run_id="run-id") - decoy.when(engine_store.engine.state_view.commands.get_is_stopped()).then_return( - True - ) + decoy.when(engine_store.get_state("run-id")).then_raise(EngineMissingError("oh no")) decoy.when(run_store.remove(run_id="run-id")).then_raise(key_error) response = client.delete("/runs/run-id") @@ -490,9 +559,9 @@ def test_delete_active_run( client: TestClient, ) -> None: """It should 409 if the run is not finished.""" - decoy.when(engine_store.engine.state_view.commands.get_is_stopped()).then_return( - False - ) + engine_state = decoy.mock(cls=StateView) + decoy.when(engine_store.get_state("run-id")).then_return(engine_state) + decoy.when(engine_state.commands.get_is_stopped()).then_return(False) response = client.delete("/runs/run-id") @@ -510,9 +579,7 @@ def test_delete_active_run_no_engine( client: TestClient, ) -> None: """It should no-op if no engine is present.""" - decoy.when(engine_store.engine.state_view.commands.get_is_stopped()).then_raise( - EngineMissingError() - ) + decoy.when(engine_store.get_state("run-id")).then_raise(EngineMissingError("oh no")) response = client.delete("/runs/run-id") diff --git a/robot-server/tests/runs/router/test_commands_router.py b/robot-server/tests/runs/router/test_commands_router.py index 1c0ffa43a99..8c539a2cdf2 100644 --- a/robot-server/tests/runs/router/test_commands_router.py +++ b/robot-server/tests/runs/router/test_commands_router.py @@ -7,6 +7,7 @@ from opentrons.protocol_engine import ( CommandStatus, EngineStatus, + StateView, commands as pe_commands, errors as pe_errors, ) @@ -14,7 +15,6 @@ from robot_server.errors import ApiError from robot_server.service.json_api import RequestModel, ResponseModel from robot_server.runs.run_models import ( - Run, BasicRun, BasicRunCreateParams, RunCommandSummary, @@ -28,10 +28,23 @@ async def test_post_run_command(decoy: Decoy, engine_store: EngineStore) -> None: - """It should add the requested command to the Protocol Engine and return it.""" + """It should add the requested command to the ProtocolEngine and return it.""" command_request = pe_commands.PauseRequest( data=pe_commands.PauseData(message="Hello") ) + + run = BasicRun( + id="run-id", + createParams=BasicRunCreateParams(), + createdAt=datetime(year=2021, month=1, day=1), + status=EngineStatus.RUNNING, + current=True, + actions=[], + commands=[], + pipettes=[], + labware=[], + ) + output_command = pe_commands.Pause( id="abc123", createdAt=datetime(year=2021, month=1, day=1), @@ -45,12 +58,46 @@ async def test_post_run_command(decoy: Decoy, engine_store: EngineStore) -> None ) response = await post_run_command( - request_body=RequestModel(data=command_request), engine_store=engine_store + request_body=RequestModel(data=command_request), + engine_store=engine_store, + run=ResponseModel(data=run), ) assert response.data == output_command +async def test_post_run_command_not_current( + decoy: Decoy, + engine_store: EngineStore, +) -> None: + """It should 400 if you try to add commands to non-current run.""" + command_request = pe_commands.PauseRequest( + data=pe_commands.PauseData(message="Hello") + ) + + run = BasicRun( + id="run-id", + createParams=BasicRunCreateParams(), + createdAt=datetime(year=2021, month=1, day=1), + status=EngineStatus.RUNNING, + current=False, + actions=[], + commands=[], + pipettes=[], + labware=[], + ) + + with pytest.raises(ApiError) as exc_info: + await post_run_command( + request_body=RequestModel(data=command_request), + engine_store=engine_store, + run=ResponseModel(data=run), + ) + + assert exc_info.value.status_code == 400 + assert exc_info.value.content["errors"][0]["id"] == "RunStopped" + + async def test_get_run_commands() -> None: """It should return a list of all commands in a run.""" command_summary = RunCommandSummary( @@ -59,20 +106,19 @@ async def test_get_run_commands() -> None: status=CommandStatus.RUNNING, ) - run_response = ResponseModel[Run]( - data=BasicRun( - id="run-id", - createParams=BasicRunCreateParams(), - createdAt=datetime(year=2021, month=1, day=1), - status=EngineStatus.RUNNING, - actions=[], - commands=[command_summary], - pipettes=[], - labware=[], - ) + run = BasicRun( + id="run-id", + createParams=BasicRunCreateParams(), + createdAt=datetime(year=2021, month=1, day=1), + status=EngineStatus.RUNNING, + current=True, + actions=[], + commands=[command_summary], + pipettes=[], + labware=[], ) - response = await get_run_commands(run=run_response) + response = await get_run_commands(run=ResponseModel(data=run)) assert response.data == [command_summary] @@ -82,6 +128,12 @@ async def test_get_run_command_by_id( engine_store: EngineStore, ) -> None: """It should return full details about a command by ID.""" + command_summary = RunCommandSummary( + id="command-id", + commandType="moveToWell", + status=CommandStatus.RUNNING, + ) + command = pe_commands.MoveToWell( id="command-id", status=CommandStatus.RUNNING, @@ -89,11 +141,28 @@ async def test_get_run_command_by_id( data=pe_commands.MoveToWellData(pipetteId="a", labwareId="b", wellName="c"), ) - decoy.when(engine_store.engine.state_view.commands.get("command-id")).then_return( - command + run = BasicRun( + id="run-id", + createParams=BasicRunCreateParams(), + createdAt=datetime(year=2021, month=1, day=1), + status=EngineStatus.RUNNING, + current=True, + actions=[], + commands=[command_summary], + pipettes=[], + labware=[], ) - response = await get_run_command(commandId="command-id", engine_store=engine_store) + engine_state = decoy.mock(cls=StateView) + + decoy.when(engine_store.get_state("run-id")).then_return(engine_state) + decoy.when(engine_state.commands.get("command-id")).then_return(command) + + response = await get_run_command( + commandId="command-id", + engine_store=engine_store, + run=ResponseModel(data=run), + ) assert response.data == command @@ -105,11 +174,28 @@ async def test_get_run_command_missing_command( """It should 404 if you attempt to get a non-existent command.""" key_error = pe_errors.CommandDoesNotExistError("oh no") - decoy.when(engine_store.engine.state_view.commands.get("command-id")).then_raise( - key_error + run = BasicRun( + id="run-id", + createParams=BasicRunCreateParams(), + createdAt=datetime(year=2021, month=1, day=1), + status=EngineStatus.RUNNING, + current=True, + actions=[], + commands=[], + pipettes=[], + labware=[], ) + engine_state = decoy.mock(cls=StateView) + decoy.when(engine_store.get_state("run-id")).then_return(engine_state) + decoy.when(engine_state.commands.get("command-id")).then_raise(key_error) + with pytest.raises(ApiError) as exc_info: - await get_run_command(commandId="command-id", engine_store=engine_store) + await get_run_command( + commandId="command-id", + engine_store=engine_store, + run=ResponseModel(data=run), + ) + assert exc_info.value.status_code == 404 assert exc_info.value.content["errors"][0]["detail"] == "oh no" diff --git a/robot-server/tests/runs/test_engine_store.py b/robot-server/tests/runs/test_engine_store.py index ecb35ca6b60..9da169f7711 100644 --- a/robot-server/tests/runs/test_engine_store.py +++ b/robot-server/tests/runs/test_engine_store.py @@ -8,8 +8,8 @@ from robot_server.runs.engine_store import ( EngineStore, - EngineConflictError, EngineMissingError, + EngineConflictError, ) @@ -23,29 +23,31 @@ def subject(decoy: Decoy) -> EngineStore: async def test_create_engine(subject: EngineStore) -> None: - """It should create an engine.""" - result = await subject.create() + """It should create an engine for a run.""" + result = await subject.create(run_id="run-id") - assert isinstance(result.runner, ProtocolRunner) - assert isinstance(result.engine, ProtocolEngine) - assert result.engine is subject.engine - assert result.runner is subject.runner + assert isinstance(subject.runner, ProtocolRunner) + assert isinstance(subject.engine, ProtocolEngine) + assert result is subject.engine.state_view + assert result is subject.get_state("run-id") -async def test_raise_if_engine_already_exists(subject: EngineStore) -> None: +async def test_archives_state_if_engine_already_exists(subject: EngineStore) -> None: """It should not create more than one engine / runner pair.""" - await subject.create() + state_1 = await subject.create(run_id="run-id-1") + await subject.runner.stop() + state_2 = await subject.create(run_id="run-id-2") - with pytest.raises(EngineConflictError): - await subject.create() + assert state_2 is subject.engine.state_view + assert state_1 is subject.get_state("run-id-1") -@pytest.mark.xfail(raises=NotImplementedError, strict=True) -async def test_cannot_persist_multiple_engines(subject: EngineStore) -> None: - """It should protect against engine creation race conditions.""" - # TODO(mc, 2021-06-14): figure out how to write a test that actually - # fails in practice when race condition is able to be hit - raise NotImplementedError("Test not yet implemented") +async def test_cannot_create_engine_if_active(subject: EngineStore) -> None: + """It should not create a new engine if the existing one is active.""" + await subject.create(run_id="run-id-1") + + with pytest.raises(EngineConflictError): + await subject.create(run_id="run-id-2") def test_raise_if_engine_does_not_exist(subject: EngineStore) -> None: @@ -59,7 +61,7 @@ def test_raise_if_engine_does_not_exist(subject: EngineStore) -> None: async def test_clear_engine(subject: EngineStore) -> None: """It should clear a stored engine entry.""" - await subject.create() + await subject.create(run_id="run-id") subject.clear() with pytest.raises(EngineMissingError): diff --git a/robot-server/tests/runs/test_run_store.py b/robot-server/tests/runs/test_run_store.py index 40ca5438b91..e9dbdc6eacc 100644 --- a/robot-server/tests/runs/test_run_store.py +++ b/robot-server/tests/runs/test_run_store.py @@ -18,6 +18,7 @@ def test_add_run() -> None: create_data=BasicRunCreateData(), created_at=datetime.now(), actions=[], + is_current=True, ) subject = RunStore() @@ -33,12 +34,14 @@ def test_update_run() -> None: create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1, hour=1, minute=1, second=1), actions=[], + is_current=True, ) updated_run = RunResource( run_id="identical-run-id", create_data=BasicRunCreateData(), created_at=datetime(year=2022, month=2, day=2, hour=2, minute=2, second=2), actions=[], + is_current=True, ) subject = RunStore() @@ -56,6 +59,7 @@ def test_get_run() -> None: create_data=BasicRunCreateData(), created_at=datetime.now(), actions=[], + is_current=False, ) subject = RunStore() @@ -81,12 +85,14 @@ def test_get_all_runs() -> None: create_data=BasicRunCreateData(), created_at=datetime.now(), actions=[], + is_current=False, ) run_2 = RunResource( run_id="run-id-2", create_data=BasicRunCreateData(), created_at=datetime.now(), actions=[], + is_current=True, ) subject = RunStore() @@ -105,6 +111,7 @@ def test_remove_run() -> None: create_data=BasicRunCreateData(), created_at=datetime.now(), actions=[], + is_current=True, ) subject = RunStore() @@ -122,3 +129,29 @@ def test_remove_run_missing_id() -> None: with pytest.raises(RunNotFoundError, match="run-id"): subject.remove(run_id="run-id") + + +def test_add_run_current_run_deactivates() -> None: + """Adding a current run should mark all others as not current.""" + run_1 = RunResource( + run_id="run-id-1", + create_data=BasicRunCreateData(), + created_at=datetime.now(), + actions=[], + is_current=True, + ) + + run_2 = RunResource( + run_id="run-id-2", + create_data=BasicRunCreateData(), + created_at=datetime.now(), + actions=[], + is_current=True, + ) + + subject = RunStore() + subject.upsert(run_1) + subject.upsert(run_2) + + assert subject.get("run-id-1").is_current is False + assert subject.get("run-id-2").is_current is True diff --git a/robot-server/tests/runs/test_run_view.py b/robot-server/tests/runs/test_run_view.py index 986cc29717a..485fde25142 100644 --- a/robot-server/tests/runs/test_run_view.py +++ b/robot-server/tests/runs/test_run_view.py @@ -42,7 +42,9 @@ def test_create_run_resource_from_none() -> None: subject = RunView() result = subject.as_resource( - run_id="run-id", created_at=created_at, create_data=create_data + run_id="run-id", + created_at=created_at, + create_data=create_data, ) assert result == RunResource( @@ -50,6 +52,7 @@ def test_create_run_resource_from_none() -> None: create_data=BasicRunCreateData(), created_at=created_at, actions=[], + is_current=True, ) @@ -68,6 +71,7 @@ def test_create_run_resource() -> None: create_data=create_data, created_at=created_at, actions=[], + is_current=True, ) @@ -88,6 +92,7 @@ def test_create_protocol_run_resource() -> None: create_data=create_data, created_at=created_at, actions=[], + is_current=True, ) @@ -113,11 +118,13 @@ def test_create_protocol_run_resource() -> None: ), created_at=current_time, actions=[], + is_current=True, ), BasicRun( id="run-id", createdAt=current_time, status=EngineStatus.READY_TO_RUN, + current=True, createParams=BasicRunCreateParams( labwareOffsets=[ LabwareOffset( @@ -141,11 +148,13 @@ def test_create_protocol_run_resource() -> None: ), created_at=current_time, actions=[], + is_current=False, ), ProtocolRun( id="run-id", createdAt=current_time, status=EngineStatus.READY_TO_RUN, + current=False, createParams=ProtocolRunCreateParams(protocolId="protocol-id"), actions=[], commands=[], @@ -155,10 +164,7 @@ def test_create_protocol_run_resource() -> None: ), ), ) -def test_to_response( - run_resource: RunResource, - expected_response: Run, -) -> None: +def test_to_response(run_resource: RunResource, expected_response: Run) -> None: """It should create the correct type of run.""" subject = RunView() result = subject.as_response( @@ -178,6 +184,7 @@ def test_to_response_maps_commands() -> None: create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1), actions=[], + is_current=True, ) command_1 = pe_commands.LoadPipette( @@ -211,6 +218,7 @@ def test_to_response_maps_commands() -> None: createParams=BasicRunCreateParams(), createdAt=datetime(year=2021, month=1, day=1), status=EngineStatus.RUNNING, + current=True, actions=[], commands=[ RunCommandSummary( @@ -236,6 +244,7 @@ def test_to_response_adds_equipment() -> None: create_data=BasicRunCreateData(), created_at=datetime(year=2021, month=1, day=1), actions=[], + is_current=True, ) labware = LoadedLabware( @@ -265,6 +274,7 @@ def test_to_response_adds_equipment() -> None: createParams=BasicRunCreateParams(), createdAt=datetime(year=2021, month=1, day=1), status=EngineStatus.RUNNING, + current=True, actions=[], commands=[], pipettes=[pipette], @@ -281,6 +291,7 @@ def test_create_action(current_time: datetime) -> None: create_data=BasicRunCreateData(), created_at=run_created_at, actions=[], + is_current=True, ) command_data = RunActionCreateData( @@ -306,4 +317,5 @@ def test_create_action(current_time: datetime) -> None: create_data=BasicRunCreateData(), created_at=run_created_at, actions=[action_result], + is_current=True, )