From bb594f1a8dc0e5da3feebe1d85f23f0edfea305e Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Fri, 11 Jun 2021 14:34:48 -0400 Subject: [PATCH 1/5] refactor(robot-server,api): wire up JSON protocol runner to /sessions API --- .../protocol_engine/protocol_engine.py | 22 ++- .../file_runner/test_python_file_runner.py | 6 +- .../test_protocol_context.py | 6 +- .../opentrons/protocol_engine/conftest.py | 12 +- .../protocol_engine/test_protocol_engine.py | 51 ++++-- .../robot_server/sessions/action_models.py | 4 +- .../robot_server/sessions/engine_store.py | 39 +++- robot-server/robot_server/sessions/router.py | 38 +++- .../robot_server/sessions/session_models.py | 28 ++- .../robot_server/sessions/session_view.py | 17 +- robot-server/tests/sessions/conftest.py | 86 +++++++++ .../tests/sessions/test_engine_store.py | 37 +++- ...ession_builder.py => test_session_view.py} | 47 ++++- .../tests/sessions/test_sessions_router.py | 168 ++++++++++++++++-- 14 files changed, 493 insertions(+), 68 deletions(-) rename robot-server/tests/sessions/{test_session_builder.py => test_session_view.py} (69%) diff --git a/api/src/opentrons/protocol_engine/protocol_engine.py b/api/src/opentrons/protocol_engine/protocol_engine.py index 1a38c94f4f0..6050a003838 100644 --- a/api/src/opentrons/protocol_engine/protocol_engine.py +++ b/api/src/opentrons/protocol_engine/protocol_engine.py @@ -11,6 +11,7 @@ from .state import StateStore, StateView from .commands import ( CommandRequestType, + PendingCommandType, CompletedCommandType, FailedCommandType, ) @@ -26,6 +27,7 @@ class ProtocolEngine: state_store: StateStore _handlers: CommandHandlers + _resources: ResourceProviders @classmethod async def create(cls, hardware: HardwareAPI) -> ProtocolEngine: @@ -38,8 +40,7 @@ async def create(cls, hardware: HardwareAPI) -> ProtocolEngine: fixed_labware = await resources.deck_data.get_deck_fixed_labware(deck_def) state_store = StateStore( - deck_definition=deck_def, - deck_fixed_labware=fixed_labware + deck_definition=deck_def, deck_fixed_labware=fixed_labware ) handlers = CommandHandlers.create( @@ -48,12 +49,13 @@ async def create(cls, hardware: HardwareAPI) -> ProtocolEngine: state=StateView.create_view(state_store), ) - return cls(state_store=state_store, handlers=handlers) + return cls(state_store=state_store, handlers=handlers, resources=resources) def __init__( self, state_store: StateStore, handlers: CommandHandlers, + resources: ResourceProviders, ) -> None: """Initialize a ProtocolEngine instance. @@ -62,6 +64,7 @@ def __init__( """ self.state_store = state_store self._handlers = handlers + self._resources = resources async def execute_command( self, @@ -94,8 +97,13 @@ async def execute_command( return done_cmd - def add_command(self, request: CommandRequestType) -> None: + def add_command(self, request: CommandRequestType) -> PendingCommandType: """Add a command to ProtocolEngine.""" - # TODO(spp, 2020-05-13): - # Generate a UUID to be used as command_id for each command added. - raise NotImplementedError + command_id = self._resources.id_generator.generate_id() + created_at = utc_now() + command_impl = request.get_implementation() + command = command_impl.create_command(created_at) + + self.state_store.handle_command(command, command_id=command_id) + + return command diff --git a/api/tests/opentrons/file_runner/test_python_file_runner.py b/api/tests/opentrons/file_runner/test_python_file_runner.py index 18d67a95777..c98134c5d6f 100644 --- a/api/tests/opentrons/file_runner/test_python_file_runner.py +++ b/api/tests/opentrons/file_runner/test_python_file_runner.py @@ -10,19 +10,19 @@ def subject() -> PythonFileRunner: return PythonFileRunner() -@pytest.mark.xfail(raises=NotImplementedError) +@pytest.mark.xfail(raises=NotImplementedError, strict=True) def test_python_runner_play(subject: PythonFileRunner) -> None: """It should be able to start the run.""" subject.play() -@pytest.mark.xfail(raises=NotImplementedError) +@pytest.mark.xfail(raises=NotImplementedError, strict=True) def test_python_runner_pause(subject: PythonFileRunner) -> None: """It should be able to pause the run.""" subject.pause() -@pytest.mark.xfail(raises=NotImplementedError) +@pytest.mark.xfail(raises=NotImplementedError, strict=True) def test_python_runner_stop(subject: PythonFileRunner) -> None: """It should be able to stop the run.""" subject.stop() diff --git a/api/tests/opentrons/protocol_api_experimental/test_protocol_context.py b/api/tests/opentrons/protocol_api_experimental/test_protocol_context.py index 0a6d4f160b9..5f44858ecf1 100644 --- a/api/tests/opentrons/protocol_api_experimental/test_protocol_context.py +++ b/api/tests/opentrons/protocol_api_experimental/test_protocol_context.py @@ -114,7 +114,7 @@ def test_load_pipette_with_bad_args( subject.load_pipette(pipette_name="p300_single", mount="west") -@pytest.mark.xfail(raises=NotImplementedError) +@pytest.mark.xfail(raises=NotImplementedError, strict=True) def test_load_pipette_with_tipracks_list(subject: ProtocolContext) -> None: """TODO: it should do something with the `tip_racks` parameter to load_pipette.""" subject.load_pipette( @@ -124,7 +124,7 @@ def test_load_pipette_with_tipracks_list(subject: ProtocolContext) -> None: ) -@pytest.mark.xfail(raises=NotImplementedError) +@pytest.mark.xfail(raises=NotImplementedError, strict=True) def test_load_pipette_with_replace(subject: ProtocolContext) -> None: """TODO: it should do something with the `replace` parameter to load_pipette.""" subject.load_pipette(pipette_name="p300_single", mount="left", replace=True) @@ -189,7 +189,7 @@ def test_load_labware_default_namespace_and_version( assert result == Labware(labware_id="abc123", engine_client=engine_client) -@pytest.mark.xfail(raises=NotImplementedError) +@pytest.mark.xfail(raises=NotImplementedError, strict=True) def test_load_labware_with_label(subject: ProtocolContext) -> None: """TODO: it should do something with the `label` parameter to load_labware.""" subject.load_labware(load_name="some_labware", location=5, label="some_label") diff --git a/api/tests/opentrons/protocol_engine/conftest.py b/api/tests/opentrons/protocol_engine/conftest.py index b1ac51b760a..fd80d1c85ef 100644 --- a/api/tests/opentrons/protocol_engine/conftest.py +++ b/api/tests/opentrons/protocol_engine/conftest.py @@ -152,17 +152,13 @@ def well_plate_def() -> LabwareDefinition: @pytest.fixture(scope="session") def reservoir_def() -> LabwareDefinition: """Get the definition of single-row reservoir.""" - return LabwareDefinition.parse_obj( - load_definition("nest_12_reservoir_15ml", 1) - ) + return LabwareDefinition.parse_obj(load_definition("nest_12_reservoir_15ml", 1)) @pytest.fixture(scope="session") def tip_rack_def() -> LabwareDefinition: """Get the definition of Opentrons 300 uL tip rack.""" - return LabwareDefinition.parse_obj( - load_definition("opentrons_96_tiprack_300ul", 1) - ) + return LabwareDefinition.parse_obj(load_definition("opentrons_96_tiprack_300ul", 1)) @pytest.fixture @@ -177,10 +173,12 @@ def store(standard_deck_def: DeckDefinitionV2) -> StateStore: @pytest.fixture def engine( mock_state_store: MagicMock, - mock_handlers: AsyncMock + mock_handlers: AsyncMock, + mock_resources: AsyncMock, ) -> ProtocolEngine: """Get a ProtocolEngine with its dependencies mocked out.""" return ProtocolEngine( state_store=mock_state_store, handlers=mock_handlers, + resources=mock_resources, ) diff --git a/api/tests/opentrons/protocol_engine/test_protocol_engine.py b/api/tests/opentrons/protocol_engine/test_protocol_engine.py index e132072896e..0a2c7e5fe01 100644 --- a/api/tests/opentrons/protocol_engine/test_protocol_engine.py +++ b/api/tests/opentrons/protocol_engine/test_protocol_engine.py @@ -33,9 +33,8 @@ def __init__(self) -> None: def __eq__(self, other: object) -> bool: """Check if a target object is a datetime that is close to now.""" - return ( - isinstance(other, datetime) and - isclose(self._now.timestamp(), other.timestamp(), rel_tol=5) + return isinstance(other, datetime) and isclose( + self._now.timestamp(), other.timestamp(), rel_tol=5 ) def __repr__(self) -> str: @@ -65,8 +64,7 @@ async def test_create_engine_initializes_state_with_deck_geometry( async def test_execute_command_creates_command( - engine: ProtocolEngine, - mock_state_store: MagicMock + engine: ProtocolEngine, mock_state_store: MagicMock ) -> None: """It should create a command in the state store when executing.""" req = MoveToWellRequest(pipetteId="123", labwareId="abc", wellName="A1") @@ -76,9 +74,9 @@ async def test_execute_command_creates_command( RunningCommand( created_at=cast(datetime, CloseToNow()), started_at=cast(datetime, CloseToNow()), - request=req + request=req, ), - command_id="unique-id" + command_id="unique-id", ) @@ -110,8 +108,7 @@ async def test_execute_command_adds_result_to_state( mock_req.get_implementation.return_value = mock_impl mock_impl.create_command.return_value = PendingCommand( - request=mock_req, - created_at=now + request=mock_req, created_at=now ) mock_impl.execute.return_value = result @@ -144,8 +141,7 @@ async def test_execute_command_adds_error_to_state( mock_req.get_implementation.return_value = mock_impl mock_impl.create_command.return_value = PendingCommand( - request=mock_req, - created_at=now + request=mock_req, created_at=now ) mock_impl.execute.side_effect = error @@ -179,7 +175,7 @@ async def test_execute_command_adds_unexpected_error_to_state( mock_req.get_implementation.return_value = mock_impl mock_impl.create_command.return_value = PendingCommand( request=mock_req, - created_at=now + created_at=now, ) mock_impl.execute.side_effect = error @@ -192,3 +188,34 @@ async def test_execute_command_adds_unexpected_error_to_state( cmd, command_id="unique-id", ) + + +def test_add_command( + engine: ProtocolEngine, + mock_handlers: AsyncMock, + mock_state_store: MagicMock, + mock_resources: AsyncMock, + now: datetime, +) -> None: + """It should add a pending command into state.""" + mock_resources.id_generator.generate_id.return_value = "command-id" + + mock_req = MagicMock(spec=MoveToWellRequest) + mock_impl = AsyncMock(spec=MoveToWellImplementation) + + mock_req.get_implementation.return_value = mock_impl + mock_impl.create_command.return_value = PendingCommand( + request=mock_req, created_at=now + ) + + result = engine.add_command(mock_req) + + assert result == PendingCommand( + created_at=cast(datetime, CloseToNow()), + request=mock_req, + ) + + mock_state_store.handle_command.assert_called_with( + result, + command_id="command-id", + ) diff --git a/robot-server/robot_server/sessions/action_models.py b/robot-server/robot_server/sessions/action_models.py index 273a67b0f83..dc04e2bc349 100644 --- a/robot-server/robot_server/sessions/action_models.py +++ b/robot-server/robot_server/sessions/action_models.py @@ -15,7 +15,7 @@ class SessionActionType(str, Enum): class SessionActionCreateData(BaseModel): """Request model for new control action creation.""" - controlType: SessionActionType + actionType: SessionActionType class SessionAction(ResourceModel): @@ -30,7 +30,7 @@ class SessionAction(ResourceModel): id: str = Field(..., description="A unique identifier to reference the command.") createdAt: datetime = Field(..., description="When the command was created.") - controlType: SessionActionType = Field( + actionType: SessionActionType = Field( ..., description="Specific type of action, which determines behavior.", ) diff --git a/robot-server/robot_server/sessions/engine_store.py b/robot-server/robot_server/sessions/engine_store.py index d8ff67dacaa..dd66b33f322 100644 --- a/robot-server/robot_server/sessions/engine_store.py +++ b/robot-server/robot_server/sessions/engine_store.py @@ -2,7 +2,9 @@ from typing import Optional from opentrons.hardware_control import API as HardwareAPI +from opentrons.file_runner import AbstractFileRunner, ProtocolFile, create_file_runner from opentrons.protocol_engine import ProtocolEngine +from robot_server.protocols import ProtocolResource class EngineConflictError(RuntimeError): @@ -33,6 +35,7 @@ def __init__(self, hardware_api: HardwareAPI) -> None: """ self._hardware_api = hardware_api self._engine: Optional[ProtocolEngine] = None + self._runner: Optional[AbstractFileRunner] = None @property def engine(self) -> ProtocolEngine: @@ -46,7 +49,19 @@ def engine(self) -> ProtocolEngine: return self._engine - async def create(self) -> ProtocolEngine: + @property + def runner(self) -> AbstractFileRunner: + """Get the persisted AbstractFileRunner. + + Raises: + EngineMissingError: Runner has not yet been created and persisted. + """ + if self._runner is None: + raise EngineMissingError("Runner not yet created.") + + return self._runner + + async def create(self, protocol: Optional[ProtocolResource]) -> ProtocolEngine: """Create and store a ProtocolEngine. Returns: @@ -56,15 +71,35 @@ async def create(self) -> ProtocolEngine: EngineConflictError: a ProtocolEngine is already present. """ # NOTE: this async. creation happens before the `self._engine` - # check intentially to avoid a race condition where `self._engine` is + # 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 ProtocolEngine.create(hardware=self._hardware_api) + runner = None if self._engine is not None: raise EngineConflictError("Cannot load multiple sessions simultaneously.") + if protocol is not None: + # TODO(mc, 2021-06-11): add multi-file support. As written, other + # parts of the API will make sure len(files) == 0, but this will + # not remain true as engine sessions are built out + protocol_file = ProtocolFile( + file_path=protocol.files[0], + file_type=protocol.protocol_type, + ) + runner = create_file_runner( + protocol_file=protocol_file, + engine=engine, + ) + + # TODO(mc, 2021-06-11): serparating load from creation is a potentially + # two-phase initialization. Revisit this, but for now keep the weirdness + # contained here in this factory method + runner.load() + self._engine = engine + self._runner = runner return engine diff --git a/robot-server/robot_server/sessions/router.py b/robot-server/robot_server/sessions/router.py index 3196e754297..263e9bb68be 100644 --- a/robot-server/robot_server/sessions/router.py +++ b/robot-server/robot_server/sessions/router.py @@ -13,11 +13,12 @@ MultiResponseModel, ) +from robot_server.protocols import ProtocolStore, get_protocol_store from .session_store import SessionStore, SessionNotFoundError from .session_view import SessionView -from .session_models import Session, SessionCreateData +from .session_models import Session, SessionCreateData, ProtocolSessionCreateData from .action_models import SessionAction, SessionActionCreateData -from .engine_store import EngineStore, EngineConflictError +from .engine_store import EngineStore, EngineConflictError, EngineMissingError from .dependencies import get_session_store, get_engine_store sessions_router = APIRouter() @@ -38,6 +39,13 @@ class SessionAlreadyActive(ErrorDetails): title: str = "Session Already Active" +class SessionActionNotAllowed(ErrorDetails): + """An error if one tries to issue an unsupported session action.""" + + id: Literal["SessionActionNotAllow"] = "SessionActionNotAllow" + title: str = "Session Action Not Allowed" + + @sessions_router.post( path="/sessions", summary="Create a session", @@ -53,6 +61,7 @@ async def create_session( session_view: SessionView = Depends(SessionView), session_store: SessionStore = Depends(get_session_store), engine_store: EngineStore = Depends(get_engine_store), + protocol_store: ProtocolStore = Depends(get_protocol_store), session_id: str = Depends(get_unique_id), created_at: datetime = Depends(get_current_time), ) -> ResponseModel[Session]: @@ -63,6 +72,7 @@ async def create_session( session_view: Session model construction interface. session_store: Session storage interface. engine_store: ProtocolEngine storage and control. + protocol_store: Protocol resource storage. session_id: Generated ID to assign to the session. created_at: Timestamp to attach to created session """ @@ -72,10 +82,14 @@ async def create_session( created_at=created_at, create_data=create_data, ) + protocol = None + + if isinstance(create_data, ProtocolSessionCreateData): + protocol = protocol_store.get(protocol_id=create_data.createParams.protocolId) try: # TODO(mc, 2021-05-28): return engine state to build response model - await engine_store.create() + await engine_store.create(protocol=protocol) except EngineConflictError as e: raise SessionAlreadyActive(detail=str(e)).as_error(status.HTTP_409_CONFLICT) @@ -176,13 +190,17 @@ async def remove_session_by_id( ), status_code=status.HTTP_201_CREATED, response_model=ResponseModel[SessionAction], - responses={status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[SessionNotFound]}}, + responses={ + status.HTTP_400_BAD_REQUEST: {"model": ErrorResponse[SessionActionNotAllowed]}, + status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[SessionNotFound]}, + }, ) async def create_session_action( sessionId: str, request_body: RequestModel[SessionActionCreateData], session_view: SessionView = Depends(SessionView), session_store: SessionStore = Depends(get_session_store), + engine_store: EngineStore = Depends(get_engine_store), action_id: str = Depends(get_unique_id), created_at: datetime = Depends(get_current_time), ) -> ResponseModel[SessionAction]: @@ -193,24 +211,30 @@ async def create_session_action( request_body: Input payload from the request body. session_view: Resource model builder. session_store: Session storage interface. + engine_store: Protocol engine and runner storage. action_id: Generated ID to assign to the control action. created_at: Timestamp to attach to the control action. """ try: prev_session = session_store.get(session_id=sessionId) - actions, next_session = session_view.with_action( + action, next_session = session_view.with_action( session=prev_session, action_id=action_id, action_data=request_body.data, created_at=created_at, ) - raise NotImplementedError("Action handling not yet implemented") + # TODO(mc, 2021-06-11): support actions other than `start` + engine_store.runner.play() except SessionNotFoundError as e: raise SessionNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) + except EngineMissingError as e: + raise SessionActionNotAllowed(detail=str(e)).as_error( + status.HTTP_400_BAD_REQUEST + ) session_store.upsert(session=next_session) - return ResponseModel(data=actions) + return ResponseModel(data=action) diff --git a/robot-server/robot_server/sessions/session_models.py b/robot-server/robot_server/sessions/session_models.py index 586a70fa250..afc5b06b2f6 100644 --- a/robot-server/robot_server/sessions/session_models.py +++ b/robot-server/robot_server/sessions/session_models.py @@ -13,6 +13,7 @@ class SessionType(str, Enum): """All available session types.""" BASIC = "basic" + PROTOCOL = "protocol" class AbstractSessionCreateData(BaseModel): @@ -39,7 +40,7 @@ class AbstractSession(ResourceModel): None, description="Configuration parameters for the session.", ) - controlCommands: List[SessionAction] = Field( + actions: List[SessionAction] = Field( ..., description="Client-initiated commands for session control.", ) @@ -57,10 +58,35 @@ class BasicSession(AbstractSession): sessionType: Literal[SessionType.BASIC] = SessionType.BASIC +class ProtocolSessionCreateParams(BaseModel): + """Creation parameters for a protocol session.""" + + protocolId: str = Field( + ..., + description="Unique identifier of the protocol this session will run.", + ) + + +class ProtocolSessionCreateData(AbstractSessionCreateData): + """Creation request data for a basic session.""" + + sessionType: Literal[SessionType.PROTOCOL] = SessionType.PROTOCOL + createParams: ProtocolSessionCreateParams + + +class ProtocolSession(AbstractSession): + """A session to execute commands without a previously loaded protocol file.""" + + sessionType: Literal[SessionType.PROTOCOL] = SessionType.PROTOCOL + createParams: ProtocolSessionCreateParams + + SessionCreateData = Union[ BasicSessionCreateData, + ProtocolSessionCreateData, ] Session = Union[ BasicSession, + ProtocolSession, ] diff --git a/robot-server/robot_server/sessions/session_view.py b/robot-server/robot_server/sessions/session_view.py index bd515c28d53..2930b4e9ee0 100644 --- a/robot-server/robot_server/sessions/session_view.py +++ b/robot-server/robot_server/sessions/session_view.py @@ -7,9 +7,11 @@ from .action_models import SessionAction, SessionActionCreateData from .session_models import ( Session, - BasicSession, SessionCreateData, + BasicSession, BasicSessionCreateData, + ProtocolSession, + ProtocolSessionCreateData, ) @@ -67,7 +69,7 @@ def with_action( actions = SessionAction( id=action_id, createdAt=created_at, - controlType=action_data.controlType, + actionType=action_data.actionType, ) updated_session = replace( @@ -93,7 +95,14 @@ def as_response(session: SessionResource) -> Session: return BasicSession.construct( id=session.session_id, createdAt=session.created_at, - controlCommands=session.actions, + actions=session.actions, + ) + if isinstance(create_data, ProtocolSessionCreateData): + return ProtocolSession.construct( + id=session.session_id, + createdAt=session.created_at, + createParams=create_data.createParams, + actions=session.actions, ) - raise ValueError(f"Invalid session store entry {session}") + raise ValueError(f"Invalid session resource {session}") diff --git a/robot-server/tests/sessions/conftest.py b/robot-server/tests/sessions/conftest.py index 12875fdeae8..d95502365ac 100644 --- a/robot-server/tests/sessions/conftest.py +++ b/robot-server/tests/sessions/conftest.py @@ -1,12 +1,21 @@ """Common test fixtures for sessions route tests.""" import pytest +import json +from pathlib import Path from decoy import Decoy +from robot_server.protocols import ProtocolStore from robot_server.sessions.session_view import SessionView from robot_server.sessions.session_store import SessionStore from robot_server.sessions.engine_store import EngineStore +@pytest.fixture +def protocol_store(decoy: Decoy) -> ProtocolStore: + """Get a mock ProtocolStore interface.""" + return decoy.create_decoy(spec=ProtocolStore) + + @pytest.fixture def session_store(decoy: Decoy) -> SessionStore: """Get a mock SessionStore interface.""" @@ -23,3 +32,80 @@ def session_view(decoy: Decoy) -> SessionView: def engine_store(decoy: Decoy) -> EngineStore: """Get a mock EngineStore interface.""" return decoy.create_decoy(spec=EngineStore) + + +@pytest.fixture +def json_protocol_file( + tmp_path: Path, + minimal_labware_def: dict, +) -> Path: + """Get an on-disk, minimal JSON protocol fixture.""" + file_path = tmp_path / "protocol-name.json" + + file_path.write_text( + json.dumps( + { + "schemaVersion": 3, + "metadata": {}, + "robot": {"model": "OT-2 Standard"}, + "pipettes": {"leftPipetteId": {"mount": "left", "name": "p300_single"}}, + "labware": { + "trashId": { + "slot": "12", + "displayName": "Trash", + "definitionId": "opentrons/opentrons_1_trash_1100ml_fixed/1", + }, + "tiprack1Id": { + "slot": "1", + "displayName": "Opentrons 96 Tip Rack 300 µL", + "definitionId": "opentrons/opentrons_96_tiprack_300ul/1", + }, + "wellplate1Id": { + "slot": "10", + "displayName": "Corning 96 Well Plate 360 µL Flat", + "definitionId": "opentrons/corning_96_wellplate_360ul_flat/1", + }, + }, + "labwareDefinitions": { + "opentrons/opentrons_1_trash_1100ml_fixed/1": minimal_labware_def, + "opentrons/opentrons_96_tiprack_300ul/1": minimal_labware_def, + "opentrons/corning_96_wellplate_360ul_flat/1": minimal_labware_def, + }, + "commands": [ + { + "command": "pickUpTip", + "params": { + "pipette": "leftPipetteId", + "labware": "tiprack1Id", + "well": "A1", + }, + }, + { + "command": "aspirate", + "params": { + "pipette": "leftPipetteId", + "volume": 51, + "labware": "wellplate1Id", + "well": "B1", + "offsetFromBottomMm": 10, + "flowRate": 10, + }, + }, + { + "command": "dispense", + "params": { + "pipette": "leftPipetteId", + "volume": 50, + "labware": "wellplate1Id", + "well": "H1", + "offsetFromBottomMm": 1, + "flowRate": 50, + }, + }, + ], + } + ), + encoding="utf-8", + ) + + return file_path diff --git a/robot-server/tests/sessions/test_engine_store.py b/robot-server/tests/sessions/test_engine_store.py index e1d61418f20..0b41f1d9e5b 100644 --- a/robot-server/tests/sessions/test_engine_store.py +++ b/robot-server/tests/sessions/test_engine_store.py @@ -1,8 +1,12 @@ """Tests for the EngineStore interface.""" import pytest +from datetime import datetime from mock import MagicMock +from pathlib import Path from opentrons.protocol_engine import ProtocolEngine +from opentrons.file_runner import JsonFileRunner +from robot_server.protocols import ProtocolResource, ProtocolFileType from robot_server.sessions.engine_store import ( EngineStore, EngineConflictError, @@ -20,19 +24,44 @@ def subject() -> EngineStore: async def test_create_engine(subject: EngineStore) -> None: """It should create an engine.""" - result = await subject.create() + result = await subject.create(protocol=None) assert result == subject.engine assert isinstance(result, ProtocolEngine) assert isinstance(subject.engine, ProtocolEngine) +async def test_create_engine_for_json_protocol( + subject: EngineStore, + json_protocol_file: Path, +) -> None: + """It should create a protocol runner. + + This test is functioning as an integration / smoke test. Ensuring that + the protocol was loaded correctly is / should be covered in unit tests + elsewhere. + """ + protocol = ProtocolResource( + protocol_id="protocol-id", + protocol_type=ProtocolFileType.JSON, + created_at=datetime.now(), + files=[json_protocol_file], + ) + + result = await subject.create(protocol=protocol) + + assert result == subject.engine + assert isinstance(result, ProtocolEngine) + assert isinstance(subject.engine, ProtocolEngine) + assert isinstance(subject.runner, JsonFileRunner) + + async def test_raise_if_engine_already_exists(subject: EngineStore) -> None: """It should not create more than one engine / runner pair.""" - await subject.create() + await subject.create(protocol=None) with pytest.raises(EngineConflictError): - await subject.create() + await subject.create(protocol=None) def test_raise_if_engine_does_not_exist(subject: EngineStore) -> None: @@ -43,7 +72,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(protocol=None) subject.clear() with pytest.raises(EngineMissingError): diff --git a/robot-server/tests/sessions/test_session_builder.py b/robot-server/tests/sessions/test_session_view.py similarity index 69% rename from robot-server/tests/sessions/test_session_builder.py rename to robot-server/tests/sessions/test_session_view.py index 6555dd7d199..bb2fd7247f4 100644 --- a/robot-server/tests/sessions/test_session_builder.py +++ b/robot-server/tests/sessions/test_session_view.py @@ -8,6 +8,9 @@ Session, BasicSession, BasicSessionCreateData, + ProtocolSession, + ProtocolSessionCreateData, + ProtocolSessionCreateParams, ) from robot_server.sessions.action_models import ( @@ -57,6 +60,28 @@ def test_create_session_resource() -> None: ) +def test_create_protocol_session_resource() -> None: + """It should create a protocol session resource view.""" + created_at = datetime.now() + create_data = ProtocolSessionCreateData( + createParams=ProtocolSessionCreateParams(protocolId="protocol-id") + ) + + subject = SessionView() + result = subject.as_resource( + session_id="session-id", + create_data=create_data, + created_at=created_at, + ) + + assert result == SessionResource( + session_id="session-id", + create_data=create_data, + created_at=created_at, + actions=[], + ) + + current_time = datetime.now() @@ -73,7 +98,23 @@ def test_create_session_resource() -> None: BasicSession( id="session-id", createdAt=current_time, - controlCommands=[], + actions=[], + ), + ), + ( + SessionResource( + session_id="session-id", + create_data=ProtocolSessionCreateData( + createParams=ProtocolSessionCreateParams(protocolId="protocol-id") + ), + created_at=current_time, + actions=[], + ), + ProtocolSession( + id="session-id", + createdAt=current_time, + createParams=ProtocolSessionCreateParams(protocolId="protocol-id"), + actions=[], ), ), ), @@ -99,7 +140,7 @@ def test_create_action(current_time: datetime) -> None: ) command_data = SessionActionCreateData( - controlType=SessionActionType.START, + actionType=SessionActionType.START, ) subject = SessionView() @@ -113,7 +154,7 @@ def test_create_action(current_time: datetime) -> None: assert action_result == SessionAction( id="control-command-id", createdAt=current_time, - controlType=SessionActionType.START, + actionType=SessionActionType.START, ) assert session_result == SessionResource( diff --git a/robot-server/tests/sessions/test_sessions_router.py b/robot-server/tests/sessions/test_sessions_router.py index 5c122b0a494..871182567bd 100644 --- a/robot-server/tests/sessions/test_sessions_router.py +++ b/robot-server/tests/sessions/test_sessions_router.py @@ -9,12 +9,23 @@ from typing import AsyncIterator from robot_server.errors import exception_handlers +from robot_server.protocols import ProtocolStore, ProtocolResource, ProtocolFileType from robot_server.sessions.session_view import ( SessionView, BasicSessionCreateData, ) -from robot_server.sessions.session_models import BasicSession -from robot_server.sessions.engine_store import EngineStore, EngineConflictError +from robot_server.sessions.session_models import ( + BasicSession, + ProtocolSession, + ProtocolSessionCreateData, + ProtocolSessionCreateParams, +) + +from robot_server.sessions.engine_store import ( + EngineStore, + EngineConflictError, + EngineMissingError, +) from robot_server.sessions.session_store import ( SessionStore, @@ -32,8 +43,10 @@ sessions_router, SessionNotFound, SessionAlreadyActive, + SessionActionNotAllowed, get_session_store, get_engine_store, + get_protocol_store, get_unique_id, get_current_time, ) @@ -46,6 +59,7 @@ def app( session_store: SessionStore, session_view: SessionView, engine_store: EngineStore, + protocol_store: ProtocolStore, unique_id: str, current_time: datetime, ) -> FastAPI: @@ -54,6 +68,7 @@ def app( app.dependency_overrides[SessionView] = lambda: session_view app.dependency_overrides[get_session_store] = lambda: session_store app.dependency_overrides[get_engine_store] = lambda: engine_store + app.dependency_overrides[get_protocol_store] = lambda: protocol_store app.dependency_overrides[get_unique_id] = lambda: unique_id app.dependency_overrides[get_current_time] = lambda: current_time app.include_router(sessions_router) @@ -96,7 +111,7 @@ async def test_create_session( expected_response = BasicSession( id=unique_id, createdAt=current_time, - controlCommands=[], + actions=[], ) decoy.when( @@ -120,7 +135,74 @@ async def test_create_session( # TODO(mc, 2021-05-27): spec the initialize method to return actual data decoy.verify( - await engine_store.create(), + await engine_store.create(protocol=None), + session_store.upsert(session=session), + ) + + +async def test_create_protocol_session( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + protocol_store: ProtocolStore, + engine_store: EngineStore, + unique_id: str, + current_time: datetime, + async_client: AsyncClient, +) -> None: + """It should be able to create a protocol session.""" + session = SessionResource( + session_id=unique_id, + created_at=current_time, + create_data=ProtocolSessionCreateData( + createParams=ProtocolSessionCreateParams(protocolId="protocol-id") + ), + actions=[], + ) + protocol = ProtocolResource( + protocol_id="protocol-id", + protocol_type=ProtocolFileType.JSON, + created_at=datetime.now(), + files=[], + ) + expected_response = ProtocolSession( + id=unique_id, + createdAt=current_time, + createParams=ProtocolSessionCreateParams(protocolId="protocol-id"), + actions=[], + ) + + decoy.when(protocol_store.get(protocol_id="protocol-id")).then_return(protocol) + + decoy.when( + session_view.as_resource( + create_data=ProtocolSessionCreateData( + createParams=ProtocolSessionCreateParams(protocolId="protocol-id") + ), + session_id=unique_id, + created_at=current_time, + ) + ).then_return(session) + + decoy.when( + session_view.as_response(session=session), + ).then_return(expected_response) + + response = await async_client.post( + "/sessions", + json={ + "data": { + "sessionType": "protocol", + "createParams": {"protocolId": "protocol-id"}, + } + }, + ) + + verify_response(response, expected_status=201, expected_data=expected_response) + + # TODO(mc, 2021-05-27): spec the initialize method to return actual data + decoy.verify( + await engine_store.create(protocol=protocol), session_store.upsert(session=session), ) @@ -150,7 +232,9 @@ async def test_create_session_conflict( ) ).then_return(session) - decoy.when(await engine_store.create()).then_raise(EngineConflictError("oh no")) + decoy.when(await engine_store.create(protocol=None)).then_raise( + EngineConflictError("oh no") + ) response = await async_client.post("/sessions") @@ -179,7 +263,7 @@ def test_get_session( expected_response = BasicSession( id="session-id", createdAt=created_at, - controlCommands=[], + actions=[], ) decoy.when(session_store.get(session_id="session-id")).then_return(session) @@ -250,12 +334,12 @@ def test_get_sessions_not_empty( response_1 = BasicSession( id="unique-id-1", createdAt=created_at_1, - controlCommands=[], + actions=[], ) response_2 = BasicSession( id="unique-id-2", createdAt=created_at_2, - controlCommands=[], + actions=[], ) decoy.when(session_store.get_all()).then_return([session_1, session_2]) @@ -312,11 +396,11 @@ def test_delete_session_with_bad_id( ) -@pytest.mark.xfail(raises=NotImplementedError) def test_create_session_action( decoy: Decoy, session_view: SessionView, session_store: SessionStore, + engine_store: EngineStore, unique_id: str, current_time: datetime, client: TestClient, @@ -325,7 +409,7 @@ def test_create_session_action( session_created_at = datetime.now() actions = SessionAction( - controlType=SessionActionType.START, + actionType=SessionActionType.START, createdAt=current_time, id=unique_id, ) @@ -350,17 +434,18 @@ def test_create_session_action( session_view.with_action( session=prev_session, action_id=unique_id, - action_data=SessionActionCreateData(controlType=SessionActionType.START), + action_data=SessionActionCreateData(actionType=SessionActionType.START), created_at=current_time, ), ).then_return((actions, next_session)) response = client.post( "/sessions/session-id/actions", - json={"data": {"controlType": "start"}}, + json={"data": {"actionType": "start"}}, ) verify_response(response, expected_status=201, expected_data=actions) + decoy.verify(engine_store.runner.play()) def test_create_session_action_with_missing_id( @@ -377,7 +462,7 @@ def test_create_session_action_with_missing_id( response = client.post( "/sessions/session-id/actions", - json={"data": {"controlType": "start"}}, + json={"data": {"actionType": "start"}}, ) verify_response( @@ -385,3 +470,60 @@ def test_create_session_action_with_missing_id( expected_status=404, expected_errors=SessionNotFound(detail=str(not_found_error)), ) + + +def test_create_session_action_without_runner( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + engine_store: EngineStore, + unique_id: str, + current_time: datetime, + client: TestClient, +) -> None: + """It should handle a start input.""" + session_created_at = datetime.now() + + actions = SessionAction( + actionType=SessionActionType.START, + createdAt=current_time, + id=unique_id, + ) + + prev_session = SessionResource( + session_id="unique-id", + create_data=BasicSessionCreateData(), + created_at=session_created_at, + actions=[], + ) + + next_session = SessionResource( + session_id="unique-id", + create_data=BasicSessionCreateData(), + created_at=session_created_at, + actions=[actions], + ) + + decoy.when(session_store.get(session_id="session-id")).then_return(prev_session) + + decoy.when( + session_view.with_action( + session=prev_session, + action_id=unique_id, + action_data=SessionActionCreateData(actionType=SessionActionType.START), + created_at=current_time, + ), + ).then_return((actions, next_session)) + + decoy.when(engine_store.runner.play()).then_raise(EngineMissingError("oh no")) + + response = client.post( + "/sessions/session-id/actions", + json={"data": {"actionType": "start"}}, + ) + + verify_response( + response, + expected_status=400, + expected_errors=SessionActionNotAllowed(detail="oh no"), + ) From 25c79f453c079edc55c1fa1fec42aba21515e397 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Fri, 11 Jun 2021 14:47:43 -0400 Subject: [PATCH 2/5] fixup: revert styling changes --- .../opentrons/protocol_engine/test_protocol_engine.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/api/tests/opentrons/protocol_engine/test_protocol_engine.py b/api/tests/opentrons/protocol_engine/test_protocol_engine.py index 0a2c7e5fe01..9b483fc8416 100644 --- a/api/tests/opentrons/protocol_engine/test_protocol_engine.py +++ b/api/tests/opentrons/protocol_engine/test_protocol_engine.py @@ -64,7 +64,8 @@ async def test_create_engine_initializes_state_with_deck_geometry( async def test_execute_command_creates_command( - engine: ProtocolEngine, mock_state_store: MagicMock + engine: ProtocolEngine, + mock_state_store: MagicMock, ) -> None: """It should create a command in the state store when executing.""" req = MoveToWellRequest(pipetteId="123", labwareId="abc", wellName="A1") @@ -108,7 +109,8 @@ async def test_execute_command_adds_result_to_state( mock_req.get_implementation.return_value = mock_impl mock_impl.create_command.return_value = PendingCommand( - request=mock_req, created_at=now + request=mock_req, + created_at=now, ) mock_impl.execute.return_value = result @@ -141,7 +143,8 @@ async def test_execute_command_adds_error_to_state( mock_req.get_implementation.return_value = mock_impl mock_impl.create_command.return_value = PendingCommand( - request=mock_req, created_at=now + request=mock_req, + created_at=now, ) mock_impl.execute.side_effect = error From 478d9d5465f6c30185dd377d61a325c63dd50a88 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Mon, 14 Jun 2021 12:21:17 -0400 Subject: [PATCH 3/5] fixup: grab some docstring typos --- robot-server/robot_server/sessions/router.py | 2 +- robot-server/robot_server/sessions/session_models.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/robot-server/robot_server/sessions/router.py b/robot-server/robot_server/sessions/router.py index 263e9bb68be..0d435e8d649 100644 --- a/robot-server/robot_server/sessions/router.py +++ b/robot-server/robot_server/sessions/router.py @@ -42,7 +42,7 @@ class SessionAlreadyActive(ErrorDetails): class SessionActionNotAllowed(ErrorDetails): """An error if one tries to issue an unsupported session action.""" - id: Literal["SessionActionNotAllow"] = "SessionActionNotAllow" + id: Literal["SessionActionNotAllowed"] = "SessionActionNotAllowed" title: str = "Session Action Not Allowed" diff --git a/robot-server/robot_server/sessions/session_models.py b/robot-server/robot_server/sessions/session_models.py index afc5b06b2f6..56f54bbd089 100644 --- a/robot-server/robot_server/sessions/session_models.py +++ b/robot-server/robot_server/sessions/session_models.py @@ -68,14 +68,14 @@ class ProtocolSessionCreateParams(BaseModel): class ProtocolSessionCreateData(AbstractSessionCreateData): - """Creation request data for a basic session.""" + """Creation request data for a protocol session.""" sessionType: Literal[SessionType.PROTOCOL] = SessionType.PROTOCOL createParams: ProtocolSessionCreateParams class ProtocolSession(AbstractSession): - """A session to execute commands without a previously loaded protocol file.""" + """A session to execute commands with a previously loaded protocol file.""" sessionType: Literal[SessionType.PROTOCOL] = SessionType.PROTOCOL createParams: ProtocolSessionCreateParams From d8b9be4770a631f688b364ead38f250df86e715f Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Mon, 14 Jun 2021 12:37:03 -0400 Subject: [PATCH 4/5] fixup: catch ProtocolNotFoundError --- .../protocol_engine/protocol_engine.py | 3 ++ .../robot_server/protocols/__init__.py | 7 +++- robot-server/robot_server/sessions/router.py | 21 +++++++--- .../tests/sessions/test_sessions_router.py | 40 ++++++++++++++++++- 4 files changed, 63 insertions(+), 8 deletions(-) diff --git a/api/src/opentrons/protocol_engine/protocol_engine.py b/api/src/opentrons/protocol_engine/protocol_engine.py index 6050a003838..71f944362a9 100644 --- a/api/src/opentrons/protocol_engine/protocol_engine.py +++ b/api/src/opentrons/protocol_engine/protocol_engine.py @@ -99,6 +99,9 @@ async def execute_command( def add_command(self, request: CommandRequestType) -> PendingCommandType: """Add a command to ProtocolEngine.""" + # TODO(mc, 2021-06-14): ID generation and timestamp generation need to + # be redone / reconsidered. Too much about command execution has leaked + # into the root ProtocolEngine class, so we should delegate downwards. command_id = self._resources.id_generator.generate_id() created_at = utc_now() command_impl = request.get_implementation() diff --git a/robot-server/robot_server/protocols/__init__.py b/robot-server/robot_server/protocols/__init__.py index a0721606521..488eae637b2 100644 --- a/robot-server/robot_server/protocols/__init__.py +++ b/robot-server/robot_server/protocols/__init__.py @@ -2,17 +2,20 @@ from opentrons.file_runner import ProtocolFileType -from .router import protocols_router +from .router import protocols_router, ProtocolNotFound from .dependencies import get_protocol_store -from .protocol_store import ProtocolStore, ProtocolResource +from .protocol_store import ProtocolStore, ProtocolResource, ProtocolNotFoundError __all__ = [ # main protocols router "protocols_router", + # common error response details + "ProtocolNotFound", # protocol state management "get_protocol_store", "ProtocolStore", "ProtocolResource", + "ProtocolNotFoundError", # convenience re-exports from opentrons "ProtocolFileType", ] diff --git a/robot-server/robot_server/sessions/router.py b/robot-server/robot_server/sessions/router.py index 0d435e8d649..910d70aaa22 100644 --- a/robot-server/robot_server/sessions/router.py +++ b/robot-server/robot_server/sessions/router.py @@ -13,7 +13,13 @@ MultiResponseModel, ) -from robot_server.protocols import ProtocolStore, get_protocol_store +from robot_server.protocols import ( + ProtocolStore, + ProtocolNotFound, + ProtocolNotFoundError, + get_protocol_store, +) + from .session_store import SessionStore, SessionNotFoundError from .session_view import SessionView from .session_models import Session, SessionCreateData, ProtocolSessionCreateData @@ -53,7 +59,8 @@ class SessionActionNotAllowed(ErrorDetails): status_code=status.HTTP_201_CREATED, response_model=ResponseModel[Session], responses={ - status.HTTP_409_CONFLICT: {"model": ErrorResponse[SessionAlreadyActive]} + status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[ProtocolNotFound]}, + status.HTTP_409_CONFLICT: {"model": ErrorResponse[SessionAlreadyActive]}, }, ) async def create_session( @@ -84,12 +91,16 @@ async def create_session( ) protocol = None - if isinstance(create_data, ProtocolSessionCreateData): - protocol = protocol_store.get(protocol_id=create_data.createParams.protocolId) - try: + if isinstance(create_data, ProtocolSessionCreateData): + protocol = protocol_store.get( + protocol_id=create_data.createParams.protocolId + ) + # TODO(mc, 2021-05-28): return engine state to build response model await engine_store.create(protocol=protocol) + except ProtocolNotFoundError as e: + raise ProtocolNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) except EngineConflictError as e: raise SessionAlreadyActive(detail=str(e)).as_error(status.HTTP_409_CONFLICT) diff --git a/robot-server/tests/sessions/test_sessions_router.py b/robot-server/tests/sessions/test_sessions_router.py index 871182567bd..05754d0872b 100644 --- a/robot-server/tests/sessions/test_sessions_router.py +++ b/robot-server/tests/sessions/test_sessions_router.py @@ -9,7 +9,13 @@ from typing import AsyncIterator from robot_server.errors import exception_handlers -from robot_server.protocols import ProtocolStore, ProtocolResource, ProtocolFileType +from robot_server.protocols import ( + ProtocolStore, + ProtocolResource, + ProtocolFileType, + ProtocolNotFoundError, + ProtocolNotFound, +) from robot_server.sessions.session_view import ( SessionView, BasicSessionCreateData, @@ -207,6 +213,38 @@ async def test_create_protocol_session( ) +async def test_create_protocol_session_missing_protocol( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + protocol_store: ProtocolStore, + engine_store: EngineStore, + unique_id: str, + current_time: datetime, + async_client: AsyncClient, +) -> None: + """It should 404 if a protocol for a session does not exist.""" + error = ProtocolNotFoundError("protocol-id") + + decoy.when(protocol_store.get(protocol_id="protocol-id")).then_raise(error) + + response = await async_client.post( + "/sessions", + json={ + "data": { + "sessionType": "protocol", + "createParams": {"protocolId": "protocol-id"}, + } + }, + ) + + verify_response( + response, + expected_status=404, + expected_errors=ProtocolNotFound(detail=str(error)), + ) + + async def test_create_session_conflict( decoy: Decoy, session_view: SessionView, From 7d58098f543dc53ceb942db43e8ca7e748c0c3a6 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Mon, 14 Jun 2021 13:56:04 -0400 Subject: [PATCH 5/5] fixup: add xfailed test for race condition --- robot-server/tests/sessions/test_engine_store.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/robot-server/tests/sessions/test_engine_store.py b/robot-server/tests/sessions/test_engine_store.py index 0b41f1d9e5b..9b3c5645f3d 100644 --- a/robot-server/tests/sessions/test_engine_store.py +++ b/robot-server/tests/sessions/test_engine_store.py @@ -64,6 +64,14 @@ async def test_raise_if_engine_already_exists(subject: EngineStore) -> None: await subject.create(protocol=None) +@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") + + def test_raise_if_engine_does_not_exist(subject: EngineStore) -> None: """It should raise if no engine exists when requested.""" with pytest.raises(EngineMissingError):