From 05adc9af0f08862ce5c74b667505cddc8a21b0e8 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 10:49:03 -0400 Subject: [PATCH 1/8] Update SQL layer. --- .../persistence/tables/__init__.py | 2 ++ .../persistence/tables/schema_6.py | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/robot-server/robot_server/persistence/tables/__init__.py b/robot-server/robot_server/persistence/tables/__init__.py index 08efd08ff70..a4bcf487e2a 100644 --- a/robot-server/robot_server/persistence/tables/__init__.py +++ b/robot-server/robot_server/persistence/tables/__init__.py @@ -12,6 +12,7 @@ action_table, data_files_table, PrimitiveParamSQLEnum, + ProtocolKindSQLEnum, ) @@ -26,4 +27,5 @@ "action_table", "data_files_table", "PrimitiveParamSQLEnum", + "ProtocolKindSQLEnum", ] diff --git a/robot-server/robot_server/persistence/tables/schema_6.py b/robot-server/robot_server/persistence/tables/schema_6.py index 71122b76fe6..d875ac4ab78 100644 --- a/robot-server/robot_server/persistence/tables/schema_6.py +++ b/robot-server/robot_server/persistence/tables/schema_6.py @@ -16,6 +16,13 @@ class PrimitiveParamSQLEnum(enum.Enum): STR = "str" +class ProtocolKindSQLEnum(enum.Enum): + """What kind a stored protocol is.""" + + STANDARD = "standard" + QUICK_TRANSFER = "quick-transfer" + + protocol_table = sqlalchemy.Table( "protocol", metadata, @@ -30,7 +37,16 @@ class PrimitiveParamSQLEnum(enum.Enum): nullable=False, ), sqlalchemy.Column("protocol_key", sqlalchemy.String, nullable=True), - sqlalchemy.Column("protocol_kind", sqlalchemy.String, nullable=True), + sqlalchemy.Column( + "protocol_kind", + sqlalchemy.Enum( + ProtocolKindSQLEnum, + values_callable=lambda obj: [e.value for e in obj], + create_constraint=True, + ), + index=True, + nullable=False, + ), ) analysis_table = sqlalchemy.Table( From 84cbf3ef72bf97535b4f6370509f2cfb2862b1a9 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 10:54:30 -0400 Subject: [PATCH 2/8] Update migration. --- .../persistence/_migrations/v5_to_v6.py | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/robot-server/robot_server/persistence/_migrations/v5_to_v6.py b/robot-server/robot_server/persistence/_migrations/v5_to_v6.py index 546226ae99a..386e4e963a6 100644 --- a/robot-server/robot_server/persistence/_migrations/v5_to_v6.py +++ b/robot-server/robot_server/persistence/_migrations/v5_to_v6.py @@ -13,6 +13,8 @@ will still be available as part of the completed analysis blob. - Adds a new analysis_csv_rtp_table to store the CSV parameters' file IDs used in analysis - Adds a new run_csv_rtp_table to store the CSV parameters' file IDs used in runs +- Converts protocol.protocol_kind to a constrained string (a SQL "enum"), makes it + non-nullable (NULL was semantically equivalent to "standard"), and adds an index. """ from pathlib import Path @@ -63,12 +65,9 @@ def _migrate_db_with_changes( source_transaction: sqlalchemy.engine.Connection, dest_transaction: sqlalchemy.engine.Connection, ) -> None: - copy_rows_unmodified( - schema_5.protocol_table, - schema_6.protocol_table, + _migrate_protocol_table_with_new_protocol_kind_col( source_transaction, dest_transaction, - order_by_rowid=True, ) _migrate_analysis_table_excluding_rtp_defaults_and_vals( source_transaction, @@ -97,6 +96,31 @@ def _migrate_db_with_changes( ) +def _migrate_protocol_table_with_new_protocol_kind_col( + source_transaction: sqlalchemy.engine.Connection, + dest_transaction: sqlalchemy.engine.Connection, +) -> None: + """Add a new 'protocol_kind' column to protocols table.""" + select_old_protocols = sqlalchemy.select(schema_5.protocol_table).order_by( + sqlite_rowid + ) + insert_new_protocol = sqlalchemy.insert(schema_6.protocol_table) + for old_row in source_transaction.execute(select_old_protocols).all(): + new_protocol_kind = ( + # Account for old_row.protocol_kind being NULL. + schema_6.ProtocolKindSQLEnum.QUICK_TRANSFER + if old_row.protocol_kind == "quick-transfer" + else schema_6.ProtocolKindSQLEnum.STANDARD + ) + dest_transaction.execute( + insert_new_protocol, + id=old_row.id, + created_at=old_row.created_at, + protocol_key=old_row.protocol_key, + protocol_kind=new_protocol_kind, + ) + + def _migrate_analysis_table_excluding_rtp_defaults_and_vals( source_transaction: sqlalchemy.engine.Connection, dest_transaction: sqlalchemy.engine.Connection, From 005a8ab84459b0f79ba0825fc93cac7e15804fc3 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 17:27:44 -0400 Subject: [PATCH 3/8] Update store and router. --- .../robot_server/protocols/protocol_models.py | 8 ----- .../robot_server/protocols/protocol_store.py | 34 ++++++++++++++----- robot-server/robot_server/protocols/router.py | 19 +++++------ .../tests/protocols/test_analyses_manager.py | 6 ++-- .../tests/protocols/test_analysis_store.py | 3 +- .../test_completed_analysis_store.py | 3 +- .../tests/protocols/test_protocol_analyzer.py | 6 ++-- .../protocols/test_protocol_auto_deleter.py | 6 ++-- .../tests/protocols/test_protocol_store.py | 31 +++++++++-------- .../tests/protocols/test_protocols_router.py | 32 ++++++++--------- .../tests/runs/router/test_base_router.py | 3 +- .../tests/runs/test_run_data_manager.py | 4 ++- 12 files changed, 85 insertions(+), 70 deletions(-) diff --git a/robot-server/robot_server/protocols/protocol_models.py b/robot-server/robot_server/protocols/protocol_models.py index decef1e8e26..c7841f25776 100644 --- a/robot-server/robot_server/protocols/protocol_models.py +++ b/robot-server/robot_server/protocols/protocol_models.py @@ -22,14 +22,6 @@ class ProtocolKind(str, Enum): STANDARD = "standard" QUICK_TRANSFER = "quick-transfer" - @staticmethod - def from_string(name: Optional[str]) -> Optional["ProtocolKind"]: - """Get the ProtocolKind from a string.""" - for item in ProtocolKind: - if name == item.value: - return item - return None - class ProtocolFile(BaseModel): """A file in a protocol.""" diff --git a/robot-server/robot_server/protocols/protocol_store.py b/robot-server/robot_server/protocols/protocol_store.py index 3c4ed9b7e3e..0488a958a12 100644 --- a/robot-server/robot_server/protocols/protocol_store.py +++ b/robot-server/robot_server/protocols/protocol_store.py @@ -24,7 +24,9 @@ analysis_primitive_type_rtp_table, analysis_csv_rtp_table, data_files_table, + ProtocolKindSQLEnum, ) +from robot_server.protocols.protocol_models import ProtocolKind _CACHE_ENTRIES = 32 @@ -41,7 +43,7 @@ class ProtocolResource: created_at: datetime source: ProtocolSource protocol_key: Optional[str] - protocol_kind: Optional[str] + protocol_kind: ProtocolKind @dataclass(frozen=True) @@ -173,7 +175,7 @@ def insert(self, resource: ProtocolResource) -> None: protocol_id=resource.protocol_id, created_at=resource.created_at, protocol_key=resource.protocol_key, - protocol_kind=resource.protocol_kind, + protocol_kind=_http_protocol_kind_to_sql(resource.protocol_kind), ) ) self._sources_by_id[resource.protocol_id] = resource.source @@ -191,7 +193,7 @@ def get(self, protocol_id: str) -> ProtocolResource: protocol_id=sql_resource.protocol_id, created_at=sql_resource.created_at, protocol_key=sql_resource.protocol_key, - protocol_kind=sql_resource.protocol_kind, + protocol_kind=_sql_protocol_kind_to_http(sql_resource.protocol_kind), source=self._sources_by_id[sql_resource.protocol_id], ) @@ -207,7 +209,7 @@ def get_all(self) -> List[ProtocolResource]: protocol_id=r.protocol_id, created_at=r.created_at, protocol_key=r.protocol_key, - protocol_kind=r.protocol_kind, + protocol_kind=_sql_protocol_kind_to_http(r.protocol_kind), source=self._sources_by_id[r.protocol_id], ) for r in all_sql_resources @@ -525,7 +527,7 @@ class _DBProtocolResource: protocol_id: str created_at: datetime protocol_key: Optional[str] - protocol_kind: Optional[str] + protocol_kind: ProtocolKindSQLEnum def _convert_sql_row_to_dataclass( @@ -540,9 +542,9 @@ def _convert_sql_row_to_dataclass( assert protocol_key is None or isinstance( protocol_key, str ), f"Protocol Key {protocol_key} not a string or None" - assert protocol_kind is None or isinstance( - protocol_kind, str - ), f"Protocol Kind {protocol_kind} not a string or None" + assert isinstance( + protocol_kind, ProtocolKindSQLEnum + ), f"Protocol Kind {protocol_kind} not the expected enum" return _DBProtocolResource( protocol_id=protocol_id, @@ -561,3 +563,19 @@ def _convert_dataclass_to_sql_values( "protocol_key": resource.protocol_key, "protocol_kind": resource.protocol_kind, } + + +def _http_protocol_kind_to_sql(http_enum: ProtocolKind) -> ProtocolKindSQLEnum: + match http_enum: + case ProtocolKind.STANDARD: + return ProtocolKindSQLEnum.STANDARD + case ProtocolKind.QUICK_TRANSFER: + return ProtocolKindSQLEnum.QUICK_TRANSFER + + +def _sql_protocol_kind_to_http(sql_enum: ProtocolKindSQLEnum) -> ProtocolKind: + match sql_enum: + case ProtocolKindSQLEnum.STANDARD: + return ProtocolKind.STANDARD + case ProtocolKindSQLEnum.QUICK_TRANSFER: + return ProtocolKind.QUICK_TRANSFER diff --git a/robot-server/robot_server/protocols/router.py b/robot-server/robot_server/protocols/router.py index 5918e2a2db7..78258a9d127 100644 --- a/robot-server/robot_server/protocols/router.py +++ b/robot-server/robot_server/protocols/router.py @@ -219,8 +219,8 @@ async def create_protocol( # noqa: C901 " always trigger an analysis (for now).", alias="runTimeParameterValues", ), - protocol_kind: Optional[ProtocolKind] = Form( - default=None, + protocol_kind: ProtocolKind = Form( + default=ProtocolKind.STANDARD, description=( "Whether this is a `standard` protocol or a `quick-transfer` protocol." "if omitted, the protocol will be `standard` by default." @@ -277,8 +277,7 @@ async def create_protocol( # noqa: C901 created_at: Timestamp to attach to the new resource. maximum_quick_transfer_protocols: Robot setting value limiting stored quick transfers protocols. """ - kind = ProtocolKind.from_string(protocol_kind) or ProtocolKind.STANDARD - if kind == ProtocolKind.QUICK_TRANSFER: + if protocol_kind == ProtocolKind.QUICK_TRANSFER: quick_transfer_protocols = [ protocol for protocol in protocol_store.get_all() @@ -340,7 +339,7 @@ async def _get_cached_protocol_analysis() -> PydanticResponse[ data = Protocol.construct( id=cached_protocol_id, createdAt=resource.created_at, - protocolKind=ProtocolKind.from_string(resource.protocol_kind), + protocolKind=resource.protocol_kind, protocolType=resource.source.config.protocol_type, robotType=resource.source.robot_type, metadata=Metadata.parse_obj(resource.source.metadata), @@ -390,11 +389,11 @@ async def _get_cached_protocol_analysis() -> PydanticResponse[ created_at=created_at, source=source, protocol_key=key, - protocol_kind=kind.value, + protocol_kind=protocol_kind, ) protocol_deleter: ProtocolAutoDeleter = protocol_auto_deleter - if kind == ProtocolKind.QUICK_TRANSFER: + if protocol_kind == ProtocolKind.QUICK_TRANSFER: protocol_deleter = quick_transfer_protocol_auto_deleter protocol_deleter.make_room_for_new_protocol() protocol_store.insert(protocol_resource) @@ -413,7 +412,7 @@ async def _get_cached_protocol_analysis() -> PydanticResponse[ data = Protocol( id=protocol_id, createdAt=created_at, - protocolKind=kind, + protocolKind=protocol_kind, protocolType=source.config.protocol_type, robotType=source.robot_type, metadata=Metadata.parse_obj(source.metadata), @@ -523,7 +522,7 @@ async def get_protocols( Protocol.construct( id=r.protocol_id, createdAt=r.created_at, - protocolKind=ProtocolKind.from_string(r.protocol_kind), + protocolKind=r.protocol_kind, protocolType=r.source.config.protocol_type, robotType=r.source.robot_type, metadata=Metadata.parse_obj(r.source.metadata), @@ -604,7 +603,7 @@ async def get_protocol_by_id( data = Protocol.construct( id=protocolId, createdAt=resource.created_at, - protocolKind=ProtocolKind.from_string(resource.protocol_kind), + protocolKind=resource.protocol_kind, protocolType=resource.source.config.protocol_type, robotType=resource.source.robot_type, metadata=Metadata.parse_obj(resource.source.metadata), diff --git a/robot-server/tests/protocols/test_analyses_manager.py b/robot-server/tests/protocols/test_analyses_manager.py index 17baa330bf5..46c46a5243f 100644 --- a/robot-server/tests/protocols/test_analyses_manager.py +++ b/robot-server/tests/protocols/test_analyses_manager.py @@ -83,7 +83,7 @@ async def test_initialize_analyzer( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) analyzer = decoy.mock(cls=protocol_analyzer.ProtocolAnalyzer) decoy.when( @@ -128,7 +128,7 @@ async def test_raises_error_and_saves_result_if_initialization_errors( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) raised_exception = Exception("Oh noooo!") enumerated_error = EnumeratedError( @@ -197,7 +197,7 @@ async def test_start_analysis( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) bool_parameter = BooleanParameter( displayName="Foo", variableName="Bar", default=True, value=False diff --git a/robot-server/tests/protocols/test_analysis_store.py b/robot-server/tests/protocols/test_analysis_store.py index e3bdb246617..7a3d979be44 100644 --- a/robot-server/tests/protocols/test_analysis_store.py +++ b/robot-server/tests/protocols/test_analysis_store.py @@ -50,6 +50,7 @@ CompletedAnalysisStore, CompletedAnalysisResource, ) +from robot_server.protocols.protocol_models import ProtocolKind from robot_server.protocols.protocol_store import ( ProtocolStore, ProtocolResource, @@ -96,7 +97,7 @@ def make_dummy_protocol_resource(protocol_id: str) -> ProtocolResource: content_hash="abc123", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) diff --git a/robot-server/tests/protocols/test_completed_analysis_store.py b/robot-server/tests/protocols/test_completed_analysis_store.py index fe5a05c62f0..3f1e5302bdf 100644 --- a/robot-server/tests/protocols/test_completed_analysis_store.py +++ b/robot-server/tests/protocols/test_completed_analysis_store.py @@ -29,6 +29,7 @@ AnalysisStatus, RunTimeParameterAnalysisData, ) +from robot_server.protocols.protocol_models import ProtocolKind from robot_server.protocols.protocol_store import ( ProtocolStore, ProtocolResource, @@ -95,7 +96,7 @@ def make_dummy_protocol_resource(protocol_id: str) -> ProtocolResource: content_hash="abc123", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) diff --git a/robot-server/tests/protocols/test_protocol_analyzer.py b/robot-server/tests/protocols/test_protocol_analyzer.py index 8b55f05840b..1e9c004999a 100644 --- a/robot-server/tests/protocols/test_protocol_analyzer.py +++ b/robot-server/tests/protocols/test_protocol_analyzer.py @@ -87,7 +87,7 @@ async def test_load_orchestrator( created_at=datetime(year=2021, month=1, day=1), source=protocol_source, protocol_key="dummy-data-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) subject = ProtocolAnalyzer( analysis_store=analysis_store, protocol_resource=protocol_resource @@ -136,7 +136,7 @@ async def test_analyze( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) analysis_command = pe_commands.WaitForResume( @@ -232,7 +232,7 @@ async def test_analyze_updates_pending_on_error( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) raised_exception = Exception("You got me!!") diff --git a/robot-server/tests/protocols/test_protocol_auto_deleter.py b/robot-server/tests/protocols/test_protocol_auto_deleter.py index 1969f64cf61..840e9e8e31a 100644 --- a/robot-server/tests/protocols/test_protocol_auto_deleter.py +++ b/robot-server/tests/protocols/test_protocol_auto_deleter.py @@ -46,7 +46,7 @@ def test_make_room_for_new_protocol( created_at=datetime(year=2020, month=1, day=1), source=mock_protocol_source, protocol_key=f"{p.protocol_id}{idx}", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) for idx, p in enumerate(usage_info) ] @@ -96,9 +96,9 @@ def test_make_room_for_new_quick_transfer_protocol( created_at=datetime(year=2020, month=1, day=1), source=mock_protocol_source, protocol_key=f"{p.protocol_id}{idx}", - protocol_kind=ProtocolKind.STANDARD.value + protocol_kind=ProtocolKind.STANDARD if idx not in [3, 4] - else ProtocolKind.QUICK_TRANSFER.value, + else ProtocolKind.QUICK_TRANSFER, ) for idx, p in enumerate(usage_info_all) ] diff --git a/robot-server/tests/protocols/test_protocol_store.py b/robot-server/tests/protocols/test_protocol_store.py index 911219c5ec9..6550db63371 100644 --- a/robot-server/tests/protocols/test_protocol_store.py +++ b/robot-server/tests/protocols/test_protocol_store.py @@ -25,6 +25,7 @@ CompletedAnalysisResource, CompletedAnalysisStore, ) +from robot_server.protocols.protocol_models import ProtocolKind from robot_server.protocols.protocol_store import ( ProtocolStore, ProtocolResource, @@ -98,7 +99,7 @@ async def test_insert_and_get_protocol( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) assert subject.has("protocol-id") is False @@ -127,7 +128,7 @@ async def test_insert_with_duplicate_key_raises( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) protocol_resource_2 = ProtocolResource( protocol_id="protocol-id", @@ -142,7 +143,7 @@ async def test_insert_with_duplicate_key_raises( content_hash="abc123", ), protocol_key="dummy-data-222", - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) subject.insert(protocol_resource_1) @@ -180,7 +181,7 @@ async def test_get_all_protocols( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) resource_2 = ProtocolResource( protocol_id="123", @@ -195,7 +196,7 @@ async def test_get_all_protocols( content_hash="abc123", ), protocol_key="dummy-data-222", - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) subject.insert(resource_1) @@ -232,7 +233,7 @@ async def test_remove_protocol( content_hash="abc123", ), protocol_key="dummy-data-111", - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) subject.insert(protocol_resource) @@ -272,7 +273,7 @@ def test_remove_protocol_conflict( content_hash="abc123", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) subject.insert(protocol_resource) @@ -307,7 +308,7 @@ def test_get_usage_info( content_hash="abc123", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) protocol_resource_2 = ProtocolResource( protocol_id="protocol-id-2", @@ -322,7 +323,7 @@ def test_get_usage_info( content_hash="abc123", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) subject.insert(protocol_resource_1) @@ -392,7 +393,7 @@ def test_get_referencing_run_ids( content_hash="abc123", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) subject.insert(protocol_resource_1) @@ -436,7 +437,7 @@ def test_get_protocol_ids( content_hash="abc1", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) protocol_resource_2 = ProtocolResource( @@ -452,7 +453,7 @@ def test_get_protocol_ids( content_hash="abc2", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) assert subject.get_all_ids() == [] @@ -487,7 +488,7 @@ async def test_insert_and_get_quick_transfer_protocol( content_hash="abc123", ), protocol_key="dummy-key-111", - protocol_kind="quick-transfer", + protocol_kind=ProtocolKind.QUICK_TRANSFER, ) assert subject.has("protocol-id") is False @@ -496,7 +497,7 @@ async def test_insert_and_get_quick_transfer_protocol( result = subject.get("protocol-id") assert result == protocol_resource - assert result.protocol_kind == "quick-transfer" + assert result.protocol_kind == ProtocolKind.QUICK_TRANSFER assert subject.has("protocol-id") is True @@ -542,7 +543,7 @@ async def test_get_referenced_data_files( content_hash="abc1", ), protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) analysis_resource1 = CompletedAnalysisResource( "analysis-id-1", diff --git a/robot-server/tests/protocols/test_protocols_router.py b/robot-server/tests/protocols/test_protocols_router.py index 1c4d2ae672f..e1e9968b232 100644 --- a/robot-server/tests/protocols/test_protocols_router.py +++ b/robot-server/tests/protocols/test_protocols_router.py @@ -157,7 +157,7 @@ async def test_get_protocols( content_hash="a_b_c", ), protocol_key="dummy-key-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) resource_2 = ProtocolResource( protocol_id="123", @@ -172,7 +172,7 @@ async def test_get_protocols( content_hash="1_2_3", ), protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) resource_3 = ProtocolResource( protocol_id="333", @@ -187,7 +187,7 @@ async def test_get_protocols( content_hash="3_3_3", ), protocol_key="dummy-key-333", - protocol_kind=ProtocolKind.QUICK_TRANSFER.value, + protocol_kind=ProtocolKind.QUICK_TRANSFER, ) analysis_1 = AnalysisSummary(id="analysis-id-abc", status=AnalysisStatus.PENDING) @@ -330,7 +330,7 @@ async def test_get_protocol_by_id( content_hash="a_b_c", ), protocol_key="dummy-key-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) analysis_summary = AnalysisSummary( @@ -426,7 +426,7 @@ async def test_create_existing_protocol( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) completed_analysis = AnalysisSummary( @@ -537,7 +537,7 @@ async def test_create_protocol( created_at=datetime(year=2021, month=1, day=1), source=protocol_source, protocol_key="dummy-key-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) pending_analysis = AnalysisSummary( @@ -659,7 +659,7 @@ async def test_create_new_protocol_with_run_time_params( created_at=datetime(year=2021, month=1, day=1), source=protocol_source, protocol_key="dummy-key-111", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) run_time_parameter = NumberParameter( displayName="My parameter", @@ -777,7 +777,7 @@ async def test_create_existing_protocol_with_no_previous_analysis( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) run_time_parameter = NumberParameter( displayName="My parameter", @@ -899,7 +899,7 @@ async def test_create_existing_protocol_with_different_run_time_params( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) completed_summary = AnalysisSummary( @@ -1033,7 +1033,7 @@ async def test_create_existing_protocol_with_same_run_time_params( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) run_time_parameter = NumberParameter( displayName="My parameter", @@ -1158,7 +1158,7 @@ async def test_create_existing_protocol_with_pending_analysis_raises( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) run_time_parameter = NumberParameter( displayName="My parameter", @@ -1603,7 +1603,7 @@ async def test_create_protocol_analyses_with_same_rtp_values( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) decoy.when(protocol_store.has(protocol_id="protocol-id")).then_return(True) decoy.when(protocol_store.get(protocol_id="protocol-id")).then_return( @@ -1679,7 +1679,7 @@ async def test_update_protocol_analyses_with_new_rtp_values( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) analysis_summaries = [ AnalysisSummary( @@ -1794,7 +1794,7 @@ async def test_update_protocol_analyses_with_forced_reanalysis( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.STANDARD.value, + protocol_kind=ProtocolKind.STANDARD, ) decoy.when(protocol_store.has(protocol_id="protocol-id")).then_return(True) decoy.when( @@ -1872,7 +1872,7 @@ async def test_create_protocol_kind_quick_transfer( created_at=datetime(year=2021, month=1, day=1), source=protocol_source, protocol_key="dummy-key-111", - protocol_kind=ProtocolKind.QUICK_TRANSFER.value, + protocol_kind=ProtocolKind.QUICK_TRANSFER, ) run_time_parameter = NumberParameter( displayName="My parameter", @@ -2000,7 +2000,7 @@ async def test_create_protocol_maximum_quick_transfer_protocols_exceeded( created_at=datetime(year=2020, month=1, day=1), source=protocol_source, protocol_key="dummy-key-222", - protocol_kind=ProtocolKind.QUICK_TRANSFER.value, + protocol_kind=ProtocolKind.QUICK_TRANSFER, ) decoy.when(protocol_store.get_all()).then_return([stored_protocol_resource]) diff --git a/robot-server/tests/runs/router/test_base_router.py b/robot-server/tests/runs/router/test_base_router.py index f03a256eb51..5f770de5ae9 100644 --- a/robot-server/tests/runs/router/test_base_router.py +++ b/robot-server/tests/runs/router/test_base_router.py @@ -18,6 +18,7 @@ ResourceLink, ) +from robot_server.protocols.protocol_models import ProtocolKind from robot_server.protocols.protocol_store import ( ProtocolNotFoundError, ProtocolResource, @@ -132,7 +133,7 @@ async def test_create_protocol_run( protocol_resource = ProtocolResource( protocol_id=protocol_id, protocol_key=None, - protocol_kind=None, + protocol_kind=ProtocolKind.STANDARD, created_at=datetime(year=2022, month=2, day=2), source=ProtocolSource( directory=Path("/dev/null"), diff --git a/robot-server/tests/runs/test_run_data_manager.py b/robot-server/tests/runs/test_run_data_manager.py index ec7a8bef69e..63716a8ebd5 100644 --- a/robot-server/tests/runs/test_run_data_manager.py +++ b/robot-server/tests/runs/test_run_data_manager.py @@ -24,6 +24,8 @@ from opentrons_shared_data.errors.exceptions import InvalidStoredData from opentrons_shared_data.labware.labware_definition import LabwareDefinition + +from robot_server.protocols.protocol_models import ProtocolKind from robot_server.protocols.protocol_store import ProtocolResource from robot_server.runs import error_recovery_mapping from robot_server.runs.error_recovery_models import ErrorRecoveryRule @@ -218,7 +220,7 @@ async def test_create_with_options( created_at=datetime(year=2022, month=2, day=2), source=None, # type: ignore[arg-type] protocol_key=None, - protocol_kind="standard", + protocol_kind=ProtocolKind.STANDARD, ) labware_offset = pe_types.LabwareOffsetCreate( From c66e65b82f9108686e4b53494fccb0d732ff0414 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 17:27:47 -0400 Subject: [PATCH 4/8] Work around FastAPI weirdness. --- robot-server/robot_server/protocols/router.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/robot-server/robot_server/protocols/router.py b/robot-server/robot_server/protocols/router.py index 78258a9d127..8c23161bd41 100644 --- a/robot-server/robot_server/protocols/router.py +++ b/robot-server/robot_server/protocols/router.py @@ -220,6 +220,8 @@ async def create_protocol( # noqa: C901 alias="runTimeParameterValues", ), protocol_kind: ProtocolKind = Form( + # This default needs to be kept in sync with the function body. + # See todo comments. default=ProtocolKind.STANDARD, description=( "Whether this is a `standard` protocol or a `quick-transfer` protocol." @@ -293,11 +295,12 @@ async def create_protocol( # noqa: C901 assert file.filename is not None buffered_files = await file_reader_writer.read(files=files) # type: ignore[arg-type] - # We have to do this isinstance check because if `runTimeParameterValues` is - # not specified in the request, then it gets assigned a Form(None) value - # instead of just a None. \(O.o)/ + # We have to do these isinstance checks because if `runTimeParameterValues` or + # `protocolKind` are not specified in the request, then they get assigned a + # Form(default) value instead of just the default value. \(O.o)/ # TODO: check if we can make our own "RTP multipart-form field" Pydantic type # so we can validate the data contents and return a better error response. + # TODO: check if this is still necessary after converting FastAPI args to Annotated. parsed_rtp_values = ( json.loads(run_time_parameter_values) if isinstance(run_time_parameter_values, str) @@ -308,6 +311,9 @@ async def create_protocol( # noqa: C901 if isinstance(run_time_parameter_files, str) else {} ) + if not isinstance(protocol_kind, ProtocolKind): + protocol_kind = ProtocolKind.STANDARD + content_hash = await file_hasher.hash(buffered_files) cached_protocol_id = protocol_store.get_id_by_hash(content_hash) From 6b964561d17e67aac24f77b68a0c98efd45cd874 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 12:13:12 -0400 Subject: [PATCH 5/8] Update table snapshot tests. --- robot-server/tests/persistence/test_tables.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/robot-server/tests/persistence/test_tables.py b/robot-server/tests/persistence/test_tables.py index 4dc121e76ed..5b38cf1c616 100644 --- a/robot-server/tests/persistence/test_tables.py +++ b/robot-server/tests/persistence/test_tables.py @@ -33,8 +33,9 @@ id VARCHAR NOT NULL, created_at DATETIME NOT NULL, protocol_key VARCHAR, - protocol_kind VARCHAR, - PRIMARY KEY (id) + protocol_kind VARCHAR(14) NOT NULL, + PRIMARY KEY (id), + CONSTRAINT protocolkindsqlenum CHECK (protocol_kind IN ('standard', 'quick-transfer')) ) """, """ @@ -114,6 +115,9 @@ 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, From f4e420730f4b39f91e163785147daf8b505fe303 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 12:58:22 -0400 Subject: [PATCH 6/8] Remove unnecessary (but harmless) `.value`s. --- robot-server/robot_server/protocols/protocol_auto_deleter.py | 2 +- robot-server/robot_server/protocols/router.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/robot-server/robot_server/protocols/protocol_auto_deleter.py b/robot-server/robot_server/protocols/protocol_auto_deleter.py index b872cab7379..c028a0c82db 100644 --- a/robot-server/robot_server/protocols/protocol_auto_deleter.py +++ b/robot-server/robot_server/protocols/protocol_auto_deleter.py @@ -27,7 +27,7 @@ def make_room_for_new_protocol(self) -> None: protocols = { p.protocol_id for p in self._protocol_store.get_all() - if p.protocol_kind == self._protocol_kind.value + if p.protocol_kind == self._protocol_kind } protocol_run_usage_info = [ diff --git a/robot-server/robot_server/protocols/router.py b/robot-server/robot_server/protocols/router.py index 8c23161bd41..20a39018ac1 100644 --- a/robot-server/robot_server/protocols/router.py +++ b/robot-server/robot_server/protocols/router.py @@ -283,7 +283,7 @@ async def create_protocol( # noqa: C901 quick_transfer_protocols = [ protocol for protocol in protocol_store.get_all() - if protocol.protocol_kind == ProtocolKind.QUICK_TRANSFER.value + if protocol.protocol_kind == ProtocolKind.QUICK_TRANSFER ] if len(quick_transfer_protocols) >= maximum_quick_transfer_protocols: raise HTTPException( From cdef865d482f717c6dcab4b35787f05ecde1e565 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 17:43:26 -0400 Subject: [PATCH 7/8] Move Form normalization to before we look at protocol_kind. --- robot-server/robot_server/protocols/router.py | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/robot-server/robot_server/protocols/router.py b/robot-server/robot_server/protocols/router.py index 20a39018ac1..1e50b6a13a9 100644 --- a/robot-server/robot_server/protocols/router.py +++ b/robot-server/robot_server/protocols/router.py @@ -279,22 +279,6 @@ async def create_protocol( # noqa: C901 created_at: Timestamp to attach to the new resource. maximum_quick_transfer_protocols: Robot setting value limiting stored quick transfers protocols. """ - if protocol_kind == ProtocolKind.QUICK_TRANSFER: - quick_transfer_protocols = [ - protocol - for protocol in protocol_store.get_all() - if protocol.protocol_kind == ProtocolKind.QUICK_TRANSFER - ] - if len(quick_transfer_protocols) >= maximum_quick_transfer_protocols: - raise HTTPException( - status_code=409, detail="Maximum quick transfer protocols exceeded" - ) - - for file in files: - # TODO(mm, 2024-02-07): Investigate whether the filename can actually be None. - assert file.filename is not None - buffered_files = await file_reader_writer.read(files=files) # type: ignore[arg-type] - # We have to do these isinstance checks because if `runTimeParameterValues` or # `protocolKind` are not specified in the request, then they get assigned a # Form(default) value instead of just the default value. \(O.o)/ @@ -314,6 +298,22 @@ async def create_protocol( # noqa: C901 if not isinstance(protocol_kind, ProtocolKind): protocol_kind = ProtocolKind.STANDARD + if protocol_kind == ProtocolKind.QUICK_TRANSFER: + quick_transfer_protocols = [ + protocol + for protocol in protocol_store.get_all() + if protocol.protocol_kind == ProtocolKind.QUICK_TRANSFER + ] + if len(quick_transfer_protocols) >= maximum_quick_transfer_protocols: + raise HTTPException( + status_code=409, detail="Maximum quick transfer protocols exceeded" + ) + + for file in files: + # TODO(mm, 2024-02-07): Investigate whether the filename can actually be None. + assert file.filename is not None + buffered_files = await file_reader_writer.read(files=files) # type: ignore[arg-type] + content_hash = await file_hasher.hash(buffered_files) cached_protocol_id = protocol_store.get_id_by_hash(content_hash) From d9d649560dd99aa1d694acd3fa7d08caf299ad72 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Mon, 29 Jul 2024 17:53:10 -0400 Subject: [PATCH 8/8] Missed a .value. --- robot-server/robot_server/runs/run_auto_deleter.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/robot-server/robot_server/runs/run_auto_deleter.py b/robot-server/robot_server/runs/run_auto_deleter.py index f4e9485f691..6760e715923 100644 --- a/robot-server/robot_server/runs/run_auto_deleter.py +++ b/robot-server/robot_server/runs/run_auto_deleter.py @@ -29,9 +29,7 @@ def make_room_for_new_run(self) -> None: # noqa: D102 protocols = self._protocol_store.get_all() protocol_ids = [p.protocol_id for p in protocols] filtered_protocol_ids = [ - p.protocol_id - for p in protocols - if p.protocol_kind == self._protocol_kind.value + p.protocol_id for p in protocols if p.protocol_kind == self._protocol_kind ] # runs with no protocols first, then oldest to newest.