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): Aspirate, dispense, and mix can perform at 0ul #13989

Merged
merged 5 commits into from
Nov 16, 2023

Conversation

andySigler
Copy link
Contributor

@andySigler andySigler commented Nov 15, 2023

Overview

I would expect .aspirate(0) and .dispense(0) to aspirate/dispense 0 ul.

Currently .aspirate(0) will aspirate the tip's max-volume, and .dispense(0) will dispense all volume currently in the tip.

Looking in git history and this line was added in instrument_context.py in 2020, where the if not volume is used to determine if the volume should be defaulted:

c_vol = self._core.get_available_volume() if not volume else volume

I'm assuming this behavior wasn't intentional.

Test Plan

Changelog

Review requests

Risk assessment

@andySigler andySigler requested a review from a team as a code owner November 15, 2023 18:33
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #13989 (c3c451c) into edge (e5eb78b) will not change coverage.
Report is 30 commits behind head on edge.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #13989   +/-   ##
=======================================
  Coverage   70.67%   70.67%           
=======================================
  Files        2505     2505           
  Lines       70832    70832           
  Branches     8738     8738           
=======================================
  Hits        50058    50058           
  Misses      18639    18639           
  Partials     2135     2135           
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

Files Coverage Δ
...i/src/opentrons/protocol_api/instrument_context.py 89.53% <ø> (ø)

@andySigler andySigler changed the title fix(api): Aspirate, dispense, and mix accept 0ul as a volume fix(api): Aspirate, dispense, and mix can perform at 0ul Nov 15, 2023
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.

Right or wrong, this is established behavior in currently-running protocol api versions and we cannot change it retroactively. Please gate this change behind being on api version 2.16 or above.

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.

Oh one more thing besides fixing tests - the docstrings for all three methods (which generate our docs) explicitly call out this behavior (see aspirate , dispense, mix ). We also might call it out in other places and need to search through them.

@sfoster1 sfoster1 requested a review from ecormany November 16, 2023 21:16
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.

Happy with functionality and docs if it passes tests but @ecormany would you mind taking a look at the docstrings?

Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for writing up the docstrings to go with the new behavior!

@sfoster1 sfoster1 merged commit 12a630b into edge Nov 16, 2023
@sfoster1 sfoster1 deleted the fix-api-pipette-aspirate-accepts-0-ul branch November 16, 2023 22:27
sfoster1 added a commit that referenced this pull request Dec 18, 2023
A previous PR (12a630b / #13989 ) changed the python protocol api in version 2.16 to allow commanding 0ul liquid handling commands like aspirate, mix, and dispense. This is useful in programmatic protocols that read out volumes to handle; they can now handle 0 volume properly. In 2.15 and previous, specifying 0 would lead to those commands doing the most volume they could (i.e. aspirate the full tip volume, dispense whatever's currently in the pipette, mix at full volume) and this likely was an unintentional side effect because of python truthiness.

However, that change only touched the python protocol API; that API would emit commands to the engine that specified 0 volume, and those were not allowed: the pydantic models for the commands and responses required strictly greater than 0 volume.

This PR

- changes the pydantic models and updates the schema to allow 0ul commands
- adds a python protocol to be an integration test
- adds unit tests for the python protocol api aspirate and dispense commands



---------

Co-authored-by: Seth Foster <[email protected]>
Co-authored-by: Max Marrone <[email protected]>
ncdiehl11 pushed a commit that referenced this pull request Dec 20, 2023
A previous PR (12a630b / #13989 ) changed the python protocol api in version 2.16 to allow commanding 0ul liquid handling commands like aspirate, mix, and dispense. This is useful in programmatic protocols that read out volumes to handle; they can now handle 0 volume properly. In 2.15 and previous, specifying 0 would lead to those commands doing the most volume they could (i.e. aspirate the full tip volume, dispense whatever's currently in the pipette, mix at full volume) and this likely was an unintentional side effect because of python truthiness.

However, that change only touched the python protocol API; that API would emit commands to the engine that specified 0 volume, and those were not allowed: the pydantic models for the commands and responses required strictly greater than 0 volume.

This PR

- changes the pydantic models and updates the schema to allow 0ul commands
- adds a python protocol to be an integration test
- adds unit tests for the python protocol api aspirate and dispense commands



---------

Co-authored-by: Seth Foster <[email protected]>
Co-authored-by: Max Marrone <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants