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

celsasser · 2021-05-11T14:56:06Z

Overview

The robot does not raise an error or warning when trying to aspirate and dispense on a tip rack. This occurs during the simulation and the tip rack is from the Opentrons labware library, not customized.

I am doing the most basic fix and testing which is to test whether destination is a tip-rack. I don't like it. It seems like there is a nicer and more general solution to validation problems. Though it looks like validation is often combined with type constraining. But have since learned/been-reminded that there is an architectural revision planned in the near future that will revise how validation is done. So, in the meantime I am sticking with the fix within.

Changelog

refactor aspirate and dispense to see if the destination is a tiprack and raise runtime error if it is.
create a test protocol to unit test
adding change to .gitignore to include package-lock.json as it does not seem to be in the project but is generated if one does npm install on the root.

Review requests

Is there a convention we use for error messages. @mcous, I think you mentioned centralizing errors and error templates?
~~Do we want to try to centralize validation. I understand that it happens at different levels, but think there could be value in attempting to group the functionality, the messaging, possible re-use.~~ See overview.
I am not familiar enough with the product to know whether tipracks are the only possible labware that may be attempted to be aspirated and dispensed to. Are there other things on the deck that we want want to beware of? Or in other words, is there a better way to know whether a target supports operations such as aspirate and dispense?

Risk assessment

None, provided nobody aspirates tips.

Doing the most basic fix and testing whether the destination is a tip-rack. I don't like it. It seems like there is a nicer and more general solution to validation problems. Though it looks like validation is often combined with type constraining. Will either put up a draft PR or pursue alternative: - refactor `aspirate` to see if the destination is a tiprack and raise runtime error if it is. - create a test protocol to unit test - adding change to .gitignore to include package-lock.json as it does not seem to be in the project but is generated if one does `npm install` on the root.

