fix(api): Do not enter recovery mode if `detect_liquid_presence()` detects no liquid #15698

SyntaxColoring · 2024-07-17T21:16:37Z

Overview

This closes EXEC-598.

Test plan

Run this protocol without any liquid:

from opentrons import protocol_api

requirements = {"apiLevel": "2.20", "robotType": "Flex"}


def run(protocol: protocol_api.ProtocolContext) -> None:
    pipette = protocol.load_instrument("flex_1channel_1000", mount="left")
    tip_rack = protocol.load_labware("opentrons_flex_96_tiprack_200ul", "C2")

    labware = protocol.load_labware("opentrons_96_wellplate_200ul_pcr_full_skirt", "D2")

    pipette.pick_up_tip(tip_rack["A1"])
    result = pipette.detect_liquid_presence(labware["A1"])
    protocol.pause("Liquid present? " + str(result))
    pipette.require_liquid_presence(labware["A1"])
    pipette.return_tip()

The detect_liquid_presence() call should return False without entering recovery mode.
Then, the require_liquid_presence() call should enter recovery mode.

Changelog

Reimplement InstrumentContext.detect_liquid_presence() atop the new tryLiquidProbe command, implemented in #15667.

Review requests

Any thoughts on my big comment in api/src/opentrons/protocol_api/core/engine/instrument.py? I think one of the "right" solutions in EXEC-598 would help. But even with that, it remains unclear to me how we want the overall system to behave when something like this happens:

The protocol asks for a detect_liquid_presence() result (True/False)
The command fails for an error unrelated to liquid probing

Risk assessment

Low.

Remove a check that always passes, guaranteed by the type checker.

…and.

Resolve conflicts in api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py.

SyntaxColoring · 2024-07-17T21:28:40Z

api/src/opentrons/protocol_api/core/engine/instrument.py

+        # The error handling here is a bit nuanced and also a bit broken:
+        #
+        # - If the hardware detects liquid, the `tryLiquidProbe` engine command will
+        #   succeed and return a height, which we'll convert to a `True` return.
+        #   Okay so far.
+        #
+        # - If the hardware detects no liquid, the `tryLiquidProbe` engine command will
+        #   suceced and return `None`, which we'll convert to a `False` return.
+        #   Still okay so far.
+        #
+        # - If there is any other error within the `tryLiquidProbe` command, things get
+        #   messy. It may kick the run into recovery mode. At that point, all bets are
+        #   off--we lose our guarantee of having a `tryLiquidProbe` command whose
+        #   `result` we can inspect. We don't know how to deal with that here, so we
+        #   currently propagate the exception up, which will quickly kill the protocol,
+        #   after a potential split second of recovery mode. It's unclear what would
+        #   be good user-facing behavior here, but it's unfortunate to kill the protocol
+        #   for an error that the engine thinks should be recoverable.


This third case, I believe, cannot happen in practice right now because our error recovery policy does not do error recovery for any tryLiquidProbe failures . But who knows how long that will last.

Is this unique to liquid probe? Wouldn't this be the case for any kind of command run from the protocol api via execute_command_without_recovery? Or even executing a command that returns structured errors with execute_command_without_recovery?

It is not unique to liquid_probe(), you're correct. It's any time PAPI uses execute_command_without_recovery(), or, even more generally, any time PAPI has potentially different error-handling ideas than PE.

sfoster1

Looks good to me, and I agree with the fundamentals of your comment - but is that really unique to liquid probe?

sfoster1 · 2024-07-18T14:25:28Z

api/src/opentrons/protocol_api/core/engine/instrument.py

+        # The error handling here is a bit nuanced and also a bit broken:
+        #
+        # - If the hardware detects liquid, the `tryLiquidProbe` engine command will
+        #   succeed and return a height, which we'll convert to a `True` return.
+        #   Okay so far.
+        #
+        # - If the hardware detects no liquid, the `tryLiquidProbe` engine command will
+        #   suceced and return `None`, which we'll convert to a `False` return.
+        #   Still okay so far.
+        #
+        # - If there is any other error within the `tryLiquidProbe` command, things get
+        #   messy. It may kick the run into recovery mode. At that point, all bets are
+        #   off--we lose our guarantee of having a `tryLiquidProbe` command whose
+        #   `result` we can inspect. We don't know how to deal with that here, so we
+        #   currently propagate the exception up, which will quickly kill the protocol,
+        #   after a potential split second of recovery mode. It's unclear what would
+        #   be good user-facing behavior here, but it's unfortunate to kill the protocol
+        #   for an error that the engine thinks should be recoverable.


