feat(api): add getNexTip protocol engine command #17038

jbleon95 · 2024-12-04T19:28:34Z

Overview

Addresses part of AUTH-1063.

This PR adds a new getNextTip protocol engine command, for use in getting next tip information directly from the engine. The command takes in 3 parameters: a pipette ID, a list of labware ID (corresponding to which tip racks to find the next available tip in), and optionally a starting tip in the form of a well name (if not included this automatically resolves to "A1", mirroring how the API version of tip tracking works. It's result returns either a new nextTipInfo object containing the labware ID and well name of the next available tip (or tip to target for non-single channel/single tip configuration pipettes), or None if no available tip can be found for that pipette and tip rack(s) combination.

Test Plan and Hands on Testing

Tested on robot with Postman, along with unit test coverage.

Changelog

added getNextTip protocol engine command to return information about next available tip

Risk assessment

Low, new command that does not affect state.

SyntaxColoring

Thanks! Code all looks good to me, but I guess I have some fundamental questions.

api/src/opentrons/protocol_engine/types.py

SyntaxColoring · 2024-12-04T19:51:25Z

api/src/opentrons/protocol_engine/commands/get_next_tip.py

+        else:
+            next_tip = None
+
+        return SuccessData(public=GetNextTipResult(nextTipInfo=next_tip))


This looks like it'll work as described, but if this command isn't actually doing anything—not affecting state nor making the robot move—I guess it doesn't seem like we should be making it a command?

Like, PAPI can get this functionality by calling state_view.tips.get_next_tip(). Or some higher-level function built around that, if it's not sufficient on its own. And the HTTP API can expose this through some non-command endpoint, probably /runs/{runId}/currentState.

Reasons to avoid making it a command include avoiding database compatibility concerns and avoiding increasing our public API surface area. Especially given weirdness about startingWellName, mentioned below.

Hmm, ya, that is a good point.

I get what you're saying, but the thing is that we don't have other well-organized means to do a "structured query", I guess you'd say. It's a component of a pickUpNextTip command, or really pickUpNextTip concept, that will need to be in transfers.

We absolutely could implement it as a public method of the engine, or of an engine state view, and plumb it up through the runner and the orchestrator and the run store and the run to an HTTP route, but that increases our public API surface area also. And this way you get to see it in the runlog.

I guess I want to pass the question back at you @SyntaxColoring as to whether error recovery or any other EXEC work will need a client to fetch the next tip from engine. We are planning on adding a pickUpNextTip command sometime later but the critical part for us is knowing which tip to pick up from, and as you correctly gathered, we will be fine using a state getter function for this since it's an internal code that'll need this info. But not sure if a client will need to fetch this info too. Since auto-tip tracking in engine is a feature that is needed by a few projects, a command seemed more useful but maybe we don't need both getNextTip and pickUpNextTip.

I get what you're saying, but the thing is that we don't have other well-organized means to do a "structured query", I guess you'd say. It's a component of a pickUpNextTip command, or really pickUpNextTip concept, that will need to be in transfers.

Sorry, I'm not following what you mean by "structured query." I might be missing context. I've seen AUTH-1063 and #16602 but I don't see the part that a separate command helps with. Should I be looking somewhere else?

I guess I want to pass the question back at you as to whether error recovery or any other EXEC work will need a client to fetch the next tip from engine.

Possibly "yes," but if so, I am like 90% sure we will want to implement that as a safe, clean, read-only GET request, instead of running a live command with a POST request and having to contend with stuff like play/pause/door-open run states.

api/src/opentrons/protocol_engine/commands/get_next_tip.py

sanni-t

Looks great!

sanni-t · 2024-12-09T14:05:48Z

api/src/opentrons/protocol_engine/types.py

+    tipOriginWell: str = Field(
+        ..., description="The (starting) well name of the next available tip(s)."


Continuing with Max's point, wouldn't 'origin' be yet another term that means the same thing as 'primary' and 'starting' tip? Is it used somewhere already or will it be a new term? Also the description says 'starting well name..'.

I came up with this with Casey but yeah, I see your point in muddying the waters with another term. I'll change it to tipStartingWell for consistency

SyntaxColoring

Agreed with Sanniti but otherwise LGTM, thanks!

SyntaxColoring · 2024-12-09T15:33:49Z

api/src/opentrons/protocol_engine/commands/get_next_tip.py

+                public=GetNextTipResult(
+                    nextTipInfo=NoTipAvailable(
+                        noTipReason=NoTipReason.STARTING_TIP_WITH_PARTIAL,
+                        message="Cannot automatically resolve next tip with starting tip and partial tip configuration.",


If this limitation is to match another limitation elsewhere in our codebase, it might be worth a comment here to describe where the underlying limitation comes from, to make things less confusing if the underlying limitation gets fixed.

ddcc4 · 2024-12-09T17:02:12Z

api/src/opentrons/protocol_engine/commands/get_next_tip.py

+
+        if (
+            starting_tip_name is not None
+            and nozzle_map.configuration != NozzleConfigurationType.FULL


What does NozzleConfigurationType.FULL mean?

The pipette is configured to pick up all tips on each channel

ddcc4 · 2024-12-09T17:06:16Z

api/src/opentrons/protocol_engine/commands/get_next_tip.py

+                next_tip = NextTipInfo(labwareId=labware_id, tipStartingWell=well_name)
+                break
+            # After the first tip rack is exhausted, starting tip no longer applies
+            starting_tip_name = None


Hm, so this implementation does not try to remember which tiprack has available tips? It just loops through all the tipracks on every call?

It'll look at all tip racks given, in order, regardless of if it's completely empty. This is similar to how the API code figures out next tip (it never unassigns a tip rack once its been depleted)

ddcc4 · 2024-12-09T17:13:51Z

api/tests/opentrons/protocol_engine/commands/test_get_next_tip.py

+    decoy.when(
+        state_view.tips.get_next_tip(
+            labware_id="123",
+            num_tips=42,


So in this test, the caller is requesting the next tip for a hypothetical pipette that has 42 tips?

Yeah, I use that to make it clear that this test is not testing veracity of testing the get_next_tip logic, but is passing the expected values to this function

ddcc4 · 2024-12-09T17:15:32Z

api/tests/opentrons/protocol_engine/commands/test_get_next_tip.py

+            starting_tip_name=None,
+            nozzle_map=mock_nozzle_map,
+        )
+    ).then_return("foo")


Hm, it might be nice if this test could demonstrate the code working when there are multiple labwares and the first one doesn't have any tips left, because I'm kind of curious how your for-loop in the implementation would work.

jbleon95 added 4 commits December 4, 2024 10:57

getNextTip implementation

380f9e2

unit test for getNextTip impl

7ce4aea

use new class to hold next tip info for result

ebcf65f

address todo comment

7fe084d

jbleon95 requested review from SyntaxColoring, sanni-t, ddcc4 and CaseyBatten December 4, 2024 19:28

jbleon95 requested review from a team as code owners December 4, 2024 19:28

SyntaxColoring reviewed Dec 4, 2024

View reviewed changes

jbleon95 added 2 commits December 6, 2024 15:56

implementation fix and return reason if no tip could be found

9f790ce

schema fix

2cac652

sanni-t approved these changes Dec 9, 2024

View reviewed changes

SyntaxColoring approved these changes Dec 9, 2024

View reviewed changes

rename property and comment per review

544c628

jbleon95 merged commit 1b95421 into edge Dec 9, 2024
48 checks passed

ddcc4 approved these changes Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add getNexTip protocol engine command #17038

feat(api): add getNexTip protocol engine command #17038

jbleon95 commented Dec 4, 2024

SyntaxColoring left a comment

SyntaxColoring Dec 4, 2024 •

edited

Loading

sanni-t Dec 4, 2024

sfoster1 Dec 4, 2024

sanni-t Dec 4, 2024 •

edited

Loading

SyntaxColoring Dec 4, 2024

sanni-t left a comment

sanni-t Dec 9, 2024

jbleon95 Dec 9, 2024

SyntaxColoring left a comment

SyntaxColoring Dec 9, 2024

ddcc4 Dec 9, 2024

jbleon95 Dec 9, 2024

ddcc4 Dec 9, 2024

jbleon95 Dec 9, 2024

ddcc4 Dec 9, 2024

jbleon95 Dec 9, 2024

ddcc4 Dec 9, 2024

		tipOriginWell: str = Field(
		..., description="The (starting) well name of the next available tip(s)."

feat(api): add getNexTip protocol engine command #17038

feat(api): add getNexTip protocol engine command #17038

Conversation

jbleon95 commented Dec 4, 2024

Overview

Test Plan and Hands on Testing

Changelog

Risk assessment

SyntaxColoring left a comment

Choose a reason for hiding this comment

SyntaxColoring Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring Dec 4, 2024 •

edited

Loading

sanni-t Dec 4, 2024 •

edited

Loading