feat(api): create instr ctx methods for checking liquid presence

[aaron-kulkarni]

Added detect_liquid and require_liquid to instrument_context. The two methods both check for liquid in a specific well, but one returns true/false and the other raises exceptions.

Also adds measure_liquid_level to instrument context. This method probes the well to find the height of the liquid.
close EXEC-544, EXEC-545, EXEC-546

Overview

Test Plan

Changelog

Review requests

In find_liquid_level in the instrument core, I couldn't find a method that would execute the command with recovery and also return the result of the command. So I settled for execute_command_without_recovery but I'm not sure if that's the right way to go about it. I created a new method called execute_command_with_result to get around this.

Risk assessment

[aaron-kulkarni]

still need to test

[ryanthecoder]

I think we should only explicity catch LiquidNotFoundException if we're going to handle them. If there is some other issue that results in an error this could let that go undetected

[sfoster1]

Couple changes we should make to the API. Also, if you merge edge into this PR it should get rid of the snapshot problem.

[sfoster1]

No, we shouldn't do this. If we're using mock components, those mock components should return data in the same way the real things do even if they get it from somewhere else - this is why we have mocks.

[sfoster1]

Suggested change

	raise WellDoesNotExistError("Well type was none.")
	raise WellDoesNotExistError("You must provide a well to check.")

[sfoster1]

this error should take the same form as the same check in the other methods

[sfoster1]

is this really an appropriate return value? Remember, this function returns a height in absolute deck coordinates, in which z=0 is the level of the deck. So this function can sometimes return, without indicating an error, that the liquid level is at the height of the deck. That doesn't make much sense to me.

I think this function should just let the PipetteLiquidNotFoundError through.

[ryanthecoder]

I agree we should pass this through. The only api call that shouldn't raise errors is detect_liquid

[sfoster1]