…vention already used (though no bug protocol scripts, I don't think)

codecov · 2021-05-11T14:56:12Z

Codecov Report

Merging #7788 (09af36a) into edge (33dd2b3) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #7788      +/-   ##
==========================================
+ Coverage   83.46%   83.48%   +0.01%     
==========================================
  Files         333      333              
  Lines       21215    21228      +13     
==========================================
+ Hits        17708    17722      +14     
+ Misses       3507     3506       -1

Impacted Files	Coverage Δ
opentrons/protocols/api_support/instrument.py	`100.00% <0.00%> (ø)`
opentrons/protocol_api/instrument_context.py	`89.16% <0.00%> (+0.10%)`	⬆️
opentrons/simulate.py	`65.08% <0.00%> (+0.59%)`	⬆️

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 33dd2b3...09af36a. Read the comment docs.

…t complexity. And happily he did 'cause I totally disregarded dispense. Have since: - moved validation to keep validation in instrument.py company - unit test them - fix linting issues

amitlissack

Less than 7 days in and you are venturing into some of our trickiest code. You are a brave soul.

api/src/opentrons/protocol_api/instrument_context.py

…e_can_dispense` to locations as it cannot be anything other - updating unit tests

celsasser · 2021-05-12T15:32:45Z

Less than 7 days in and you are venturing into some of our trickiest code. You are a brave soul.

Haha, thanks! It was really educational and also touched a number of things and made me know how little I know...which I should have known...

sanni-t

Definitely some of our trickiest code

api/tests/opentrons/test_simulate.py

sanni-t · 2021-05-12T16:48:10Z

api/src/opentrons/protocols/api_support/instrument.py

@@ -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(


(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.

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

Does this conflict with this feedback - https://github.com/Opentrons/opentrons/pull/7788/files#r630606222?

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.

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.

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

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.

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!?

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:?

@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....

@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.

mcous

This changeset looks pretty good to me! Only putting this as a "change request" to make sure we don't accidentally merge before sorting out the API version

api/src/opentrons/protocol_api/instrument_context.py

mcous

API version bump changes look good. Added additional context for what I meant by "adding a gate"

api/docs/v2/versioning.rst

api/src/opentrons/protocol_api/instrument_context.py

…using lint complexity issues. Silencing 'cause I don't feel like refactoring the method as there is no complexity introduced that the method has not already had a taste of.

…ain types that may be aspirated and dispensed to `Location`

Co-authored-by: Mike Cousins <[email protected]>

mcous

🎥

curtelsasser added 2 commits May 10, 2021 17:47

- refactor test protocol name so that it doesn't deviate from the con…

f6fe164

…vention already used (though no bug protocol scripts, I don't think)

celsasser requested a review from sfoster1 May 11, 2021 14:56

celsasser requested review from a team as code owners May 11, 2021 14:56

celsasser removed the request for review from sfoster1 May 11, 2021 14:58

celsasser closed this May 11, 2021

SyntaxColoring linked an issue May 11, 2021 that may be closed by this pull request

bug: simulation allows aspirating and dispensing on a tip rack #7552

Closed

curtelsasser added 2 commits May 11, 2021 16:18

Refactoring my initial approach 'cause old man linter complained abou…

32f882a

…t complexity. And happily he did 'cause I totally disregarded dispense. Have since: - moved validation to keep validation in instrument.py company - unit test them - fix linting issues

Merge remote-tracking branch 'origin/edge' into api_fix_aspirate_tip

0c67aad

celsasser reopened this May 11, 2021

- fix my comments

e873a6e

celsasser force-pushed the api_fix_aspirate_tip branch from e9820f9 to e873a6e Compare May 11, 2021 20:41

- fix linting

5db7343

amitlissack reviewed May 11, 2021

View reviewed changes

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

curtelsasser added 2 commits May 12, 2021 10:56

Merge remote-tracking branch 'origin/edge' into api_fix_aspirate_tip

2153b77

- addressing PR feedback and updating such that we constrain `validat…

c5a9e21

…e_can_dispense` to locations as it cannot be anything other - updating unit tests

sanni-t reviewed May 12, 2021

View reviewed changes

mcous suggested changes May 12, 2021

View reviewed changes

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

- bumping version as requested and documenting change

5535701

mcous reviewed May 12, 2021

View reviewed changes

api/docs/v2/versioning.rst Outdated Show resolved Hide resolved

api/docs/v2/versioning.rst Outdated Show resolved Hide resolved

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

curtelsasser added 2 commits May 12, 2021 16:23

- API version checking validation and have come full circle and am ca…

6983b63

…using lint complexity issues. Silencing 'cause I don't feel like refactoring the method as there is no complexity introduced that the method has not already had a taste of.

- addressing more feedback which pointed out that I am able to constr…

dc62508

…ain types that may be aspirated and dispensed to `Location`

celsasser force-pushed the api_fix_aspirate_tip branch from 03dc712 to dc62508 Compare May 13, 2021 19:23

celsasser and others added 3 commits May 14, 2021 11:19

- taking suggested change to change log

55a3478

Co-authored-by: Mike Cousins <[email protected]>

- update version to next release as suggested

ede0869

Co-authored-by: Mike Cousins <[email protected]>

Merge remote-tracking branch 'origin/edge' into api_fix_aspirate_tip

09af36a

mcous approved these changes May 20, 2021

View reviewed changes

celsasser merged commit 1d624d8 into edge May 20, 2021

celsasser deleted the api_fix_aspirate_tip branch May 20, 2021 14:33

mcous mentioned this pull request Aug 16, 2022

fix(api): allow aspirate/dispense at arbitrary locations #11352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

celsasser commented May 11, 2021 •

edited

Loading

codecov bot commented May 11, 2021 •

edited

Loading

amitlissack left a comment

celsasser commented May 12, 2021

sanni-t left a comment

sanni-t May 12, 2021 •

edited

Loading

celsasser May 12, 2021 •

edited

Loading

sanni-t May 12, 2021 •

edited

Loading

celsasser May 12, 2021 •

edited

Loading

celsasser May 12, 2021 •

edited

Loading

sanni-t May 13, 2021

celsasser May 13, 2021 •

edited

Loading

celsasser May 13, 2021

mcous left a comment

mcous left a comment

mcous left a comment

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

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

Conversation

celsasser commented May 11, 2021 • edited Loading

Overview

Changelog

Review requests

Risk assessment

codecov bot commented May 11, 2021 • edited Loading

Codecov Report

amitlissack left a comment

Choose a reason for hiding this comment

celsasser commented May 12, 2021

sanni-t left a comment

Choose a reason for hiding this comment

sanni-t May 12, 2021 • edited Loading

Choose a reason for hiding this comment

celsasser May 12, 2021 • edited Loading

Choose a reason for hiding this comment

sanni-t May 12, 2021 • edited Loading

Choose a reason for hiding this comment

celsasser May 12, 2021 • edited Loading

Choose a reason for hiding this comment

celsasser May 12, 2021 • edited Loading

Choose a reason for hiding this comment

sanni-t May 13, 2021

Choose a reason for hiding this comment

celsasser May 13, 2021 • edited Loading

Choose a reason for hiding this comment

celsasser May 13, 2021

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

mcous left a comment

Choose a reason for hiding this comment

celsasser commented May 11, 2021 •

edited

Loading

codecov bot commented May 11, 2021 •

edited

Loading

sanni-t May 12, 2021 •

edited

Loading

celsasser May 12, 2021 •

edited

Loading

sanni-t May 12, 2021 •

edited

Loading

celsasser May 12, 2021 •

edited

Loading

celsasser May 12, 2021 •

edited

Loading

celsasser May 13, 2021 •

edited

Loading