refactor(robot-server): move run time params to their own DB table #15650
Conversation
…erification process and database table
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
| sqlalchemy.Column( | ||
| "run_time_parameter_values_and_defaults", | ||
| "parameter_variable_name", |
There was a problem hiding this comment.
It might make sense to throw in unique=True on the parameter_variable_name column.
Whoops, no, sorry, definitely not just on the parameter_variable_name column. It would need to be on the "analysis_id", "parameter_variable_name" combination.
This is just a nice-to-have.
| schema_5.run_command_table, | ||
| source_transaction, | ||
| dest_transaction, | ||
| order_by_rowid=True, |
There was a problem hiding this comment.
Nitpick: I think this is harmless, but order_by_rowid can be False for run_command_table because it has explicitly-declared row_id and index_in_run columns.
I wonder if order_by_rowid=True is actually any slower or more memory-hungry than =False. Maybe it should just always be True to simplify things.
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #15650 +/- ##
===========================================
+ Coverage 63.25% 92.43% +29.18%
===========================================
Files 287 77 -210
Lines 15013 1283 -13730
===========================================
- Hits 9497 1186 -8311
+ Misses 5516 97 -5419
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Nice, I'm always for breaking stuff into more steps to better mirror semantics. I have a couple things I'd love to see changed but if stuff depends on this getting merged it's ok:
- Remove some null checks by slightly reorganizing
- Make remaining checks not just
asserts but actual errors - Just saying,
Scalarfeels like a better name thanPrimitive
| import robot_server.errors.error_mappers as em | ||
|
|
||
|
|
||
| class FailedToInitializeAnalyzer(Exception): |
There was a problem hiding this comment.
Should this be an enumerated error so it can carry more context?
| parameter, | ||
| prev_value_and_default, | ||
| ) in rtp_values_and_defaults_in_last_analysis.items(): | ||
| assert set(param.variableName for param in new_parameters) == set( |
There was a problem hiding this comment.
Are we sure about this one? I really don't like using assert in this way in production code. If we're going to be paranoid, why not have this function (1) log something big and then (2) just return False, since if these parameter names don't match then they're definitionally different?
There was a problem hiding this comment.
@sfoster1 can you explain why you don't like using assert in this way?
I prefer assert rather than silently logging because it makes sure that the error is actually brought to attention. It's a practice we've followed in most of robot server and engine code. If we only log the error and continue running (by returning False in this case), then the server might run into some other error down the line because failing this assertion means that there's a bug in our code somewhere else. So it's better to fail right away and know why it failed rather than failing a little later with a possibly unclear reason.
| @@ -407,22 +409,22 @@ def add( | |||
| self, | |||
| protocol_id: str, | |||
| analysis_id: str, | |||
| ) -> PendingAnalysis: | |||
| run_time_parameters: List[RunTimeParameter], | |||
| ) -> None: | |||
There was a problem hiding this comment.
why change this? feels like a nice convenience thing to return the newly created object, and it's a smaller diff
| with self._sql_engine.begin() as transaction: | ||
| results = transaction.execute(statement).all() | ||
|
|
||
| rtps: Dict[str, PrimitiveAllowedTypes] = {} |
There was a problem hiding this comment.
nit:
resources = [PrimitiveParameterResource.from_sql_row(row) for row in results]
rtps = {param. parameter_variable_name: param.parameter_value for param in resources}
? Means you don't have to explicitly type rtps
| @@ -34,45 +37,57 @@ def __init__( | |||
| """Initialize the analyzer and its dependencies.""" | |||
| self._analysis_store = analysis_store | |||
| self._protocol_resource = protocol_resource | |||
| self._orchestrator: Optional[RunOrchestrator] = None | |||
There was a problem hiding this comment.
Looking around the code, it seems like the first thing you always do with a protocol analyzer is call load_runner (now load_orchestrator) with the RTPs, including in this pr. Given that, instead of having load_orchestrator can we
- Require an orchestrator instance as an initializer param, meaning it's not-null by construction
- make
create_protocol_analyzertake the optionalrun_time_paramvalues and build this there
That way we don't need all these asserts and we make it impossible to call get_verified_run_time_parameters in an illegal way.
|
Thanks for the feedback @sfoster1. I'll address them in a follow-up PR since this one's quite overdue for merging. |
Closes AUTH-527
Overview
This PR moves primitive RTPs to their own table and refactors the analysis-generation process. This entire refactor makes it easier to add on CSV-parameter handling.
Architecture and database changes:
Here's what the new analysis flow looks like on a high level-
flowchart TD %% Nodes A1(Protocol) A2(Run time param values from client) B1(Upload protocol endpoint) C1("Initialize analyzer: \n Validate and set run time parameters \n using definitions in protocol \nand values passed by client") D1("Create a not-ok analysis result. \nSave result to DB. \nReturn analysis completed result.") D2("Compare the newly validated RTP values \nwith those of the last analysis (if any) \nof this protocol.") E1("No analysis required. Use last analysis in analyses list.") E2("In a background task, initiate new analysis using the new RTP values.") F1("Pending analysis is created") F2("Return pending analysis summary response") %% Edge connections between nodes A1 ---> B1 A2 ---> B1 B1 ---> C1 C1 -- Initialization/RTP validation failed --> D1 C1 -- RTP values validated successfully --> D2 D2 -- Matching RTPs --> E1 D2 -- Non-matching RTPs --> E2 --> F1 --> F2flowchart TD %% Nodes EF("Analyze task") G1("Pending analysis is updated to a \n'Completed analysis' with an error.") G2("Pending analysis is updated to a \n successful 'Completed analysis'.") End(" \nAnalysis and RTP values are saved to DB.") %% Edge connections between nodes EF -- Analysis Error --> G1 --> End EF -- Sucessful analysis --> G2 --> EndTest Plan
Changelog
AnalysesManager,ProtocolAnalyzer,AnalysisStoreand protocolsrouterto use the new flow as shown above.Review requests
Risk assessment
Medium-high. It changes quite a critical part of the protocol upload and analysis process. But the fact that all integration tests and analyses snapshots are passing makes me feel quite confident that the existing behavior of these endpoints has not changed by this work.