From a186775675423eb070ca67d3b5055963d2f39841 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Thu, 24 Jun 2021 15:40:34 -0400 Subject: [PATCH] refactor(robot-server): add ProtocolEngine commands to /sessions responses Closes #7871 --- .../file_runner/command_queue_worker.py | 2 +- api/src/opentrons/protocol_engine/__init__.py | 6 + .../protocol_engine/commands/__init__.py | 63 ++++- .../commands/command_unions.py | 52 +++- .../protocol_engine/protocol_engine.py | 2 +- .../protocol_engine/state/commands.py | 10 +- .../file_runner/test_command_queue_worker.py | 4 +- .../state/test_command_view.py | 28 +- .../protocol_engine/test_protocol_engine.py | 6 +- .../robot_server/service/json_api/response.py | 2 +- robot-server/robot_server/sessions/router.py | 118 ++++++++- .../robot_server/sessions/schema_models.py | 43 ++++ .../robot_server/sessions/session_models.py | 15 +- .../robot_server/sessions/session_view.py | 15 +- .../tests/sessions/test_session_view.py | 63 ++++- .../tests/sessions/test_sessions_router.py | 239 ++++++++++++++++-- 16 files changed, 579 insertions(+), 89 deletions(-) create mode 100644 robot-server/robot_server/sessions/schema_models.py diff --git a/api/src/opentrons/file_runner/command_queue_worker.py b/api/src/opentrons/file_runner/command_queue_worker.py index a1a439b271e2..09c422a917c7 100644 --- a/api/src/opentrons/file_runner/command_queue_worker.py +++ b/api/src/opentrons/file_runner/command_queue_worker.py @@ -89,7 +89,7 @@ def _next_command( ) -> Optional[str]: if self._keep_running: # Will be None if the engine has no commands left. - return self._engine.state_view.commands.get_next_command() + return self._engine.state_view.commands.get_next_queued() else: return None diff --git a/api/src/opentrons/protocol_engine/__init__.py b/api/src/opentrons/protocol_engine/__init__.py index 68631378f7c9..1ecb489c38c4 100644 --- a/api/src/opentrons/protocol_engine/__init__.py +++ b/api/src/opentrons/protocol_engine/__init__.py @@ -8,6 +8,7 @@ from .create_protocol_engine import create_protocol_engine from .protocol_engine import ProtocolEngine from .errors import ProtocolEngineError +from .commands import Command, CommandRequest, CommandStatus, CommandType from .state import State, StateView, LabwareData from .types import ( DeckLocation, @@ -24,6 +25,11 @@ "ProtocolEngine", # error types "ProtocolEngineError", + # top level command unions and values + "Command", + "CommandRequest", + "CommandStatus", + "CommandType", # state interfaces and models "State", "StateView", diff --git a/api/src/opentrons/protocol_engine/commands/__init__.py b/api/src/opentrons/protocol_engine/commands/__init__.py index 30502dbb2cd5..bcfc873a3313 100644 --- a/api/src/opentrons/protocol_engine/commands/__init__.py +++ b/api/src/opentrons/protocol_engine/commands/__init__.py @@ -15,37 +15,71 @@ from .command import AbstractCommandImpl, BaseCommand, BaseCommandRequest, CommandStatus from .command_mapper import CommandMapper -from .command_unions import Command, CommandRequest, CommandResult - +from .command_unions import Command, CommandRequest, CommandResult, CommandType from .add_labware_definition import ( AddLabwareDefinition, - AddLabwareDefinitionRequest, AddLabwareDefinitionData, + AddLabwareDefinitionRequest, AddLabwareDefinitionResult, + AddLabwareDefinitionCommandType, +) + +from .aspirate import ( + Aspirate, + AspirateData, + AspirateRequest, + AspirateResult, + AspirateCommandType, ) -from .aspirate import Aspirate, AspirateRequest, AspirateData, AspirateResult -from .dispense import Dispense, DispenseRequest, DispenseData, DispenseResult -from .drop_tip import DropTip, DropTipRequest, DropTipData, DropTipResult + +from .dispense import ( + Dispense, + DispenseData, + DispenseRequest, + DispenseResult, + DispenseCommandType, +) + +from .drop_tip import ( + DropTip, + DropTipData, + DropTipRequest, + DropTipResult, + DropTipCommandType, +) + from .load_labware import ( LoadLabware, - LoadLabwareRequest, LoadLabwareData, + LoadLabwareRequest, LoadLabwareResult, + LoadLabwareCommandType, ) + from .load_pipette import ( LoadPipette, - LoadPipetteRequest, LoadPipetteData, + LoadPipetteRequest, LoadPipetteResult, + LoadPipetteCommandType, ) + from .move_to_well import ( MoveToWell, - MoveToWellRequest, MoveToWellData, + MoveToWellRequest, MoveToWellResult, + MoveToWellCommandType, +) + +from .pick_up_tip import ( + PickUpTip, + PickUpTipData, + PickUpTipRequest, + PickUpTipResult, + PickUpTipCommandType, ) -from .pick_up_tip import PickUpTip, PickUpTipRequest, PickUpTipData, PickUpTipResult __all__ = [ @@ -55,6 +89,7 @@ "Command", "CommandRequest", "CommandResult", + "CommandType", # base interfaces "AbstractCommandImpl", "BaseCommand", @@ -65,39 +100,47 @@ "LoadLabwareRequest", "LoadLabwareData", "LoadLabwareResult", + "LoadLabwareCommandType", # add labware definition command models "AddLabwareDefinition", "AddLabwareDefinitionRequest", "AddLabwareDefinitionData", "AddLabwareDefinitionResult", + "AddLabwareDefinitionCommandType", # load pipette command models "LoadPipette", "LoadPipetteRequest", "LoadPipetteData", "LoadPipetteResult", + "LoadPipetteCommandType", # move to well command models "MoveToWell", "MoveToWellRequest", "MoveToWellData", "MoveToWellResult", + "MoveToWellCommandType", # pick up tip command models "PickUpTip", "PickUpTipRequest", "PickUpTipData", "PickUpTipResult", + "PickUpTipCommandType", # drop tip command models "DropTip", "DropTipRequest", "DropTipData", "DropTipResult", + "DropTipCommandType", # aspirate command models "Aspirate", "AspirateRequest", "AspirateData", "AspirateResult", + "AspirateCommandType", # dispense command models "Dispense", "DispenseRequest", "DispenseData", "DispenseResult", + "DispenseCommandType", ] diff --git a/api/src/opentrons/protocol_engine/commands/command_unions.py b/api/src/opentrons/protocol_engine/commands/command_unions.py index 524b7d3b403e..5b62ebe22c15 100644 --- a/api/src/opentrons/protocol_engine/commands/command_unions.py +++ b/api/src/opentrons/protocol_engine/commands/command_unions.py @@ -5,14 +5,41 @@ AddLabwareDefinition, AddLabwareDefinitionRequest, AddLabwareDefinitionResult, + AddLabwareDefinitionCommandType, +) + +from .aspirate import Aspirate, AspirateRequest, AspirateResult, AspirateCommandType + +from .dispense import Dispense, DispenseRequest, DispenseResult, DispenseCommandType + +from .drop_tip import DropTip, DropTipRequest, DropTipResult, DropTipCommandType +from .load_labware import ( + LoadLabware, + LoadLabwareRequest, + LoadLabwareResult, + LoadLabwareCommandType, +) + +from .load_pipette import ( + LoadPipette, + LoadPipetteRequest, + LoadPipetteResult, + LoadPipetteCommandType, +) + +from .move_to_well import ( + MoveToWell, + MoveToWellRequest, + MoveToWellResult, + MoveToWellCommandType, +) + +from .pick_up_tip import ( + PickUpTip, + PickUpTipRequest, + PickUpTipResult, + PickUpTipCommandType, ) -from .aspirate import Aspirate, AspirateRequest, AspirateResult -from .dispense import Dispense, DispenseRequest, DispenseResult -from .drop_tip import DropTip, DropTipRequest, DropTipResult -from .load_labware import LoadLabware, LoadLabwareRequest, LoadLabwareResult -from .load_pipette import LoadPipette, LoadPipetteRequest, LoadPipetteResult -from .move_to_well import MoveToWell, MoveToWellRequest, MoveToWellResult -from .pick_up_tip import PickUpTip, PickUpTipRequest, PickUpTipResult Command = Union[ AddLabwareDefinition, @@ -25,6 +52,17 @@ PickUpTip, ] +CommandType = Union[ + AddLabwareDefinitionCommandType, + AspirateCommandType, + DispenseCommandType, + DropTipCommandType, + LoadLabwareCommandType, + LoadPipetteCommandType, + MoveToWellCommandType, + PickUpTipCommandType, +] + CommandRequest = Union[ AddLabwareDefinitionRequest, AspirateRequest, diff --git a/api/src/opentrons/protocol_engine/protocol_engine.py b/api/src/opentrons/protocol_engine/protocol_engine.py index 519f521efd9c..49bb5795c735 100644 --- a/api/src/opentrons/protocol_engine/protocol_engine.py +++ b/api/src/opentrons/protocol_engine/protocol_engine.py @@ -56,7 +56,7 @@ def add_command(self, request: CommandRequest) -> Command: async def execute_command_by_id(self, command_id: str) -> Command: """Execute a protocol engine command by its identifier.""" - queued_command = self.state_view.commands.get_command_by_id(command_id) + queued_command = self.state_view.commands.get(command_id) running_command = self._command_mapper.update_command( command=queued_command, diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index 618d9a3faed5..cb3a283640ca 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -2,7 +2,7 @@ from __future__ import annotations from collections import OrderedDict from dataclasses import dataclass, replace -from typing import List, Optional, Tuple +from typing import List, Optional from ..commands import Command, CommandStatus from ..errors import CommandDoesNotExistError @@ -41,7 +41,7 @@ def __init__(self, state: CommandState) -> None: """Initialize the view of command state with its underlying data.""" self._state = state - def get_command_by_id(self, command_id: str) -> Command: + def get(self, command_id: str) -> Command: """Get a command by its unique identifier.""" # TODO(mc, 2021-06-17): raise on missing ID, to line up with other state views try: @@ -49,16 +49,16 @@ def get_command_by_id(self, command_id: str) -> Command: except KeyError: raise CommandDoesNotExistError(f"Command {command_id} does not exist") - def get_all_commands(self) -> List[Tuple[str, Command]]: + def get_all(self) -> List[Command]: """Get a list of all commands in state, paired with their respective IDs. Entries are returned in the order of first-added command to last-added command. Replacing a command (to change its status, for example) keeps its place in the ordering. """ - return [entry for entry in self._state.commands_by_id.items()] + return list(self._state.commands_by_id.values()) - def get_next_command(self) -> Optional[str]: + def get_next_queued(self) -> Optional[str]: """Return the next request in line to be executed. Normally, this corresponds to the earliest-added command that's currently diff --git a/api/tests/opentrons/file_runner/test_command_queue_worker.py b/api/tests/opentrons/file_runner/test_command_queue_worker.py index 8fb9cdc0a1b1..d6e65fa68985 100644 --- a/api/tests/opentrons/file_runner/test_command_queue_worker.py +++ b/api/tests/opentrons/file_runner/test_command_queue_worker.py @@ -30,7 +30,7 @@ def state_view_with_commands( ) -> MagicMock: """Create a state view fixture with pending commands.""" pending_commands = commands + [None] - state_view.commands.get_next_command.side_effect = pending_commands + state_view.commands.get_next_queued.side_effect = pending_commands return state_view @@ -54,7 +54,7 @@ async def test_play_no_pending( state_view: MagicMock, ) -> None: """It should not execute any commands.""" - state_view.commands.get_next_command.return_value = None + state_view.commands.get_next_queued.return_value = None subject.play() await subject.wait_to_be_idle() diff --git a/api/tests/opentrons/protocol_engine/state/test_command_view.py b/api/tests/opentrons/protocol_engine/state/test_command_view.py index 6799d1382772..e23c0c51e43d 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_view.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_view.py @@ -24,12 +24,12 @@ def get_command_view( return CommandView(state=state) -def test_get_command_by_id() -> None: +def test_get_by_id() -> None: """It should get a command by ID from state.""" command = create_completed_command(command_id="command-id") subject = get_command_view(commands_by_id=[("command-id", command)]) - assert subject.get_command_by_id("command-id") == command + assert subject.get("command-id") == command def test_get_command_bad_id() -> None: @@ -38,10 +38,10 @@ def test_get_command_bad_id() -> None: subject = get_command_view(commands_by_id=[("command-id", command)]) with pytest.raises(errors.CommandDoesNotExistError): - subject.get_command_by_id("asdfghjkl") + subject.get("asdfghjkl") -def test_get_all_commands() -> None: +def test_get_all() -> None: """It should get all the commands from the state.""" command_1 = create_completed_command(command_id="command-id-1") command_2 = create_running_command(command_id="command-id-2") @@ -55,14 +55,10 @@ def test_get_all_commands() -> None: ] ) - assert subject.get_all_commands() == [ - ("command-id-1", command_1), - ("command-id-2", command_2), - ("command-id-3", command_3), - ] + assert subject.get_all() == [command_1, command_2, command_3] -def test_get_next_command_returns_first_pending() -> None: +def test_get_next_queued_returns_first_pending() -> None: """It should return the first command that's pending.""" pending_command = create_pending_command() running_command = create_running_command() @@ -77,10 +73,10 @@ def test_get_next_command_returns_first_pending() -> None: ] ) - assert subject.get_next_command() == "command-id-3" + assert subject.get_next_queued() == "command-id-3" -def test_get_next_command_returns_none_when_no_pending() -> None: +def test_get_next_queued_returns_none_when_no_pending() -> None: """It should return None if there are no pending commands to return.""" running_command = create_running_command(command_id="command-id-1") completed_command = create_completed_command(command_id="command-id-2") @@ -88,7 +84,7 @@ def test_get_next_command_returns_none_when_no_pending() -> None: subject = get_command_view() - assert subject.get_next_command() is None + assert subject.get_next_queued() is None subject = get_command_view( commands_by_id=[ @@ -98,10 +94,10 @@ def test_get_next_command_returns_none_when_no_pending() -> None: ] ) - assert subject.get_next_command() is None + assert subject.get_next_queued() is None -def test_get_next_command_returns_none_when_earlier_command_failed() -> None: +def test_get_next_queued_returns_none_when_earlier_command_failed() -> None: """It should return None if any prior-added command is failed.""" running_command = create_running_command(command_id="command-id-1") completed_command = create_completed_command(command_id="command-id-2") @@ -117,4 +113,4 @@ def test_get_next_command_returns_none_when_earlier_command_failed() -> None: ] ) - assert subject.get_next_command() is None + assert subject.get_next_queued() is None diff --git a/api/tests/opentrons/protocol_engine/test_protocol_engine.py b/api/tests/opentrons/protocol_engine/test_protocol_engine.py index 97358366d352..3eba31de6c6a 100644 --- a/api/tests/opentrons/protocol_engine/test_protocol_engine.py +++ b/api/tests/opentrons/protocol_engine/test_protocol_engine.py @@ -134,9 +134,9 @@ async def test_execute_command_by_id( data=data, ) - decoy.when( - state_store.state_view.commands.get_command_by_id("command-id") - ).then_return(queued_command) + decoy.when(state_store.state_view.commands.get("command-id")).then_return( + queued_command + ) decoy.when(resources.model_utils.get_timestamp()).then_return(started_at) diff --git a/robot-server/robot_server/service/json_api/response.py b/robot-server/robot_server/service/json_api/response.py index 1cf00cf34518..14dcd5176799 100644 --- a/robot-server/robot_server/service/json_api/response.py +++ b/robot-server/robot_server/service/json_api/response.py @@ -22,7 +22,7 @@ class ResourceModel(ResponseDataModel): id: str = Field(..., description="Unique identifier of the resource.") -ResponseDataT = TypeVar("ResponseDataT", bound=ResponseDataModel) +ResponseDataT = TypeVar("ResponseDataT", bound=BaseModel) DESCRIPTION_DATA = "the document’s primary data" diff --git a/robot-server/robot_server/sessions/router.py b/robot-server/robot_server/sessions/router.py index ffadad216cc5..2190018f1628 100644 --- a/robot-server/robot_server/sessions/router.py +++ b/robot-server/robot_server/sessions/router.py @@ -1,9 +1,11 @@ """Router for /sessions endpoints.""" from fastapi import APIRouter, Depends, status from datetime import datetime -from typing import Optional +from typing import Optional, Union from typing_extensions import Literal +from opentrons.protocol_engine import commands as pe_commands, errors as pe_errors + from robot_server.errors import ErrorDetails, ErrorResponse from robot_server.service.dependencies import get_current_time, get_unique_id @@ -23,8 +25,9 @@ from .session_store import SessionStore, SessionNotFoundError from .session_view import SessionView -from .session_models import Session, SessionCreateData, ProtocolSessionCreateData +from .session_models import Session, SessionCommandSummary, ProtocolSessionCreateData from .action_models import SessionAction, SessionActionCreateData +from .schema_models import CreateSessionRequest, SessionResponse, SessionCommandResponse from .engine_store import EngineStore, EngineConflictError, EngineMissingError from .dependencies import get_session_store, get_engine_store @@ -38,6 +41,13 @@ class SessionNotFound(ErrorDetails): title: str = "Session Not Found" +class CommandNotFound(ErrorDetails): + """An error if a given session command is not found.""" + + id: Literal["CommandNotFound"] = "CommandNotFound" + title: str = "Session Command Not Found" + + # TODO(mc, 2021-05-28): evaluate multi-session logic class SessionAlreadyActive(ErrorDetails): """An error if one tries to create a new session while one is already active.""" @@ -58,14 +68,16 @@ class SessionActionNotAllowed(ErrorDetails): summary="Create a session", description="Create a new session to track robot interaction.", status_code=status.HTTP_201_CREATED, - response_model=ResponseModel[Session], + # TODO(mc, 2021-06-23): mypy >= 0.780 broke Unions as `response_model` + # see https://github.com/tiangolo/fastapi/issues/2279 + response_model=SessionResponse, # type: ignore[arg-type] responses={ status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[ProtocolNotFound]}, status.HTTP_409_CONFLICT: {"model": ErrorResponse[SessionAlreadyActive]}, }, ) async def create_session( - request_body: Optional[RequestModel[SessionCreateData]] = None, + request_body: Optional[CreateSessionRequest] = None, session_view: SessionView = Depends(SessionView), session_store: SessionStore = Depends(get_session_store), engine_store: EngineStore = Depends(get_engine_store), @@ -106,7 +118,8 @@ async def create_session( raise SessionAlreadyActive(detail=str(e)).as_error(status.HTTP_409_CONFLICT) session_store.upsert(session=session) - data = session_view.as_response(session=session) + commands = engine_store.engine.state_view.commands.get_all() + data = session_view.as_response(session=session, commands=commands) return ResponseModel(data=data) @@ -121,16 +134,21 @@ async def create_session( async def get_sessions( session_view: SessionView = Depends(SessionView), session_store: SessionStore = Depends(get_session_store), + engine_store: EngineStore = Depends(get_engine_store), ) -> MultiResponseModel[Session]: """Get all sessions. Args: session_view: Session model construction interface. - session_store: Session storage interface + session_store: Session storage interface. + engine_store: ProtocolEngine storage and control. """ - data = [ - session_view.as_response(session=session) for session in session_store.get_all() - ] + data = [] + + for session in session_store.get_all(): + # TODO(mc, 2021-06-23): add multi-engine support + commands = engine_store.engine.state_view.commands.get_all() + data.append(session_view.as_response(session=session, commands=commands)) return MultiResponseModel(data=data) @@ -140,27 +158,32 @@ async def get_sessions( summary="Get a session", description="Get a specific session by its unique identifier.", status_code=status.HTTP_200_OK, - response_model=ResponseModel[Session], + # TODO(mc, 2021-06-23): mypy >= 0.780 broke Unions as `response_model` + # see https://github.com/tiangolo/fastapi/issues/2279 + response_model=SessionResponse, # type: ignore[arg-type] responses={status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[SessionNotFound]}}, ) async def get_session( sessionId: str, session_view: SessionView = Depends(SessionView), session_store: SessionStore = Depends(get_session_store), + engine_store: EngineStore = Depends(get_engine_store), ) -> ResponseModel[Session]: """Get a session by its ID. Args: - sessionId: Session ID pulled from URL + sessionId: Session ID pulled from URL. session_view: Session model construction interface. - session_store: Session storage interface + session_store: Session storage interface. + engine_store: ProtocolEngine storage and control. """ try: session = session_store.get(session_id=sessionId) except SessionNotFoundError as e: raise SessionNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) - data = session_view.as_response(session=session) + commands = engine_store.engine.state_view.commands.get_all() + data = session_view.as_response(session=session, commands=commands) return ResponseModel(data=data) @@ -252,3 +275,72 @@ async def create_session_action( session_store.upsert(session=next_session) return ResponseModel(data=action) + + +@sessions_router.get( + path="/sessions/{sessionId}/commands", + summary="Get a list of all protocol commands in the session", + description=( + "Get a list of all commands in the session and their statuses. " + "This endpoint returns command summaries. Use " + "`GET /sessions/{sessionId}/commands/{commandId}` to all information " + "available for a given command." + ), + status_code=status.HTTP_200_OK, + response_model=MultiResponseModel[SessionCommandSummary], + responses={ + status.HTTP_404_NOT_FOUND: {"model": ErrorResponse[SessionNotFound]}, + }, +) +async def get_session_commands( + session: ResponseModel[Session] = Depends(get_session), +) -> MultiResponseModel[SessionCommandSummary]: + """Get a summary of all commands in a session. + + Arguments: + session: Session response model, provided by the route handler for + `GET /session/{sessionId}` + """ + return MultiResponseModel(data=session.data.commands) + + +@sessions_router.get( + path="/sessions/{sessionId}/commands/{commandId}", + summary="Get full details about a specific command in the session", + description=( + "Get a command along with any associated payload, result, and " + "execution information." + ), + status_code=status.HTTP_200_OK, + # TODO(mc, 2021-06-23): mypy >= 0.780 broke Unions as `response_model` + # see https://github.com/tiangolo/fastapi/issues/2279 + response_model=SessionCommandResponse, # type: ignore[arg-type] + responses={ + status.HTTP_404_NOT_FOUND: { + "model": Union[ + ErrorResponse[SessionNotFound], + ErrorResponse[CommandNotFound], + ] + }, + }, +) +async def get_session_command( + commandId: str, + engine_store: EngineStore = Depends(get_engine_store), + session: ResponseModel[Session] = Depends(get_session), +) -> ResponseModel[pe_commands.Command]: + """Get a specific command from a session. + + Arguments: + commandId: Command identifier, pulled from route parameter. + engine_store: Protocol engine and runner storage. + session: Session response model, provided by the route handler for + `GET /session/{sessionId}`. Present to ensure 404 if session + not found. + """ + try: + command = engine_store.engine.state_view.commands.get(commandId) + except pe_errors.CommandDoesNotExistError as e: + raise CommandNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) + + return ResponseModel(data=command) diff --git a/robot-server/robot_server/sessions/schema_models.py b/robot-server/robot_server/sessions/schema_models.py new file mode 100644 index 000000000000..2dd7371b21fe --- /dev/null +++ b/robot-server/robot_server/sessions/schema_models.py @@ -0,0 +1,43 @@ +"""HTTP request and response models. + +These models exist to ensure the /sessions API is mapped to a reasonable +OpenAPI Spec. Given the large number of classes and unions involved, +the request and response models need to be ordered carefully. + +Mostly, this involves making sure we have responses that are `Union`s +of `ResponseModel`s rather than `ResponseModel`s of `Union`s. +""" +from typing import Union + +from opentrons.protocol_engine import commands as pe_commands +from robot_server.service.json_api import RequestModel, ResponseModel + + +from .session_models import ( + BasicSessionCreateData, + ProtocolSessionCreateData, + BasicSession, + ProtocolSession, +) + + +CreateSessionRequest = Union[ + RequestModel[BasicSessionCreateData], + RequestModel[ProtocolSessionCreateData], +] + +SessionResponse = Union[ + ResponseModel[BasicSession], + ResponseModel[ProtocolSession], +] + +SessionCommandResponse = Union[ + ResponseModel[pe_commands.AddLabwareDefinition], + ResponseModel[pe_commands.Aspirate], + ResponseModel[pe_commands.Dispense], + ResponseModel[pe_commands.DropTip], + ResponseModel[pe_commands.LoadLabware], + ResponseModel[pe_commands.LoadPipette], + ResponseModel[pe_commands.MoveToWell], + ResponseModel[pe_commands.PickUpTip], +] diff --git a/robot-server/robot_server/sessions/session_models.py b/robot-server/robot_server/sessions/session_models.py index 56f54bbd0897..e0eaf5272c03 100644 --- a/robot-server/robot_server/sessions/session_models.py +++ b/robot-server/robot_server/sessions/session_models.py @@ -5,6 +5,7 @@ from typing import List, Optional, Union from typing_extensions import Literal +from opentrons.protocol_engine import CommandStatus, CommandType from robot_server.service.json_api import ResourceModel from .action_models import SessionAction @@ -29,6 +30,14 @@ class AbstractSessionCreateData(BaseModel): ) +class SessionCommandSummary(ResourceModel): + """A stripped down model of a full Command for usage in a Session response.""" + + id: str = Field(..., description="Unique command identifier.") + commandType: CommandType = Field(..., description="Specific type of command.") + status: CommandStatus = Field(..., description="Execution status of the command.") + + class AbstractSession(ResourceModel): """Base session resource model.""" @@ -42,7 +51,11 @@ class AbstractSession(ResourceModel): ) actions: List[SessionAction] = Field( ..., - description="Client-initiated commands for session control.", + description="Client-initiated session control actions.", + ) + commands: List[SessionCommandSummary] = Field( + ..., + description="Protocol commands queued, running, or executed for the session.", ) diff --git a/robot-server/robot_server/sessions/session_view.py b/robot-server/robot_server/sessions/session_view.py index 2930b4e9ee05..8d07ed833b9a 100644 --- a/robot-server/robot_server/sessions/session_view.py +++ b/robot-server/robot_server/sessions/session_view.py @@ -1,8 +1,9 @@ """Session response model factory.""" from dataclasses import replace from datetime import datetime -from typing import Optional, Tuple +from typing import List, Optional, Tuple +from opentrons.protocol_engine import Command as ProtocolEngineCommand from .session_store import SessionResource from .action_models import SessionAction, SessionActionCreateData from .session_models import ( @@ -12,6 +13,7 @@ BasicSessionCreateData, ProtocolSession, ProtocolSessionCreateData, + SessionCommandSummary, ) @@ -80,7 +82,10 @@ def with_action( return actions, updated_session @staticmethod - def as_response(session: SessionResource) -> Session: + def as_response( + session: SessionResource, + commands: List[ProtocolEngineCommand], + ) -> Session: """Transform a session resource into its public response model. Arguments: @@ -90,12 +95,17 @@ def as_response(session: SessionResource) -> Session: Session response model representing the same resource. """ create_data = session.create_data + command_summaries = [ + SessionCommandSummary(id=c.id, commandType=c.commandType, status=c.status) + for c in commands + ] if isinstance(create_data, BasicSessionCreateData): return BasicSession.construct( id=session.session_id, createdAt=session.created_at, actions=session.actions, + commands=command_summaries, ) if isinstance(create_data, ProtocolSessionCreateData): return ProtocolSession.construct( @@ -103,6 +113,7 @@ def as_response(session: SessionResource) -> Session: createdAt=session.created_at, createParams=create_data.createParams, actions=session.actions, + commands=command_summaries, ) raise ValueError(f"Invalid session resource {session}") diff --git a/robot-server/tests/sessions/test_session_view.py b/robot-server/tests/sessions/test_session_view.py index bb2fd7247f43..990c35299561 100644 --- a/robot-server/tests/sessions/test_session_view.py +++ b/robot-server/tests/sessions/test_session_view.py @@ -2,6 +2,13 @@ import pytest from datetime import datetime +from opentrons.types import MountType +from opentrons.protocol_engine import ( + CommandStatus, + PipetteName, + commands as pe_commands, +) + from robot_server.sessions.session_store import SessionResource from robot_server.sessions.session_view import SessionView from robot_server.sessions.session_models import ( @@ -11,6 +18,7 @@ ProtocolSession, ProtocolSessionCreateData, ProtocolSessionCreateParams, + SessionCommandSummary, ) from robot_server.sessions.action_models import ( @@ -99,6 +107,7 @@ def test_create_protocol_session_resource() -> None: id="session-id", createdAt=current_time, actions=[], + commands=[], ), ), ( @@ -115,6 +124,7 @@ def test_create_protocol_session_resource() -> None: createdAt=current_time, createParams=ProtocolSessionCreateParams(protocolId="protocol-id"), actions=[], + commands=[], ), ), ), @@ -125,7 +135,58 @@ def test_to_response( ) -> None: """It should create a BasicSession if session_data is None.""" subject = SessionView() - assert subject.as_response(session_resource) == expected_response + result = subject.as_response(session=session_resource, commands=[]) + assert result == expected_response + + +def test_to_response_maps_commands() -> None: + """It should map ProtocolEngine commands to SessionCommandSummary models.""" + session_resource = SessionResource( + session_id="session-id", + create_data=BasicSessionCreateData(), + created_at=datetime(year=2021, month=1, day=1), + actions=[], + ) + + command_1 = pe_commands.LoadPipette( + id="command-1", + status=CommandStatus.RUNNING, + createdAt=datetime(year=2022, month=2, day=2), + data=pe_commands.LoadPipetteData( + mount=MountType.LEFT, + pipetteName=PipetteName.P300_SINGLE, + ), + ) + + command_2 = pe_commands.MoveToWell( + id="command-2", + status=CommandStatus.QUEUED, + createdAt=datetime(year=2023, month=3, day=3), + data=pe_commands.MoveToWellData(pipetteId="a", labwareId="b", wellName="c"), + ) + + subject = SessionView() + result = subject.as_response( + session=session_resource, commands=[command_1, command_2] + ) + + assert result == BasicSession( + id="session-id", + createdAt=datetime(year=2021, month=1, day=1), + actions=[], + commands=[ + SessionCommandSummary( + id="command-1", + commandType="loadPipette", + status=CommandStatus.RUNNING, + ), + SessionCommandSummary( + id="command-2", + commandType="moveToWell", + status=CommandStatus.QUEUED, + ), + ], + ) def test_create_action(current_time: datetime) -> None: diff --git a/robot-server/tests/sessions/test_sessions_router.py b/robot-server/tests/sessions/test_sessions_router.py index 05754d0872b7..a35a46d47410 100644 --- a/robot-server/tests/sessions/test_sessions_router.py +++ b/robot-server/tests/sessions/test_sessions_router.py @@ -8,6 +8,12 @@ from httpx import AsyncClient from typing import AsyncIterator +from opentrons.protocol_engine import ( + CommandStatus, + commands as pe_commands, + errors as pe_errors, +) + from robot_server.errors import exception_handlers from robot_server.protocols import ( ProtocolStore, @@ -25,6 +31,7 @@ ProtocolSession, ProtocolSessionCreateData, ProtocolSessionCreateParams, + SessionCommandSummary, ) from robot_server.sessions.engine_store import ( @@ -50,6 +57,7 @@ SessionNotFound, SessionAlreadyActive, SessionActionNotAllowed, + CommandNotFound, get_session_store, get_engine_store, get_protocol_store, @@ -118,8 +126,11 @@ async def test_create_session( id=unique_id, createdAt=current_time, actions=[], + commands=[], ) + decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) + decoy.when( session_view.as_resource( create_data=BasicSessionCreateData(), @@ -129,7 +140,7 @@ async def test_create_session( ).then_return(session) decoy.when( - session_view.as_response(session=session), + session_view.as_response(session=session, commands=[]), ).then_return(expected_response) response = await async_client.post( @@ -176,6 +187,7 @@ async def test_create_protocol_session( createdAt=current_time, createParams=ProtocolSessionCreateParams(protocolId="protocol-id"), actions=[], + commands=[], ) decoy.when(protocol_store.get(protocol_id="protocol-id")).then_return(protocol) @@ -190,8 +202,10 @@ async def test_create_protocol_session( ) ).then_return(session) + decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) + decoy.when( - session_view.as_response(session=session), + session_view.as_response(session=session, commands=[]), ).then_return(expected_response) response = await async_client.post( @@ -287,6 +301,7 @@ def test_get_session( decoy: Decoy, session_view: SessionView, session_store: SessionStore, + engine_store: EngineStore, client: TestClient, ) -> None: """It should be able to get a session by ID.""" @@ -302,11 +317,15 @@ def test_get_session( id="session-id", createdAt=created_at, actions=[], + commands=[], ) decoy.when(session_store.get(session_id="session-id")).then_return(session) + + decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) + decoy.when( - session_view.as_response(session=session), + session_view.as_response(session=session, commands=[]), ).then_return(expected_response) response = client.get("/sessions/session-id") @@ -338,7 +357,7 @@ def test_get_sessions_empty( session_store: SessionStore, client: TestClient, ) -> None: - """It should return an empty collection response with no sessions exist.""" + """It should return an empty collection response when no sessions exist.""" decoy.when(session_store.get_all()).then_return([]) response = client.get("/sessions") @@ -350,11 +369,12 @@ def test_get_sessions_not_empty( decoy: Decoy, session_view: SessionView, session_store: SessionStore, + engine_store: EngineStore, client: TestClient, ) -> None: - """It should return an empty collection response with no sessions exist.""" + """It should a collection response when a session exists.""" + # TODO(mc, 2021-06-23): add actual multi-session support created_at_1 = datetime.now() - created_at_2 = datetime.now() session_1 = SessionResource( session_id="unique-id-1", @@ -362,39 +382,25 @@ def test_get_sessions_not_empty( created_at=created_at_1, actions=[], ) - session_2 = SessionResource( - session_id="unique-id-2", - create_data=BasicSessionCreateData(), - created_at=created_at_2, - actions=[], - ) response_1 = BasicSession( id="unique-id-1", createdAt=created_at_1, actions=[], - ) - response_2 = BasicSession( - id="unique-id-2", - createdAt=created_at_2, - actions=[], + commands=[], ) - decoy.when(session_store.get_all()).then_return([session_1, session_2]) + decoy.when(session_store.get_all()).then_return([session_1]) - decoy.when( - session_view.as_response(session=session_1), - ).then_return(response_1) + decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) decoy.when( - session_view.as_response(session=session_2), - ).then_return(response_2) + session_view.as_response(session=session_1, commands=[]), + ).then_return(response_1) response = client.get("/sessions") - verify_response( - response, expected_status=200, expected_data=[response_1, response_2] - ) + verify_response(response, expected_status=200, expected_data=[response_1]) def test_delete_session_by_id( @@ -565,3 +571,184 @@ def test_create_session_action_without_runner( expected_status=400, expected_errors=SessionActionNotAllowed(detail="oh no"), ) + + +def test_get_session_commands( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + engine_store: EngineStore, + client: TestClient, +) -> None: + """It should return a list of all commands in a session.""" + session = SessionResource( + session_id="session-id", + create_data=BasicSessionCreateData(), + created_at=datetime(year=2021, month=1, day=1), + actions=[], + ) + + command = pe_commands.MoveToWell( + id="command-id", + status=CommandStatus.RUNNING, + createdAt=datetime(year=2022, month=2, day=2), + data=pe_commands.MoveToWellData(pipetteId="a", labwareId="b", wellName="c"), + ) + + command_summary = SessionCommandSummary( + id="command-id", + commandType="moveToWell", + status=CommandStatus.RUNNING, + ) + + session_response = BasicSession( + id="session-id", + createdAt=datetime(year=2021, month=1, day=1), + actions=[], + commands=[command_summary], + ) + + decoy.when(session_store.get(session_id="session-id")).then_return(session) + + decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([command]) + + decoy.when( + session_view.as_response(session=session, commands=[command]), + ).then_return(session_response) + + response = client.get("/sessions/session-id/commands") + + verify_response(response, expected_status=200, expected_data=[command_summary]) + + +def test_get_session_commands_missing_session( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + engine_store: EngineStore, + client: TestClient, +) -> None: + """It should 404 if you attempt to get the commands for a non-existent session.""" + key_error = SessionNotFoundError(session_id="session-id") + + decoy.when(session_store.get(session_id="session-id")).then_raise(key_error) + + response = client.get("/sessions/session-id/commands") + + verify_response( + response, + expected_status=404, + expected_errors=SessionNotFound(detail=str(key_error)), + ) + + +def test_get_session_command_by_id( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + engine_store: EngineStore, + client: TestClient, +) -> None: + """It should return full details about a command by ID.""" + session = SessionResource( + session_id="session-id", + create_data=BasicSessionCreateData(), + created_at=datetime(year=2021, month=1, day=1), + actions=[], + ) + + session_response = BasicSession( + id="session-id", + createdAt=datetime(year=2021, month=1, day=1), + actions=[], + commands=[], + ) + + command = pe_commands.MoveToWell( + id="command-id", + status=CommandStatus.RUNNING, + createdAt=datetime(year=2022, month=2, day=2), + data=pe_commands.MoveToWellData(pipetteId="a", labwareId="b", wellName="c"), + ) + + decoy.when(session_store.get(session_id="session-id")).then_return(session) + + decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([command]) + + decoy.when( + session_view.as_response(session=session, commands=[command]), + ).then_return(session_response) + + decoy.when(engine_store.engine.state_view.commands.get("command-id")).then_return( + command + ) + + response = client.get("/sessions/session-id/commands/command-id") + + verify_response(response, expected_status=200, expected_data=command) + + +def test_get_session_command_missing_session( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + engine_store: EngineStore, + client: TestClient, +) -> None: + """It should 404 if you attempt to get a command for a non-existent session.""" + key_error = SessionNotFoundError(session_id="session-id") + + decoy.when(session_store.get(session_id="session-id")).then_raise(key_error) + + response = client.get("/sessions/session-id/commands/command-id") + + verify_response( + response, + expected_status=404, + expected_errors=SessionNotFound(detail=str(key_error)), + ) + + +def test_get_session_command_missing_command( + decoy: Decoy, + session_view: SessionView, + session_store: SessionStore, + engine_store: EngineStore, + client: TestClient, +) -> None: + """It should 404 if you attempt to get a non-existent command.""" + session = SessionResource( + session_id="session-id", + create_data=BasicSessionCreateData(), + created_at=datetime(year=2021, month=1, day=1), + actions=[], + ) + + session_response = BasicSession( + id="session-id", + createdAt=datetime(year=2021, month=1, day=1), + actions=[], + commands=[], + ) + + key_error = pe_errors.CommandDoesNotExistError("oh no") + + decoy.when(session_store.get(session_id="session-id")).then_return(session) + + decoy.when(engine_store.engine.state_view.commands.get_all()).then_return([]) + + decoy.when( + session_view.as_response(session=session, commands=[]), + ).then_return(session_response) + + decoy.when(engine_store.engine.state_view.commands.get("command-id")).then_raise( + key_error + ) + + response = client.get("/sessions/session-id/commands/command-id") + + verify_response( + response, + expected_status=404, + expected_errors=CommandNotFound(detail=str(key_error)), + )