Suggested change

	result = self._engine_client.execute_command_without_recovery(
	result = self._engine_client.execute_command(

we definitely want to recover from this command

[SyntaxColoring]

I think the problem here is that one of this method's callers, InstrumentContext.get_liquid_height(), is returning the probed height. And if the liquidProbe command fails and we let the frontend drive error recovery, there isn't a probed height in the general case. The client can choose to skip to the next command without retrying the failed liquidProbe.

We can certainly solve this for InstrumentContext.require_liquid(). Just, yeah, call execute_command() (with recovery) and discard the liquidProbe result. A successful liquidProbe, or a failed-but-recovered liquidProbe, will return from execute_command(). A failed-and-not-recovered liquidProbe will raise from execute_command(), and we should let that propagate up.

But for InstrumentContext.get_liquid_height() and InstrumentContext.detect_liquid_presence(), it's unclear to me what we would even want to happen.

[SyntaxColoring]

Recap of in-person discussion. Looks like you've already done some of this by the time I'm posting this comment.

InstrumentContext.require_liquid_presence()

We want this method to give the client the opportunity for error recovery, but raise an exception if that ultimately fails. That's what SyncClient.execute_command(), so under the hood, this should be implemented using that. If that raises an exception, we should just let it propagate.

InstrumentContext.detect_liquid_presence()

We want this method to straight-up return True/False without giving the client the opportunity for error recovery. So, under the hood:

• We should implement it via SyncClient.execute_command_without_recovery().

• If the probe detects no liquid, execute_command_without_recovery() will raise that as an exception. We should look for that and translate it into a False return.

  We can catch ProtocolCommandFailedError, and then check if exception.original_error is an instance of LiquidNotFoundError. If so, we should translate it into a False return. Otherwise, we should re-raise the exception to let it propagate.

I think this will expose a bug where the robot will mistakenly go into error recovery mode (because "the command failed") and try to keep running the protocol (because we're catching the exception, translating it to False, and letting the protocol continue). We can take that on in a separate PR: EXEC-598.

InstrumentContext.measure_liquid_height()

We want to keep this internal and experimental for now and keep our options open for the future. So:

• We should say in its docstring that it's internal and experimental, and exclude it from the public docs.opentrons.com website. Here is an example of how to exclude it from docs.opentrons.com.
• We should implement this via SyncClient.execute_command_without_recovery(). So if no liquid is detected, it will raise an exception. We should just let that propagate.

The reason we want to keep this internal and experimental is that we haven't really fleshed out how to support error recovery on PAPI methods that return results. Does the UI flow need to have a step to retry the failed probe? Should we just raise some documented exception for the protocol author to catch and deal with how they please?

[SyntaxColoring]

Thanks, this is coming along! Agreed with the existing comments.

[SyntaxColoring]

I think the problem here is that one of this method's callers, InstrumentContext.get_liquid_height(), is returning the probed height. And if the liquidProbe command fails and we let the frontend drive error recovery, there isn't a probed height in the general case. The client can choose to skip to the next command without retrying the failed liquidProbe.

We can certainly solve this for InstrumentContext.require_liquid(). Just, yeah, call execute_command() (with recovery) and discard the liquidProbe result. A successful liquidProbe, or a failed-but-recovered liquidProbe, will return from execute_command(). A failed-and-not-recovered liquidProbe will raise from execute_command(), and we should let that propagate up.

But for InstrumentContext.get_liquid_height() and InstrumentContext.detect_liquid_presence(), it's unclear to me what we would even want to happen.

[aaron-kulkarni]

I implemented the changes that we discussed in standup as I understood them. Probably forgot something or misunderstood it so if I did just let me know

Changes include:
In the bool only method, change to execute_command_without_recovery
In the bool or exception method, changed nothing
In the float or exception method, change to execute_command_without_recovery

Also edited the tests to reflect this

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (5ce130b) to head (520058f).
Report is 125 commits behind head on edge.

Additional details and impacted files

@@             Coverage Diff             @@
##             edge   #15555       +/-   ##
===========================================
+ Coverage   60.43%   92.43%   +32.00%     
===========================================
  Files         154       77       -77     
  Lines       12119     1283    -10836     
===========================================
- Hits         7324     1186     -6138     
+ Misses       4795       97     -4698     

Flag	Coverage Δ
g-code-testing	92.43% <ø> (?)
hardware	?
shared-data	?

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

see 231 files with indirect coverage changes

Requested changes have been implemented

[ryanthecoder]

lgtm

[SyntaxColoring]

I think we should remove this new execute_command_with_result() method for now. It seems like it's contributing to confusion about the tradeoff between returning a result and allowing for error recovery.

Thankfully, we shouldn't need it for the behavior that we want to implement right now. See my other comment.

[SyntaxColoring]

Recap of in-person discussion. Looks like you've already done some of this by the time I'm posting this comment.

InstrumentContext.require_liquid_presence()

We want this method to give the client the opportunity for error recovery, but raise an exception if that ultimately fails. That's what SyncClient.execute_command(), so under the hood, this should be implemented using that. If that raises an exception, we should just let it propagate.

InstrumentContext.detect_liquid_presence()

We want this method to straight-up return True/False without giving the client the opportunity for error recovery. So, under the hood:

• We should implement it via SyncClient.execute_command_without_recovery().

• If the probe detects no liquid, execute_command_without_recovery() will raise that as an exception. We should look for that and translate it into a False return.

  We can catch ProtocolCommandFailedError, and then check if exception.original_error is an instance of LiquidNotFoundError. If so, we should translate it into a False return. Otherwise, we should re-raise the exception to let it propagate.

I think this will expose a bug where the robot will mistakenly go into error recovery mode (because "the command failed") and try to keep running the protocol (because we're catching the exception, translating it to False, and letting the protocol continue). We can take that on in a separate PR: EXEC-598.

InstrumentContext.measure_liquid_height()

We want to keep this internal and experimental for now and keep our options open for the future. So:

• We should say in its docstring that it's internal and experimental, and exclude it from the public docs.opentrons.com website. Here is an example of how to exclude it from docs.opentrons.com.
• We should implement this via SyncClient.execute_command_without_recovery(). So if no liquid is detected, it will raise an exception. We should just let that propagate.

The reason we want to keep this internal and experimental is that we haven't really fleshed out how to support error recovery on PAPI methods that return results. Does the UI flow need to have a step to retry the failed probe? Should we just raise some documented exception for the protocol author to catch and deal with how they please?

[SyntaxColoring]

For API usability, I think we would ideally have this return the height from the bottom of the well, instead of from the deck. That seems more relevant because it wouldn't be coupled to the labware offsets that the operator happened to choose at the beginning of the run.

This doesn't need to happen right now as long as we're marking this method internal and experimental.

[SyntaxColoring]

I think this gets a lot cleaner if you split this into two methods:

1. One that does SyncClient.execute_command().
2. One that does SyncClient.execute_command_without_recovery().

SyncClient is drawing a distinction those two fundamentally modes of operation, representing a tradeoff between having a command result and allowing for error recovery, and representing that tradeoff with two different method signatures. Those map cleanly to the public InstrumentContext behaviors that we want to implement, if we maintain the two-method distinction through InstrumentCore.