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): Fix floating point errors and backwards incompatibility in volume validation #14253

Merged
merged 19 commits into from
Jan 12, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Dec 19, 2023

Overview

This fixes two bugs: RQA-2001 and RESC-182.

RQA-2001 is that if you had an incorrect protocol like this:

pipette.pick_up_tip()
pipette.aspirate(50)
pipette.dispense(100)  # bad, invalid volume

...then it has started to raise an exception instead of silently clamping 100 to 50. This is a good thing to do, but it's a backwards-incompatible change, and it should have been behind an apiLevel bump. This PR makes the change opt-in, behind "apiLevel": "2.17".

RESC-182 is related. If you had a correct protocol like this:

requirements = {"robotType": "OT-2", "apiLevel": "2.16"}

def run(protocol):
    tip_rack = protocol.load_labware("opentrons_96_tiprack_300ul", location="9")
    well_plate = protocol.load_labware("opentrons_24_tuberack_eppendorf_1.5ml_safelock_snapcap", location="10")

    pipette = protocol.load_instrument("p300_single_gen2", mount="left", tip_racks=[tip_rack])

    pipette.pick_up_tip()
    pipette.distribute(
        volume=[22.7, 22.7],
        source=well_plate["A1"],
        dest=[well_plate["B1"], well_plate["B2"]],
        air_gap=10,
        new_tip="never",
        disposal_volume=0,
    )
    pipette.drop_tip()

...then it has started to raise "invalid volume" errors stemming from floating point rounding errors. This is more straightforwardly a bug; it should just not do that.

Test Plan

I've tested this with the protocols from the tickets.

Background

The long-standing historical behavior (since #2481 at least) seems to be that we clamp dispenses but do strict bounds-checking on aspirates. I'm not sure why they differed from each other. I'm also not sure how the strict bounds-checking on aspirates never caused any problems with floating point rounding.

In RSS-330 / PR #13546, we added strict bounds-checking to the Protocol Engine dispense command. We mistakenly thought we were fixing an omission and bringing simulation in line with execution, but we were actually making a breaking change to the interface. And, separately, we forgot about floating point error.

Changelog

  • At the Protocol Engine level, lean into doing strict bounds-checking everywhere, instead of clamping.
    • Unlike the historical behavior, do it for both aspirates and dispenses.
    • Add a tolerance to account for floating point rounding.
    • This is a potentially breaking change for anyone directly using Protocol Engine (e.g. generating JSON protocols, or using the HTTP /runs API). We could perhaps add a "clamp": true param to dispense to change that, but I suspect it's not worthwhile.
    • I don't believe this is a breaking change for anyone using Protocol Designer, because Protocol Designer already does its own bookkeeping of the pipette volume.
    • This is not a breaking change for anyone using the Python Protocol API, because of the next bullet point.
  • Preserve backwards-compatible behavior for Python Protocol API users by adding conditional clamping based on "apiLevel".

Review requests

  • Do we agree with the breaking change for the Protocol Engine dispense command?
  • Is it correct to apply this bounds-checking to only the virtual pipette handler, or do we also want to do it on the hardware pipette handler?

Risk assessment

Medium.

@SyntaxColoring SyntaxColoring added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Dec 19, 2023
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7a3b2da) 68.53% compared to head (3a51a90) 68.58%.
Report is 6 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14253      +/-   ##
==========================================
+ Coverage   68.53%   68.58%   +0.04%     
==========================================
  Files        2421     2513      +92     
  Lines       67108    71392    +4284     
  Branches     8823     9089     +266     
==========================================
+ Hits        45993    48961    +2968     
- Misses      19040    20340    +1300     
- Partials     2075     2091      +16     
Flag Coverage Δ
app 65.53% <ø> (-2.61%) ⬇️
g-code-testing 96.48% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 34.93% <ø> (ø)
shared-data 75.12% <ø> (+2.09%) ⬆️
step-generation 86.84% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
api/src/opentrons/hardware_control/ot3api.py 79.58% <ø> (ø)
...i/src/opentrons/protocol_api/instrument_context.py 89.18% <ø> (ø)
...src/opentrons/protocol_engine/commands/aspirate.py 100.00% <ø> (ø)
...src/opentrons/protocol_engine/commands/dispense.py 100.00% <ø> (ø)
...trons/protocol_engine/commands/pipetting_common.py 100.00% <ø> (ø)
...src/opentrons/protocol_engine/errors/exceptions.py 100.00% <ø> (ø)
...c/opentrons/protocol_engine/execution/pipetting.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/pipettes.py 100.00% <ø> (ø)
...src/opentrons/protocols/api_support/definitions.py 100.00% <ø> (ø)

... and 299 files with indirect coverage changes

