From 33100e7a0d5da74b298b7c1be833fa300c85643d Mon Sep 17 00:00:00 2001 From: TamarZanzouri Date: Mon, 25 Nov 2024 14:55:02 -0500 Subject: [PATCH] feat(api, robot-server): get historical and current command errors (#16697) --- .../protocol_engine/state/command_history.py | 12 + .../protocol_engine/state/commands.py | 12 +- .../state/test_command_state.py | 100 ++++- .../state/test_command_store_old.py | 11 - .../state/test_command_view_old.py | 42 +- .../data_files/data_files_store.py | 2 +- .../persistence/_migrations/v6_to_v7.py | 4 +- .../persistence/_migrations/v7_to_v8.py | 135 +++++++ .../persistence/file_and_directory_names.py | 2 +- .../persistence/persistence_directory.py | 5 +- .../persistence/tables/__init__.py | 4 +- .../persistence/tables/schema_8.py | 358 ++++++++++++++++++ .../robot_server/runs/router/base_router.py | 7 +- .../robot_server/runs/run_data_manager.py | 31 +- .../runs/run_orchestrator_store.py | 4 +- robot-server/robot_server/runs/run_store.py | 131 ++++++- robot-server/tests/persistence/test_tables.py | 132 ++++++- .../tests/runs/router/test_base_router.py | 15 +- .../tests/runs/test_run_data_manager.py | 20 +- robot-server/tests/runs/test_run_store.py | 102 ++++- 20 files changed, 1019 insertions(+), 110 deletions(-) create mode 100644 robot-server/robot_server/persistence/_migrations/v7_to_v8.py create mode 100644 robot-server/robot_server/persistence/tables/schema_8.py diff --git a/api/src/opentrons/protocol_engine/state/command_history.py b/api/src/opentrons/protocol_engine/state/command_history.py index d555764e54e..0879a7cd130 100644 --- a/api/src/opentrons/protocol_engine/state/command_history.py +++ b/api/src/opentrons/protocol_engine/state/command_history.py @@ -24,6 +24,9 @@ class CommandHistory: _all_command_ids: List[str] """All command IDs, in insertion order.""" + _all_failed_command_ids: List[str] + """All failed command IDs, in insertion order.""" + _all_command_ids_but_fixit_command_ids: List[str] """All command IDs besides fixit command intents, in insertion order.""" @@ -47,6 +50,7 @@ class CommandHistory: def __init__(self) -> None: self._all_command_ids = [] + self._all_failed_command_ids = [] self._all_command_ids_but_fixit_command_ids = [] self._queued_command_ids = OrderedSet() self._queued_setup_command_ids = OrderedSet() @@ -101,6 +105,13 @@ def get_all_commands(self) -> List[Command]: for command_id in self._all_command_ids ] + def get_all_failed_commands(self) -> List[Command]: + """Get all failed commands.""" + return [ + self._commands_by_id[command_id].command + for command_id in self._all_failed_command_ids + ] + def get_filtered_command_ids(self, include_fixit_commands: bool) -> List[str]: """Get all fixit command IDs.""" if include_fixit_commands: @@ -242,6 +253,7 @@ def set_command_failed(self, command: Command) -> None: self._remove_queue_id(command.id) self._remove_setup_queue_id(command.id) self._set_most_recently_completed_command_id(command.id) + self._all_failed_command_ids.append(command.id) def _add(self, command_id: str, command_entry: CommandEntry) -> None: """Create or update a command entry.""" diff --git a/api/src/opentrons/protocol_engine/state/commands.py b/api/src/opentrons/protocol_engine/state/commands.py index 4d2009aae80..da99c14ef3e 100644 --- a/api/src/opentrons/protocol_engine/state/commands.py +++ b/api/src/opentrons/protocol_engine/state/commands.py @@ -228,9 +228,6 @@ class CommandState: This value can be used to generate future hashes. """ - failed_command_errors: List[ErrorOccurrence] - """List of command errors that occurred during run execution.""" - has_entered_error_recovery: bool """Whether the run has entered error recovery.""" @@ -269,7 +266,6 @@ def __init__( run_started_at=None, latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=error_recovery_policy, has_entered_error_recovery=False, ) @@ -366,7 +362,6 @@ def _handle_fail_command_action(self, action: FailCommandAction) -> None: notes=action.notes, ) self._state.failed_command = self._state.command_history.get(action.command_id) - self._state.failed_command_errors.append(public_error_occurrence) if ( prev_entry.command.intent in (CommandIntent.PROTOCOL, None) @@ -706,7 +701,12 @@ def get_error(self) -> Optional[ErrorOccurrence]: def get_all_errors(self) -> List[ErrorOccurrence]: """Get the run's full error list, if there was none, returns an empty list.""" - return self._state.failed_command_errors + failed_commands = self._state.command_history.get_all_failed_commands() + return [ + command_error.error + for command_error in failed_commands + if command_error.error is not None + ] def get_has_entered_recovery_mode(self) -> bool: """Get whether the run has entered recovery mode.""" 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 fde0d66e654..c52cd8ca74d 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_state.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_state.py @@ -30,6 +30,7 @@ from opentrons.protocol_engine.state.commands import ( CommandStore, CommandView, + CommandErrorSlice, ) from opentrons.protocol_engine.state.config import Config from opentrons.protocol_engine.state.update_types import StateUpdate @@ -193,7 +194,7 @@ def test_command_failure(error_recovery_type: ErrorRecoveryType) -> None: ) assert subject_view.get("command-id") == expected_failed_command - assert subject.state.failed_command_errors == [expected_error_occurrence] + assert subject_view.get_all_errors() == [expected_error_occurrence] def test_command_failure_clears_queues() -> None: @@ -255,7 +256,7 @@ def test_command_failure_clears_queues() -> None: assert subject_view.get_running_command_id() is None assert subject_view.get_queue_ids() == OrderedSet() assert subject_view.get_next_to_execute() is None - assert subject.state.failed_command_errors == [expected_error_occurance] + assert subject_view.get_all_errors() == [expected_error_occurance] def test_setup_command_failure_only_clears_setup_command_queue() -> None: @@ -555,7 +556,7 @@ def test_door_during_error_recovery() -> None: subject.handle_action(play) assert subject_view.get_status() == EngineStatus.AWAITING_RECOVERY assert subject_view.get_next_to_execute() == "command-id-2" - assert subject.state.failed_command_errors == [expected_error_occurance] + assert subject_view.get_all_errors() == [expected_error_occurance] @pytest.mark.parametrize("close_door_before_queueing", [False, True]) @@ -732,7 +733,7 @@ def test_error_recovery_type_tracking() -> None: id="c2-error", createdAt=datetime(year=2023, month=3, day=3), error=exception ) - assert subject.state.failed_command_errors == [ + assert view.get_all_errors() == [ error_occurrence_1, error_occurrence_2, ] @@ -1100,3 +1101,94 @@ def test_get_state_update_for_false_positive() -> None: subject.handle_action(resume_from_recovery) assert subject_view.get_state_update_for_false_positive() == empty_state_update + + +def test_get_errors_slice_empty() -> None: + """It should return an empty error list.""" + subject = CommandStore( + config=_make_config(), + error_recovery_policy=_placeholder_error_recovery_policy, + is_door_open=False, + ) + subject_view = CommandView(subject.state) + result = subject_view.get_errors_slice(cursor=0, length=2) + + assert result == CommandErrorSlice(commands_errors=[], cursor=0, total_length=0) + + +def test_get_errors_slice() -> None: + """It should return a slice of all command errors.""" + subject = CommandStore( + config=_make_config(), + error_recovery_policy=_placeholder_error_recovery_policy, + is_door_open=False, + ) + + subject_view = CommandView(subject.state) + + queue_1 = actions.QueueCommandAction( + request=commands.WaitForResumeCreate( + params=commands.WaitForResumeParams(), key="command-key-1" + ), + request_hash=None, + created_at=datetime(year=2021, month=1, day=1), + command_id="command-id-1", + ) + subject.handle_action(queue_1) + queue_2_setup = actions.QueueCommandAction( + request=commands.WaitForResumeCreate( + params=commands.WaitForResumeParams(), + intent=commands.CommandIntent.SETUP, + key="command-key-2", + ), + request_hash=None, + created_at=datetime(year=2021, month=1, day=1), + command_id="command-id-2", + ) + subject.handle_action(queue_2_setup) + queue_3_setup = actions.QueueCommandAction( + request=commands.WaitForResumeCreate( + params=commands.WaitForResumeParams(), + intent=commands.CommandIntent.SETUP, + key="command-key-3", + ), + request_hash=None, + created_at=datetime(year=2021, month=1, day=1), + command_id="command-id-3", + ) + subject.handle_action(queue_3_setup) + + run_2_setup = actions.RunCommandAction( + command_id="command-id-2", + started_at=datetime(year=2022, month=2, day=2), + ) + subject.handle_action(run_2_setup) + fail_2_setup = actions.FailCommandAction( + command_id="command-id-2", + running_command=subject_view.get("command-id-2"), + error_id="error-id", + failed_at=datetime(year=2023, month=3, day=3), + error=errors.ProtocolEngineError(message="oh no"), + notes=[], + type=ErrorRecoveryType.CONTINUE_WITH_ERROR, + ) + subject.handle_action(fail_2_setup) + + result = subject_view.get_errors_slice(cursor=1, length=3) + + assert result == CommandErrorSlice( + [ + ErrorOccurrence( + id="error-id", + createdAt=datetime(2023, 3, 3, 0, 0), + isDefined=False, + errorType="ProtocolEngineError", + errorCode="4000", + detail="oh no", + errorInfo={}, + wrappedErrors=[], + ) + ], + cursor=0, + total_length=1, + ) diff --git a/api/tests/opentrons/protocol_engine/state/test_command_store_old.py b/api/tests/opentrons/protocol_engine/state/test_command_store_old.py index d5f171b7ea9..881719ba7ad 100644 --- a/api/tests/opentrons/protocol_engine/state/test_command_store_old.py +++ b/api/tests/opentrons/protocol_engine/state/test_command_store_old.py @@ -333,7 +333,6 @@ def test_command_store_handles_pause_action(pause_source: PauseSource) -> None: recovery_target=None, latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -363,7 +362,6 @@ def test_command_store_handles_play_action(pause_source: PauseSource) -> None: run_started_at=datetime(year=2021, month=1, day=1), latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -398,7 +396,6 @@ def test_command_store_handles_finish_action() -> None: run_started_at=datetime(year=2021, month=1, day=1), latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -453,7 +450,6 @@ def test_command_store_handles_stop_action( run_started_at=datetime(year=2021, month=1, day=1), latest_protocol_command_hash=None, stopped_by_estop=from_estop, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -491,7 +487,6 @@ def test_command_store_handles_stop_action_when_awaiting_recovery() -> None: run_started_at=datetime(year=2021, month=1, day=1), latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -525,7 +520,6 @@ def test_command_store_cannot_restart_after_should_stop() -> None: run_started_at=None, latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -672,7 +666,6 @@ def test_command_store_wraps_unknown_errors() -> None: recovery_target=None, latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -742,7 +735,6 @@ def __init__(self, message: str) -> None: run_started_at=None, latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -778,7 +770,6 @@ def test_command_store_ignores_stop_after_graceful_finish() -> None: run_started_at=datetime(year=2021, month=1, day=1), latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -814,7 +805,6 @@ def test_command_store_ignores_finish_after_non_graceful_stop() -> None: run_started_at=datetime(year=2021, month=1, day=1), latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) @@ -850,7 +840,6 @@ def test_handles_hardware_stopped() -> None: run_started_at=None, latest_protocol_command_hash=None, stopped_by_estop=False, - failed_command_errors=[], error_recovery_policy=matchers.Anything(), has_entered_error_recovery=False, ) 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 f7b1d6cd31f..0cbef9cf474 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 @@ -28,19 +28,17 @@ CommandState, CommandView, CommandSlice, - CommandErrorSlice, CommandPointer, RunResult, QueueStatus, ) -from opentrons.protocol_engine.state.command_history import CommandEntry +from opentrons.protocol_engine.state.command_history import CommandEntry, CommandHistory from opentrons.protocol_engine.errors import ProtocolCommandFailedError, ErrorOccurrence from opentrons_shared_data.errors.codes import ErrorCodes -from opentrons.protocol_engine.state.command_history import CommandHistory from opentrons.protocol_engine.state.update_types import StateUpdate from .command_fixtures import ( @@ -77,7 +75,6 @@ def get_command_view( # noqa: C901 finish_error: Optional[errors.ErrorOccurrence] = None, commands: Sequence[cmd.Command] = (), latest_command_hash: Optional[str] = None, - failed_command_errors: Optional[List[ErrorOccurrence]] = None, has_entered_error_recovery: bool = False, ) -> CommandView: """Get a command view test subject.""" @@ -121,7 +118,6 @@ def get_command_view( # noqa: C901 run_started_at=run_started_at, latest_protocol_command_hash=latest_command_hash, stopped_by_estop=False, - failed_command_errors=failed_command_errors or [], has_entered_error_recovery=has_entered_error_recovery, error_recovery_policy=_placeholder_error_recovery_policy, ) @@ -1031,42 +1027,6 @@ def test_get_slice_default_cursor_running() -> 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=0, length=2) - - assert result == CommandErrorSlice(commands_errors=[], cursor=0, total_length=0) - - -def test_get_errors_slice() -> None: - """It should return a slice of all command errors.""" - error_1 = ErrorOccurrence.construct(id="error-id-1") # type: ignore[call-arg] - error_2 = ErrorOccurrence.construct(id="error-id-2") # type: ignore[call-arg] - error_3 = ErrorOccurrence.construct(id="error-id-3") # type: ignore[call-arg] - error_4 = ErrorOccurrence.construct(id="error-id-4") # type: ignore[call-arg] - - subject = get_command_view( - failed_command_errors=[error_1, error_2, error_3, error_4] - ) - - result = subject.get_errors_slice(cursor=1, length=3) - - assert result == CommandErrorSlice( - commands_errors=[error_2, error_3, error_4], - cursor=1, - total_length=4, - ) - - result = subject.get_errors_slice(cursor=-3, length=10) - - assert result == CommandErrorSlice( - commands_errors=[error_1, error_2, error_3, error_4], - cursor=0, - total_length=4, - ) - - def test_get_slice_without_fixit() -> None: """It should select a cursor based on the running command, if present.""" command_1 = create_succeeded_command(command_id="command-id-1") diff --git a/robot-server/robot_server/data_files/data_files_store.py b/robot-server/robot_server/data_files/data_files_store.py index 28257dbb8d2..e0ef8fefa44 100644 --- a/robot-server/robot_server/data_files/data_files_store.py +++ b/robot-server/robot_server/data_files/data_files_store.py @@ -15,7 +15,7 @@ analysis_csv_rtp_table, run_csv_rtp_table, ) -from robot_server.persistence.tables.schema_7 import DataFileSourceSQLEnum +from robot_server.persistence.tables import DataFileSourceSQLEnum from .models import DataFileSource, FileIdNotFoundError, FileInUseError diff --git a/robot-server/robot_server/persistence/_migrations/v6_to_v7.py b/robot-server/robot_server/persistence/_migrations/v6_to_v7.py index f2c0f2ad93d..29cd284e942 100644 --- a/robot-server/robot_server/persistence/_migrations/v6_to_v7.py +++ b/robot-server/robot_server/persistence/_migrations/v6_to_v7.py @@ -16,7 +16,7 @@ import sqlalchemy from ..database import sql_engine_ctx, sqlite_rowid -from ..tables import DataFileSourceSQLEnum, schema_7 +from ..tables import schema_7 from .._folder_migrator import Migration from ..file_and_directory_names import ( @@ -102,6 +102,6 @@ def _migrate_data_files_table_with_new_source_col( """Add a new 'source' column to data_files table.""" dest_transaction.execute( sqlalchemy.update(schema_7.data_files_table).values( - {"source": DataFileSourceSQLEnum.UPLOADED} + {"source": schema_7.DataFileSourceSQLEnum.UPLOADED} ) ) diff --git a/robot-server/robot_server/persistence/_migrations/v7_to_v8.py b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py new file mode 100644 index 00000000000..a5ed950a8dc --- /dev/null +++ b/robot-server/robot_server/persistence/_migrations/v7_to_v8.py @@ -0,0 +1,135 @@ +"""Migrate the persistence directory from schema 7 to 8. + +Summary of changes from schema 7: + +- Adds a new command_error to store the commands error in the commands table +- Adds a new command_status to store the commands status in the commands table +""" + +import json +from pathlib import Path +from contextlib import ExitStack +import shutil +from typing import Any + +import sqlalchemy + +from ..database import sql_engine_ctx +from ..tables import schema_8 +from .._folder_migrator import Migration + +from ..file_and_directory_names import ( + DB_FILE, +) +from ..tables.schema_8 import CommandStatusSQLEnum + + +class Migration7to8(Migration): # noqa: D101 + def migrate(self, source_dir: Path, dest_dir: Path) -> None: + """Migrate the persistence directory from schema 6 to 7.""" + # Copy over all existing directories and files to new version + for item in source_dir.iterdir(): + if item.is_dir(): + shutil.copytree(src=item, dst=dest_dir / item.name) + else: + shutil.copy(src=item, dst=dest_dir / item.name) + + dest_db_file = dest_dir / DB_FILE + + with ExitStack() as exit_stack: + dest_engine = exit_stack.enter_context(sql_engine_ctx(dest_db_file)) + + dest_transaction = exit_stack.enter_context(dest_engine.begin()) + + def add_column( + engine: sqlalchemy.engine.Engine, + table_name: str, + column: Any, + ) -> None: + column_type = column.type.compile(engine.dialect) + engine.execute( + f"ALTER TABLE {table_name} ADD COLUMN {column.key} {column_type}" + ) + + add_column( + dest_engine, + schema_8.run_command_table.name, + schema_8.run_command_table.c.command_error, + ) + + add_column( + dest_engine, + schema_8.run_command_table.name, + schema_8.run_command_table.c.command_status, + ) + + _add_missing_indexes(dest_transaction=dest_transaction) + + _migrate_command_table_with_new_command_error_col_and_command_status( + dest_transaction=dest_transaction + ) + + +def _add_missing_indexes(dest_transaction: sqlalchemy.engine.Connection) -> None: + # todo(2024-11-20): Probably add the indexes missing from prior migrations here. + # https://opentrons.atlassian.net/browse/EXEC-827 + dest_transaction.execute( + "CREATE UNIQUE INDEX ix_run_run_id_command_status_index_in_run ON run_command (run_id, command_status, index_in_run);" + ) + + +def _migrate_command_table_with_new_command_error_col_and_command_status( + dest_transaction: sqlalchemy.engine.Connection, +) -> None: + """Add a new 'command_error' and 'command_status' column to run_command_table table.""" + commands_table = schema_8.run_command_table + select_commands = sqlalchemy.select(commands_table) + commands_to_update = [] + for row in dest_transaction.execute(select_commands).all(): + data = json.loads(row.command) + new_command_error = ( + # Account for old_row.command["error"] being null. + None + if "error" not in row.command or data["error"] is None + else json.dumps(data["error"]) + ) + # parse json as enum + new_command_status = _convert_commands_status_to_sql_command_status( + data["status"] + ) + commands_to_update.append( + { + "_id": row.row_id, + "command_error": new_command_error, + "command_status": new_command_status, + } + ) + + if len(commands_to_update) > 0: + update_commands = ( + sqlalchemy.update(commands_table) + .where(commands_table.c.row_id == sqlalchemy.bindparam("_id")) + .values( + { + "command_error": sqlalchemy.bindparam("command_error"), + "command_status": sqlalchemy.bindparam("command_status"), + } + ) + ) + dest_transaction.execute(update_commands, commands_to_update) + + +def _convert_commands_status_to_sql_command_status( + status: str, +) -> CommandStatusSQLEnum: + match status: + case "queued": + return CommandStatusSQLEnum.QUEUED + case "running": + return CommandStatusSQLEnum.RUNNING + case "failed": + return CommandStatusSQLEnum.FAILED + case "succeeded": + return CommandStatusSQLEnum.SUCCEEDED + case _: + assert False, "command status is unknown" diff --git a/robot-server/robot_server/persistence/file_and_directory_names.py b/robot-server/robot_server/persistence/file_and_directory_names.py index 7074dd6db2f..220a32e7673 100644 --- a/robot-server/robot_server/persistence/file_and_directory_names.py +++ b/robot-server/robot_server/persistence/file_and_directory_names.py @@ -8,7 +8,7 @@ from typing import Final -LATEST_VERSION_DIRECTORY: Final = "7.1" +LATEST_VERSION_DIRECTORY: Final = "8" DECK_CONFIGURATION_FILE: Final = "deck_configuration.json" PROTOCOLS_DIRECTORY: Final = "protocols" diff --git a/robot-server/robot_server/persistence/persistence_directory.py b/robot-server/robot_server/persistence/persistence_directory.py index c6b40ce10ab..1f6a9fb6733 100644 --- a/robot-server/robot_server/persistence/persistence_directory.py +++ b/robot-server/robot_server/persistence/persistence_directory.py @@ -11,7 +11,7 @@ from anyio import Path as AsyncPath, to_thread from ._folder_migrator import MigrationOrchestrator -from ._migrations import up_to_3, v3_to_v4, v4_to_v5, v5_to_v6, v6_to_v7 +from ._migrations import up_to_3, v3_to_v4, v4_to_v5, v5_to_v6, v6_to_v7, v7_to_v8 from .file_and_directory_names import LATEST_VERSION_DIRECTORY _TEMP_PERSISTENCE_DIR_PREFIX: Final = "opentrons-robot-server-" @@ -59,7 +59,8 @@ def make_migration_orchestrator(prepared_root: Path) -> MigrationOrchestrator: # Subdirectory "7" was previously used on our edge branch for an in-dev # schema that was never released to the public. It may be present on # internal robots. - v6_to_v7.Migration6to7(subdirectory=LATEST_VERSION_DIRECTORY), + v6_to_v7.Migration6to7(subdirectory="7.1"), + v7_to_v8.Migration7to8(subdirectory=LATEST_VERSION_DIRECTORY), ], temp_file_prefix="temp-", ) diff --git a/robot-server/robot_server/persistence/tables/__init__.py b/robot-server/robot_server/persistence/tables/__init__.py index fa0129a4ee6..56e149d6dfd 100644 --- a/robot-server/robot_server/persistence/tables/__init__.py +++ b/robot-server/robot_server/persistence/tables/__init__.py @@ -1,7 +1,7 @@ """SQL database schemas.""" # Re-export the latest schema. -from .schema_7 import ( +from .schema_8 import ( metadata, protocol_table, analysis_table, @@ -17,6 +17,7 @@ ProtocolKindSQLEnum, BooleanSettingKey, DataFileSourceSQLEnum, + CommandStatusSQLEnum, ) @@ -36,4 +37,5 @@ "ProtocolKindSQLEnum", "BooleanSettingKey", "DataFileSourceSQLEnum", + "CommandStatusSQLEnum", ] diff --git a/robot-server/robot_server/persistence/tables/schema_8.py b/robot-server/robot_server/persistence/tables/schema_8.py new file mode 100644 index 00000000000..c92dd4645c7 --- /dev/null +++ b/robot-server/robot_server/persistence/tables/schema_8.py @@ -0,0 +1,358 @@ +"""v8 of our SQLite schema.""" +import enum +import sqlalchemy + +from robot_server.persistence._utc_datetime import UTCDateTime + +metadata = sqlalchemy.MetaData() + + +class PrimitiveParamSQLEnum(enum.Enum): + """Enum type to store primitive param type.""" + + INT = "int" + FLOAT = "float" + BOOL = "bool" + STR = "str" + + +class ProtocolKindSQLEnum(enum.Enum): + """What kind a stored protocol is.""" + + STANDARD = "standard" + QUICK_TRANSFER = "quick-transfer" + + +class DataFileSourceSQLEnum(enum.Enum): + """The source this data file is from.""" + + UPLOADED = "uploaded" + GENERATED = "generated" + + +class CommandStatusSQLEnum(enum.Enum): + """Command status sql enum.""" + + QUEUED = "queued" + RUNNING = "running" + SUCCEEDED = "succeeded" + FAILED = "failed" + + +protocol_table = sqlalchemy.Table( + "protocol", + metadata, + sqlalchemy.Column( + "id", + sqlalchemy.String, + primary_key=True, + ), + sqlalchemy.Column( + "created_at", + UTCDateTime, + nullable=False, + ), + sqlalchemy.Column("protocol_key", sqlalchemy.String, nullable=True), + sqlalchemy.Column( + "protocol_kind", + sqlalchemy.Enum( + ProtocolKindSQLEnum, + values_callable=lambda obj: [e.value for e in obj], + validate_strings=True, + create_constraint=True, + ), + index=True, + nullable=False, + ), +) + +analysis_table = sqlalchemy.Table( + "analysis", + metadata, + sqlalchemy.Column( + "id", + sqlalchemy.String, + primary_key=True, + ), + sqlalchemy.Column( + "protocol_id", + sqlalchemy.String, + sqlalchemy.ForeignKey("protocol.id"), + index=True, + nullable=False, + ), + sqlalchemy.Column( + "analyzer_version", + sqlalchemy.String, + nullable=False, + ), + sqlalchemy.Column( + "completed_analysis", + # Stores a JSON string. See CompletedAnalysisStore. + sqlalchemy.String, + nullable=False, + ), +) + +analysis_primitive_type_rtp_table = sqlalchemy.Table( + "analysis_primitive_rtp_table", + metadata, + sqlalchemy.Column( + "row_id", + sqlalchemy.Integer, + primary_key=True, + ), + sqlalchemy.Column( + "analysis_id", + sqlalchemy.ForeignKey("analysis.id"), + nullable=False, + ), + sqlalchemy.Column( + "parameter_variable_name", + sqlalchemy.String, + nullable=False, + ), + sqlalchemy.Column( + "parameter_type", + sqlalchemy.Enum( + PrimitiveParamSQLEnum, + values_callable=lambda obj: [e.value for e in obj], + create_constraint=True, + # todo(mm, 2024-09-24): Can we add validate_strings=True here? + ), + nullable=False, + ), + sqlalchemy.Column( + "parameter_value", + sqlalchemy.String, + nullable=False, + ), +) + +analysis_csv_rtp_table = sqlalchemy.Table( + "analysis_csv_rtp_table", + metadata, + sqlalchemy.Column( + "row_id", + sqlalchemy.Integer, + primary_key=True, + ), + sqlalchemy.Column( + "analysis_id", + sqlalchemy.ForeignKey("analysis.id"), + nullable=False, + ), + sqlalchemy.Column( + "parameter_variable_name", + sqlalchemy.String, + nullable=False, + ), + sqlalchemy.Column( + "file_id", + sqlalchemy.ForeignKey("data_files.id"), + nullable=True, + ), +) + +run_table = sqlalchemy.Table( + "run", + metadata, + sqlalchemy.Column( + "id", + sqlalchemy.String, + primary_key=True, + ), + sqlalchemy.Column( + "created_at", + UTCDateTime, + nullable=False, + ), + sqlalchemy.Column( + "protocol_id", + sqlalchemy.String, + sqlalchemy.ForeignKey("protocol.id"), + nullable=True, + ), + sqlalchemy.Column( + "state_summary", + sqlalchemy.String, + nullable=True, + ), + sqlalchemy.Column("engine_status", sqlalchemy.String, nullable=True), + sqlalchemy.Column("_updated_at", UTCDateTime, nullable=True), + sqlalchemy.Column( + "run_time_parameters", + # Stores a JSON string. See RunStore. + sqlalchemy.String, + nullable=True, + ), +) + +action_table = sqlalchemy.Table( + "action", + metadata, + sqlalchemy.Column( + "id", + sqlalchemy.String, + primary_key=True, + ), + sqlalchemy.Column("created_at", UTCDateTime, nullable=False), + sqlalchemy.Column("action_type", sqlalchemy.String, nullable=False), + sqlalchemy.Column( + "run_id", + sqlalchemy.String, + sqlalchemy.ForeignKey("run.id"), + nullable=False, + ), +) + +run_command_table = sqlalchemy.Table( + "run_command", + metadata, + sqlalchemy.Column("row_id", sqlalchemy.Integer, primary_key=True), + sqlalchemy.Column( + "run_id", sqlalchemy.String, sqlalchemy.ForeignKey("run.id"), nullable=False + ), + # command_index in commands enumeration + sqlalchemy.Column("index_in_run", sqlalchemy.Integer, nullable=False), + sqlalchemy.Column("command_id", sqlalchemy.String, nullable=False), + sqlalchemy.Column("command", sqlalchemy.String, nullable=False), + sqlalchemy.Column( + "command_intent", + sqlalchemy.String, + # nullable=True to match the underlying SQL, which is nullable because of a bug + # in the migration that introduced this column. This is not intended to ever be + # null in practice. + nullable=True, + ), + sqlalchemy.Column("command_error", sqlalchemy.String, nullable=True), + sqlalchemy.Column( + "command_status", + sqlalchemy.Enum( + CommandStatusSQLEnum, + values_callable=lambda obj: [e.value for e in obj], + validate_strings=True, + # nullable=True because it was easier for the migration to add the column + # this way. This is not intended to ever be null in practice. + nullable=True, + # todo(mm, 2024-11-20): We want create_constraint=True here. Something + # about the way we compare SQL in test_tables.py is making that difficult-- + # even when we correctly add the constraint in the migration, the SQL + # doesn't compare equal to what create_constraint=True here would emit. + create_constraint=False, + ), + ), + sqlalchemy.Index( + "ix_run_run_id_command_id", # An arbitrary name for the index. + "run_id", + "command_id", + unique=True, + ), + sqlalchemy.Index( + "ix_run_run_id_index_in_run", # An arbitrary name for the index. + "run_id", + "index_in_run", + unique=True, + ), + sqlalchemy.Index( + "ix_run_run_id_command_status_index_in_run", # An arbitrary name for the index. + "run_id", + "command_status", + "index_in_run", + unique=True, + ), +) + +data_files_table = sqlalchemy.Table( + "data_files", + metadata, + sqlalchemy.Column( + "id", + sqlalchemy.String, + primary_key=True, + ), + sqlalchemy.Column( + "name", + sqlalchemy.String, + nullable=False, + ), + sqlalchemy.Column( + "file_hash", + sqlalchemy.String, + nullable=False, + ), + sqlalchemy.Column( + "created_at", + UTCDateTime, + nullable=False, + ), + sqlalchemy.Column( + "source", + sqlalchemy.Enum( + DataFileSourceSQLEnum, + values_callable=lambda obj: [e.value for e in obj], + validate_strings=True, + # create_constraint=False to match the underlying SQL, which omits + # the constraint because of a bug in the migration that introduced this + # column. This is not intended to ever have values other than those in + # DataFileSourceSQLEnum. + create_constraint=False, + ), + # nullable=True to match the underlying SQL, which is nullable because of a bug + # in the migration that introduced this column. This is not intended to ever be + # null in practice. + nullable=True, + ), +) + +run_csv_rtp_table = sqlalchemy.Table( + "run_csv_rtp_table", + metadata, + sqlalchemy.Column( + "row_id", + sqlalchemy.Integer, + primary_key=True, + ), + sqlalchemy.Column( + "run_id", + sqlalchemy.ForeignKey("run.id"), + nullable=False, + ), + sqlalchemy.Column( + "parameter_variable_name", + sqlalchemy.String, + nullable=False, + ), + sqlalchemy.Column( + "file_id", + sqlalchemy.ForeignKey("data_files.id"), + nullable=True, + ), +) + + +class BooleanSettingKey(enum.Enum): + """Keys for boolean settings.""" + + ENABLE_ERROR_RECOVERY = "enable_error_recovery" + + +boolean_setting_table = sqlalchemy.Table( + "boolean_setting", + metadata, + sqlalchemy.Column( + "key", + sqlalchemy.Enum( + BooleanSettingKey, + values_callable=lambda obj: [e.value for e in obj], + validate_strings=True, + create_constraint=True, + ), + primary_key=True, + ), + sqlalchemy.Column( + "value", + sqlalchemy.Boolean, + nullable=False, + ), +) diff --git a/robot-server/robot_server/runs/router/base_router.py b/robot-server/robot_server/runs/router/base_router.py index 23153669d41..a57ed636647 100644 --- a/robot-server/robot_server/runs/router/base_router.py +++ b/robot-server/robot_server/runs/router/base_router.py @@ -527,14 +527,13 @@ async def get_run_commands_error( run_data_manager: Run data retrieval interface. """ try: - all_errors = run_data_manager.get_command_errors(run_id=runId) - total_length = len(all_errors) + all_errors_count = run_data_manager.get_command_errors_count(run_id=runId) if cursor is None: - if len(all_errors) > 0: + if all_errors_count > 0: # Get the most recent error, # which we can find just at the end of the list. - cursor = total_length - 1 + cursor = all_errors_count - 1 else: cursor = 0 diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index cbbcd022eb6..f5a06fa8172 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -15,7 +15,6 @@ CommandErrorSlice, CommandPointer, Command, - ErrorOccurrence, ) from opentrons.protocol_engine.types import ( PrimitiveRunTimeParamValuesType, @@ -368,18 +367,16 @@ async def update(self, run_id: str, current: Optional[bool]) -> Union[Run, BadRu next_current = current if current is False else True if next_current is False: - ( - commands, - state_summary, - parameters, - ) = await self._run_orchestrator_store.clear() + run_result = await self._run_orchestrator_store.clear() + state_summary = run_result.state_summary + parameters = run_result.parameters run_resource: Union[ RunResource, BadRunResource ] = self._run_store.update_run_state( run_id=run_id, - summary=state_summary, - commands=commands, - run_time_parameters=parameters, + summary=run_result.state_summary, + commands=run_result.commands, + run_time_parameters=run_result.parameters, ) self._runs_publisher.publish_pre_serialized_commands_notification(run_id) else: @@ -429,7 +426,7 @@ def get_commands_slice( def get_command_error_slice( self, run_id: str, cursor: int, length: int ) -> CommandErrorSlice: - """Get a slice of run commands. + """Get a slice of run commands errors. Args: run_id: ID of the run. @@ -443,9 +440,9 @@ def get_command_error_slice( return self._run_orchestrator_store.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() + return self._run_store.get_commands_errors_slice( + run_id=run_id, cursor=cursor, length=length + ) def get_current_command(self, run_id: str) -> Optional[CommandPointer]: """Get the "current" command, if any. @@ -504,13 +501,11 @@ def get_command(self, run_id: str, command_id: str) -> Command: return self._run_store.get_command(run_id=run_id, command_id=command_id) - def get_command_errors(self, run_id: str) -> list[ErrorOccurrence]: + def get_command_errors_count(self, run_id: str) -> int: """Get all command errors.""" if run_id == self._run_orchestrator_store.current_run_id: - return self._run_orchestrator_store.get_command_errors() - - # TODO(tz, 8-5-2024): Change this to return the error list from the DB when we implement https://opentrons.atlassian.net/browse/EXEC-655. - raise RunNotCurrentError() + return len(self._run_orchestrator_store.get_command_errors()) + return self._run_store.get_command_errors_count(run_id) def get_nozzle_maps(self, run_id: str) -> Mapping[str, NozzleMapInterface]: """Get current nozzle maps keyed by pipette id.""" diff --git a/robot-server/robot_server/runs/run_orchestrator_store.py b/robot-server/robot_server/runs/run_orchestrator_store.py index b4bd757150e..a8ad429db4a 100644 --- a/robot-server/robot_server/runs/run_orchestrator_store.py +++ b/robot-server/robot_server/runs/run_orchestrator_store.py @@ -293,7 +293,9 @@ async def clear(self) -> RunResult: self._run_orchestrator = None return RunResult( - state_summary=run_data, commands=commands, parameters=run_time_parameters + state_summary=run_data, + commands=commands, + parameters=run_time_parameters, ) # todo(mm, 2024-11-15): Are all of these pass-through methods helpful? diff --git a/robot-server/robot_server/runs/run_store.py b/robot-server/robot_server/runs/run_store.py index 6ab8665c454..0148f20058b 100644 --- a/robot-server/robot_server/runs/run_store.py +++ b/robot-server/robot_server/runs/run_store.py @@ -11,7 +11,14 @@ from pydantic import ValidationError from opentrons.util.helpers import utc_now -from opentrons.protocol_engine import StateSummary, CommandSlice, CommandIntent +from opentrons.protocol_engine import ( + StateSummary, + CommandSlice, + CommandIntent, + ErrorOccurrence, + CommandErrorSlice, + CommandStatus, +) from opentrons.protocol_engine.commands import Command from opentrons.protocol_engine.types import RunTimeParameter @@ -38,6 +45,7 @@ from .action_models import RunAction, RunActionType from .run_models import RunNotFoundError +from ..persistence.tables import CommandStatusSQLEnum log = logging.getLogger(__name__) @@ -179,6 +187,12 @@ def update_run_state( "command_intent": str(command.intent.value) if command.intent else CommandIntent.PROTOCOL, + "command_error": pydantic_to_json(command.error) + if command.error + else None, + "command_status": _convert_commands_status_to_sql_command_status( + command.status + ), }, ) @@ -537,6 +551,107 @@ def get_all_commands_as_preserialized_list( commands_result = transaction.scalars(select_commands).all() return commands_result + def get_command_errors_count(self, run_id: str) -> int: + """Get run commands errors count from the store. + + Args: + run_id: Run ID to pull commands from. + + Returns: + The number of commands errors. + + Raises: + RunNotFoundError: The given run ID was not found. + """ + with self._sql_engine.begin() as transaction: + if not self._run_exists(run_id, transaction): + raise RunNotFoundError(run_id=run_id) + + select_count = sqlalchemy.select(sqlalchemy.func.count()).where( + and_( + run_command_table.c.run_id == run_id, + run_command_table.c.command_status == CommandStatusSQLEnum.FAILED, + ) + ) + errors_count: int = transaction.execute(select_count).scalar_one() + return errors_count + + def get_commands_errors_slice( + self, + run_id: str, + length: int, + cursor: Optional[int], + ) -> CommandErrorSlice: + """Get a slice of run commands errors from the store. + + Args: + run_id: Run ID to pull commands from. + length: Number of commands to return. + cursor: The starting index of the slice in the whole collection. + If `None`, up to `length` elements at the end of the collection will + be returned. + + Returns: + A collection of command errors as well as the actual cursor used and + the total length of the collection. + + Raises: + RunNotFoundError: The given run ID was not found. + """ + with self._sql_engine.begin() as transaction: + if not self._run_exists(run_id, transaction): + raise RunNotFoundError(run_id=run_id) + + select_count = sqlalchemy.select(sqlalchemy.func.count()).where( + and_( + run_command_table.c.run_id == run_id, + run_command_table.c.command_status == CommandStatusSQLEnum.FAILED, + ) + ) + count_result: int = transaction.execute(select_count).scalar_one() + + actual_cursor = cursor if cursor is not None else count_result - length + # Clamp to [0, count_result). + # cursor is 0 based index and row number starts from 1. + actual_cursor = max(0, min(actual_cursor, count_result - 1)) + 1 + select_command_errors = ( + sqlalchemy.select( + sqlalchemy.func.row_number().over().label("row_num"), + run_command_table, + ) + .where( + and_( + run_command_table.c.run_id == run_id, + run_command_table.c.command_status + == CommandStatusSQLEnum.FAILED, + ) + ) + .order_by(run_command_table.c.index_in_run) + .subquery() + ) + + select_slice = ( + sqlalchemy.select(select_command_errors.c.command_error) + .where( + and_( + select_command_errors.c.row_num >= actual_cursor, + select_command_errors.c.row_num < actual_cursor + length, + ) + ) + .order_by(select_command_errors.c.index_in_run) + ) + slice_result = transaction.execute(select_slice).all() + + sliced_commands: List[ErrorOccurrence] = [ + json_to_pydantic(ErrorOccurrence, row.command_error) for row in slice_result + ] + + return CommandErrorSlice( + cursor=actual_cursor, + total_length=count_result, + commands_errors=sliced_commands, + ) + @lru_cache(maxsize=_CACHE_ENTRIES) def get_command(self, run_id: str, command_id: str) -> Command: """Get run command by id. @@ -712,3 +827,17 @@ def _convert_state_to_sql_values( "_updated_at": utc_now(), "run_time_parameters": pydantic_list_to_json(run_time_parameters), } + + +def _convert_commands_status_to_sql_command_status( + status: CommandStatus, +) -> CommandStatusSQLEnum: + match status: + case CommandStatus.QUEUED: + return CommandStatusSQLEnum.QUEUED + case CommandStatus.RUNNING: + return CommandStatusSQLEnum.RUNNING + case CommandStatus.FAILED: + return CommandStatusSQLEnum.FAILED + case CommandStatus.SUCCEEDED: + return CommandStatusSQLEnum.SUCCEEDED diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 642d2506e93..6363ed8f47f 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -18,6 +18,7 @@ schema_5, schema_6, schema_7, + schema_8, ) # The statements that we expect to emit when we create a fresh database. @@ -110,6 +111,8 @@ command_id VARCHAR NOT NULL, command VARCHAR NOT NULL, command_intent VARCHAR, + command_error VARCHAR, + command_status VARCHAR(9), PRIMARY KEY (row_id), FOREIGN KEY(run_id) REFERENCES run (id) ) @@ -121,6 +124,9 @@ CREATE UNIQUE INDEX ix_run_run_id_index_in_run ON run_command (run_id, index_in_run) """, """ + CREATE UNIQUE INDEX ix_run_run_id_command_status_index_in_run ON run_command (run_id, command_status, index_in_run) + """, + """ CREATE INDEX ix_protocol_protocol_kind ON protocol (protocol_kind) """, """ @@ -155,7 +161,130 @@ ] -EXPECTED_STATEMENTS_V7 = EXPECTED_STATEMENTS_LATEST +EXPECTED_STATEMENTS_V8 = EXPECTED_STATEMENTS_LATEST + + +EXPECTED_STATEMENTS_V7 = [ + """ + CREATE TABLE protocol ( + id VARCHAR NOT NULL, + created_at DATETIME NOT NULL, + protocol_key VARCHAR, + protocol_kind VARCHAR(14) NOT NULL, + PRIMARY KEY (id), + CONSTRAINT protocolkindsqlenum CHECK (protocol_kind IN ('standard', 'quick-transfer')) + ) + """, + """ + CREATE TABLE analysis ( + id VARCHAR NOT NULL, + protocol_id VARCHAR NOT NULL, + analyzer_version VARCHAR NOT NULL, + completed_analysis VARCHAR NOT NULL, + PRIMARY KEY (id), + FOREIGN KEY(protocol_id) REFERENCES protocol (id) + ) + """, + """ + CREATE TABLE analysis_primitive_rtp_table ( + row_id INTEGER NOT NULL, + analysis_id VARCHAR NOT NULL, + parameter_variable_name VARCHAR NOT NULL, + parameter_type VARCHAR(5) NOT NULL, + parameter_value VARCHAR NOT NULL, + PRIMARY KEY (row_id), + FOREIGN KEY(analysis_id) REFERENCES analysis (id), + CONSTRAINT primitiveparamsqlenum CHECK (parameter_type IN ('int', 'float', 'bool', 'str')) + ) + """, + """ + CREATE TABLE analysis_csv_rtp_table ( + row_id INTEGER NOT NULL, + analysis_id VARCHAR NOT NULL, + parameter_variable_name VARCHAR NOT NULL, + file_id VARCHAR, + PRIMARY KEY (row_id), + FOREIGN KEY(analysis_id) REFERENCES analysis (id), + FOREIGN KEY(file_id) REFERENCES data_files (id) + ) + """, + """ + CREATE INDEX ix_analysis_protocol_id ON analysis (protocol_id) + """, + """ + CREATE TABLE run ( + id VARCHAR NOT NULL, + created_at DATETIME NOT NULL, + protocol_id VARCHAR, + state_summary VARCHAR, + engine_status VARCHAR, + _updated_at DATETIME, + run_time_parameters VARCHAR, + PRIMARY KEY (id), + FOREIGN KEY(protocol_id) REFERENCES protocol (id) + ) + """, + """ + CREATE TABLE action ( + id VARCHAR NOT NULL, + created_at DATETIME NOT NULL, + action_type VARCHAR NOT NULL, + run_id VARCHAR NOT NULL, + PRIMARY KEY (id), + FOREIGN KEY(run_id) REFERENCES run (id) + ) + """, + """ + CREATE TABLE run_command ( + row_id INTEGER NOT NULL, + run_id VARCHAR NOT NULL, + index_in_run INTEGER NOT NULL, + command_id VARCHAR NOT NULL, + command VARCHAR NOT NULL, + command_intent VARCHAR, + PRIMARY KEY (row_id), + FOREIGN KEY(run_id) REFERENCES run (id) + ) + """, + """ + CREATE UNIQUE INDEX ix_run_run_id_command_id ON run_command (run_id, command_id) + """, + """ + CREATE UNIQUE INDEX ix_run_run_id_index_in_run ON run_command (run_id, index_in_run) + """, + """ + CREATE INDEX ix_protocol_protocol_kind ON protocol (protocol_kind) + """, + """ + CREATE TABLE data_files ( + id VARCHAR NOT NULL, + name VARCHAR NOT NULL, + file_hash VARCHAR NOT NULL, + created_at DATETIME NOT NULL, + source VARCHAR(9), + PRIMARY KEY (id) + ) + """, + """ + CREATE TABLE run_csv_rtp_table ( + row_id INTEGER NOT NULL, + run_id VARCHAR NOT NULL, + parameter_variable_name VARCHAR NOT NULL, + file_id VARCHAR, + PRIMARY KEY (row_id), + FOREIGN KEY(run_id) REFERENCES run (id), + FOREIGN KEY(file_id) REFERENCES data_files (id) + ) + """, + """ + CREATE TABLE boolean_setting ( + "key" VARCHAR(21) NOT NULL, + value BOOLEAN NOT NULL, + PRIMARY KEY ("key"), + CONSTRAINT booleansettingkey CHECK ("key" IN ('enable_error_recovery')) + ) + """, +] EXPECTED_STATEMENTS_V6 = [ @@ -554,6 +683,7 @@ def _normalize_statement(statement: str) -> str: ("metadata", "expected_statements"), [ (latest_metadata, EXPECTED_STATEMENTS_LATEST), + (schema_8.metadata, EXPECTED_STATEMENTS_V8), (schema_7.metadata, EXPECTED_STATEMENTS_V7), (schema_6.metadata, EXPECTED_STATEMENTS_V6), (schema_5.metadata, EXPECTED_STATEMENTS_V5), diff --git a/robot-server/tests/runs/router/test_base_router.py b/robot-server/tests/runs/router/test_base_router.py index aa27d37e66b..bb7f723138f 100644 --- a/robot-server/tests/runs/router/test_base_router.py +++ b/robot-server/tests/runs/router/test_base_router.py @@ -759,13 +759,7 @@ async def test_get_run_commands_errors( ) ).then_raise(RunNotCurrentError("oh no!")) - error = pe_errors.ErrorOccurrence( - id="error-id", - errorType="PrettyBadError", - createdAt=datetime(year=2024, month=4, day=4), - detail="Things are not looking good.", - ) - decoy.when(mock_run_data_manager.get_command_errors("run-id")).then_return([error]) + decoy.when(mock_run_data_manager.get_command_errors_count("run-id")).then_return(1) with pytest.raises(ApiError): result = await get_run_commands_error( @@ -787,7 +781,7 @@ async def test_get_run_commands_errors_raises_no_run( createdAt=datetime(year=2024, month=4, day=4), detail="Things are not looking good.", ) - decoy.when(mock_run_data_manager.get_command_errors("run-id")).then_return([error]) + decoy.when(mock_run_data_manager.get_command_errors_count("run-id")).then_return(1) command_error_slice = CommandErrorSlice( cursor=1, total_length=3, commands_errors=[error] @@ -831,10 +825,7 @@ async def test_get_run_commands_errors_defualt_cursor( expected_cursor_result: int, ) -> None: """It should return a list of all commands errors in a run.""" - print(error_list) - decoy.when(mock_run_data_manager.get_command_errors("run-id")).then_return( - error_list - ) + decoy.when(mock_run_data_manager.get_command_errors_count("run-id")).then_return(1) command_error_slice = CommandErrorSlice( cursor=expected_cursor_result, total_length=3, commands_errors=error_list diff --git a/robot-server/tests/runs/test_run_data_manager.py b/robot-server/tests/runs/test_run_data_manager.py index d27e1aebaff..a26baacadbf 100644 --- a/robot-server/tests/runs/test_run_data_manager.py +++ b/robot-server/tests/runs/test_run_data_manager.py @@ -935,16 +935,30 @@ def test_get_commands_slice_current_run( assert expected_command_slice == result -def test_get_commands_errors_slice__not_current_run_raises( +def test_get_commands_errors_slice_historical_run( decoy: Decoy, subject: RunDataManager, mock_run_orchestrator_store: RunOrchestratorStore, + mock_run_store: RunStore, ) -> None: """Should get a sliced command error list from engine store.""" + expected_commands_errors_result = [ + ErrorOccurrence.construct(id="error-id") # type: ignore[call-arg] + ] + + command_error_slice = CommandErrorSlice( + cursor=1, total_length=3, commands_errors=expected_commands_errors_result + ) + decoy.when(mock_run_orchestrator_store.current_run_id).then_return("run-not-id") - with pytest.raises(RunNotCurrentError): - subject.get_command_error_slice("run-id", 1, 2) + decoy.when(mock_run_store.get_commands_errors_slice("run-id", 2, 1)).then_return( + command_error_slice + ) + + result = subject.get_command_error_slice("run-id", 1, 2) + + assert command_error_slice == result def test_get_commands_errors_slice_current_run( diff --git a/robot-server/tests/runs/test_run_store.py b/robot-server/tests/runs/test_run_store.py index 17a5c3b252f..ab8e5f10fdf 100644 --- a/robot-server/tests/runs/test_run_store.py +++ b/robot-server/tests/runs/test_run_store.py @@ -36,6 +36,7 @@ CommandSlice, Liquid, EngineStatus, + ErrorOccurrence, ) from opentrons.types import MountType, DeckSlotName @@ -59,7 +60,7 @@ def subject( @pytest.fixture def protocol_commands() -> List[pe_commands.Command]: - """Get a StateSummary value object.""" + """Get protocol commands list.""" return [ pe_commands.WaitForResume( id="pause-1", @@ -99,6 +100,61 @@ def protocol_commands() -> List[pe_commands.Command]: ] +@pytest.fixture +def protocol_commands_errors() -> List[pe_commands.Command]: + """Get protocol commands errors list.""" + return [ + pe_commands.WaitForResume( + id="pause-4", + key="command-key", + status=pe_commands.CommandStatus.SUCCEEDED, + createdAt=datetime(year=2022, month=2, day=2), + params=pe_commands.WaitForResumeParams(message="hey world"), + result=pe_commands.WaitForResumeResult(), + intent=pe_commands.CommandIntent.PROTOCOL, + ), + pe_commands.WaitForResume( + id="pause-1", + key="command-key", + status=pe_commands.CommandStatus.FAILED, + createdAt=datetime(year=2021, month=1, day=1), + params=pe_commands.WaitForResumeParams(message="hello world"), + result=pe_commands.WaitForResumeResult(), + intent=pe_commands.CommandIntent.PROTOCOL, + error=ErrorOccurrence.construct( + id="error-id", + createdAt=datetime(2024, 1, 1), + errorType="blah-blah", + detail="test details", + ), + ), + pe_commands.WaitForResume( + id="pause-2", + key="command-key", + status=pe_commands.CommandStatus.FAILED, + createdAt=datetime(year=2022, month=2, day=2), + params=pe_commands.WaitForResumeParams(message="hey world"), + result=pe_commands.WaitForResumeResult(), + intent=pe_commands.CommandIntent.PROTOCOL, + error=ErrorOccurrence.construct( + id="error-id-2", + createdAt=datetime(2024, 1, 1), + errorType="blah-blah", + detail="test details", + ), + ), + pe_commands.WaitForResume( + id="pause-3", + key="command-key", + status=pe_commands.CommandStatus.SUCCEEDED, + createdAt=datetime(year=2022, month=2, day=2), + params=pe_commands.WaitForResumeParams(message="hey world"), + result=pe_commands.WaitForResumeResult(), + intent=pe_commands.CommandIntent.PROTOCOL, + ), + ] + + @pytest.fixture def state_summary() -> StateSummary: """Get a StateSummary test object.""" @@ -289,6 +345,50 @@ async def test_update_run_state( ) +async def test_update_run_state_command_with_errors( + subject: RunStore, + state_summary: StateSummary, + protocol_commands_errors: List[pe_commands.Command], + run_time_parameters: List[pe_types.RunTimeParameter], + mock_runs_publisher: mock.Mock, +) -> None: + """It should be able to update a run state to the store.""" + commands_with_errors = [ + command + for command in protocol_commands_errors + if command.status == pe_commands.CommandStatus.FAILED + ] + action = RunAction( + actionType=RunActionType.PLAY, + createdAt=datetime(year=2022, month=2, day=2, tzinfo=timezone.utc), + id="action-id", + ) + + subject.insert( + run_id="run-id", + protocol_id=None, + created_at=datetime(year=2021, month=1, day=1, tzinfo=timezone.utc), + ) + + subject.update_run_state( + run_id="run-id", + summary=state_summary, + commands=protocol_commands_errors, + run_time_parameters=run_time_parameters, + ) + + subject.insert_action(run_id="run-id", action=action) + command_errors_result = subject.get_commands_errors_slice( + run_id="run-id", + length=5, + cursor=0, + ) + + assert command_errors_result.commands_errors == [ + item.error for item in commands_with_errors + ] + + async def test_insert_and_get_csv_rtp( subject: RunStore, data_files_store: DataFilesStore,