Is this unique to liquid probe? Wouldn't this be the case for any kind of command run from the protocol api via execute_command_without_recovery? Or even executing a command that returns structured errors with execute_command_without_recovery?

api/src/opentrons/protocol_api/core/engine/instrument.py

TamarZanzouri

LGTM! small typo fix

Co-authored-by: TamarZanzouri <[email protected]>

SyntaxColoring · 2024-07-18T17:08:01Z

Failing tests look like a setuptools update thing, unrelated to this PR.

SyntaxColoring added 5 commits July 17, 2024 13:45

Minor fixup.

45b90cb

Remove a check that always passes, guaranteed by the type checker.

Reimplement detect_liquid_presence() atop the new tryLiquidProbe comm…

4d4e67c

…and.

Minor unit test fixup.

e98dd7a

Update unit tests.

3466fbb

More minor unit test fixups.

ef0eead

SyntaxColoring requested a review from a team as a code owner July 17, 2024 21:16

SyntaxColoring requested a review from a team July 17, 2024 21:16

Merge branch 'edge' into papi_try_liquid_probe

8112650

Resolve conflicts in api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py.

SyntaxColoring commented Jul 17, 2024

View reviewed changes

sfoster1 approved these changes Jul 18, 2024

View reviewed changes

TamarZanzouri reviewed Jul 18, 2024

View reviewed changes

api/src/opentrons/protocol_api/core/engine/instrument.py Outdated Show resolved Hide resolved

TamarZanzouri approved these changes Jul 18, 2024

View reviewed changes

SyntaxColoring and others added 2 commits July 18, 2024 11:08

Typo.

dd00c3d

Co-authored-by: TamarZanzouri <[email protected]>

Merge branch 'edge' into papi_try_liquid_probe

26d69b1

Opentrons deleted a comment from github-actions bot Jul 18, 2024

SyntaxColoring merged commit 641194f into edge Jul 18, 2024
37 of 44 checks passed

SyntaxColoring deleted the papi_try_liquid_probe branch July 18, 2024 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): Do not enter recovery mode if `detect_liquid_presence()` detects no liquid #15698

fix(api): Do not enter recovery mode if `detect_liquid_presence()` detects no liquid #15698

SyntaxColoring commented Jul 17, 2024 •

edited

Loading

SyntaxColoring Jul 17, 2024 •

edited

Loading

sfoster1 Jul 18, 2024

SyntaxColoring Jul 18, 2024

sfoster1 left a comment

sfoster1 Jul 18, 2024

TamarZanzouri left a comment

SyntaxColoring commented Jul 18, 2024

fix(api): Do not enter recovery mode if detect_liquid_presence() detects no liquid #15698

fix(api): Do not enter recovery mode if detect_liquid_presence() detects no liquid #15698

Conversation

SyntaxColoring commented Jul 17, 2024 • edited Loading

Overview

Test plan

Changelog

Review requests

Risk assessment

SyntaxColoring Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

sfoster1 Jul 18, 2024

Choose a reason for hiding this comment

SyntaxColoring Jul 18, 2024

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

sfoster1 Jul 18, 2024

Choose a reason for hiding this comment

TamarZanzouri left a comment

Choose a reason for hiding this comment

SyntaxColoring commented Jul 18, 2024

fix(api): Do not enter recovery mode if `detect_liquid_presence()` detects no liquid #15698

fix(api): Do not enter recovery mode if `detect_liquid_presence()` detects no liquid #15698

SyntaxColoring commented Jul 17, 2024 •

edited

Loading

SyntaxColoring Jul 17, 2024 •

edited

Loading