@SyntaxColoring SyntaxColoring added api Affects the `api` project papi-v2 Python API V2 protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs labels Jan 2, 2024
@SyntaxColoring SyntaxColoring changed the base branch from chore_release-7.1.0 to edge January 10, 2024 20:49
@SyntaxColoring SyntaxColoring requested a review from a team January 10, 2024 21:29
@SyntaxColoring SyntaxColoring changed the title fix(api): Clamp dispense volumes for compatibility with older Protocol API versions fix(api): Fix floating point errors and backwards incompatibility in volume validation Jan 10, 2024
@SyntaxColoring SyntaxColoring force-pushed the backwards_compatible_dispense_clamping branch from 78c56ea to fe19e37 Compare January 10, 2024 22:27
@SyntaxColoring SyntaxColoring force-pushed the backwards_compatible_dispense_clamping branch from fe19e37 to fc9e39b Compare January 10, 2024 22:30
@SyntaxColoring SyntaxColoring removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Jan 10, 2024
@SyntaxColoring SyntaxColoring marked this pull request as ready for review January 10, 2024 23:10
@SyntaxColoring SyntaxColoring requested review from a team as code owners January 10, 2024 23:10
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.

Yeah, this looks good to me! I agree with the PE dispense change, nice fix.

Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

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

LGTM, keeps everything consistent now.

Copy link
Contributor

Choose a reason for hiding this comment

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

If everyone else is cool with this to-do hanging out in the release notes file for now, it's fine with me too. Otherwise, we can offload it to a comment on RTC-384 until the time is right.

@SyntaxColoring
Copy link
Contributor Author

I'm not sure what the deal was with the timing out G-Code-Confirm test, but it works when I trigger it manually.

I'm unable to easily test on an actual robot at the moment because of unrelated boot problems, but I'm satisfied with our automated test coverage for now.

@SyntaxColoring SyntaxColoring merged commit 876bd1e into edge Jan 12, 2024
48 of 49 checks passed
@SyntaxColoring SyntaxColoring deleted the backwards_compatible_dispense_clamping branch January 12, 2024 15:47
mjhuff pushed a commit that referenced this pull request Jan 23, 2024
DerekMaggio added a commit that referenced this pull request Mar 7, 2024
# Overview

