From 555077b9634fb9652088491e96a8961a6213e65a Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 16 May 2024 13:03:24 -0400 Subject: [PATCH 01/10] Add recoveryTarget field. --- .../robot_server/runs/command_models.py | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/robot-server/robot_server/runs/command_models.py b/robot-server/robot_server/runs/command_models.py index 281223bd155..a41411efce9 100644 --- a/robot-server/robot_server/runs/command_models.py +++ b/robot-server/robot_server/runs/command_models.py @@ -4,6 +4,7 @@ """ from datetime import datetime +from textwrap import dedent from typing import Optional from pydantic import BaseModel, Field @@ -29,18 +30,17 @@ class CommandLinkMeta(BaseModel): runId: str = Field(..., description="The ID of the command's run.") commandId: str = Field(..., description="The ID of the command.") - index: int = Field(..., description="Index of the command in the overall list.") - key: str = Field(..., description="Value of the current command's `key` field.") - createdAt: datetime = Field( - ..., - description="When the current command was created.", + index: int = Field( + ..., description="The index of the command in the run's overall command list." ) + key: str = Field(..., description="The value of the command's `key` field.") + createdAt: datetime = Field(..., description="When the command was created.") class CommandLink(BaseModel): """A link to a command resource.""" - href: str = Field(..., description="The path to a command") + href: str = Field(..., description="The HTTP API path to the command") meta: CommandLinkMeta = Field(..., description="Information about the command.") @@ -49,5 +49,18 @@ class CommandCollectionLinks(BaseModel): current: Optional[CommandLink] = Field( None, - description="Path to the currently running or next queued command.", + description="Information about the currently running or next queued command.", + ) + + recoveryTarget: Optional[CommandLink] = Field( + None, + description=dedent( + """\ + Information about the command currently undergoing error recovery. + + This is basically the most recent protocol command to have failed, + except that once you complete error recovery (see `GET /runs/{id}/actions`), + this goes back to being `null` or omitted. + """ + ), ) From 42329452e111b8f58d0af0917083a24967099daf Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 16 May 2024 14:50:29 -0400 Subject: [PATCH 02/10] Rename get_final_command() -> get_most_recently_finalized_command(). The original name could be misread as "the final command in the run," which it is usually not. --- .../protocol_engine/state/commands.py | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index 8a4098b0a8b..a3a12886f5f 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -606,13 +606,13 @@ def get_current(self) -> Optional[CurrentCommand]: index=running_command.index, ) - final_command = self.get_final_command() - if final_command: + most_recently_finalized_command = self.get_most_recently_finalized_command() + if most_recently_finalized_command: return CurrentCommand( - command_id=final_command.command.id, - command_key=final_command.command.key, - created_at=final_command.command.createdAt, - index=final_command.index, + command_id=most_recently_finalized_command.command.id, + command_key=most_recently_finalized_command.command.key, + created_at=most_recently_finalized_command.command.createdAt, + index=most_recently_finalized_command.index, ) return None @@ -676,7 +676,7 @@ def get_is_running(self) -> bool: """Get whether the protocol is running & queued commands should be executed.""" return self._state.queue_status == QueueStatus.RUNNING - def get_final_command(self) -> Optional[CommandEntry]: + def get_most_recently_finalized_command(self) -> Optional[CommandEntry]: """Get the most recent command that has reached its final `status`. See get_command_is_final.""" run_requested_to_stop = self._state.run_result is not None @@ -689,22 +689,22 @@ def get_final_command(self) -> Optional[CommandEntry]: else: return self._state.command_history.get_prev(tail_command.command.id) else: - final_command = self._state.command_history.get_terminal_command() + most_recently_finalized = self._state.command_history.get_terminal_command() # This iteration is effectively O(1) as we'll only ever have to iterate one or two times at most. - while final_command is not None: + while most_recently_finalized is not None: next_command = self._state.command_history.get_next( - final_command.command.id + most_recently_finalized.command.id ) if ( next_command is not None and next_command.command.status != CommandStatus.QUEUED and next_command.command.status != CommandStatus.RUNNING ): - final_command = next_command + most_recently_finalized = next_command else: break - return final_command + return most_recently_finalized def get_command_is_final(self, command_id: str) -> bool: """Get whether a given command has reached its final `status`. From ed298ab77f76ac4c93db11da500aa9e77fd4608d Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 16 May 2024 18:59:03 -0400 Subject: [PATCH 03/10] Rename CurrentCommand -> CommandPointer. --- api/src/opentrons/protocol_engine/__init__.py | 4 ++-- api/src/opentrons/protocol_engine/state/__init__.py | 4 ++-- api/src/opentrons/protocol_engine/state/commands.py | 10 +++++----- .../protocol_engine/state/test_command_view_old.py | 8 ++++---- .../maintenance_runs/maintenance_run_data_manager.py | 4 ++-- robot-server/robot_server/runs/run_data_manager.py | 4 ++-- .../service/notifications/publishers/runs_publisher.py | 8 ++++---- robot-server/tests/commands/test_router.py | 4 ++-- .../maintenance_runs/router/test_commands_router.py | 4 ++-- robot-server/tests/runs/router/test_commands_router.py | 4 ++-- robot-server/tests/runs/test_run_data_manager.py | 4 ++-- .../notifications/publishers/test_runs_publisher.py | 8 ++++---- 12 files changed, 33 insertions(+), 33 deletions(-) diff --git a/api/src/opentrons/protocol_engine/__init__.py b/api/src/opentrons/protocol_engine/__init__.py index eb62ee7f33a..17e28bcdf32 100644 --- a/api/src/opentrons/protocol_engine/__init__.py +++ b/api/src/opentrons/protocol_engine/__init__.py @@ -22,7 +22,7 @@ CommandType, CommandIntent, ) -from .state import State, StateView, StateSummary, CommandSlice, CurrentCommand, Config +from .state import State, StateView, StateSummary, CommandSlice, CommandPointer, Config from .plugins import AbstractPlugin from .types import ( @@ -85,7 +85,7 @@ "State", "StateView", "CommandSlice", - "CurrentCommand", + "CommandPointer", # public value interfaces and models "LabwareOffset", "LabwareOffsetCreate", diff --git a/api/src/opentrons/protocol_engine/state/__init__.py b/api/src/opentrons/protocol_engine/state/__init__.py index cd6f1bb2b68..80a88350263 100644 --- a/api/src/opentrons/protocol_engine/state/__init__.py +++ b/api/src/opentrons/protocol_engine/state/__init__.py @@ -7,7 +7,7 @@ CommandState, CommandView, CommandSlice, - CurrentCommand, + CommandPointer, ) from .command_history import CommandEntry from .labware import LabwareState, LabwareView @@ -39,7 +39,7 @@ "CommandState", "CommandView", "CommandSlice", - "CurrentCommand", + "CommandPointer", "CommandEntry", # labware state and values "LabwareState", diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index 38c3a17b464..bc6ba7fed6e 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -107,8 +107,8 @@ class CommandSlice: @dataclass(frozen=True) -class CurrentCommand: - """The "current" command's ID and index in the overall commands list.""" +class CommandPointer: + """Brief info about a command and where to find it.""" command_id: str command_key: str @@ -593,7 +593,7 @@ def get_queue_ids(self) -> OrderedSet[str]: """Get the IDs of all queued protocol commands, in FIFO order.""" return self._state.command_history.get_queue_ids() - def get_current(self) -> Optional[CurrentCommand]: + def get_current(self) -> Optional[CommandPointer]: """Return the "current" command, if any. The "current" command is the command that is currently executing, @@ -601,7 +601,7 @@ def get_current(self) -> Optional[CurrentCommand]: """ running_command = self._state.command_history.get_running_command() if running_command: - return CurrentCommand( + return CommandPointer( command_id=running_command.command.id, command_key=running_command.command.key, created_at=running_command.command.createdAt, @@ -610,7 +610,7 @@ def get_current(self) -> Optional[CurrentCommand]: most_recently_finalized_command = self.get_most_recently_finalized_command() if most_recently_finalized_command: - return CurrentCommand( + return CommandPointer( command_id=most_recently_finalized_command.command.id, command_key=most_recently_finalized_command.command.key, created_at=most_recently_finalized_command.command.createdAt, 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 19a2515a3e6..0be2beba529 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 @@ -25,7 +25,7 @@ CommandState, CommandView, CommandSlice, - CurrentCommand, + CommandPointer, RunResult, QueueStatus, ) @@ -846,7 +846,7 @@ def test_get_current() -> None: queued_command_ids=[], commands=[command], ) - assert subject.get_current() == CurrentCommand( + assert subject.get_current() == CommandPointer( index=0, command_id="command-id", command_key="command-key", @@ -866,7 +866,7 @@ def test_get_current() -> None: subject = get_command_view(commands=[command_1, command_2]) subject.state.command_history._set_terminal_command_id(command_1.id) - assert subject.get_current() == CurrentCommand( + assert subject.get_current() == CommandPointer( index=1, command_id="command-id-2", command_key="key-2", @@ -886,7 +886,7 @@ def test_get_current() -> None: subject = get_command_view(commands=[command_1, command_2]) subject.state.command_history._set_terminal_command_id(command_1.id) - assert subject.get_current() == CurrentCommand( + assert subject.get_current() == CommandPointer( index=1, command_id="command-id-2", command_key="key-2", diff --git a/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py b/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py index b18fa4eb4dd..5410ea4c014 100644 --- a/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py +++ b/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py @@ -7,7 +7,7 @@ LabwareOffsetCreate, StateSummary, CommandSlice, - CurrentCommand, + CommandPointer, Command, ) @@ -183,7 +183,7 @@ def get_commands_slice( ) return the_slice - def get_current_command(self, run_id: str) -> Optional[CurrentCommand]: + def get_current_command(self, run_id: str) -> Optional[CommandPointer]: """Get the "current" command, if any. See `ProtocolEngine.state_view.commands.get_current()` for the definition diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index 2b10b1d3220..8836c897c9a 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -9,7 +9,7 @@ LabwareOffsetCreate, StateSummary, CommandSlice, - CurrentCommand, + CommandPointer, Command, ) from opentrons.protocol_engine.types import RunTimeParamValuesType @@ -373,7 +373,7 @@ def get_commands_slice( run_id=run_id, cursor=cursor, length=length ) - def get_current_command(self, run_id: str) -> Optional[CurrentCommand]: + def get_current_command(self, run_id: str) -> Optional[CommandPointer]: """Get the "current" command, if any. See `ProtocolEngine.state_view.commands.get_current()` for the definition diff --git a/robot-server/robot_server/service/notifications/publishers/runs_publisher.py b/robot-server/robot_server/service/notifications/publishers/runs_publisher.py index 056877b7ec6..6a5b3544f01 100644 --- a/robot-server/robot_server/service/notifications/publishers/runs_publisher.py +++ b/robot-server/robot_server/service/notifications/publishers/runs_publisher.py @@ -3,7 +3,7 @@ from dataclasses import dataclass from typing import Callable, Optional -from opentrons.protocol_engine import CurrentCommand, StateSummary, EngineStatus +from opentrons.protocol_engine import CommandPointer, StateSummary, EngineStatus from server_utils.fastapi_utils.app_state import ( AppState, @@ -20,7 +20,7 @@ class RunHooks: """Generated during a protocol run. Utilized by RunsPublisher.""" run_id: str - get_current_command: Callable[[str], Optional[CurrentCommand]] + get_current_command: Callable[[str], Optional[CommandPointer]] get_state_summary: Callable[[str], Optional[StateSummary]] @@ -28,7 +28,7 @@ class RunHooks: class EngineStateSlice: """Protocol Engine state relevant to RunsPublisher.""" - current_command: Optional[CurrentCommand] = None + current_command: Optional[CommandPointer] = None state_summary_status: Optional[EngineStatus] = None @@ -54,7 +54,7 @@ def __init__( async def initialize( self, run_id: str, - get_current_command: Callable[[str], Optional[CurrentCommand]], + get_current_command: Callable[[str], Optional[CommandPointer]], get_state_summary: Callable[[str], Optional[StateSummary]], ) -> None: """Initialize RunsPublisher with necessary information derived from the current run. diff --git a/robot-server/tests/commands/test_router.py b/robot-server/tests/commands/test_router.py index 59f0a7127c9..2d8dc6ac435 100644 --- a/robot-server/tests/commands/test_router.py +++ b/robot-server/tests/commands/test_router.py @@ -6,7 +6,7 @@ from opentrons.protocol_engine import ( ProtocolEngine, CommandSlice, - CurrentCommand, + CommandPointer, commands as pe_commands, ) from opentrons.protocol_engine.errors import CommandDoesNotExistError @@ -147,7 +147,7 @@ async def test_get_commands_list( ) decoy.when(protocol_engine.state_view.commands.get_current()).then_return( - CurrentCommand( + CommandPointer( command_id="abc123", command_key="command-key-1", created_at=datetime(year=2021, month=1, day=1), diff --git a/robot-server/tests/maintenance_runs/router/test_commands_router.py b/robot-server/tests/maintenance_runs/router/test_commands_router.py index 153122308b3..86028c31d06 100644 --- a/robot-server/tests/maintenance_runs/router/test_commands_router.py +++ b/robot-server/tests/maintenance_runs/router/test_commands_router.py @@ -6,7 +6,7 @@ from opentrons.protocol_engine import ( CommandSlice, - CurrentCommand, + CommandPointer, ProtocolEngine, commands as pe_commands, errors as pe_errors, @@ -199,7 +199,7 @@ async def test_get_run_commands( decoy.when( mock_maintenance_run_data_manager.get_current_command("run-id") ).then_return( - CurrentCommand( + CommandPointer( command_id="current-command-id", command_key="current-command-key", created_at=datetime(year=2024, month=4, day=4), diff --git a/robot-server/tests/runs/router/test_commands_router.py b/robot-server/tests/runs/router/test_commands_router.py index 437368966f5..8bf771ad302 100644 --- a/robot-server/tests/runs/router/test_commands_router.py +++ b/robot-server/tests/runs/router/test_commands_router.py @@ -6,7 +6,7 @@ from opentrons.protocol_engine import ( CommandSlice, - CurrentCommand, + CommandPointer, ProtocolEngine, CommandNote, commands as pe_commands, @@ -321,7 +321,7 @@ async def test_get_run_commands( ) decoy.when(mock_run_data_manager.get_current_command("run-id")).then_return( - CurrentCommand( + CommandPointer( command_id="current-command-id", command_key="current-command-key", created_at=datetime(year=2024, month=4, day=4), diff --git a/robot-server/tests/runs/test_run_data_manager.py b/robot-server/tests/runs/test_run_data_manager.py index 12ced28fdb0..100f57a4fef 100644 --- a/robot-server/tests/runs/test_run_data_manager.py +++ b/robot-server/tests/runs/test_run_data_manager.py @@ -13,7 +13,7 @@ commands, types as pe_types, CommandSlice, - CurrentCommand, + CommandPointer, ErrorOccurrence, LoadedLabware, LoadedPipette, @@ -847,7 +847,7 @@ def test_get_current_command( run_command: commands.Command, ) -> None: """Should get current command from engine store.""" - expected_current = CurrentCommand( + expected_current = CommandPointer( command_id=run_command.id, command_key=run_command.key, created_at=run_command.createdAt, diff --git a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py index f8fdaf0cf9f..5d22f8c8428 100644 --- a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py +++ b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py @@ -4,12 +4,12 @@ from unittest.mock import MagicMock, AsyncMock from robot_server.service.notifications import RunsPublisher, Topics -from opentrons.protocol_engine import CurrentCommand, EngineStatus, StateSummary +from opentrons.protocol_engine import CommandPointer, EngineStatus, StateSummary -def mock_curent_command(command_id: str) -> CurrentCommand: - """Create a mock CurrentCommand.""" - return CurrentCommand( +def mock_curent_command(command_id: str) -> CommandPointer: + """Create a mock CommandPointer.""" + return CommandPointer( command_id=command_id, command_key="1", index=0, From c56dc8d7065d2d89bda333b17c37ecc8ed8ba56f Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 20 May 2024 12:04:10 -0400 Subject: [PATCH 04/10] Return current recovery target ID from StateView. --- api/src/opentrons/protocol_engine/state/commands.py | 6 +++++- .../protocol_engine/state/test_command_state.py | 10 +++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index bc6ba7fed6e..9cb7118fc87 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -751,9 +751,13 @@ def get_all_commands_final(self) -> bool: return no_command_running and no_command_to_execute + def get_recovery_target_id(self) -> Optional[str]: + """Return the ID of the command currently undergoing error recovery, if any.""" + return self._state.recovery_target_command_id + def get_recovery_in_progress_for_command(self, command_id: str) -> bool: """Return whether the given command failed and its error recovery is in progress.""" - return self._state.recovery_target_command_id == command_id + return self.get_recovery_target_id() == command_id def raise_fatal_command_error(self) -> None: """Raise the run's fatal command error, if there was one, as an exception. diff --git a/api/tests/opentrons/protocol_engine/state/test_command_state.py b/api/tests/opentrons/protocol_engine/state/test_command_state.py index 742abf3e6e9..339d97b901a 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_state.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_state.py @@ -356,8 +356,8 @@ def test_error_recovery_type_tracking() -> None: assert view.get_error_recovery_type("c2") == ErrorRecoveryType.FAIL_RUN -def test_get_recovery_in_progress_for_command() -> None: - """It should return whether error recovery is in progress for the given command.""" +def test_recovery_target_tracking() -> None: + """It should keep track of the command currently undergoing error recovery.""" subject = CommandStore(config=_make_config(), is_door_open=False) subject_view = CommandView(subject.state) @@ -382,12 +382,14 @@ def test_get_recovery_in_progress_for_command() -> None: subject.handle_action(fail_1) # c1 failed recoverably and we're currently recovering from it. + assert subject_view.get_recovery_target_id() == "c1" assert subject_view.get_recovery_in_progress_for_command("c1") resume_from_1_recovery = actions.ResumeFromRecoveryAction() subject.handle_action(resume_from_1_recovery) # c1 failed recoverably, but we've already completed its recovery. + assert subject_view.get_recovery_target_id() is None assert not subject_view.get_recovery_in_progress_for_command("c1") queue_2 = actions.QueueCommandAction( @@ -411,6 +413,7 @@ def test_get_recovery_in_progress_for_command() -> None: subject.handle_action(fail_2) # c2 failed recoverably and we're currently recovering from it. + assert subject_view.get_recovery_target_id() == "c2" assert subject_view.get_recovery_in_progress_for_command("c2") # ...and that means we're *not* currently recovering from c1, # even though it failed recoverably before. @@ -439,7 +442,8 @@ def test_get_recovery_in_progress_for_command() -> None: subject.handle_action(fail_3) # c3 failed, but not recoverably. - assert not subject_view.get_recovery_in_progress_for_command("c2") + assert subject_view.get_recovery_target_id() is None + assert not subject_view.get_recovery_in_progress_for_command("c3") def test_final_state_after_estop() -> None: From 7420bc3f625b68350db5cc2797f383e3fd9f1fe5 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 20 May 2024 14:11:05 -0400 Subject: [PATCH 05/10] Return a CommandPointer, not just an ID. --- .../protocol_engine/state/commands.py | 19 +++++++++++++++---- .../state/test_command_state.py | 12 ++++++++---- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index 9cb7118fc87..deda08d046a 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -751,13 +751,24 @@ def get_all_commands_final(self) -> bool: return no_command_running and no_command_to_execute - def get_recovery_target_id(self) -> Optional[str]: - """Return the ID of the command currently undergoing error recovery, if any.""" - return self._state.recovery_target_command_id + def get_recovery_target(self) -> Optional[CommandPointer]: + """Return the command currently undergoing error recovery, if any.""" + recovery_target_command_id = self._state.recovery_target_command_id + if recovery_target_command_id is None: + return None + else: + entry = self._state.command_history.get(recovery_target_command_id) + return CommandPointer( + command_id=entry.command.id, + command_key=entry.command.key, + created_at=entry.command.createdAt, + index=entry.index, + ) def get_recovery_in_progress_for_command(self, command_id: str) -> bool: """Return whether the given command failed and its error recovery is in progress.""" - return self.get_recovery_target_id() == command_id + pointer = self.get_recovery_target() + return pointer is not None and pointer.command_id == command_id def raise_fatal_command_error(self) -> None: """Raise the run's fatal command error, if there was one, as an exception. diff --git a/api/tests/opentrons/protocol_engine/state/test_command_state.py b/api/tests/opentrons/protocol_engine/state/test_command_state.py index 339d97b901a..6d079dac570 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_state.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_state.py @@ -382,14 +382,16 @@ def test_recovery_target_tracking() -> None: subject.handle_action(fail_1) # c1 failed recoverably and we're currently recovering from it. - assert subject_view.get_recovery_target_id() == "c1" + recovery_target = subject_view.get_recovery_target() + assert recovery_target is not None + assert recovery_target.command_id == "c1" assert subject_view.get_recovery_in_progress_for_command("c1") resume_from_1_recovery = actions.ResumeFromRecoveryAction() subject.handle_action(resume_from_1_recovery) # c1 failed recoverably, but we've already completed its recovery. - assert subject_view.get_recovery_target_id() is None + assert subject_view.get_recovery_target() is None assert not subject_view.get_recovery_in_progress_for_command("c1") queue_2 = actions.QueueCommandAction( @@ -413,7 +415,9 @@ def test_recovery_target_tracking() -> None: subject.handle_action(fail_2) # c2 failed recoverably and we're currently recovering from it. - assert subject_view.get_recovery_target_id() == "c2" + recovery_target = subject_view.get_recovery_target() + assert recovery_target is not None + assert recovery_target.command_id == "c2" assert subject_view.get_recovery_in_progress_for_command("c2") # ...and that means we're *not* currently recovering from c1, # even though it failed recoverably before. @@ -442,7 +446,7 @@ def test_recovery_target_tracking() -> None: subject.handle_action(fail_3) # c3 failed, but not recoverably. - assert subject_view.get_recovery_target_id() is None + assert subject_view.get_recovery_target() is None assert not subject_view.get_recovery_in_progress_for_command("c3") From 5b99fb9e20a6501cdb59efbb5b72f114f9528fe5 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 20 May 2024 14:19:57 -0400 Subject: [PATCH 06/10] Wire up to `GET /[maintenance_]runs/commands` endpoints. --- .../maintenance_run_data_manager.py | 20 +++++++++- .../router/commands_router.py | 38 ++++++++++++------- .../runs/router/commands_router.py | 38 ++++++++++++------- .../robot_server/runs/run_data_manager.py | 20 +++++++++- .../router/test_commands_router.py | 27 +++++++++++-- .../tests/runs/router/test_commands_router.py | 20 +++++++++- 6 files changed, 131 insertions(+), 32 deletions(-) diff --git a/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py b/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py index 5410ea4c014..d9bd2b11e92 100644 --- a/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py +++ b/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py @@ -194,7 +194,25 @@ def get_current_command(self, run_id: str) -> Optional[CommandPointer]: """ if self._engine_store.current_run_id == run_id: return self._engine_store.engine.state_view.commands.get_current() - return None + else: + # todo(mm, 2024-05-20): + # For historical runs to behave consistently with the current run, + # this should the most recently completed command, not `None`. + return None + + def get_recovery_target_command(self, run_id: str) -> Optional[CommandPointer]: + """Get the current error recovery target. + + See `ProtocolEngine.state_view.commands.get_recovery_target()`. + + Args: + run_id: ID of the run. + """ + if self._engine_store.current_run_id == run_id: + return self._engine_store.engine.state_view.commands.get_recovery_target() + else: + # Historical runs can't have any ongoing error recovery. + return None def get_command(self, run_id: str, command_id: str) -> Command: """Get a run's command by ID. diff --git a/robot-server/robot_server/maintenance_runs/router/commands_router.py b/robot-server/robot_server/maintenance_runs/router/commands_router.py index cd74d1e114a..053e970abce 100644 --- a/robot-server/robot_server/maintenance_runs/router/commands_router.py +++ b/robot-server/robot_server/maintenance_runs/router/commands_router.py @@ -7,6 +7,7 @@ from fastapi import APIRouter, Depends, Query, status from opentrons.protocol_engine import ( + CommandPointer, ProtocolEngine, commands as pe_commands, ) @@ -220,6 +221,7 @@ async def get_run_commands( raise RunNotFound(detail=str(e)).as_error(status.HTTP_404_NOT_FOUND) from e current_command = run_data_manager.get_current_command(run_id=runId) + recovery_target_command = run_data_manager.get_recovery_target_command(run_id=runId) data = [ MaintenanceRunCommandSummary.construct( @@ -242,19 +244,10 @@ async def get_run_commands( totalLength=command_slice.total_length, ) - links = CommandCollectionLinks() - - if current_command is not None: - links.current = CommandLink( - href=f"/runs/{runId}/commands/{current_command.command_id}", - meta=CommandLinkMeta( - runId=runId, - commandId=current_command.command_id, - index=current_command.index, - key=current_command.command_key, - createdAt=current_command.created_at, - ), - ) + links = CommandCollectionLinks.construct( + current=_make_command_link(runId, current_command), + recoveryTarget=_make_command_link(runId, recovery_target_command), + ) return await PydanticResponse.create( content=MultiBody.construct(data=data, meta=meta, links=links), @@ -302,3 +295,22 @@ async def get_run_command( content=SimpleBody.construct(data=command), status_code=status.HTTP_200_OK, ) + + +def _make_command_link( + run_id: str, command_pointer: Optional[CommandPointer] +) -> Optional[CommandLink]: + return ( + CommandLink.construct( + href=f"/maintenance_runs/{run_id}/commands/{command_pointer.command_id}", + meta=CommandLinkMeta( + runId=run_id, + commandId=command_pointer.command_id, + index=command_pointer.index, + key=command_pointer.command_key, + createdAt=command_pointer.created_at, + ), + ) + if command_pointer is not None + else None + ) diff --git a/robot-server/robot_server/runs/router/commands_router.py b/robot-server/robot_server/runs/router/commands_router.py index 20a87e3dba7..257b8c6c9aa 100644 --- a/robot-server/robot_server/runs/router/commands_router.py +++ b/robot-server/robot_server/runs/router/commands_router.py @@ -8,6 +8,7 @@ from opentrons.protocol_engine import ( + CommandPointer, ProtocolEngine, commands as pe_commands, errors as pe_errors, @@ -285,6 +286,7 @@ async def get_run_commands( raise RunNotFound.from_exc(e).as_error(status.HTTP_404_NOT_FOUND) from e current_command = run_data_manager.get_current_command(run_id=runId) + recovery_target_command = run_data_manager.get_recovery_target_command(run_id=runId) data = [ RunCommandSummary.construct( @@ -308,19 +310,10 @@ async def get_run_commands( totalLength=command_slice.total_length, ) - links = CommandCollectionLinks() - - if current_command is not None: - links.current = CommandLink( - href=f"/runs/{runId}/commands/{current_command.command_id}", - meta=CommandLinkMeta( - runId=runId, - commandId=current_command.command_id, - index=current_command.index, - key=current_command.command_key, - createdAt=current_command.created_at, - ), - ) + links = CommandCollectionLinks.construct( + current=_make_command_link(runId, current_command), + recoveryTarget=_make_command_link(runId, recovery_target_command), + ) return await PydanticResponse.create( content=MultiBody.construct(data=data, meta=meta, links=links), @@ -416,3 +409,22 @@ async def get_run_command( content=SimpleBody.construct(data=command), status_code=status.HTTP_200_OK, ) + + +def _make_command_link( + run_id: str, command_pointer: Optional[CommandPointer] +) -> Optional[CommandLink]: + return ( + CommandLink.construct( + href=f"/runs/{run_id}/commands/{command_pointer.command_id}", + meta=CommandLinkMeta( + runId=run_id, + commandId=command_pointer.command_id, + index=command_pointer.index, + key=command_pointer.command_key, + createdAt=command_pointer.created_at, + ), + ) + if command_pointer is not None + else None + ) diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index 8836c897c9a..1635f64ed33 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -384,7 +384,25 @@ def get_current_command(self, run_id: str) -> Optional[CommandPointer]: """ if self._engine_store.current_run_id == run_id: return self._engine_store.engine.state_view.commands.get_current() - return None + else: + # todo(mm, 2024-05-20): + # For historical runs to behave consistently with the current run, + # this should the most recently completed command, not `None`. + return None + + def get_recovery_target_command(self, run_id: str) -> Optional[CommandPointer]: + """Get the current error recovery target. + + See `ProtocolEngine.state_view.commands.get_recovery_target()`. + + Args: + run_id: ID of the run. + """ + if self._engine_store.current_run_id == run_id: + return self._engine_store.engine.state_view.commands.get_recovery_target() + else: + # Historical runs can't have any ongoing error recovery. + return None def get_command(self, run_id: str, command_id: str) -> Command: """Get a run's command by ID. diff --git a/robot-server/tests/maintenance_runs/router/test_commands_router.py b/robot-server/tests/maintenance_runs/router/test_commands_router.py index 86028c31d06..c9e5b0b1a05 100644 --- a/robot-server/tests/maintenance_runs/router/test_commands_router.py +++ b/robot-server/tests/maintenance_runs/router/test_commands_router.py @@ -1,4 +1,4 @@ -"""Tests for the /runs/.../commands routes.""" +"""Tests for the /maintenance_runs/.../commands routes.""" import pytest from datetime import datetime @@ -206,6 +206,17 @@ async def test_get_run_commands( index=101, ) ) + decoy.when( + mock_maintenance_run_data_manager.get_recovery_target_command("run-id") + ).then_return( + CommandPointer( + command_id="recovery-target-command-id", + command_key="recovery-target-command-key", + created_at=datetime(year=2025, month=5, day=5), + index=202, + ) + ) + decoy.when( mock_maintenance_run_data_manager.get_commands_slice( run_id="run-id", @@ -243,7 +254,7 @@ async def test_get_run_commands( assert result.content.meta == MultiBodyMeta(cursor=1, totalLength=3) assert result.content.links == CommandCollectionLinks( current=CommandLink( - href="/runs/run-id/commands/current-command-id", + href="/maintenance_runs/run-id/commands/current-command-id", meta=CommandLinkMeta( runId="run-id", commandId="current-command-id", @@ -251,7 +262,17 @@ async def test_get_run_commands( createdAt=datetime(year=2024, month=4, day=4), index=101, ), - ) + ), + recoveryTarget=CommandLink( + href="/maintenance_runs/run-id/commands/recovery-target-command-id", + meta=CommandLinkMeta( + runId="run-id", + commandId="recovery-target-command-id", + key="recovery-target-command-key", + createdAt=datetime(year=2025, month=5, day=5), + index=202, + ), + ), ) assert result.status_code == 200 diff --git a/robot-server/tests/runs/router/test_commands_router.py b/robot-server/tests/runs/router/test_commands_router.py index 8bf771ad302..0bc39d2355d 100644 --- a/robot-server/tests/runs/router/test_commands_router.py +++ b/robot-server/tests/runs/router/test_commands_router.py @@ -328,6 +328,14 @@ async def test_get_run_commands( index=101, ) ) + decoy.when(mock_run_data_manager.get_recovery_target_command("run-id")).then_return( + CommandPointer( + command_id="recovery-target-command-id", + command_key="recovery-target-command-key", + created_at=datetime(year=2025, month=5, day=5), + index=202, + ) + ) decoy.when( mock_run_data_manager.get_commands_slice( run_id="run-id", @@ -374,7 +382,17 @@ async def test_get_run_commands( createdAt=datetime(year=2024, month=4, day=4), index=101, ), - ) + ), + recoveryTarget=CommandLink( + href="/runs/run-id/commands/recovery-target-command-id", + meta=CommandLinkMeta( + runId="run-id", + commandId="recovery-target-command-id", + key="recovery-target-command-key", + createdAt=datetime(year=2025, month=5, day=5), + index=202, + ), + ), ) assert result.status_code == 200 From df28d16a25ab1afa87289ada6bfe3009d51eccd5 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 20 May 2024 17:58:34 -0400 Subject: [PATCH 07/10] Rename `recoveryTarget` -> `currentlyRecoveringFrom`. --- .../robot_server/maintenance_runs/router/commands_router.py | 2 +- robot-server/robot_server/runs/command_models.py | 2 +- robot-server/robot_server/runs/router/commands_router.py | 2 +- .../tests/maintenance_runs/router/test_commands_router.py | 2 +- robot-server/tests/runs/router/test_commands_router.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/robot-server/robot_server/maintenance_runs/router/commands_router.py b/robot-server/robot_server/maintenance_runs/router/commands_router.py index 053e970abce..f64f96ee726 100644 --- a/robot-server/robot_server/maintenance_runs/router/commands_router.py +++ b/robot-server/robot_server/maintenance_runs/router/commands_router.py @@ -246,7 +246,7 @@ async def get_run_commands( links = CommandCollectionLinks.construct( current=_make_command_link(runId, current_command), - recoveryTarget=_make_command_link(runId, recovery_target_command), + currentlyRecoveringFrom=_make_command_link(runId, recovery_target_command), ) return await PydanticResponse.create( diff --git a/robot-server/robot_server/runs/command_models.py b/robot-server/robot_server/runs/command_models.py index 9a304f3d136..5da1038f470 100644 --- a/robot-server/robot_server/runs/command_models.py +++ b/robot-server/robot_server/runs/command_models.py @@ -55,7 +55,7 @@ class CommandCollectionLinks(BaseModel): ), ) - recoveryTarget: Optional[CommandLink] = Field( + currentlyRecoveringFrom: Optional[CommandLink] = Field( None, description=( "Information about the command currently undergoing error recovery." diff --git a/robot-server/robot_server/runs/router/commands_router.py b/robot-server/robot_server/runs/router/commands_router.py index 257b8c6c9aa..563979c2c41 100644 --- a/robot-server/robot_server/runs/router/commands_router.py +++ b/robot-server/robot_server/runs/router/commands_router.py @@ -312,7 +312,7 @@ async def get_run_commands( links = CommandCollectionLinks.construct( current=_make_command_link(runId, current_command), - recoveryTarget=_make_command_link(runId, recovery_target_command), + currentlyRecoveringFrom=_make_command_link(runId, recovery_target_command), ) return await PydanticResponse.create( diff --git a/robot-server/tests/maintenance_runs/router/test_commands_router.py b/robot-server/tests/maintenance_runs/router/test_commands_router.py index c9e5b0b1a05..5d5e66abff5 100644 --- a/robot-server/tests/maintenance_runs/router/test_commands_router.py +++ b/robot-server/tests/maintenance_runs/router/test_commands_router.py @@ -263,7 +263,7 @@ async def test_get_run_commands( index=101, ), ), - recoveryTarget=CommandLink( + currentlyRecoveringFrom=CommandLink( href="/maintenance_runs/run-id/commands/recovery-target-command-id", meta=CommandLinkMeta( runId="run-id", diff --git a/robot-server/tests/runs/router/test_commands_router.py b/robot-server/tests/runs/router/test_commands_router.py index 0bc39d2355d..d0cf29ecd85 100644 --- a/robot-server/tests/runs/router/test_commands_router.py +++ b/robot-server/tests/runs/router/test_commands_router.py @@ -383,7 +383,7 @@ async def test_get_run_commands( index=101, ), ), - recoveryTarget=CommandLink( + currentlyRecoveringFrom=CommandLink( href="/runs/run-id/commands/recovery-target-command-id", meta=CommandLinkMeta( runId="run-id", From 44efbb9248a28efbf4c2ca7d8d27e64090b456dd Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 20 May 2024 18:01:27 -0400 Subject: [PATCH 08/10] Update TypeScript binding. --- api-client/src/runs/commands/types.ts | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/api-client/src/runs/commands/types.ts b/api-client/src/runs/commands/types.ts index a984ae70366..fda38a4aa2a 100644 --- a/api-client/src/runs/commands/types.ts +++ b/api-client/src/runs/commands/types.ts @@ -15,16 +15,18 @@ export interface CommandDetail { } export interface CommandsLinks { - current: { - // link to the currently executing command - href: string - meta: { - runId: string - commandId: string - key: string - createdAt: string - index: number - } + current: CommandsLink + currentlyRecoveringFrom: CommandsLink +} + +interface CommandsLink { + href: string + meta: { + runId: string + commandId: string + key: string + createdAt: string + index: number } } From f4704d544dc5a50e6958b569d81261d3ffc09245 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 20 May 2024 18:11:54 -0400 Subject: [PATCH 09/10] Typo. --- .../maintenance_runs/maintenance_run_data_manager.py | 2 +- robot-server/robot_server/runs/run_data_manager.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py b/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py index d9bd2b11e92..92b7eeb5cd1 100644 --- a/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py +++ b/robot-server/robot_server/maintenance_runs/maintenance_run_data_manager.py @@ -197,7 +197,7 @@ def get_current_command(self, run_id: str) -> Optional[CommandPointer]: else: # todo(mm, 2024-05-20): # For historical runs to behave consistently with the current run, - # this should the most recently completed command, not `None`. + # this should be the most recently completed command, not `None`. return None def get_recovery_target_command(self, run_id: str) -> Optional[CommandPointer]: diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index 1635f64ed33..5f69ba5a960 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -387,7 +387,7 @@ def get_current_command(self, run_id: str) -> Optional[CommandPointer]: else: # todo(mm, 2024-05-20): # For historical runs to behave consistently with the current run, - # this should the most recently completed command, not `None`. + # this should be the most recently completed command, not `None`. return None def get_recovery_target_command(self, run_id: str) -> Optional[CommandPointer]: From e568f68415e7e9783d33a982ef38cb610a75a5ba Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 21 May 2024 11:10:58 -0400 Subject: [PATCH 10/10] Fix: `current` and `currentlyRecoveringFrom` need to be nullable and omittable. --- api-client/src/runs/commands/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api-client/src/runs/commands/types.ts b/api-client/src/runs/commands/types.ts index fda38a4aa2a..f92af15eab5 100644 --- a/api-client/src/runs/commands/types.ts +++ b/api-client/src/runs/commands/types.ts @@ -15,8 +15,8 @@ export interface CommandDetail { } export interface CommandsLinks { - current: CommandsLink - currentlyRecoveringFrom: CommandsLink + current?: CommandsLink | null + currentlyRecoveringFrom?: CommandsLink | null } interface CommandsLink {