feat(api): define & execute v3 json protocols #3312

IanLondon · 2019-04-05T23:01:42Z

overview

Closes #3110 and implements execution in APIv2

Does NOT implement execution in APIv1, I think we don't need to support that because by the time v3 JSON Protocols are out in the wild, we'll have moved to APIv2? If not, we can update the APIv1 executor later (I think...)

changelog

define v3 json protocol schema
support v3 JSON protocol execution in APIv2 executor
split JSON protocol version specific execution fns into their own .py files
add unit tests for v3 execution fns

review requests

NOTE: the example protocols and the schema are different than #3307. Following a suggestion from @sfoster1, defaultValues key has been deleted in favor of explicit per-command params

v3jsonProtocolExamples.zip

Review V3 schema def
Are there other parts of the commands still using implicit values that should be make into params?
Check out the schema, make sure it's designed in a sensible way that accomplishes the things we've talked about and doesn't block anything we need
Play with the example v3 protocol files and make sure the schema validates them when they should and fails to validate "bad" protocol files

npm install -g ajv-cli then you can use the ajv CLI to validate v3 JSON protocols with the new schema:

ajv validate -s shared-data/protocol-json-schema/protocolSchemaV3.json -r shared-data/labware-json-schema/labware-schema.json -d path/to/exampleProtocol.json (the -r pulls in the v2 labware schema so it resolve the reference to opentronsLabwareSchemaV2 in the definitions section)

The V3 protocols should run correctly on the robot (with APIv2 feature flag enabled) -- we need to carefully test the offset and flow rates and make sure they're going in as intended
Pre-V3 protocols should still run on the robot, the same as before (eg V1 protocols exported from PD)
Custom labware created by labware-designer should work when embedded in these protocols!

* define v3 json protocol schema * support v3 JSON protocol execution in APIv2 executor Closes #3110

codecov · 2019-04-05T23:10:51Z

Codecov Report

Merging #3312 into edge will increase coverage by 1.16%.
The diff coverage is 74.29%.

@@            Coverage Diff            @@
##             edge   #3312      +/-   ##
=========================================
+ Coverage   53.63%   54.8%   +1.16%     
=========================================
  Files         716     721       +5     
  Lines       20907   21748     +841     
=========================================
+ Hits        11213   11918     +705     
- Misses       9694    9830     +136

Impacted Files	Coverage Δ
api/src/opentrons/protocol_api/execute.py	`67.67% <16%> (-12.74%)`	⬇️
api/src/opentrons/protocol_api/execute_v1.py	`76.47% <76.47%> (ø)`
api/src/opentrons/protocol_api/execute_v3.py	`85.71% <85.71%> (ø)`
labware-library/src/components/App/Page.js	`50% <0%> (-50%)`	⬇️
...-library/src/components/LabwareList/LabwareCard.js	`31.81% <0%> (-4.77%)`	⬇️
labware-library/src/filters.js	`80% <0%> (-1.82%)`	⬇️
...pi/src/opentrons/legacy_api/containers/__init__.py	`78.19% <0%> (-1.44%)`	⬇️
api/src/opentrons/config/feature_flags.py	`94.11% <0%> (-1.34%)`	⬇️
labware-library/src/index.js	`0% <0%> (ø)`	⬆️
labware-library/src/definitions.js	`0% <0%> (ø)`	⬆️
... and 9 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 f6789cb...e082c6d. Read the comment docs.

api/src/opentrons/protocol_api/execute.py

sfoster1

Some code review changes but the structure looks good

sfoster1 · 2019-04-08T17:02:33Z

api/src/opentrons/protocol_api/execute.py

-        dispatch_json(true_context, protocol_json, ins, lw)
+        protocol_version = get_protocol_schema_version(protocol_json)
+        if (protocol_version) > 3:
+            raise RuntimeError('JSON Protocol version {} is not yet supported \


May fail lint; \ is not required inside braces, you can do

raise RuntimError('Json Protocol version {} is not yet supported' 'in this version of the API')

make -C api lint is working for me as-is but would you prefer this?

raise RuntimeError(( 'JSON Protocol version {} is not yet ' + 'supported in this version of the API') .format(protocol_version))

You can also do this fancy new thing (similar to javascript evaluated strings)

f'Json Protocol version {protocol_version} is not yet supported in this version of the API' if you don't want to use format

sfoster1 · 2019-04-08T17:04:37Z

api/src/opentrons/protocol_api/execute_v1.py

+    pipette_data = protocol.get('pipettes', {})
+    pipettes_by_id = {}
+    for pipette_id, props in pipette_data.items():
+        model = props.get('model')


If these keys are mandatory we should access them via dict indexing: model = props['model'] which will fail fast with a KeyError if the protocol is malformed and the keys aren't present, and fixes issues with types since get() returns Optional[T]

sfoster1 · 2019-04-08T17:05:58Z

api/src/opentrons/protocol_api/execute_v3.py

+def load_pipettes_from_json(
+        ctx: ProtocolContext,
+        protocol: Dict[Any, Any]) -> Dict[str, InstrumentContext]:
+    pipette_data = protocol.get('pipettes', {})


Accesses to mandatory keys should be dict indexing (protocol['pipettes']) to avoid typing issues and fail fast

sfoster1 · 2019-04-08T17:06:09Z

api/src/opentrons/protocol_api/execute_v3.py

+
+
+def _get_well(loaded_labware: Dict[str, labware.Labware],
+              params: Dict[str, Any]):


return type docs?

api/src/opentrons/protocol_api/execute_v3.py

sfoster1 · 2019-04-08T17:07:02Z

api/src/opentrons/protocol_api/execute_v3.py

+        ctx: ProtocolContext,
+        protocol: Dict[Any, Any]) -> Dict[str, labware.Labware]:
+    protocol_labware = protocol.get('labware', {})
+    definitions = protocol.get('labwareDefinitions', {})


Mandatory keys?

sfoster1 · 2019-04-08T17:07:40Z

api/src/opentrons/protocol_api/execute_v3.py

+            # TODO: Ian 2018-11-06 remove this fallback to 'model' when
+            # backwards-compatability for JSON protocols with versioned
+            # pipettes is dropped (next JSON protocol schema major bump)
+            pipette_name = protocol_pipette_data.get('model')


This TODO is in fact resolved in v3, right?

sfoster1 · 2019-04-08T17:08:30Z

api/tests/opentrons/protocol_api/test_execute_v3.py

+    # trough def with arbitrary ID
+    data = {
+        "labwareDefinitions": {
+            "someTroughDef": json.loads("""{"ordering":[["A1"],["A2"],["A3"],["A4"],["A5"],["A6"],["A7"],["A8"],["A9"],["A10"],["A11"],["A12"]],"otId":"THIS IS A CUSTOM ID","deprecated":false,"metadata":{"displayName":"CUSTOM 12 Channel Trough","displayVolumeUnits":"mL","displayCategory":"trough"},"cornerOffsetFromSlot":{"x":0,"y":0.32,"z":0},"dimensions":{"overallLength":127.76,"overallWidth":85.8,"overallHeight":44.45},"parameters":{"format":"trough","isTiprack":false,"isMagneticModuleCompatible":false,"loadName":"usa_scientific_12_trough_22_ml","quirks":["centerMultichannelOnWells"]},"wells":{"A1":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":13.94,"y":42.9,"z":2.29},"A2":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":23.03,"y":42.9,"z":2.29},"A3":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":32.12,"y":42.9,"z":2.29},"A4":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":41.21,"y":42.9,"z":2.29},"A5":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":50.3,"y":42.9,"z":2.29},"A6":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":59.39,"y":42.9,"z":2.29},"A7":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":68.48,"y":42.9,"z":2.29},"A8":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":77.57,"y":42.9,"z":2.29},"A9":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":86.66,"y":42.9,"z":2.29},"A10":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":95.75,"y":42.9,"z":2.29},"A11":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":104.84,"y":42.9,"z":2.29},"A12":{"shape":"rectangular","depth":42.16,"length":8.33,"width":71.88,"totalLiquidVolume":22000,"x":113.93,"y":42.9,"z":2.29}},"brand":{"brand":"USA Scientific","brandId":["1061-8150"]}}""")  # noqa


Maybe put this in a data file in this dir?

sfoster1 · 2019-04-08T17:08:42Z

api/tests/opentrons/protocol_api/test_execute_v3.py

+
+def test_dispatch_commands(monkeypatch, loop):
+    protocol_data = {
+        "schemaVersion": "3",


Put it in a data file in this dir?

api/src/opentrons/protocol_api/execute_v3.py

… keys

Laura-Danielle · 2019-04-08T18:07:56Z

api/src/opentrons/protocol_api/execute.py

+def get_protocol_schema_version(protocol_json: Dict[Any, Any]) -> int:
+    # v3 and above uses `schemaVersion: integer`
+    version = protocol_json.get('schemaVersion')
+    if None is not version:


Could you change this to either:

if protocol_json.get('schemaVersion'): return protocol_json.get('schemaVersion')

or:

version = protocol_json.get('schemaVersion') if version: return version

Laura-Danielle · 2019-04-08T18:09:24Z

api/src/opentrons/protocol_api/execute.py

+        return 1
+    if (legacyKebabVersion == '2.0.0'):
+        return 2
+    if (legacyKebabVersion is not None):


Same here:

if legacyKebabVersion:

Also may want to change format to if/elif as that's normally used for conditional checking

pylint yells at you if you do

if: return else: other stuff

I personally disagree but if that's what the linter says that's what the linter says

Laura-Danielle · 2019-04-08T18:10:18Z

api/src/opentrons/protocol_api/execute.py

@@ -364,8 +159,22 @@ def run_protocol(protocol_code: Any = None,
    if None is not protocol_code:


Same here for checking whether a variable is populated (if protocol_code)

Laura-Danielle · 2019-04-08T18:12:17Z

api/src/opentrons/protocol_api/execute.py

-        dispatch_json(true_context, protocol_json, ins, lw)
+        protocol_version = get_protocol_schema_version(protocol_json)
+        if (protocol_version) > 3:
+            raise RuntimeError('JSON Protocol version {} is not yet supported \


You can also do this fancy new thing (similar to javascript evaluated strings)

f'Json Protocol version {protocol_version} is not yet supported in this version of the API' if you don't want to use format

Laura-Danielle · 2019-04-08T18:17:35Z

api/src/opentrons/protocol_api/execute_v1.py

+            .get(params.get('pipette'), {})
+        pipette_name = protocol_pipette_data.get('name')
+
+        if (not pipette_name):


don't need parentheses here

Laura-Danielle · 2019-04-08T18:31:42Z

shared-data/protocol-json-schema/protocolSchemaV3.json

+      "type": "object",
+      "properties": {
+        "name": {
+          "description": "Name of the application that created the protocol. Should be namespaced under the organization or individual who owns the organization, eg \"opentrons/protocol-designer\"",


Is the name always guaranteed to be opentrons/protocol-designer? Should we restrict it to that value only?

nope, it's open for any 3rd party JSON protocol creator app to use

Laura-Danielle

🛫

b-cooper

🌺

mcous

🐎

IanLondon · 2019-04-09T15:05:49Z

api/src/opentrons/protocol_api/execute_v3.py

+    }
+
+
+def load_labware_from_json_defs(


@sfoster1 unlike the old load_labware_from_json_loadnames, this doesn't handle fixed-trash or slot 12 differently. We talked about having the protocol define the fixed trash def and have it as a labware in slot 12, because users will pretty much always use the 1.1L version. Does that all seem OK?

Yeah I'm OK with that.

sfoster1 · 2019-04-09T15:29:03Z

api/src/opentrons/protocol_api/execute.py

+        protocol_version = get_protocol_schema_version(protocol_json)
+        if protocol_version > 3:
+            raise RuntimeError(
+                f'JSON Protocol version {protocol_version } is not yet ' +


Suggested change

f'JSON Protocol version {protocol_version } is not yet ' +

f'JSON Protocol version {protocol_version} is not yet ' +

feat(api): define & execute v3 json protocols

899a642

* define v3 json protocol schema * support v3 JSON protocol execution in APIv2 executor Closes #3110

IanLondon added api Affects the `api` project ready for review json protocol schema labels Apr 5, 2019

IanLondon requested review from mcous, sfoster1, b-cooper, btmorr and Laura-Danielle April 5, 2019 23:01

IanLondon mentioned this pull request Apr 5, 2019

feat(shared-data): create v3 JSON protocol schema def #3307

Closed

IanLondon added WIP and removed ready for review labels Apr 5, 2019

add tests

2b08a48

IanLondon added ready for review WIP and removed WIP ready for review labels Apr 8, 2019

split out v1 vs v3 execution code

519bf65

IanLondon added ready for review and removed WIP labels Apr 8, 2019

IanLondon mentioned this pull request Apr 8, 2019

feat(api): publish pause and delay commands in python and JSON #3310

Merged

3 tasks

sfoster1 reviewed Apr 8, 2019

View reviewed changes

api/src/opentrons/protocol_api/execute.py Outdated Show resolved Hide resolved

fixup typo

db4c2e2

sfoster1 requested changes Apr 8, 2019

View reviewed changes

api/src/opentrons/protocol_api/execute_v3.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_api/execute_v3.py Outdated Show resolved Hide resolved

move json dispatch fixtures to files; do not use Dict.get on required…

e59529c

… keys

Laura-Danielle requested changes Apr 8, 2019

View reviewed changes

IanLondon added 2 commits April 8, 2019 14:33

type cleanup; move trough def to fixture json

a9fa1fa

use dispatcher map for v3 executor

6505a6b

cleanup re: Laura PR comments

03c41c9

Laura-Danielle approved these changes Apr 8, 2019

View reviewed changes

b-cooper approved these changes Apr 8, 2019

View reviewed changes

mcous approved these changes Apr 9, 2019

View reviewed changes

IanLondon commented Apr 9, 2019

View reviewed changes

sfoster1 reviewed Apr 9, 2019

View reviewed changes

sfoster1 approved these changes Apr 9, 2019

View reviewed changes

delete extra space

e082c6d

IanLondon merged commit 988407d into edge Apr 9, 2019

IanLondon deleted the api_execute-v3-json-protocols branch April 9, 2019 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): define & execute v3 json protocols #3312

feat(api): define & execute v3 json protocols #3312

IanLondon commented Apr 5, 2019 •

edited

Loading

codecov bot commented Apr 5, 2019 •

edited

Loading

sfoster1 left a comment

sfoster1 Apr 8, 2019

IanLondon Apr 8, 2019

Laura-Danielle Apr 8, 2019

sfoster1 Apr 8, 2019

sfoster1 Apr 8, 2019

sfoster1 Apr 8, 2019

sfoster1 Apr 8, 2019

sfoster1 Apr 8, 2019

sfoster1 Apr 8, 2019

sfoster1 Apr 8, 2019

Laura-Danielle Apr 8, 2019

Laura-Danielle Apr 8, 2019

sfoster1 Apr 8, 2019

Laura-Danielle Apr 8, 2019

Laura-Danielle Apr 8, 2019

Laura-Danielle Apr 8, 2019

Laura-Danielle Apr 8, 2019

IanLondon Apr 8, 2019

Laura-Danielle left a comment

b-cooper left a comment

mcous left a comment

IanLondon Apr 9, 2019

sfoster1 Apr 9, 2019

sfoster1 Apr 9, 2019



		def _get_well(loaded_labware: Dict[str, labware.Labware],
		params: Dict[str, Any]):

		@@ -364,8 +159,22 @@ def run_protocol(protocol_code: Any = None,
		if None is not protocol_code:

	f'JSON Protocol version {protocol_version } is not yet ' +
	f'JSON Protocol version {protocol_version} is not yet ' +

feat(api): define & execute v3 json protocols #3312

feat(api): define & execute v3 json protocols #3312

Conversation

IanLondon commented Apr 5, 2019 • edited Loading

overview

changelog

review requests

codecov bot commented Apr 5, 2019 • edited Loading

Codecov Report

sfoster1 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

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

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

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

Laura-Danielle left a comment

Choose a reason for hiding this comment

b-cooper left a comment

Choose a reason for hiding this comment

mcous 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

IanLondon commented Apr 5, 2019 •

edited

Loading

codecov bot commented Apr 5, 2019 •

edited

Loading