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

fix(api): simulation allows aspirating and dispensing on a tip rack #7788

Merged
merged 14 commits into from
May 20, 2021
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ pip-delete-this-directory.txt

# node packages
node_modules
package-lock.json

# Unit test / coverage reports
htmlcov/
Expand Down
6 changes: 6 additions & 0 deletions api/docs/v2/versioning.rst
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ This table lists the correspondence between Protocol API versions and robot soft
+-------------+-----------------------------+
| 2.10 | 4.3.0 |
+-------------+-----------------------------+
| 2.11 | 4.3.1 |
celsasser marked this conversation as resolved.
Show resolved Hide resolved
+-------------+-----------------------------+


Changes in API Versions
Expand Down Expand Up @@ -207,3 +209,7 @@ Version 2.9
Version 2.10
++++++++++++
- In Python protocols requesting API version 2.10, moving to the same well twice in a row with different pipettes no longer results in strange diagonal movements.

Version 2.11
++++++++++++
- In Python protocols requesting API version 2.11, validation to prevent aspiration and dispensation to tip-racks.
celsasser marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 3 additions & 1 deletion api/src/opentrons/protocol_api/instrument_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from opentrons.protocols.advanced_control.mix import mix_from_kwargs
from opentrons.protocols.api_support.instrument import \
validate_blowout_location, tip_length_for, validate_tiprack, \
determine_drop_target
determine_drop_target, validate_can_aspirate, validate_can_dispense
from opentrons.protocols.api_support.labware_like import LabwareLike
from opentrons.protocol_api.module_contexts import ThermocyclerContext
from opentrons.protocols.api_support.util import (
Expand Down Expand Up @@ -183,6 +183,7 @@ def aspirate(self,
" method that moves to a location (such as move_to or "
"dispense) must previously have been called so the robot "
"knows where it is.")
validate_can_aspirate(dest)
celsasser marked this conversation as resolved.
Show resolved Hide resolved

if self.current_volume == 0:
# Make sure we're at the top of the labware and clear of any
Expand Down Expand Up @@ -286,6 +287,7 @@ def dispense(self,
" method that moves to a location (such as move_to or "
"aspirate) must previously have been called so the robot "
"knows where it is.")
validate_can_dispense(loc)
celsasser marked this conversation as resolved.
Show resolved Hide resolved

c_vol = self.current_volume if not volume else volume

Expand Down
2 changes: 1 addition & 1 deletion api/src/opentrons/protocols/api_support/definitions.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from .types import APIVersion

MAX_SUPPORTED_VERSION = APIVersion(2, 10)
MAX_SUPPORTED_VERSION = APIVersion(2, 11)
#: The maximum supported protocol API version in this release

V2_MODULE_DEF_VERSION = APIVersion(2, 3)
Expand Down
46 changes: 45 additions & 1 deletion api/src/opentrons/protocols/api_support/instrument.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import logging
from typing import Optional, Any
from typing import Optional, Any, Union

from opentrons import types
from opentrons.calibration_storage import get
Expand Down Expand Up @@ -104,3 +104,47 @@ def determine_drop_target(
assert tr.is_tiprack
z_height = return_height * tr.tip_length
return location.top(-z_height)


def validate_can_aspirate(
Copy link
Member

@sanni-t sanni-t May 12, 2021

Choose a reason for hiding this comment

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

(Ignore if you are already working on this) I think we can make a single validation for both aspirate and dispense. In fact, I feel like your _is_tiprack function might be just the thing we need.

Copy link
Contributor Author

@celsasser celsasser May 12, 2021

Choose a reason for hiding this comment

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

That makes sense, but I guess I was/am thinking two (or three) things:

  1. Does this conflict with this feedback - https://github.com/Opentrons/opentrons/pull/7788/files#r630606222?
  2. I was thinking about the possibilities of validation. Right now it is very simple, but conceptually (in a future where we know more about the state of the deck) it seems like it could be capable of more. Such as validating that there is stuff in the location. Or that there is a tube in the location. Or if dispensing is the tube going to overflow.... Maybe I am making up stuff that we don't care about.
  3. Isolating validation. It seems like it has the potential to be a whole layer of responsibility of its own? But also, I ran into lint complexity issues when I attempted to make the change inline (https://github.com/Opentrons/opentrons/pull/7788/files#diff-6811e2f4d45b8d159abed031a1b00c50e69ecc5aa7d8e19a674527fb4be3298cR186).

Please let me know what you think. My mission certainly isn't to unnecessarily change the way you guys do things.

Copy link
Member

@sanni-t sanni-t May 12, 2021

Choose a reason for hiding this comment

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

(^might be my uninformed suggestion, so let me know if there's a specific reason for having separate logic for aspirate vs dispense)

Copy link
Contributor Author

@celsasser celsasser May 12, 2021

Choose a reason for hiding this comment

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

At the moment, none. But in a future where we knew what was in them then I can see (provided I am right and aspirate means to suck stuff out of a location) different concerns for the two. Sort of like I was saying in #2 above. For example, what if nothing is in the location from which you are trying to aspirate. Or if a location is beyond capacity once a dispense is performed. And then there is the differences in messaging that we send back in the exception. But here I think you are suggesting that I move the test and exception throwing into instrument_context? There I ran into complexity issues with the linter, but That was a three line conditional vs. a two line conditional.

Copy link
Contributor Author

@celsasser celsasser May 12, 2021

Choose a reason for hiding this comment

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

Was just thinking about dispense and the possibilities if we were smart(er). For example, let's pretend that we know whatever is the pipette when mixed with whatever is in the destination will cause an 💥 . That made me laugh. Don't think we are anywhere close to that. And maybe the user wants to blow themselves up. Who are we to judge!?

Copy link
Member

Choose a reason for hiding this comment

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

Got it! Thanks for that explanation! So now I understand the thought process behind making two separate validations for aspirate & dispense but I still don't get why their logic is different. Can't both take a types.Location and just check for if labware.parent and labware.parent.is_tiprack:?

Copy link
Contributor Author

@celsasser celsasser May 13, 2021

Choose a reason for hiding this comment

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

@sanni-t - I changed it as a reaction to this feedback - #7788 (comment). It seems that we always know that the location is a Location and that being the case agreed with @amitlissack in that we don't need to add variability if we don't need it; thinking that it might lead to confusing conclusions (in the brains of other developers). (sorry, didn't fully understand what you were saying). Let me see....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sanni-t - yes, you are totally right. I think I was too literal about @amitlissack 's suggestion. I see now that they both are constrained to Location. Thanks and fixing.

location: Union[Labware, Well, types.Location]) -> None:
""" Can one aspirate on the given `location` or not? This method is
pretty basic and will probably remain so (?) as the future holds neat
ambitions for how validation is implemented. And as robots become more
intelligent more rigorous testing will be possible

Args:
location: target for aspiration

Raises:
RuntimeError:
"""
if _is_tiprack(location):
raise RuntimeError("Cannot aspirate a tiprack")


def validate_can_dispense(location: types.Location) -> None:
""" Can one dispense to the given `location` or not? This method is
pretty basic and will probably remain so (?) as the future holds neat
ambitions for how validation is implemented. And as robots become more
intelligent more rigorous testing will be possible

Args:
location: target for dispense

Raises:
RuntimeError:
"""
labware = location.labware.as_labware()
if labware.parent and labware.parent.is_tiprack:
raise RuntimeError("Cannot dispense to a tiprack")


def _is_tiprack(location: Union[Labware, Well, types.Location]) -> bool:
if isinstance(location, Labware):
return location.is_tiprack
elif isinstance(location, Well):
return location.parent.is_tiprack
else:
labware = location.labware.as_labware()
return labware.parent and labware.parent.is_tiprack
22 changes: 22 additions & 0 deletions api/tests/opentrons/data/bug_aspirate_tip.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from opentrons import protocol_api

# metadata
metadata = {
'protocolName': 'Bug 7552',
'author': 'Name <[email protected]>',
'description': 'Simulation allows aspirating and dispensing on a tip rack',
'apiLevel': '2.7'
}


# protocol run function. the part after the colon lets your editor know
# where to look for autocomplete suggestions
def run(protocol: protocol_api.ProtocolContext):
# labware
plate = protocol.load_labware('geb_96_tiprack_10ul', 4)

# pipettes
pipette = protocol.load_instrument('p300_single', 'left', tip_racks=[plate])

# commands
pipette.transfer(5, plate.wells_by_name()['A1'], plate.wells_by_name()['B1'])
29 changes: 28 additions & 1 deletion api/tests/opentrons/protocols/api_support/test_instrument.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

import pytest
from opentrons.protocol_api.labware import Well
from opentrons.protocols.api_support.instrument import determine_drop_target
from opentrons.protocols.api_support.instrument import determine_drop_target, \
validate_can_aspirate, \
validate_can_dispense
from opentrons.protocols.geometry.well_geometry import WellGeometry
from opentrons.protocols.context.well import WellImplementation
from opentrons.protocols.api_support.types import APIVersion
Expand Down Expand Up @@ -48,3 +50,28 @@ def test_determine_drop_target(
r = determine_drop_target(api_version, well, 0.5)
assert r.labware.object == well
assert r.point == expected_point


def test_validate_can_aspirate(ctx):
well_plate = ctx.load_labware('corning_96_wellplate_360ul_flat', 1)
tip_rack = ctx.load_labware('opentrons_96_tiprack_300ul', 2)
# test type `Labware`
validate_can_aspirate(well_plate)
# test type `Well`
validate_can_aspirate(well_plate.wells()[0])
# test type `Location`
validate_can_aspirate(well_plate.wells()[0].top())
with pytest.raises(RuntimeError):
validate_can_aspirate(tip_rack)
with pytest.raises(RuntimeError):
validate_can_aspirate(tip_rack.wells_by_name()['A1'])
with pytest.raises(RuntimeError):
validate_can_aspirate(tip_rack.wells_by_name()['A1'].top())


def test_validate_can_dispense(ctx):
well_plate = ctx.load_labware('corning_96_wellplate_360ul_flat', 1)
tip_rack = ctx.load_labware('opentrons_96_tiprack_300ul', 2)
validate_can_dispense(well_plate.wells()[0].top())
with pytest.raises(RuntimeError):
validate_can_dispense(tip_rack.wells_by_name()['A1'].top())
7 changes: 7 additions & 0 deletions api/tests/opentrons/test_simulate.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,3 +116,10 @@ def test_simulate_extra_labware(protocol, protocol_file, monkeypatch):
ctx = simulate.get_protocol_api('2.0')
with pytest.raises(FileNotFoundError):
ctx.load_labware("fixture_12_trough", 1, namespace='fixture')


@pytest.mark.parametrize('protocol_file', ['bug_aspirate_tip.py'])
def test_simulate_aspirate_tip(protocol, protocol_file, monkeypatch):
monkeypatch.setenv('OT_API_FF_allowBundleCreation', '1')
celsasser marked this conversation as resolved.
Show resolved Hide resolved
with pytest.raises(ExceptionInProtocolError):
simulate.simulate(protocol.filelike, 'bug_aspirate_tip.py')