From be7bd0b95f2faca61cec53f22f50cf33f5ce2fd1 Mon Sep 17 00:00:00 2001 From: tamarzanzouri Date: Tue, 20 Aug 2024 15:24:33 -0400 Subject: [PATCH] cherry pick fixes --- .../protocol_engine/state/command_history.py | 32 +++++++++++++++++-- .../protocol_engine/state/commands.py | 20 ++---------- .../protocol_runner/run_orchestrator.py | 4 +-- .../state/test_command_view_old.py | 12 +++---- robot-server/robot_server/commands/router.py | 4 ++- .../maintenance_run_orchestrator_store.py | 4 ++- .../runs/router/commands_router.py | 8 +++++ .../robot_server/runs/run_data_manager.py | 6 +++- .../runs/run_orchestrator_store.py | 9 +++--- robot-server/tests/commands/test_router.py | 6 +++- .../tests/runs/router/test_commands_router.py | 12 ++++--- .../tests/runs/test_run_data_manager.py | 12 ++++--- 12 files changed, 85 insertions(+), 44 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/command_history.py b/api/src/opentrons/protocol_engine/state/command_history.py index 4b1ff38f3e7..185492a71c8 100644 --- a/api/src/opentrons/protocol_engine/state/command_history.py +++ b/api/src/opentrons/protocol_engine/state/command_history.py @@ -97,6 +97,28 @@ def get_all_commands(self) -> List[Command]: for command_id in self._all_command_ids ] + def get_filtered_command_ids( + self, + command_intents: list[CommandIntent] = [ + CommandIntent.PROTOCOL, + CommandIntent.SETUP, + ], + ) -> List[str]: + filtered_command = self._commands_by_id + if CommandIntent.FIXIT not in command_intents: + filtered_command = { + key: val + for key, val in self._commands_by_id.items() + if val.command.intent != CommandIntent.FIXIT + } + if CommandIntent.SETUP not in command_intents: + filtered_command = { + key: val + for key, val in filtered_command.items() + if val.command.intent != CommandIntent.SETUP + } + return list(filtered_command.keys()) + def get_filtered_queue_ids(self, all_commands: bool) -> list[str]: print(list(self.get_fixit_queue_ids())) return [ @@ -109,9 +131,15 @@ def get_all_ids(self) -> List[str]: """Get all command IDs.""" return self._all_command_ids - def get_slice(self, start: int, stop: int) -> List[Command]: - """Get a list of commands between start and stop.""" + def get_slice( + self, start: int, stop: int, command_ids: Optional[list[str]] = None + ) -> List[Command]: + """Get a list of commands between start and stop.""" """Get a list of commands between start and stop.""" commands = self._all_command_ids[start:stop] + selected_command_ids = ( + command_ids if command_ids is not None else self._all_command_ids + ) + commands = selected_command_ids[start:stop] return [self._commands_by_id[command].command for command in commands] def get_tail_command(self) -> Optional[CommandEntry]: diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index f5a9400d7f9..5c97567379d 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -579,22 +579,8 @@ def get_all(self) -> List[Command]: """ return self._state.command_history.get_all_commands() - def get_filtered_queue_ids(self, all_commands: bool) -> List[str]: - """Get a list of all filtered commands in state. - - 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. - - If all_commands is True, retunred list will contain all command intents. - If False, return list will contain only safe commands. - """ - return self._state.command_history.get_filtered_queue_ids( - all_commands=all_commands - ) - def get_slice( - self, cursor: Optional[int], length: int, all_commands: bool + self, cursor: Optional[int], length: int, include_fixit_commands: bool ) -> CommandSlice: """Get a subset of commands around a given cursor. @@ -610,8 +596,8 @@ def get_slice( if include_fixit_commands else [CommandIntent.PROTOCOL, CommandIntent.SETUP] ) - running_command = self._state.command_history.get_running_command() - queued_command_ids = self._state.command_history.get_queue_ids() + running_command = self._state.command_history.get_running_command() + queued_command_ids = self._state.command_history.get_queue_ids() total_length = len(command_ids) # TODO(mm, 2024-05-17): This looks like it's attempting to do the same thing diff --git a/api/src/opentrons/protocol_runner/run_orchestrator.py b/api/src/opentrons/protocol_runner/run_orchestrator.py index 398e1e8eb67..71620cf9171 100644 --- a/api/src/opentrons/protocol_runner/run_orchestrator.py +++ b/api/src/opentrons/protocol_runner/run_orchestrator.py @@ -257,7 +257,7 @@ def get_current_command(self) -> Optional[CommandPointer]: return self._protocol_engine.state_view.commands.get_current() def get_command_slice( - self, cursor: Optional[int], length: int, all_commands: bool + self, cursor: Optional[int], length: int, include_fixit_commands: bool ) -> CommandSlice: """Get a slice of run commands. @@ -267,7 +267,7 @@ def get_command_slice( all_commands: Get all command intents. """ return self._protocol_engine.state_view.commands.get_slice( - cursor=cursor, length=length, all_commands=all_commands + cursor=cursor, length=length, include_fixit_commands=include_fixit_commands ) def get_command_error_slice( 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 82a37ac4cec..9db20307dab 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=0, length=2, all_commands=True) + result = subject.get_slice(cursor=0, length=2, include_fixit_commands=True) assert result == CommandSlice(commands=[], cursor=0, total_length=0) @@ -918,7 +918,7 @@ def test_get_slice() -> None: subject = get_command_view(commands=[command_1, command_2, command_3, command_4]) - result = subject.get_slice(cursor=1, length=3, all_commands=True) + result = subject.get_slice(cursor=1, length=3, include_fixit_commands=True) assert result == CommandSlice( commands=[command_2, command_3, command_4], @@ -926,7 +926,7 @@ def test_get_slice() -> None: total_length=4, ) - result = subject.get_slice(cursor=-3, length=10, all_commands=True) + result = subject.get_slice(cursor=-3, length=10, include_fixit_commands=True) assert result == CommandSlice( commands=[command_1, command_2, command_3, command_4], @@ -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, all_commands=True) + result = subject.get_slice(cursor=None, length=3, include_fixit_commands=True) 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, all_commands=True) + result = subject.get_slice(cursor=None, length=3, include_fixit_commands=True) assert result == CommandSlice( commands=[command_3, command_4], @@ -997,7 +997,7 @@ def test_get_slice_default_cursor_running() -> None: running_command_id="command-id-3", ) - result = subject.get_slice(cursor=None, length=2, all_commands=True) + result = subject.get_slice(cursor=None, length=2, include_fixit_commands=True) assert result == CommandSlice( commands=[command_3, command_4], diff --git a/robot-server/robot_server/commands/router.py b/robot-server/robot_server/commands/router.py index ef4c58c692c..e4d2d4a9f13 100644 --- a/robot-server/robot_server/commands/router.py +++ b/robot-server/robot_server/commands/router.py @@ -161,7 +161,9 @@ async def get_commands_list( cursor: Cursor index for the collection response. pageLength: Maximum number of items to return. """ - cmd_slice = orchestrator.get_command_slice(cursor=cursor, length=pageLength) + cmd_slice = orchestrator.get_command_slice( + cursor=cursor, length=pageLength, include_fixit_commands=True + ) commands = cast(List[StatelessCommand], cmd_slice.commands) meta = MultiBodyMeta(cursor=cmd_slice.cursor, totalLength=cmd_slice.total_length) diff --git a/robot-server/robot_server/maintenance_runs/maintenance_run_orchestrator_store.py b/robot-server/robot_server/maintenance_runs/maintenance_run_orchestrator_store.py index 169875d4b7d..4aa0c38b323 100644 --- a/robot-server/robot_server/maintenance_runs/maintenance_run_orchestrator_store.py +++ b/robot-server/robot_server/maintenance_runs/maintenance_run_orchestrator_store.py @@ -226,7 +226,9 @@ def get_command_slice( cursor: Requested index of first command in the returned slice. length: Length of slice to return. """ - return self.run_orchestrator.get_command_slice(cursor=cursor, length=length) + return self.run_orchestrator.get_command_slice( + cursor=cursor, length=length, include_fixit_commands=False + ) def get_current_command(self) -> Optional[CommandPointer]: """Get the "current" command, if any.""" diff --git a/robot-server/robot_server/runs/router/commands_router.py b/robot-server/robot_server/runs/router/commands_router.py index 56ff466bf84..1d8d233235b 100644 --- a/robot-server/robot_server/runs/router/commands_router.py +++ b/robot-server/robot_server/runs/router/commands_router.py @@ -285,6 +285,11 @@ async def get_run_commands( description="The maximum number of commands in the list to return.", ), ] = _DEFAULT_COMMAND_LIST_LENGTH, + includeFixitCommands: bool = Query( + True, + description="If `true`, return all commands (protocol, setup, fixit)." + " If `false`, only return safe commands (protocol, setup).", + ), ) -> PydanticResponse[MultiBody[RunCommandSummary, CommandCollectionLinks]]: """Get a summary of a set of commands in a run. @@ -293,12 +298,15 @@ async def get_run_commands( cursor: Cursor index for the collection response. pageLength: Maximum number of items to return. run_data_manager: Run data retrieval interface. + includeFixitCommands: If `true`, return all commands." + " If `false`, only return safe commands. """ try: command_slice = run_data_manager.get_commands_slice( run_id=runId, cursor=cursor, length=pageLength, + include_fixit_commands=includeFixitCommands, ) except RunNotFoundError as e: raise RunNotFound.from_exc(e).as_error(status.HTTP_404_NOT_FOUND) from e diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index 64cdb5fafb5..ad47a80ab8c 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -373,6 +373,7 @@ def get_commands_slice( run_id: str, cursor: Optional[int], length: int, + include_fixit_commands: bool, ) -> CommandSlice: """Get a slice of run commands. @@ -380,13 +381,16 @@ def get_commands_slice( run_id: ID of the run. cursor: Requested index of first command in the returned slice. length: Length of slice to return. + include_fixit_commands: Include fixit commands. Raises: RunNotFoundError: The given run identifier was not found in the database. """ if run_id == self._run_orchestrator_store.current_run_id: return self._run_orchestrator_store.get_command_slice( - cursor=cursor, length=length + cursor=cursor, + length=length, + include_fixit_commands=include_fixit_commands, ) # Let exception propagate diff --git a/robot-server/robot_server/runs/run_orchestrator_store.py b/robot-server/robot_server/runs/run_orchestrator_store.py index 72fb80d1ef2..8b52430a12e 100644 --- a/robot-server/robot_server/runs/run_orchestrator_store.py +++ b/robot-server/robot_server/runs/run_orchestrator_store.py @@ -330,17 +330,18 @@ def get_current_command(self) -> Optional[CommandPointer]: return self.run_orchestrator.get_current_command() def get_command_slice( - self, - cursor: Optional[int], - length: int, + self, cursor: Optional[int], length: int, include_fixit_commands: bool ) -> CommandSlice: """Get a slice of run commands. Args: cursor: Requested index of first command in the returned slice. length: Length of slice to return. + include_fixit_commands: Include fixit commands. """ - return self.run_orchestrator.get_command_slice(cursor=cursor, length=length) + return self.run_orchestrator.get_command_slice( + cursor=cursor, length=length, include_fixit_commands=include_fixit_commands + ) def get_command_error_slice( self, diff --git a/robot-server/tests/commands/test_router.py b/robot-server/tests/commands/test_router.py index 259af673fe9..4ccc1a1acc1 100644 --- a/robot-server/tests/commands/test_router.py +++ b/robot-server/tests/commands/test_router.py @@ -137,7 +137,11 @@ async def test_get_commands_list( index=0, ) ) - decoy.when(run_orchestrator.get_command_slice(cursor=1337, length=42)).then_return( + decoy.when( + run_orchestrator.get_command_slice( + cursor=1337, length=42, include_fixit_commands=True + ) + ).then_return( CommandSlice(commands=[command_1, command_2], cursor=0, total_length=2) ) diff --git a/robot-server/tests/runs/router/test_commands_router.py b/robot-server/tests/runs/router/test_commands_router.py index 38a09706826..a2febf0f132 100644 --- a/robot-server/tests/runs/router/test_commands_router.py +++ b/robot-server/tests/runs/router/test_commands_router.py @@ -345,9 +345,7 @@ async def test_get_run_commands( ) decoy.when( mock_run_data_manager.get_commands_slice( - run_id="run-id", - cursor=None, - length=42, + run_id="run-id", cursor=None, length=42, include_fixit_commands=True ) ).then_return(CommandSlice(commands=[command], cursor=1, total_length=3)) @@ -412,7 +410,9 @@ async def test_get_run_commands_empty( """It should return an empty commands list if no commands.""" decoy.when(mock_run_data_manager.get_current_command("run-id")).then_return(None) decoy.when( - mock_run_data_manager.get_commands_slice(run_id="run-id", cursor=21, length=42) + mock_run_data_manager.get_commands_slice( + run_id="run-id", cursor=21, length=42, include_fixit_commands=True + ) ).then_return(CommandSlice(commands=[], cursor=0, total_length=0)) result = await get_run_commands( @@ -436,7 +436,9 @@ async def test_get_run_commands_not_found( not_found_error = RunNotFoundError("oh no") decoy.when( - mock_run_data_manager.get_commands_slice(run_id="run-id", cursor=21, length=42) + mock_run_data_manager.get_commands_slice( + run_id="run-id", cursor=21, length=42, include_fixit_commands=True + ) ).then_raise(not_found_error) decoy.when(mock_run_data_manager.get_current_command(run_id="run-id")).then_raise( not_found_error diff --git a/robot-server/tests/runs/test_run_data_manager.py b/robot-server/tests/runs/test_run_data_manager.py index d067e51828e..303b567a865 100644 --- a/robot-server/tests/runs/test_run_data_manager.py +++ b/robot-server/tests/runs/test_run_data_manager.py @@ -848,7 +848,9 @@ def test_get_commands_slice_from_db( decoy.when( mock_run_store.get_commands_slice(run_id="run_id", cursor=1, length=2) ).then_return(expected_command_slice) - result = subject.get_commands_slice(run_id="run_id", cursor=1, length=2) + result = subject.get_commands_slice( + run_id="run_id", cursor=1, length=2, include_fixit_commands=True + ) assert expected_command_slice == result @@ -875,11 +877,11 @@ def test_get_commands_slice_current_run( commands=expected_commands_result, cursor=1, total_length=3 ) decoy.when(mock_run_orchestrator_store.current_run_id).then_return("run-id") - decoy.when(mock_run_orchestrator_store.get_command_slice(1, 2)).then_return( + decoy.when(mock_run_orchestrator_store.get_command_slice(1, 2, True)).then_return( expected_command_slice ) - result = subject.get_commands_slice("run-id", 1, 2) + result = subject.get_commands_slice("run-id", 1, 2, include_fixit_commands=True) assert expected_command_slice == result @@ -929,7 +931,9 @@ def test_get_commands_slice_from_db_run_not_found( mock_run_store.get_commands_slice(run_id="run-id", cursor=1, length=2) ).then_raise(RunNotFoundError(run_id="run-id")) with pytest.raises(RunNotFoundError): - subject.get_commands_slice(run_id="run-id", cursor=1, length=2) + subject.get_commands_slice( + run_id="run-id", cursor=1, length=2, include_fixit_commands=True + ) def test_get_current_command(