Add protocols pulled from v7.2.0 pull requests for Analysis Snapshot
testing. All work filed under
[RQA-2434](https://opentrons.atlassian.net/browse/RQA-2434)

# Test Plan

- [x] Add partial tip pickup protocols from Sanitti
- [x] Add ABR protocol from
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23)
- [x] Run new protocols locally and verify results
- [x] Run in workflow dispatch action and verify success
[[link]](https://github.com/Opentrons/opentrons/actions/runs/8160792870/job/22308167177)

# Changelog

Here are the protocols added and where I found them:

- [RQA-2098](https://opentrons.atlassian.net/browse/RQA-2098)
-
Flex_P1000_96_Gripper_TC_TM_HS_AnalysisError_GripperCollisionWithTips.json
- #14491
  - Flex_None_None_TC_2_14_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_15_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_16_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_17_verifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_14_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_15_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_16_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_17_VerifyThermocyclerLoadedSlots.py
- #14475
-
Flex_None_None_TC_2_16_AnalysisError_TrashBinAndThermocyclerConflict.py
  - OT2_None_None_2_16_verifyDoesNotDeadlock.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin1.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin2.py
- #14547
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLid.py
- #14522
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLidClips.py
- Dispense functionality validation
  - OT2_P300M_P20S_TC_HS_TM_2_15_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_16_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_17_dispense_changes.py
- #14253
  - OT2_P300S_None_2_16_verifyNoFloatingPointErrorInPipetting.py

# Review requests

There are a few pull requests that I had questions about. I will @ the
people that worked on them to answer the questions

- #14437
- @sfoster1, Can I use the protocol found in
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23) to get this to
happen? Is there another way?
- #14509
- @SyntaxColoring, can analysis capture this change or is this only
relavant during actual protocol runtime?
- #14510
- @TamarZanzouri or @SyntaxColoring, can I raise a generic python
exception inside the protocol to trigger this functionality?
- #14512
- @Laura-Danielle or @SyntaxColoring, Iant to validate my logic. This
not a good canidate for snapshot testing because you have to run LPC and
Calibration which is not taken into account during analysis. Can you
confirm?

# Risk assessment

None


[RQA-2434]:
https://opentrons.atlassian.net/browse/RQA-2434?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RQA-2098]:
https://opentrons.atlassian.net/browse/RQA-2098?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RABR-23]:
https://opentrons.atlassian.net/browse/RABR-23?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
Add protocols pulled from v7.2.0 pull requests for Analysis Snapshot
testing. All work filed under
[RQA-2434](https://opentrons.atlassian.net/browse/RQA-2434)

- [x] Add partial tip pickup protocols from Sanitti
- [x] Add ABR protocol from
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23)
- [x] Run new protocols locally and verify results
- [x] Run in workflow dispatch action and verify success
[[link]](https://github.com/Opentrons/opentrons/actions/runs/8160792870/job/22308167177)

Here are the protocols added and where I found them:

- [RQA-2098](https://opentrons.atlassian.net/browse/RQA-2098)
-
Flex_P1000_96_Gripper_TC_TM_HS_AnalysisError_GripperCollisionWithTips.json
- #14491
  - Flex_None_None_TC_2_14_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_15_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_16_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_17_verifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_14_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_15_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_16_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_17_VerifyThermocyclerLoadedSlots.py
- #14475
-
Flex_None_None_TC_2_16_AnalysisError_TrashBinAndThermocyclerConflict.py
  - OT2_None_None_2_16_verifyDoesNotDeadlock.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin1.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin2.py
- #14547
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLid.py
- #14522
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLidClips.py
- Dispense functionality validation
  - OT2_P300M_P20S_TC_HS_TM_2_15_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_16_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_17_dispense_changes.py
- #14253
  - OT2_P300S_None_2_16_verifyNoFloatingPointErrorInPipetting.py

There are a few pull requests that I had questions about. I will @ the
people that worked on them to answer the questions

- #14437
- @sfoster1, Can I use the protocol found in
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23) to get this to
happen? Is there another way?
- #14509
- @SyntaxColoring, can analysis capture this change or is this only
relavant during actual protocol runtime?
- #14510
- @TamarZanzouri or @SyntaxColoring, can I raise a generic python
exception inside the protocol to trigger this functionality?
- #14512
- @Laura-Danielle or @SyntaxColoring, Iant to validate my logic. This
not a good canidate for snapshot testing because you have to run LPC and
Calibration which is not taken into account during analysis. Can you
confirm?

None

[RQA-2434]:
https://opentrons.atlassian.net/browse/RQA-2434?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RQA-2098]:
https://opentrons.atlassian.net/browse/RQA-2098?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RABR-23]:
https://opentrons.atlassian.net/browse/RABR-23?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Carlos-fernandez pushed a commit that referenced this pull request Jun 3, 2024
Add protocols pulled from v7.2.0 pull requests for Analysis Snapshot
testing. All work filed under
[RQA-2434](https://opentrons.atlassian.net/browse/RQA-2434)

- [x] Add partial tip pickup protocols from Sanitti
- [x] Add ABR protocol from
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23)
- [x] Run new protocols locally and verify results
- [x] Run in workflow dispatch action and verify success
[[link]](https://github.com/Opentrons/opentrons/actions/runs/8160792870/job/22308167177)

Here are the protocols added and where I found them:

- [RQA-2098](https://opentrons.atlassian.net/browse/RQA-2098)
-
Flex_P1000_96_Gripper_TC_TM_HS_AnalysisError_GripperCollisionWithTips.json
- #14491
  - Flex_None_None_TC_2_14_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_15_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_16_verifyThermocyclerLoadedSlots.py
  - Flex_None_None_TC_2_17_verifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_14_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_15_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_16_VerifyThermocyclerLoadedSlots.py
  - OT2_None_None_TC_2_17_VerifyThermocyclerLoadedSlots.py
- #14475
-
Flex_None_None_TC_2_16_AnalysisError_TrashBinAndThermocyclerConflict.py
  - OT2_None_None_2_16_verifyDoesNotDeadlock.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin1.py
-
OT2_None_None_HS_2_16_AnalysisError_HeaterShakerConflictWithTrashBin2.py
- #14547
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLid.py
- #14522
-
Flex_P1000_96_None_TC_2_16_AnalysisError_pipetteCollisionWithThermocyclerLidClips.py
- Dispense functionality validation
  - OT2_P300M_P20S_TC_HS_TM_2_15_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_16_dispense_changes.py
  - OT2_P300M_P20S_TC_HS_TM_2_17_dispense_changes.py
- #14253
  - OT2_P300S_None_2_16_verifyNoFloatingPointErrorInPipetting.py

There are a few pull requests that I had questions about. I will @ the
people that worked on them to answer the questions

- #14437
- @sfoster1, Can I use the protocol found in
[RABR-23](https://opentrons.atlassian.net/browse/RABR-23) to get this to
happen? Is there another way?
- #14509
- @SyntaxColoring, can analysis capture this change or is this only
relavant during actual protocol runtime?
- #14510
- @TamarZanzouri or @SyntaxColoring, can I raise a generic python
exception inside the protocol to trigger this functionality?
- #14512
- @Laura-Danielle or @SyntaxColoring, Iant to validate my logic. This
not a good canidate for snapshot testing because you have to run LPC and
Calibration which is not taken into account during analysis. Can you
confirm?

None

[RQA-2434]:
https://opentrons.atlassian.net/browse/RQA-2434?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RQA-2098]:
https://opentrons.atlassian.net/browse/RQA-2098?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

[RABR-23]:
https://opentrons.atlassian.net/browse/RABR-23?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project papi-v2 Python API V2 protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants