Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): move-to-slot JSON protocol command #3242

Merged
merged 12 commits into from
Mar 26, 2019
Merged

Conversation

IanLondon
Copy link
Contributor

@IanLondon IanLondon commented Mar 21, 2019

overview

This an an experimental feature intended to allow users to control pipette position without reference to aspirating and dispensing, and without reference to labware/modules. Slot seems like a reasonable reference to use, especially since Slot 1's origin coincides with the origin of the deck. This should be generally useful for applications from colony picking to potential future deck calibration move-to's.

For details of the move-to-slot command params, see the new JSON schema v2 file (called out in a comment below).

The move-to-slot command can be marked as non-experimental once we move past APIv1, which limits control of the move_to command. But as-is, the behavior is different in APIv1 vs APIv2.

This addition necessitates a new JSON schema, version 2. PD will stay at JSON Protocol Schema v1, because it doesn't read commands and doesn't need to write move-to-slot commands anytime soon. I expect we'll jump PD from JSON protocol v1 right to v3 with embedded labware defs and other major changes. I also expect we'll carry over this move-to-slot command to JSON protocol schema v3 by camel-casing the keys and not changing anything else with the command.

changelog

  • add args to InstrumentContext.move_to and plan_moves to allow more control of motion
  • new JSON protocol schema v2 - only addition is new move-to-slot command
  • support move-to-slot command in APIv1 and APIv2 JSON executors

review requests

Here is a test file moveToTest.json.zip - to run in APIv2, change the labware on L61 from "96-flat" to "generic_96_wellplate_380_ul".

  • Review JSON schema with JSON Schema Validator (test the move-to-slot command, it should fail validation on nonsense inputs and pass on good inputs)
  • New "description" text is JSON is sufficiently readable as documentation
  • Python API code review
  • Test on robot with API v1, with listed limitations
  • Test on robot with API v2, force-direct and minimum-z-height should have full functionality

sfoster1 and others added 10 commits March 20, 2019 12:40
This could let people move to things in their protocols and force direct motion
or an arc through a specific height, similar to the strategy keyword in the
legacy api move.
* create new JSON Schema v2 def, with move-to-well command
* APIv1 and APIv2 JSON executors support move-to-well cmd
@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #3242 into edge will increase coverage by 1.32%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3242      +/-   ##
==========================================
+ Coverage   53.71%   55.04%   +1.32%     
==========================================
  Files         699      701       +2     
  Lines       20470    21621    +1151     
==========================================
+ Hits        10996    11901     +905     
- Misses       9474     9720     +246
Impacted Files Coverage Δ
api/src/opentrons/protocol_api/execute.py 80.41% <0%> (-3.46%) ⬇️
api/src/opentrons/protocol_api/geometry.py 98.09% <100%> (+0.01%) ⬆️
api/src/opentrons/protocol_api/contexts.py 84.38% <100%> (+0.02%) ⬆️
protocol-designer/src/labware-ingred/utils.js 20% <0%> (-13.34%) ⬇️
...col-designer/src/labware-ingred/actions/actions.js 39.53% <0%> (-2.41%) ⬇️
api/src/opentrons/config/__init__.py 65.88% <0%> (-1.26%) ⬇️
api/src/opentrons/config/robot_configs.py 98.66% <0%> (-0.19%) ⬇️
...mponents/InstrumentSettings/ModulesCardContents.js 0% <0%> (ø) ⬆️
...StepEditForm/forms/MoveLiquid/SourceDestHeaders.js 0% <0%> (ø) ⬆️
...evel/getDisabledFields/getDisabledFieldsMixForm.js 0% <0%> (ø) ⬆️
... and 19 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 341385c...b1d91ae. Read the comment docs.

}
},

{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the only new section

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to change a docstring but other than that good

:param minimum_z_height: An optional height to retract the pipette to
before moving. If not specified, it will be generated
based on the labware from which and to which the
pipette is moving; if it is 0, the pipette will move
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't accurate anymore I think


elif command_type == 'move-to-slot':
slot = params.get('slot')
if slot not in [str(s+1) for s in range(12)]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we do slot in context.deck

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's too permissive b/c it will work fine with integers even though the schema says it must be a string: "enum": ["1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12"].

But I'm pretty sure 1 in context.deck and "1" in context.deck will both be True... didn't test though

Probably the best thing to do is to throw before execution even begins if the protocol does not conform to a schema supported by the executor, but until we have something like that I don't want users to run out-of-spec JSON and have things work fine - then they'll not bother making it a string and we'll have a tough time making a migration script to move their protocols to future JSON schema versions :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree with that but still, "what the deck will accept" is exactly the source of truth we should be checking here, not duplicating logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Seth about duplicating logic and to extend that thought, this would be a great place to start using the ot2Standard.json deck definition in shared data. And a helper method to grab the slot ids from the definition would return an array of strings by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source of truth for the JSON executor about what inputs are valid should always be the JSON Protocol schema def, not the Deck class of a certain version of the Python API, or anything else outside the JSON Protocol schema def matching the protocol version of the uploaded protocol.

Permissivity in the executors is dangerous. Our JSON Protocol schema is not intended to be read only by the JSON executor(s) in our Python server/API. For example, we might want to display commands info in Run App or Protocol Library from the JSON file directly. If we encourage protocols to deviate from the schema definition by having a permissive executor, our JSON protocols "in the wild" will become much harder to work in all these cases. As a superuser, if my protocol runs fine when I use integers for slots, I'm not going to really care if I'm going against the schema -- I likely won't even know that my protocol is invalid because it runs fine on the robot. Then if Run App wants to add a new feature that reads the slots param and expects a string, we have to either make users convert their invalid protocols to string slots, or handle that out-of-spec case to the application.

I think the long-term right thing to do is adding JSON schema validation checks to the executors, which would throw an error if the uploaded protocol does not validate under the JSON Protocol schema that matches that uploaded protocol's schema version ("protocol-schema" / "protocolSchema" key). We should probably do that pretty soon. Then, we can take this type of "is the input valid?" assertions out of the executors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌾 This is an awesome addition.


elif command_type == 'move-to-slot':
slot = params.get('slot')
if slot not in [str(s+1) for s in range(12)]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Seth about duplicating logic and to extend that thought, this would be a great place to start using the ot2Standard.json deck definition in shared data. And a helper method to grab the slot ids from the definition would return an array of strings by default.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I do need one change reverted before you merge

@@ -5,8 +5,8 @@
from opentrons.protocol_api import labware
from opentrons.hardware_control.types import CriticalPoint

labware_name = 'generic_96_wellplate_380_ul'
trough_name = 'usa_scientific_12_trough_22_ml'
labware_name = 'generic_96_wellPlate_380_uL'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you revert these changes? All lower-case is now the correct thing to do

@codecov
Copy link

codecov bot commented Mar 26, 2019

Codecov Report

Merging #3242 into edge will increase coverage by 1.32%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3242      +/-   ##
==========================================
+ Coverage   53.71%   55.04%   +1.32%     
==========================================
  Files         699      701       +2     
  Lines       20470    21621    +1151     
==========================================
+ Hits        10996    11901     +905     
- Misses       9474     9720     +246
Impacted Files Coverage Δ
api/src/opentrons/protocol_api/execute.py 80.41% <0%> (-3.46%) ⬇️
api/src/opentrons/protocol_api/geometry.py 98.09% <100%> (+0.01%) ⬆️
api/src/opentrons/protocol_api/contexts.py 84.38% <100%> (+0.02%) ⬆️
protocol-designer/src/labware-ingred/utils.js 20% <0%> (-13.34%) ⬇️
...col-designer/src/labware-ingred/actions/actions.js 39.53% <0%> (-2.41%) ⬇️
api/src/opentrons/config/__init__.py 65.88% <0%> (-1.26%) ⬇️
api/src/opentrons/config/robot_configs.py 98.66% <0%> (-0.19%) ⬇️
...mponents/InstrumentSettings/ModulesCardContents.js 0% <0%> (ø) ⬆️
...StepEditForm/forms/MoveLiquid/SourceDestHeaders.js 0% <0%> (ø) ⬆️
...evel/getDisabledFields/getDisabledFieldsMixForm.js 0% <0%> (ø) ⬆️
... and 19 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 341385c...b1d91ae. Read the comment docs.

@IanLondon IanLondon merged commit cef5123 into edge Mar 26, 2019
@IanLondon IanLondon deleted the api_move-to-slot-command branch March 26, 2019 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project json protocol schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants