feat(protocol-engine): Support loading pipettes and labware from JSON protocols #7950

SyntaxColoring · 2021-06-15T21:42:28Z

Overview

When a JSON protocol is uploaded through the experimental HTTP API, to be run through protocol engine, implement the missing parts of loading the pipettes and labware that the JSON protocol uses. Closes #7946.

Changelog

Main changes:

Changed CommandTranslator to take in an entire protocol at once, instead of going command-by-command.
- Changed calling code and its tests accordingly.
Gave CommandTranslator the ability to translate a JSON protocol's choices of pipettes and labware into Protocol Engine LoadPipetteRequests, AddLabwareDefinitionRequests, and LoadLabwareRequests.

Also:

Clarified that LoadPipetteRequest.pipetteName is the pipette load name, as opposed to the model name or something.
Left a maybe unnecessary comment in opentrons/protocols/models/__init.py__ describing when (I think) it should re-export things from its child submodules. I had gone down an unproductive path adding re-exports for everything I could possibly need from that tree.

Deferred work

I made a mess of the tests, which I will clean up in a follow-up PR. See the todo comment at the top of test_command_translator.py for the fix approach that @mcous and I agreed on.
- For alternative approaches considered, see these todo comments from an earlier version of this PR, which I've since removed: feat(protocol-engine): Support loading pipettes and labware from JSON protocols #7950 (comment), feat(protocol-engine): Support loading pipettes and labware from JSON protocols #7950 (comment).
- Ticket for follow-up: Clean up CommandTranslator tests #7958
There's some confusing "id"/"URI"/"UUID" terminology between PD and PE that we should reconcile at some pont.

Other stuff discussed in in-person reviews

@sanni-t and I considered renaming CommandTranslator to ProtocolTranslator, since it now takes in and translates an entire JSON protocol at once, rather than going command-by-command. This could have been necessary if we chose a testing approach where command translation is a separate class from protocol translation, but we didn't, so this renaming doesn't feel worthwhile to me anymore.

Review requests

I would focus on the production code, since we know the tests to be messy and we already have a good path in mind to fix them.

Risk assessment

Low. Only used in our experimental Protocol Engine work, behind a feature flag.

codecov · 2021-06-15T21:45:23Z

Codecov Report

Merging #7950 (5f3187f) into edge (1061b5b) will increase coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #7950      +/-   ##
==========================================
+ Coverage   85.93%   86.06%   +0.13%     
==========================================
  Files         348      348              
  Lines       21232    21471     +239     
==========================================
+ Hits        18245    18479     +234     
- Misses       2987     2992       +5

