feat(api): add v4 JSON executor #5221

shlokamin · 2020-03-12T18:47:47Z

overview

This PR closes #4868 by adding the v4 JSON executor to the api

changelog

Add support for V4 JSON schema in v4 executor
Move temperature module await temperature command down to module level
Update versioning notes
Updated API docs to mention only two pipettes are supported
Add start_set_temp and await_temp to run log

review requests

Code review
Create a protocol with magnetic + temperature modules in PD with their respective steps. Upload to an OT-2 using the run app and validate that the protocol runs smoothly.
Test the new temperature module start_set_temperature and await_temperature commands.
Test both gen 1 and gen2 modules
Make sure commands show up in run log. Note, await temp will show up as a "pause" in the run log
~~Regression test that protocols that do not use modules still behave as expected~~ Nevermind if you export a protocol without modules it will export as a v3 protocol, and therefore use the v3 executor

…3-executor-test branch)

…e.py

…tor-test

#4868

codecov · 2020-03-13T13:53:02Z

Codecov Report

Merging #5221 into edge will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             edge    #5221   +/-   ##
=======================================
  Coverage   62.14%   62.14%           
=======================================
  Files        1054     1054           
  Lines       30225    30225           
=======================================
  Hits        18784    18784           
  Misses      11441    11441

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 312599f...312599f. Read the comment docs.

) * create system/wifi module and move some functions from server/endpoints/networking there. * more refactoring * code clean up and restructuring. * move eap option validation from network endpoints to wifi. rewrote test_key_lifecycle to test the key api. not the http api. * reworked test_networking_endpoints to only test the http interaction with the key functions. Not the underlying file system. That belongs in the test_wifi module. * test_network_status added to nmcli. this didn't belong in the server endpoint tests as it is really testing our ability to communicate with nmcli. * remove deprecated comment * start robot_server networking endpoints * progress * keys is a reserved word. use an alias. * list keys tests pass * networking tests pass minus the disconnect * disconnect tests * tests for wificonfiguration model validation * add todo note regarding wifi disconnect * fix lint errors * cannot import log_control.MAX_RECORDS due to syslog import failure. * clean up the instances of multiple response types. * rename V1ErrorResponse to V1BasicResponse. It isn't always used for errors. Name was misleading. * fix bad test * fix lint error * fix bugs that were revealed when running on PI. Configure didn't work because we weren't testing how the nmcli.configure call was made. get wifi/keys failed due to validation errors on pydantic model due to attribute named keys. also, someone (me) didn't port the aiohttp unit tests properly. closes #5143

…cutor

sfoster1

Preliminary change requests to make this a bit more idiomatic python and support cancel

sfoster1 · 2020-03-13T15:57:51Z

api/src/opentrons/protocol_api/execute_v4.py

+    return modules_by_id
+
+
+def _engage_magnet(modules, params) -> None:


We should type the args here. It may be a good idea to make a LoadedModules type variable so we don't have to have Dict[str, ModuleContext] or whatever there all the time

sfoster1 · 2020-03-13T16:04:45Z

api/src/opentrons/protocol_api/execute_v4.py

+    # while holding, because the temperature delta will be exceeded.
+    # when this flicker happens, this function would sleep for a moment longer
+
+    if status == 'heating':


We should push this logic down all the way to the hardware_control.modules.TemperatureModule, so cancelling works here.

The way cancel is currently implemented is in the execution manager in hardware control with an asyncio condition wrapping a state that can be running, paused, or cancelling. When you call a hardware control function that checks the condition (or one of the whitelisted ones for cancellation), you can be cancelled.

As this code stands, the cancellation won't actually happen here - you could be awaiting for a very long time, especially if there's an error in the tempdeck communications.

Making a tempdeck.await_temperature(target: float) function will fix this. We can register it as cancellable with the execution manager, which means it will get cancelled when a cancel happens; or we can implement some more complex logic that pairs monitoring the condition with awaiting the temperature.

The other open question is, what's supposed to happen to the protocol state when this is running? How does it interact with pause? This is as much a UX question as a technical question (@howisthisnamenottakenyet ).

Eek, forgot about cancel. I'm fine with moving this down to the module level, that means we're exposing another "hidden" thing to the module context though. But I guess there isn't really another option.

it's really of equal status to the other one though - they come as a pair.

Honestly it's reasonable at this point to unhide it - we wanted to keep it hidden because you could set the temperature, but waiting for it wasn't exposed. If properly waiting for it is exposed, we might as well have it be part of the python api. What do you think @pantslakz and @howisthisnamenottakenyet

sfoster1 · 2020-03-13T16:08:10Z

api/src/opentrons/protocol_api/execute_v4.py

