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

feat(api): Add prepare to aspirate to PAPI and engine #13827

Merged
merged 12 commits into from
Nov 1, 2023

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Oct 23, 2023

Add the ability to explicitly call prepare_for_aspirate() to the python protocol API.

We make sure the pipette is prepared to aspirate before any aspiration, which is good, but the way we make this absolute is that if the pipette isn't ready for aspirate when you call aspirate(), we move it to the top of the current well (if necessary) to move the plunger up. This is a problem for users who are, for instance, building explicit prewetting behavior out of protocol API calls. It's common to move the pipette into the liquid and then delay to let the liquid settle before pipetting; if the pipette then moves up to prepare for aspirate before coming back down, that settling is undone. By adding the ability to explicitly prepare_for_aspirate(), the user can call it while they know the pipette is above the well.

This is added in a new 2.16 API level, which will ship in 7.1.

The command needs to be added to the API, but also to the engine (the aspirate command was, and is, the place where the fallback occurred rather than being a separately-issued command) and the sync client.

Review requests

  • This PR does the sort of minimum-viable change of adding the command, adding the API, and making the API call the command. It does not touch any of the built in make-sure-we're-ready behaviors in either the PAPI implementation or the protocol engine. Should it? Should we make the engine emit a second command here? I don't think so - I think that's a better thing for a hypothetical resolver layer to do - but I'm interested to hear if you disagree
  • Did I mess something up about the new api level? It's been a while

Testing

  • Write a protocol that would incur the jump-out-of-the-well prepare behavior and
    • on 2.16, add a prepare_for_aspirate() ahead of time, and make sure it prepares when the command says and doesn't come out of the well before aspirate
    • on 2.16, don't add a prepare_for_aspirate(), and make sure it still does the prep behavior

@sfoster1 sfoster1 requested review from andySigler and a team October 23, 2023 18:31
@sfoster1 sfoster1 requested review from a team as code owners October 23, 2023 18:31
@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #13827 (a9bb0f3) into edge (5dc8819) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #13827   +/-   ##
=======================================
  Coverage   70.86%   70.86%           
=======================================
  Files        2501     2501           
  Lines       70340    70340           
  Branches     8600     8600           
=======================================
  Hits        49845    49845           
  Misses      18399    18399           
  Partials     2096     2096           
Flag Coverage Δ
app 68.87% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 45.51% <ø> (ø)
react-api-client 65.82% <ø> (ø)
shared-data 72.96% <ø> (ø)
step-generation 84.95% <ø> (ø)

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.62% <ø> (ø)
...c/opentrons/protocol_engine/clients/sync_client.py 100.00% <ø> (ø)
...src/opentrons/protocol_engine/commands/__init__.py 100.00% <ø> (ø)
...entrons/protocol_engine/commands/command_unions.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/pipettes.py 100.00% <ø> (ø)

@ecormany
Copy link
Contributor

A terminology question while this is still in progress…
Currently the function is named prepare_for_aspirate, but much of our natural writing (and speaking) about it uses the phrase "prepare to aspirate". The to-construction means that we are treating aspirate as a verb. The for-construction means that we are treating aspirate as a noun. I think the noun form is more jargon-y either way you slice it:

  • It could refer to the API method aspirate().
  • It could be a different noun aspirate. You won't find a dictionary entry that defines aspirate (n.) as the act of sucking something in with air, even though we may use it that way inside Opentrons, or some scientists may use it that way. We have a natural tendency to use aspiration as the noun for the action (you'll find it throughout this very PR).

tl;dr: Is it more natural to name this function prepare_to_aspirate?

@SyntaxColoring SyntaxColoring self-requested a review October 24, 2023 15:58
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Yay, love it!

This PR does the sort of minimum-viable change of adding the command, adding the API, and making the API call the command. It does not touch any of the built in make-sure-we're-ready behaviors in either the PAPI implementation or the protocol engine. Should it? Should we make the engine emit a second command here? I don't think so - I think that's a better thing for a hypothetical resolver layer to do - but I'm interested to hear if you disagree

I think you're right not to touch the built-in make-sure-we're-ready behaviors in PAPI or Protocol Engine. That sounds like a breaking behavioral change in Protocol Engine, especially.

I think ideally, for composability and ease of reasoning, these always would have been two separate Protocol Engine commands, and the aspirate Protocol Engine command never would have prepared implicitly. But we're committed to that interface now. 🤷‍♂️

Did I mess something up about the new api level? It's been a while

This looks right to me.

shared-data/command/schemas/7.json Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
@sfoster1
Copy link
Member Author

behavior is correct after a653ad1

@sfoster1
Copy link
Member Author

Called prepare_to_aspirate/PrepareToAspirate as of 1a28dc2

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Changes since last review look great, thanks. One optional docstring suggestion.

api/src/opentrons/protocol_api/instrument_context.py Outdated Show resolved Hide resolved
@sfoster1 sfoster1 force-pushed the add-prepare-to-aspirate branch from 1a28dc2 to 5aa9f99 Compare October 31, 2023 15:26
sfoster1 and others added 12 commits November 1, 2023 09:35
Having this explicitly will let users do more interesting things with configure_for_volume.
Add the ability to explicitly call prepare_for_aspirate() to the python
protocol API.

We make sure the pipette is prepared to aspirate before any aspiration,
which is good, but the way we make this absolute is that if the pipette
isn't ready for aspirate when you call aspirate(), we move it to the top
of the current well (if necessary) to move the plunger up. This is a
problem for users who are, for instance, building explicit prewetting
behavior out of protocol API calls. It's common to move the pipette into
the liquid and then delay to let the liquid settle before pipetting; if
the pipette then moves up to prepare for aspirate before coming back
down, that settling is undone. By adding the ability to explicitly
prepare_for_aspirate(), the user can call it while they know the pipette
is above the well.
The phrasing makes a lot more sense in context, so change the protocol
api name and the protocol engine command name to
PrepareToAspirate/prepare_to_aspirate rather than for. Do _not_ change
the hardware controller name because other stuff depends on that and I
personally like it more, and correspondingly do _not_ change the engine
pipetting handler name because that should correspond with the hardware interface.
@sfoster1 sfoster1 force-pushed the add-prepare-to-aspirate branch from 13c7b1b to a9bb0f3 Compare November 1, 2023 13:36
@sfoster1 sfoster1 merged commit f8c53e8 into edge Nov 1, 2023
61 checks passed
@sfoster1 sfoster1 deleted the add-prepare-to-aspirate branch November 1, 2023 14:36
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.

4 participants