Impacted Files	Coverage Δ
robot-server/robot_server/router.py	`90.47% <0.00%> (-2.39%)`	⬇️
opentrons/protocol_engine/state/pipettes.py	`96.87% <0.00%> (-1.74%)`	⬇️
opentrons/protocols/models/__init__.py	`100.00% <0.00%> (ø)`
opentrons/file_runner/json_file_runner.py	`100.00% <0.00%> (ø)`
opentrons/protocol_engine/state/motion.py	`100.00% <0.00%> (ø)`
opentrons/protocol_engine/state/labware.py	`100.00% <0.00%> (ø)`
opentrons/protocol_engine/state/geometry.py	`100.00% <0.00%> (ø)`
...er/robot_server/service/legacy/routers/__init__.py	`100.00% <0.00%> (ø)`
opentrons/protocol_engine/state/state_store.py	`98.14% <0.00%> (+0.18%)`	⬆️
robot-server/robot_server/app.py	`94.23% <0.00%> (+2.80%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1061b5b...5f3187f. Read the comment docs.

mcous · 2021-06-17T17:42:45Z

Just did some real world testing, and it's looking like this is fundamentally working! We've got a little issue with the command state, but otherwise this looks like it's doing the thing 🎉

As we've just implemented things in pipette and labware translation, yes, these IDs are the same.

* Revert some thoughtless re-exports I made earlier in this PR. * Say what I think actually should be re-exported here, so I don't make the same mistake again.

api/src/opentrons/protocols/runner/json_proto/command_translator.py

api/tests/opentrons/conftest.py

api/tests/opentrons/protocols/runnner/json_proto/test_command_translator.py

mcous

Some discussion points, but covered most of this on chat. Got a few discussion points below, but otherwise this looks to be fundamentally working and the TODOs check out

mcous · 2021-06-17T21:02:29Z

api/src/opentrons/protocols/models/__init__.py

+# Convenience re-exports of models that are especially common or important.
+# More detailed sub-models are always available through the underlying
+# submodules.
+#
+# If re-exporting something, its name should still make sense when it's separatred
+# from the name of its parent submodule. e.g. re-exporting models.json_protocol.Labware
+# as models.Labware could be confusing.


I see, this is referring to the from .json_protocol import Model as JsonProtocol re-export business? Agree that should be avoided

Not totally sure we're talking about the same thing.

I had gone down an unproductive path of adding re-exports for everything I could possibly need from json_protocol and labware_definitions to this __init__.py. I tried re-exporting json_protocol.Labware, json_protocol.AllCommands, and so on.

Somewhere in the middle of this PR, I realized that this would cause confusing naming problems. For example, models.Labware sounds like it's about Opentrons labware definitions in general, but as a re-export of json_protocols.Labware, it's actually a very specific JSON protocol thing.

So this comment is meant to say, "I know we're re-exporting some things here at the top level, but don't get tempted into doing this for absolutely everything. Here's where it does and doesn't make sense."

json_protocol.Model is funky but I don't think the concern is specific to it.

mcous · 2021-06-17T21:07:46Z

api/src/opentrons/protocols/runner/json_proto/command_translator.py

-    PickUpTipRequest, DropTipRequest
-)
-from opentrons.protocols.models import json_protocol as models
+import opentrons.types


I find import foo.bar to be very confusing; it's was non-obvious to me that you then proceed to use foo.bar in the code.

Given that we use exactly two structures from here, I would've written this as:

from opentrons.types import DeckSlotName, MountType

Mostly because I'm lazy and it's less typing everywhere, and doesn't feel to me like it's a loss in understanding because the name is consistent everywhere and most of us are working in editors / IDEs with type info + click-to-follow

mcous · 2021-06-17T21:13:05Z

api/src/opentrons/protocols/runner/json_proto/command_translator.py

-        Translate a PD/JSON protocol to Protocol Engine commands.
+    def translate(self, protocol: models.JsonProtocol) -> PECommands:
+        """Return all Protocol Engine commands required to run the given protocol."""
+        result: List[pe.commands.CommandRequestType] = []


pe.commands.WhateverThing makes me feel a little like we're locking into the internal shape of the protocol_engine module. It sort of makes me think that this translator belongs in the protocol_engine module itself (perhaps in clients or something

Because we mention commands over and over every time we type pe.commands.WhateverThing, as opposed to once at the top of the file like from protocol_engine.commands import ...?

That's true.

I think in this particular case, it gets resolved if we do from protocol_engine import commands as pe_commands, as you suggested and as I've just written about in Slack.

This also brings up a second discussion of whether submodule names are / can be / should be part of the whole collective package's stable interface. I'll write about this separately.

api/tests/opentrons/protocols/runnner/json_proto/test_command_translator.py

SyntaxColoring · 2021-06-17T22:06:22Z

Merging this now to make it easier to test running protocols on robots (yay!). But feel free to keep leaving comments here (and maybe ping me) for stuff to clarify/fix/change in a follow-up.

SyntaxColoring added 2 commits June 15, 2021 16:01

Make CommandTranslator take in the whole protocol at once.

c710d2b

WIP: Emit AddLabwareDefinitionRequests and LoadLabwareRequests.

5993eaf

SyntaxColoring added 10 commits June 16, 2021 11:11

Fix up whitespace.

0cf8246

Clean up imports.

bff7c34

Assert loading a labware comes after adding its respective definition.

a2aeaef

Fix whitespace.

c8611da

Clarify that LoadPipetteRequest.pipetteName is the load name.

902d068

Emit LoadPipetteRequests.

85ba063

Avoid casts where unnecessary.

c873c58

Translate liquid-handling commands. Also, general cleanup.

8286a60

Simplify imports.

02bd382

Use "pe" instead of "protocol_engine".

c2693b7

SyntaxColoring added 7 commits June 17, 2021 13:53

Resolve todo.

37a18a9

As we've just implemented things in pipette and labware translation, yes, these IDs are the same.

Clean up test imports.

d70e952

Elaborate on testing todo comment.

6c8efe5

Autoformat tests with black.

f377529

Minimize, and document intent of, modules/__init__.py re-exports.

2f5c2d2

* Revert some thoughtless re-exports I made earlier in this PR. * Say what I think actually should be re-exported here, so I don't make the same mistake again.

Clarify todo comment for duplicated JSON protocol fixture.

0ab504f

Delete stray todo.

8aebd6e

SyntaxColoring marked this pull request as ready for review June 17, 2021 19:06

SyntaxColoring requested a review from a team as a code owner June 17, 2021 19:06

SyntaxColoring commented Jun 17, 2021

View reviewed changes

api/src/opentrons/protocols/runner/json_proto/command_translator.py Show resolved Hide resolved

SyntaxColoring commented Jun 17, 2021

View reviewed changes

api/tests/opentrons/conftest.py Outdated Show resolved Hide resolved

SyntaxColoring commented Jun 17, 2021

View reviewed changes

api/tests/opentrons/protocols/runnner/json_proto/test_command_translator.py Outdated Show resolved Hide resolved

SyntaxColoring commented Jun 17, 2021

View reviewed changes

api/tests/opentrons/protocols/runnner/json_proto/test_command_translator.py Outdated Show resolved Hide resolved

SyntaxColoring marked this pull request as draft June 17, 2021 19:41

Type minimal_labware_def2() as returning a dict.

4d97774

mcous added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Jun 17, 2021

mcous added the protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs label Jun 17, 2021

SyntaxColoring added 2 commits June 17, 2021 16:33

Type all methods as returning lists, not iterables.

b3fe396

Update test todo comment to reflect in-person review discussion.

5f3187f

mcous reviewed Jun 17, 2021

View reviewed changes

api/tests/opentrons/protocols/runnner/json_proto/test_command_translator.py Show resolved Hide resolved

SyntaxColoring marked this pull request as ready for review June 17, 2021 20:54

mcous approved these changes Jun 17, 2021

View reviewed changes

SyntaxColoring merged commit e43c80f into edge Jun 17, 2021

mcous deleted the commandtranslator_pipettes_and_labware branch June 17, 2021 22:05

sanni-t reviewed Jun 17, 2021

View reviewed changes

api/tests/opentrons/protocols/runnner/json_proto/test_command_translator.py Show resolved Hide resolved

SyntaxColoring mentioned this pull request Jun 17, 2021

Clean up CommandTranslator tests #7958

Closed

SyntaxColoring mentioned this pull request Jun 29, 2021

refactor(api): Clean up CommandTranslator tests #8021

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(protocol-engine): Support loading pipettes and labware from JSON protocols #7950

feat(protocol-engine): Support loading pipettes and labware from JSON protocols #7950

SyntaxColoring commented Jun 15, 2021 •

edited

Loading

codecov bot commented Jun 15, 2021 •

edited

Loading

mcous commented Jun 17, 2021

mcous left a comment

mcous Jun 17, 2021

SyntaxColoring Jun 17, 2021 •

edited

Loading

mcous Jun 17, 2021 •

edited

Loading

mcous Jun 17, 2021

SyntaxColoring Jun 17, 2021 •

edited

Loading

SyntaxColoring commented Jun 17, 2021 •

edited

Loading

feat(protocol-engine): Support loading pipettes and labware from JSON protocols #7950

feat(protocol-engine): Support loading pipettes and labware from JSON protocols #7950

Conversation

SyntaxColoring commented Jun 15, 2021 • edited Loading

Overview

Changelog

Deferred work

Other stuff discussed in in-person reviews

Review requests

Risk assessment

codecov bot commented Jun 15, 2021 • edited Loading

Codecov Report

mcous commented Jun 17, 2021

mcous left a comment

Choose a reason for hiding this comment

mcous Jun 17, 2021

Choose a reason for hiding this comment

SyntaxColoring Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

mcous Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

mcous Jun 17, 2021

Choose a reason for hiding this comment

SyntaxColoring Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring commented Jun 17, 2021 • edited Loading

SyntaxColoring commented Jun 15, 2021 •

edited

Loading

codecov bot commented Jun 15, 2021 •

edited

Loading

SyntaxColoring Jun 17, 2021 •

edited

Loading

mcous Jun 17, 2021 •

edited

Loading

SyntaxColoring Jun 17, 2021 •

edited

Loading

SyntaxColoring commented Jun 17, 2021 •

edited

Loading