+            asyncio.sleep(0.2)
+
+
+dispatcher_map: Dict[Any, Any] = {


This map and the code in dispatch_json looks like it will work, but I'd prefer to avoid magic strings in favor of enums - it's a bit easier to type systematically, since we don't have literal types (yet) in mypy. In addition, python enums can have arbitrary objects as values, so you could have something like

class JsonCommandHandler(NamedTuple): # could also be a typed generic passed down to the handler element (see above re: C++ brain) name: str handler: Callable[] # appropriately type class JsonCommand: "delay": JsonCommandHandler(...)

api/src/opentrons/protocol_api/execute_v4.py

sfoster1 · 2020-03-13T16:12:04Z

api/src/opentrons/protocol_api/module_geometry.py

-                         'Valid names (ignoring case): '
-                         '"' + '", "'.join(alias_map.keys()) + '"')
+                         'Valid names (ignoring case): ''"' + '", "'
+                         .join(model_map.keys()) + '", "' + '", "'


This is intended to be a literate error for python protocols; it may be better to avoid mentioning the model_map at all, or to explicitly mention it as a lesser alternative, something like:

"Valid names (ignoring case): {alias_map.keys()}

You can also refer to modules by their exact model: {model_map.keys()}"

sfoster1 · 2020-03-13T16:13:05Z

api/src/opentrons/protocol_api/module_contexts.py

@@ -232,6 +232,16 @@ def target(self):
        """ Current target temperature in C"""
        return self._module.target

+    @property  # type: ignore
+    @requires_version(2, 0)


This should require version 2.3, since it will be new in 2.3. Also, changes or additions to api-exposed methods should be accompanied with a change note in the version docs

closes #5119

Closes #5117 * refactor add/select step selectors for mocking, and add tests

api/src/opentrons/hardware_control/modules/tempdeck.py

sfoster1 · 2020-03-16T19:01:06Z

api/src/opentrons/hardware_control/modules/tempdeck.py

+
+        t = self._loop.create_task(_await_temperature(awaiting_temperature))
+        await self.make_cancellable(t)
+        return await t


what does this return? if it's nothing, we don't need to return; if it's something, we should annotate the return type in both the inner closure and the wrapper

sfoster1 · 2020-03-16T19:01:32Z

api/src/opentrons/hardware_control/modules/tempdeck.py

+            status = self.status
+
+            if status == 'heating':
+                while (self.temperature < awaiting_temperature):


unnecessary parens

sfoster1 · 2020-03-16T19:01:41Z

api/src/opentrons/hardware_control/modules/tempdeck.py

+                    asyncio.sleep(0.2)
+
+            elif status == 'cooling':
+                while (self.temperature > awaiting_temperature):


unnecessary parens

Clear out engageHeight from MAGNET steps when magnetic module model is edited. Closes #5225

…on (#5259) Closes #5227

Closes #5226

…api (#5252) * robot endpoint progress: home, light, and position implemented with unittests. * add test for mount with z too low to aiohttp * move api implemented and unit tested. moved camera into its own router. * lint issues fixed * move ThreadAsyncLock into hardware_control package. * rename to threaded_async_lock and at it to hardware_control export * use ThreadedAsyncLock in home and move handlers. * fix lint errors * add model validator tests. more clean up. closes #5191

This commit make a new function _do_plunger_home which homes a plunger on a mount, if it exists, and then moves that plunger to its configured bottom. This is now used both by API.home_plunger (which was where this behavior was before) but also by a call to API.home() that involves pipettes, ensuring that plungers are always moved to the bottom after they are homed. Closes #5164

…5187) Avoids an issue where the ejector would contact the tip rack body. Closes #5186

…Height (#5262) Closes #5230 * introduce `.meta` to `HydratedFormData`

This reverts commit 9cb4940.

sfoster1

LGTM once qa approves. In the future we probably shouldn't merge edge back into longstanding branches though, it really makes a mess of the diff.

shlokamin · 2020-03-24T22:42:28Z

Yeah that's my bad. I saw there was a conflict in the versioning notes so I thought I would merge and fix, but it really just made the tree unintelligible

sfoster1 and others added 19 commits February 26, 2020 12:41

wip modules gen2

0c95ca7

fixup: address comments, fix tests

ef90f66

fixup: actually fix tests

48eee9d

fixup: fix lint

3b85269

fixup: changes to make FE lives easier

0d1eef8

wip: try using mocks instead of MockPipette. (based on api_refactor-v…

682de0c

…3-executor-test branch)

flow_rate spies working with ugly verbose mocking

4726514

unit tests, no integration-with-mock

2f2c69e

fix linting

9db1a6e

refactor to avoid passing protocol_data around for no reason

73e8aaf

refactor v3 executor helper fns to use minimum set of args

8a4fe9d

execute_v4 stubs

1b1dcd5

take executor helpers and tests as far as we can waiting on other work

b1d5627

implement load_modules_from_json, start hooking v4 executor to execut…

6411f9e

…e.py

Merge remote-tracking branch 'origin/edge' into api_refactor-v3-execu…

cb48f02

…tor-test

feat(api): add v4 JSON executor

2f36339

#4868

feat(api): add v4 JSON executor

d3c68ec

#4868

feat: include all vscode configs in gitignore (#5222)

30a4e97

fix error string interpolation

46b3412

amitlissack and others added 2 commits March 13, 2020 10:55

Merge branch 'api_refactor-v3-executor-test' into api_add-v4-json-exe…

d4950b6

…cutor

sfoster1 requested changes Mar 13, 2020

View reviewed changes

Kadee80 and others added 4 commits March 13, 2020 17:19

feat(protocol-designer): Add module prefix to labware (#5205)

1f979dd

closes #5119

feat(protocol-designer): add modal to add a pause after set temp (#5182)

dbae680

Closes #5117 * refactor add/select step selectors for mocking, and add tests

move temperature module await temperature command down to module level

e0660e6

add versioning notes

4f87ac3

shlokamin commented Mar 16, 2020

View reviewed changes

api/src/opentrons/hardware_control/modules/tempdeck.py Show resolved Hide resolved

format test json protocol

5dba05c

sfoster1 reviewed Mar 16, 2020

View reviewed changes

IanLondon and others added 14 commits March 23, 2020 16:51

feat(protocol-designer): clear out engageHeight (#5254)

2417071

Clear out engageHeight from MAGNET steps when magnetic module model is edited. Closes #5225

docs(shared-data): update v4 protocol schema - engageHeight descripti…

2e631c0

…on (#5259) Closes #5227

feat(shared-data): correct existing labware defs engageHeight (#5261)

767054d

Closes #5226

use enum values as keys

16dc5f0

fix(api): return tips to tipracks from the same height as drop tip (#…

82187ed

…5187) Avoids an issue where the ejector would contact the tip rack body. Closes #5186

feat(protocol-designer): use different ranges for gen1 vs gen2 engage…

434ca61

…Height (#5262) Closes #5230 * introduce `.meta` to `HydratedFormData`

compute model name constant

9cb4940

update load module test to include all module names and aliases

23c2652

update labware test

9c52315

Revert "compute model name constant"

d34d640

This reverts commit 9cb4940.

add json parse test for v4 fixture

6e76f02

add json execute smoke test for v4 fixture

f0d4ca2

shlokamin requested a review from sfoster1 March 24, 2020 15:40

shlokamin added 2 commits March 24, 2020 13:30

fix lowercase name in test

734055f

Merge branch 'api_add-v4-json-executor' into edge

b0ec698

shlokamin requested a review from a team March 24, 2020 17:58

shlokamin requested review from a team as code owners March 24, 2020 17:58

shlokamin added 2 commits March 24, 2020 17:51

add require version for start set temperature command

dc326cd

add start set temp and await temp to run log

d7b1090

sfoster1 approved these changes Mar 24, 2020

View reviewed changes

shlokamin added 3 commits March 25, 2020 11:38

remove await temperature pause from the run log

6f86ed8

add await temp command to run log

ca6dd95

remove unnecessary msg argument

312599f

nusrat813 approved these changes Mar 25, 2020

View reviewed changes

shlokamin merged commit e81cb56 into edge Mar 25, 2020

mcous deleted the api_add-v4-json-executor branch May 6, 2020 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add v4 JSON executor #5221

feat(api): add v4 JSON executor #5221

shlokamin commented Mar 12, 2020 •

edited

Loading

codecov bot commented Mar 13, 2020 •

edited

Loading

sfoster1 left a comment

sfoster1 Mar 13, 2020

sfoster1 Mar 13, 2020

shlokamin Mar 13, 2020

sfoster1 Mar 13, 2020

sfoster1 Mar 13, 2020

sfoster1 Mar 13, 2020

sfoster1 Mar 13, 2020

sfoster1 Mar 16, 2020

sfoster1 Mar 16, 2020

sfoster1 Mar 16, 2020

sfoster1 left a comment

shlokamin commented Mar 24, 2020

		return modules_by_id


		def _engage_magnet(modules, params) -> None:

feat(api): add v4 JSON executor #5221

feat(api): add v4 JSON executor #5221

Conversation

shlokamin commented Mar 12, 2020 • edited Loading

overview

changelog

review requests

codecov bot commented Mar 13, 2020 • 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

sfoster1 left a comment

Choose a reason for hiding this comment

shlokamin commented Mar 24, 2020

shlokamin commented Mar 12, 2020 •

edited

Loading

codecov bot commented Mar 13, 2020 •

edited

Loading