feat: pipette.dispense() should warn user when dispensing less than requested volume

[nickcrider]

Overview

pipette.dispense() checks that it does not dispense more than the current_volume of the pipette but does not warn the user when dispensing less volume than requested. This error should be caught at the API level to warn users that they have not aspirated enough liquid to complete the requested dispense().

Implementation details

Add a CurrentVolumeError that is thrown if volume > self.current_volume rather than silently dispensing self.current_volume

https://github.com/Opentrons/opentrons/blob/f5541153b1580496703317c20f0114d483e2edf5/api/src/opentrons/legacy_api/instruments/pipette.py#L557

Design

if volume > self.current_volume:
    raise CurrentVolumeError(
    f'Dispense commands are not allowed if there is not enough liquid '
    f'remaining in the pipette to complete the operation. '
    f'The {self.mount} pipette currently contains {self.current_volume} ul'
    f'and cannot dispense the requested {self.volume} ul')

Acceptance criteria

Code which dispenses current_voume instead of the requested volume should not succeed silently.

The following nonsensical protocol should raise an API error:

from opentrons import protocol_api
metadata = {'apiLevel': '2.2'}

def run(protocol: protocol_api.ProtocolContext):

    tiprack_multi = protocol.load_labware('opentrons_96_tiprack_300ul', '10')
    plate_96well = protocol.load_labware('nest_96_wellplate_100ul_pcr_full_skirt', '1')
    p_single = protocol.load_instrument('p1000_single', 'right', tip_racks=[tiprack_multi])

    p_single.pick_up_tip()
    p_single.dispense(1000, plate_96well["A1"])

Additionally, this should also not succeed silently:

p_single.pick_up_tip()
p_single.aspirate(1000, plate_96well["A1"])
p_single.dispense(500)
p_single.dispense(500)
p_single.dispense(500)

┆Issue is synchronized with this Wrike Task by Unito

[nickcrider]

Align the error message with the corresponding one in pipette.asiprate() https://github.com/Opentrons/opentrons/blob/f5541153b1580496703317c20f0114d483e2edf5/api/src/opentrons/legacy_api/instruments/pipette.py#L459

if volume > self.current_volume:
    raise RuntimeWarning(
        'Pipette with current volume of {0} cannot dispense volume {1}'
        .format(
            self.current_volume,
            volume)
    )

[SyntaxColoring]

I might disagree, on grounds of generality.

For example, you might want to do something like this:

# Dilute.
total_volume = 100
sample_volumes = [0, 25, 50, 75, 100]
for i, s in enumerate(sample_volumes):
    p.transfer(s, sample, dest[i])
    p.transfer(total_volume-s, water, dest[i])

(I'm using transfer() here, but the same would apply to dispense().)

dest[0] gets a 0 µL transfer of sample, and dest[4] gets a 0 µL transfer of water, which I think is a sensible way of writing it. With the proposed warning, the first and last transfers would have to be special-cased.

But I don’t have much protocol development experience, so I wouldn't know if this is done mistakenly more than it's done intentionally.

[nickcrider]

I agree here, and realize I made a mistake when I wrote this up. If you ask for dispense(0) the API should allow you to have it. I am more worried about the situation where the user asks for a particular volume and is getting less than they expected without being told so by the API.

For example, consider this bit of code from a customer:

    for s in samples:
        m300.pick_up_tip()
        if volume <=190:
            m300.aspirate(volume=10, location=s.top(10), rate=1.0)
        m300.transfer(volume, s.bottom(height), m300.trash_container.top(5), new_tip='never', air_gap=10, blow_out = True)
        m300.dispense(200)
        m300.delay(seconds = 2)
        m300.dispense(50)
        m300.drop_tip()

https://github.com/UK-CoVid19/OpentronDev/blob/cb7b3cc0d46a20803441d677195024dbc1abb05b/protocols/RNA%20Extraction%20MD%20V9.py#L437

Looks like my code is correct but I had a brainfart about what min does when writing this up. I'll go ahead and edit the part about 0 out and hopefully we will be on the same page.

[AJamesPhillips]

Letting code continue but log a warning would be great 👍

[SyntaxColoring]

Oh, I see. Yeah, this doesn't make sense and shouldn't be allowed:

metadata = {"apiLevel": "2.2"}
def run(protocol):
    tip_rack = protocol.load_labware("opentrons_96_tiprack_300ul", 1)
    plate = protocol.load_labware("agilent_1_reservoir_290ml", 2)
    p300 = protocol.load_instrument("p300_single_gen2", mount="left", tip_racks=[tip_rack])
    p300.pick_up_tip()
    p300.aspirate(100, plate["A1"]) # "Aspirating 100.0 uL from A1 of Agilent 1 Well Reservoir 290 mL on 2 at 1.0 speed"
    p300.dispense(300, plate["A1"]) # "Dispensing 300.0 uL into A1 of Agilent 1 Well Reservoir 290 mL on 2 at 1.0 speed"
    p300.dispense(200, plate["A1"]) # "Dispensing 200.0 uL into A1 of Agilent 1 Well Reservoir 290 mL on 2 at 1.0 speed"
    p300.dispense(100, plate["A1"]) # "Dispensing 100.0 uL into A1 of Agilent 1 Well Reservoir 290 mL on 2 at 1.0 speed"

I don't have a robot to test on right now, but it sounds like the first dispense() is clamped to 100 µL, and the second two dispenses are just ignored, despite what the log says?

I'd advocate that this should be an error, not a warning. I don't think the app can currently show protocol warnings, for one thing. (I've seen some messages show up in opentrons_simulate but not in the app's run log.) But even if it could...I feel like this is either definitely okay or definitely not okay?

A nuance: do we want to allow dispensing below zero down to the blowout position?

[AJamesPhillips]

it sounds like the first dispense() is clamped to 100 µL

From the code it looks like that is correct: https://github.com/Opentrons/opentrons/blob/f5541153b1580496703317c20f0114d483e2edf5/api/src/opentrons/legacy_api/instruments/pipette.py#L557

I'd still suggest a warning or log message with [WARNING] prepended might be serve users better.

I'd advocate that this should be an error

Fair enough. I'll agree to disagree :) At the moment though it doesn't error so this would (obviously) be a breaking change and need a major release.

[SyntaxColoring]

I'd advocate that this should be an error

Fair enough. I'll agree to disagree :)

I'm definitely willing to have my mind changed on this, but I'll elaborate a little.

I think warnings are kind of bad, in general.

• If I can safely ignore a warning, why was it shown to me in the first place? It adds noise and trains me to ignore messages.
• If I can't safely ignore a warning, why am I allowed to try? That is, why isn't it an error? The API ought to be difficult to misuse.

The only example of good warnings that I can think of are deprecation warnings.

Part of my background is in C++, whose compilers have a jillion optional warnings. A best practice is enabling all of them and treating all of them as errors, because that stops us from writing bad code. I can recite c++ -std=c++17 -pedantic -Wall -Wextra -Werror by heart. 🙄

---

At the moment though it doesn't error so this would (obviously) be a breaking change and need a major release.

It would be breaking, yeah. We'd probably tie it to apiLevel. If you use metadata = {"apiLevel": "2.2"}, you'd get the current behavior. If you use metadata = {"apiLevel": "2.4"} (or whatever API version this ends up in), you'd get the error or warning.