feat(robot-server, api): accept RTP overrides via /protocols and inject them in python executor #14688

sanni-t · 2024-03-19T15:00:03Z

Overview

Updates the POST /protocols API to accept an additional runTimeParameterValues form-data field. This field is expected to be a stringified JSON composed of key-value pairs of the parameter variable name and its override value.

These override values are converted into a dictionary and passed into the protocol runner, which makes them available to the python executor.

Test Plan

Send a POST protocol request and verify that an analysis is created
POST the same protocol again and verify that the response still gives the same analysis & protocol IDs
Send a protocol with runTimeParameterValues in request data and verify the request goes through, and a new analysis is created on the same protocol ID

Known issues

every time a protocol creation request contains a non-empty runTimeParameterValues data, analysis is triggered, regardless of whether we have previously analyzed protocols with the same param values
if no param values are sent with an existing protocol, a new analysis is NOT triggered, regardless of whether the previous analyses were done using any RTP values or not

These will be addressed in AUTH-70

Review requests

check for usual code sanity

Risk assessment

High. Affects the correctness of order and contents of analysis summaries returned by the POST /protocols endpoint when using run time parameters. This behavior will remain broken until AUTH-70 is implemented. Until then, please be cautious that a protocol analysis request that uses custom RTP values will modify an existing protocol analysis database such that the app might pick the wrong analysis for a run. I will recommend deleting such protocols (and runs) from the database after you are done testing, especially if you are testing on a shared robot.

…passed

codecov · 2024-03-19T15:04:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.33%. Comparing base (6695f9e) to head (32011f8).
Report is 43 commits behind head on edge.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #14688   +/-   ##
=======================================
  Coverage   67.33%   67.33%           
=======================================
  Files        2485     2485           
  Lines       71386    71386           
  Branches     9030     9030           
=======================================
  Hits        48069    48069           
  Misses      21173    21173           
  Partials     2144     2144

Flag	Coverage Δ
g-code-testing	`92.43% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
api/src/opentrons/execute.py	`54.23% <ø> (ø)`
api/src/opentrons/protocol_engine/types.py	`97.50% <ø> (ø)`
...i/src/opentrons/protocol_reader/protocol_reader.py	`100.00% <ø> (ø)`
...i/src/opentrons/protocol_runner/legacy_wrappers.py	`100.00% <ø> (ø)`
...i/src/opentrons/protocol_runner/protocol_runner.py	`100.00% <ø> (ø)`
api/src/opentrons/protocols/execution/execute.py	`90.90% <ø> (ø)`
api/src/opentrons/simulate.py	`57.73% <ø> (ø)`
...ot-server/robot_server/protocols/analysis_store.py	`100.00% <ø> (ø)`
...server/robot_server/protocols/protocol_analyzer.py	`100.00% <ø> (ø)`
robot-server/robot_server/protocols/router.py	`100.00% <ø> (ø)`
... and 1 more

SyntaxColoring · 2024-03-19T21:30:06Z

robot-server/robot_server/protocols/router.py

+            task_runner.run(
+                protocol_analyzer.analyze,
+                protocol_resource=resource,
+                analysis_id=analysis_id,
+                run_time_param_overrides=parsed_rtp,
+            )
+            analysis_store.add_pending(
+                protocol_id=cached_protocol_id,
+                analysis_id=analysis_id,
+            )


This will probably need to be implemented in a different PR, but at some point before releasing this, we'll need to cap the maximum number of protocol analyses. I'm curious if you have any thoughts on what the limit will be?

Ya, we'll have to figure that out. How did we land on the max number of protocols and runs to store? Is there a hard cut off for memory availability we need to keep in mind?

It's mostly how long it would take to parse them all into Pydantic models if we had to do some kind of migration.

jbleon95

Some small style nitpicks but otherwise LGTM!

jbleon95 · 2024-03-20T18:03:05Z

api/src/opentrons/protocols/execution/execute.py

+def run_protocol(
+    protocol: Protocol,
+    context: ProtocolContext,
+    run_time_param_overrides: Optional[Dict[str, Union[float, bool, str]]] = None,


We should use the PE type here if possible

I had a TODO for this exact thing. I/ pycharm must have removed it during refactor. Putting it back in.

jbleon95 · 2024-03-20T18:04:02Z

robot-server/robot_server/protocols/router.py

@@ -184,6 +189,7 @@ async def create_protocol(
    Arguments:
        files: List of uploaded files, from form-data.
        key: Optional key for client-side tracking
+        runTimeParameterValues: Key value pairs of run-time parameters defined in a protocol.


Let's make this snake_case for consistency

It's actually camel case for robot-server HTTP API consistency purposes.

SyntaxColoring

hype

SyntaxColoring · 2024-03-20T19:38:58Z

robot-server/robot_server/protocols/router.py

+    runTimeParameterValues: Optional[str] = Form(
+        default=None,
+        description="Key value pairs of run-time parameters defined in a protocol.",
+    ),


It might be worth clarifying in this description that:

This is a string holding a JSON object.

This affects the auto-started analysis resource (GET /protocols/{id}/analyses/{id}).

robot-server/tests/integration/http_api/protocols/test_analyses.tavern.yaml

SyntaxColoring · 2024-03-20T19:42:36Z

robot-server/tests/integration/http_api/protocols/test_analyses.tavern.yaml

+      save:
+        json:
+          protocol_id: data.id
+          analysis_id: data.analysisSummaries[1].id


Looks like this is unused.

Suggested change

save:

json:

protocol_id: data.id

analysis_id: data.analysisSummaries[1].id

SyntaxColoring · 2024-03-20T19:43:58Z

robot-server/robot_server/protocols/router.py

+        # 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)/
+        parsed_rtp = json.loads(runTimeParameterValues)


Nit, doesn't have to happen in this PR: This will raise a 500 error if the client passes invalid JSON. Ideally, it would return a 422 instead. I wonder if there's a way to make a custom "run-time parameter multipart form field" Pydantic type so FastAPI can do that for us.

Hmm.. interesting idea. I'll add a TODO

Closes AUTH-229 # Overview Updates the `/protocols` endpoints to always maintain the order of list of analyses as most-recently-started-analysis last, making sure to verify if a new analysis needs to be triggered because of new run-time-parameter values for a previously uploaded protocol. # Risk assessment Medium. Does database update and fixes the analysis order that was broken by #14688 --------- Co-authored-by: Max Marrone <[email protected]>

…ct them in python executor (#14688) Closes AUTH-64, AUTH-65 Updates the POST /protocols API to accept an additional runTimeParameterValues form-data field. This field is expected to be a stringified JSON composed of key-value pairs of the parameter variable name and its override value. These override values are converted into a dictionary and passed into the protocol runner, which makes them available to the python executor. Risk assessment: High. Affects the correctness of order and contents of analysis summaries returned by the POST /protocols endpoint when using run time parameters. This behavior will remain broken until AUTH-70 is implemented. Until then, please be cautious that a protocol analysis request that uses custom RTP values will modify an existing protocol analysis database such that the app might pick the wrong analysis for a run. I will recommend deleting such protocols (and runs) from the database after you are done testing, especially if you are testing on a shared robot.

Closes AUTH-229 # Overview Updates the `/protocols` endpoints to always maintain the order of list of analyses as most-recently-started-analysis last, making sure to verify if a new analysis needs to be triggered because of new run-time-parameter values for a previously uploaded protocol. # Risk assessment Medium. Does database update and fixes the analysis order that was broken by #14688 --------- Co-authored-by: Max Marrone <[email protected]>

sanni-t added 6 commits March 18, 2024 09:09

protocol upload takes RTP values and sends them to python executor

e61e161

pass RTP overrides via runner instead of through protocol resource

aa0f808

create_protocols endpoint triggers reanalysis when RTP overrides are …

e4740c5

…passed

updated robot server tests

c451654

api updates, test fixes

9c9cbbf

api test fixes

91bdf23

fix test

8fbb9ca

SyntaxColoring reviewed Mar 19, 2024

View reviewed changes

naming refactor

205dcb7

sanni-t marked this pull request as ready for review March 20, 2024 16:28

sanni-t requested review from a team as code owners March 20, 2024 16:28

sanni-t requested review from a team and removed request for a team March 20, 2024 16:28

jbleon95 approved these changes Mar 20, 2024

View reviewed changes

sanni-t added 2 commits March 20, 2024 15:09

forgot to update args in runner.run callers

71ee6ec

add todo

3bc8a61

SyntaxColoring approved these changes Mar 20, 2024

View reviewed changes

addressed review comments

32011f8

sanni-t merged commit fce980f into edge Mar 20, 2024
22 checks passed

sanni-t deleted the AUTH-64-protocols-endpoint-to-accept-runtime-parameter-values branch March 20, 2024 21:14

sanni-t mentioned this pull request Apr 1, 2024

fix(robot-server): maintain correct order of protocol analyses #14762

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(robot-server, api): accept RTP overrides via /protocols and inject them in python executor #14688

feat(robot-server, api): accept RTP overrides via /protocols and inject them in python executor #14688

sanni-t commented Mar 19, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading

SyntaxColoring Mar 19, 2024

sanni-t Mar 20, 2024

SyntaxColoring Mar 20, 2024

jbleon95 left a comment

jbleon95 Mar 20, 2024

sanni-t Mar 20, 2024

jbleon95 Mar 20, 2024

sanni-t Mar 20, 2024

SyntaxColoring left a comment

SyntaxColoring Mar 20, 2024

SyntaxColoring Mar 20, 2024

SyntaxColoring Mar 20, 2024

sanni-t Mar 20, 2024

feat(robot-server, api): accept RTP overrides via /protocols and inject them in python executor #14688

feat(robot-server, api): accept RTP overrides via /protocols and inject them in python executor #14688

Conversation

sanni-t commented Mar 19, 2024 • edited Loading

Overview

Test Plan

Known issues

Review requests

Risk assessment

codecov bot commented Mar 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbleon95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t commented Mar 19, 2024 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading