fix(api): allow aspirate/dispense at arbitrary locations #11352
Conversation
Codecov Report
@@ Coverage Diff @@
## release_6.1.0 #11352 +/- ##
=================================================
+ Coverage 66.59% 67.35% +0.76%
=================================================
Files 447 464 +17
Lines 6624 6855 +231
Branches 1173 1173
=================================================
+ Hits 4411 4617 +206
- Misses 2014 2039 +25
Partials 199 199
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| if location.labware.is_module and reject_module: | ||
| raise ValueError("Cannot aspirate/dispense directly to a module") |
There was a problem hiding this comment.
I agree with your thoughts in the PR description about this fundamentally being about confusion around Location.labware and its LabwareLike type.
In an ideal world, I think we could refactor things to reduce that confusion:
- The name of the
location.labwareattribute is confusing because it's not necessarily a labware.- We could rename it to something like
location.logical_anchor.
- We could rename it to something like
LabwareLikeis basically a union, but worse, because it erases the static type of the object.- Our methods could accept plain
Unions instead ofLabwareLiketo spell out more precisely what kinds of locations they accept.
- Our methods could accept plain
- The name of the
LabwareLiketype is confusing, in my opinion. Wells, modules, deck slots, andNoneare labware-unlike in many meaningful ways, but we mush them all into theLabwareLikebox.- Same solution as above: let methods spell out what they accept on a case-by-case basis.
We'd have to be careful to preserve backwards compatibility, and APIv2 might not be worth the investment. But...it would be nice.
| @@ -195,7 +195,9 @@ def aspirate( | |||
| "knows where it is." | |||
| ) | |||
| if self.api_version >= APIVersion(2, 11): | |||
| instrument.validate_can_aspirate(dest) | |||
| instrument.validate_takes_liquid( | |||
| location=dest, reject_module=self.api_version >= APIVersion(2, 13) | |||
There was a problem hiding this comment.
The reject_module bool arg is a nice way of handling this behavioral change across versions. 👍
| @@ -255,3 +255,4 @@ Version 2.13 | |||
|
|
|||
| - :py:meth:`.InstrumentContext.drop_tip` now has a ``prep_after`` parameter. | |||
| - :py:meth:`.InstrumentContext.home` may home *both* pipettes as needed to avoid collision risks. | |||
| - :py:meth:`.InstrumentContext.aspirate` and :py:meth:`.InstrumentContext.dispense` will avoid interacting directly with modules. | |||
There was a problem hiding this comment.
I think this validation is a good idea, but for my understanding: this is an independent change from the bug fix, right?
There was a problem hiding this comment.
Yes, came up naturally once the function started working with LabwareLike correctly. I decided that a fix guarded by the API version was a better move than leaving a TODO in the code, given that there was already a TODO pointing out the bug this PR set out to solve
There was a problem hiding this comment.
LGTM.
Agree with @SyntaxColoring 's points above about disambiguating locations in the future.
Co-authored-by: Max Marrone <[email protected]>
Overview
The addition of tip-rack checking for aspirate/dispense in #7788 introduced a regression where previously valid
aspirateanddispensecalls to arbitrary deck coordinates would fail. This PR fixes the problems with the logic that was added in that PR to ensureaspirateanddispensework as expected.Closes #11302, re: RSS-70
Changelog
Review requests
This bug was caused by misunderstandings with how to use the
LabwareLikeinterface viaLocation.labware. We're experiencing similar issues in #11296.See reproduction protocol in RSS-70 to validate that this PR does its job.
Risk assessment
Low