From 7822aaf4cfa4b82b25f985f8a9f9b4b25761e617 Mon Sep 17 00:00:00 2001 From: tamarzanzouri Date: Mon, 5 Aug 2024 14:13:48 -0400 Subject: [PATCH] pr suggestions. PE non optional cursor --- .../protocol_engine/state/commands.py | 19 +++---------------- .../protocol_runner/run_orchestrator.py | 18 ++++++++++++++++++ .../state/test_command_view_old.py | 10 +++++----- .../robot_server/runs/router/base_router.py | 10 ++++++---- .../robot_server/runs/run_data_manager.py | 1 + .../tests/runs/router/test_base_router.py | 2 +- .../tests/runs/router/test_commands_router.py | 4 ++-- 7 files changed, 36 insertions(+), 28 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index d4d1eaf059e..c725c561ac3 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -630,26 +630,13 @@ def get_slice( def get_errors_slice( self, - cursor: Optional[int], + cursor: int, length: int, ) -> CommandErrorSlice: - """Get a subset of commands error around a given cursor. - - If the cursor is omitted, a cursor will be selected automatically - based most recent error. - """ + """Get a subset of commands error around a given cursor.""" + # start is inclusive, stop is exclusive all_errors = self.get_all_errors() total_length = len(all_errors) - - if cursor is None: - if len(all_errors) > 0: - # Get the most recent error, - # which we can find just at the end of the list. - cursor = total_length - 1 - else: - cursor = total_length - length - - # start is inclusive, stop is exclusive actual_cursor = max(0, min(cursor, total_length - 1)) stop = min(total_length, actual_cursor + length) diff --git a/api/src/opentrons/protocol_runner/run_orchestrator.py b/api/src/opentrons/protocol_runner/run_orchestrator.py index 65da1d612b0..4dca9f49984 100644 --- a/api/src/opentrons/protocol_runner/run_orchestrator.py +++ b/api/src/opentrons/protocol_runner/run_orchestrator.py @@ -23,6 +23,7 @@ CommandSlice, CommandErrorSlice, DeckType, + ErrorOccurrence, ) from ..protocol_engine.errors import RunStoppedError from ..protocol_engine.types import ( @@ -279,8 +280,21 @@ def get_command_error_slice( Args: cursor: Requested index of first error in the returned slice. + If the cursor is omitted, a cursor will be selected automatically + based on the last error occurence. length: Length of slice to return. """ + all_errors = self.get_all_command_errors() + total_length = len(all_errors) + + if cursor is None: + if len(all_errors) > 0: + # Get the most recent error, + # which we can find just at the end of the list. + cursor = total_length - 1 + else: + cursor = 0 + return self._protocol_engine.state_view.commands.get_errors_slice( cursor=cursor, length=length ) @@ -297,6 +311,10 @@ def get_all_commands(self) -> List[Command]: """Get all run commands.""" return self._protocol_engine.state_view.commands.get_all() + def get_all_command_errors(self) -> List[ErrorOccurrence]: + """Get all run command errors.""" + return self._protocol_engine.state_view.commands.get_all_errors() + def get_run_status(self) -> EngineStatus: """Get the current execution status of the engine.""" return self._protocol_engine.state_view.commands.get_status() diff --git a/api/tests/opentrons/protocol_engine/state/test_command_view_old.py b/api/tests/opentrons/protocol_engine/state/test_command_view_old.py index d751e562c00..61e1b95eb8d 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_view_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_view_old.py @@ -904,7 +904,7 @@ def test_get_current() -> None: def test_get_slice_empty() -> None: """It should return a slice from the tail if no current command.""" subject = get_command_view(commands=[]) - result = subject.get_slice(cursor=None, length=2) + result = subject.get_slice(cursor=0, length=2) assert result == CommandSlice(commands=[], cursor=0, total_length=0) @@ -944,7 +944,7 @@ def test_get_slice_default_cursor_no_current() -> None: subject = get_command_view(commands=[command_1, command_2, command_3, command_4]) - result = subject.get_slice(cursor=None, length=3) + result = subject.get_slice(cursor=0, length=3) assert result == CommandSlice( commands=[command_2, command_3, command_4], @@ -975,7 +975,7 @@ def test_get_slice_default_cursor_failed_command() -> None: failed_command=CommandEntry(index=2, command=command_3), ) - result = subject.get_slice(cursor=None, length=3) + result = subject.get_slice(cursor=0, length=3) assert result == CommandSlice( commands=[command_3, command_4], @@ -1032,7 +1032,7 @@ def test_get_slice_default_cursor_queued() -> None: def test_get_errors_slice_empty() -> None: """It should return a slice from the tail if no current command.""" subject = get_command_view(failed_command_errors=[]) - result = subject.get_errors_slice(cursor=None, length=2) + result = subject.get_errors_slice(cursor=0, length=2) assert result == CommandErrorSlice(commands_errors=[], cursor=0, total_length=0) @@ -1077,7 +1077,7 @@ def test_get_slice_default_cursor_last_error() -> None: failed_command_errors=[error_1, error_2, error_3, error_4, error_5] ) - result = subject.get_errors_slice(cursor=None, length=2) + result = subject.get_errors_slice(cursor=0, length=2) assert result == CommandErrorSlice( commands_errors=[error_5], diff --git a/robot-server/robot_server/runs/router/base_router.py b/robot-server/robot_server/runs/router/base_router.py index 265ee2e89cc..73e8ae2f9ab 100644 --- a/robot-server/robot_server/runs/router/base_router.py +++ b/robot-server/robot_server/runs/router/base_router.py @@ -26,6 +26,7 @@ RequestModel, SimpleBody, SimpleEmptyBody, + SimpleMultiBody, MultiBody, MultiBodyMeta, ResourceLink, @@ -426,7 +427,7 @@ async def put_error_recovery_policy( "information available for a given command." ), responses={ - status.HTTP_200_OK: {"model": MultiBody[list[pe_errors.ErrorOccurrence]]}, + status.HTTP_200_OK: {"model": SimpleMultiBody[list[pe_errors.ErrorOccurrence]]}, status.HTTP_409_CONFLICT: {"model": ErrorBody[RunStopped]}, }, ) @@ -445,7 +446,7 @@ async def get_run_commands_error( description="The maximum number of command errors in the list to return.", ), run_data_manager: RunDataManager = Depends(get_run_data_manager), -) -> PydanticResponse[MultiBody[list[pe_errors.ErrorOccurrence]]]: +) -> PydanticResponse[SimpleMultiBody[list[pe_errors.ErrorOccurrence]]]: """Get a summary of a set of command errors in a run. Arguments: @@ -469,8 +470,9 @@ async def get_run_commands_error( ) return await PydanticResponse.create( - content=MultiBody.construct( - data=command_error_slice.commands_errors, meta=meta + content=SimpleMultiBody.construct( + data=command_error_slice.commands_errors, # type:ignore[arg-type] + meta=meta, ), status_code=status.HTTP_200_OK, ) diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index 03662382d1f..2f1784d5006 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -399,6 +399,7 @@ def get_command_error_slice( cursor=cursor, length=length ) + # TODO(tz, 8-5-2024): Change this to return to error list from the DB when we implement https://opentrons.atlassian.net/browse/EXEC-655. raise RunNotCurrentError() def get_current_command(self, run_id: str) -> Optional[CommandPointer]: diff --git a/robot-server/tests/runs/router/test_base_router.py b/robot-server/tests/runs/router/test_base_router.py index 40653fc326e..dc9bb7bf959 100644 --- a/robot-server/tests/runs/router/test_base_router.py +++ b/robot-server/tests/runs/router/test_base_router.py @@ -680,7 +680,7 @@ async def test_get_run_commands_errors_raises_no_run( pageLength=42, ) - assert result.content.data == [ + assert list(result.content.data) == [ pe_errors.ErrorOccurrence( id="error-id", errorType="PrettyBadError", diff --git a/robot-server/tests/runs/router/test_commands_router.py b/robot-server/tests/runs/router/test_commands_router.py index 0aee5afbec1..4b49ffe5d3b 100644 --- a/robot-server/tests/runs/router/test_commands_router.py +++ b/robot-server/tests/runs/router/test_commands_router.py @@ -13,7 +13,7 @@ ) from robot_server.errors.error_responses import ApiError -from robot_server.service.json_api import MultiBodyMeta, ResponseList +from robot_server.service.json_api import MultiBodyMeta from robot_server.runs.command_models import ( RequestModelWithCommandCreate, @@ -23,7 +23,7 @@ ) from robot_server.runs.run_store import CommandNotFoundError, RunStore from robot_server.runs.run_orchestrator_store import RunOrchestratorStore -from robot_server.runs.run_data_manager import RunDataManager, RunNotCurrentError +from robot_server.runs.run_data_manager import RunDataManager from robot_server.runs.run_models import RunCommandSummary, RunNotFoundError from robot_server.runs.router.commands_router import ( create_run_command,