feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() #17289

sanni-t · 2025-01-16T17:43:38Z

Closes AUTH-1295, AUTH-1299

Overview

adds InstrumentContext.load_name to fetch the python API load name of a pipette
updates LiquidClass.get_for() to accept the a loaded instrument object and loaded tiprack object for pipette and tiprack args respectively.
changes the tiprack argument name of get_for() from tiprack to tip_rack to be consistent with API naming conventions.

Test Plan and Hands on Testing

Added unit and integration tests

Review requests

Note that the LiquidClass.get_for() method will still accept the pipette name and tiprack URI as arguments. This is to allow fetching liquid class properties without needing to load pipettes and tipracks. Let me know if this is not a good idea
Any naming alternatives to InstrumentContext.load_name? I want to avoid any confusion with the existing name property and this new property

Risk assessment

None

ddcc4 · 2025-01-17T18:54:36Z

Any naming alternatives to InstrumentContext.load_name? I want to avoid any confusion with the existing name property and this new property

Don't we already consistently call it load_name throughout the docs? What's the concern about calling it load_name here then?

ddcc4 · 2025-01-17T18:52:13Z

api/src/opentrons/protocol_api/_liquid.py

        """Get liquid class transfer properties for the specified pipette and tip."""
+        from . import InstrumentContext, Labware


Why can't we just import this at the top of the file (and get rid of the if TYPE_CHECKING bit)?

You are clearly using the symbols InstrumentContext and Labware in your implementation (versus only importing it for the typechecker), so why not treat it as a normal import?

Adding to this, I can see limited use of getting liquid class properties without having to provide the names, but I'd almost rather that be it's own function which would allow us to remove this internal import. That or we could always flip around the isinstance to check for str instead of InstrumentContext or Labware

Why can't we just import this at the top of the file (and get rid of the if TYPE_CHECKING bit)?

Because it results in a circular import for InstrumentContext. InstrumentContext already imports from _liquid.

I can see limited use of getting liquid class properties without having to provide the names, but I'd almost rather that be it's own function

Not sure I understand. Can you elaborate?

That or we could always flip around the isinstance to check for str instead of InstrumentContext or Labware

If we want to verify that the args are not of some third, unsupported type then we will have to check if isinstance of InstrumentContext and Labware. Sure we can maybe check that if it's not a string and if there's an error doing pipette.load_name or tiprack.uri, the args are wrong. But it's not as foolproof as doing isinstance checks.

SyntaxColoring

TY! Looks good except for a possible GEN1/GEN2 cross-compatibility thing.

SyntaxColoring · 2025-01-17T21:01:31Z

api/src/opentrons/protocol_api/_liquid.py

@@ -64,18 +67,42 @@ def name(self) -> str:
    def display_name(self) -> str:
        return self._display_name

-    def get_for(self, pipette: str, tiprack: str) -> TransferProperties:
+    def get_for(
+        self, pipette: Union[str, InstrumentContext], tiprack: Union[str, Labware]


Nit: "tip rack" is two words, so can we name the argument tip_rack instead of tiprack?

SyntaxColoring · 2025-01-17T21:04:58Z

api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py

+    def get_load_name(self) -> str:
+        """Get the pipette's requested API load name.
+
+        For OT-2 pipettes, this is the same as pipette name.
+        """
+        return self.get_hardware_state()["name"]
+


For OT-2 pipettes, this is the same as pipette name.

Not always, I don't think. What about GEN1/GEN2 cross-compatibility? If a protocol requests p300_single but gets a p300_single_gen2, I'd expect load_name to return p300_single. I think this get_hardware_state()-based implementation will return p300_single_gen2.

I don't have a robot in front of me to test this, so feel free to dismiss this change request if I'm wrong about this.

This is what I was missing! The description of InstrumentCore.get_pipette_name() (which is what InstrumentContext.name returns) says

Will match the load name of the actually loaded pipette, which may differ from the requested load name.

Now I know what it means.
I actually want to return the name of the loaded pipette p300_single_gen2 and not the requested pipette p300_single.

I feel like fixing InstrumentContext.name might be the more appropriate thing here. We can gate the change behind API version. Otherwise InstrumentContext.name would just be something that returns the same thing as InstrumentContext.load_name for OT-2 pipettes and something unusable for the API users when using a Flex.

👍 Makes sense.

Sorry, now I'm confused, but I wasn't following this too closely.

When public users use liquid classes, they'll call get_for(pipette_name, tiprack_name), right?

In this example here, is the pipette_name going to be p300_single or p300_single_gen2?

(If your answer is p300_single_gen2, won't that be confusing to the user?)

In this example here, is the pipette_name going to be p300_single or p300_single_gen2?

p300_single_gen2

(If your answer is p300_single_gen2, won't that be confusing to the user?)

It won't be confusing. The pipette name method will return p300_single_gen2 because that's the pipette that's actually attached to the robot and being used. All properties of that instrument context's instance (eg. default flow rates, plunger speeds, model, firmware version, etc) are properties of the attached pipette, and not the requested pipette (if it is different). So following those lines, it would be expected to get the actual name of the pipette that is attached, rather than the one requested.
There might be a reason to return the requested pipette name, but that's not the concern here and if we decide to add that, it should be called something different like requested_pipette_name.

I meant confusing as in: As far as the user knows, they requested p300_single, so they would expect to continue refer to the pipette by that name in all the code they write. Or are you saying that the user is responsible for knowing that p300_single gets mapped to p300_single_gen2?

This will come up when the user tries to write their own liquid classes, right? Like if the user has to write:

my_pipette = protocol.load_instrument("p300_single") my_liquid_class = { "???": { "blah_tiprack_300ul": { "aspirate": {..},

what should the user put for ???, and how will they know that's what they need to put there?

Heads up for the sake of this discussion that the term "load name" is a little ambiguous.

One axis is: The opentrons.protocol_api-level strings exposed in this table, versus the internal names defined by opentrons.protocol_engine.

And another axis is: The pipette that the protocol requested, versus the pipette that the system provided.

"Load name" can be about either or both of those axes, depending on how you define it.

(So in this PR, @sanni-t is correcting the first axis—the public names from the table versus the internal names. In #17289 (comment), @ddcc4 is bringing up a point about the second axis—requested vs. provided.)

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

api/src/opentrons/protocols/api_support/definitions.py

SyntaxColoring · 2025-01-30T19:31:03Z

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

+        if self._protocol_core.api_version < _FLEX_PIPETTE_NAMES_FIXED_IN:
+            return (
+                pipette.pipetteName.value
+                if isinstance(pipette.pipetteName, PipetteNameType)
+                else pipette.pipetteName
+            )


I worry a bit about having an apiLevel-based conditional like this in the "deeper" opentrons.protocol_api.core layer when it could otherwise be in the opentrons.protocol_api layer. I don't think we want our internal code to have to cope with a vague concept, "the pipette name," whose precise definition changes depending on the apiLevel. I think InstrumentCore could instead expose more precise concepts like "the load name specified in the ProtocolContext.load_instrument() call," etc. Then when we write our internal code, we explicitly choose the one that's appropriate. And the user-facing opentrons.protocol_api layer may choose between them in an apiLevel-dependent way.

This sounds good too.

api/src/opentrons/protocol_api/instrument_context.py

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

SyntaxColoring · 2025-01-30T19:59:29Z

api/src/opentrons/protocol_api/core/legacy/legacy_instrument_core.py

+    def get_load_name(self) -> str:
+        """Get the pipette's requested API load name.
+
+        For OT-2 pipettes, this is the same as pipette name.
+        """
+        return self.get_hardware_state()["name"]
+


Heads up for the sake of this discussion that the term "load name" is a little ambiguous.

One axis is: The opentrons.protocol_api-level strings exposed in this table, versus the internal names defined by opentrons.protocol_engine.

And another axis is: The pipette that the protocol requested, versus the pipette that the system provided.

"Load name" can be about either or both of those axes, depending on how you define it.

(So in this PR, @sanni-t is correcting the first axis—the public names from the table versus the internal names. In #17289 (comment), @ddcc4 is bringing up a point about the second axis—requested vs. provided.)

SyntaxColoring

Looks good to me once tests and lint pass. I think the failure is just because of unrelated breakage in edge, so maybe just make sure everything is passing locally.

@ddcc4's comment is a good point but I think the question of requested vs. provided is outside the intended scope of this PR, right? And my comment isn't a blocker.

ddcc4 · 2025-01-30T20:47:26Z

@ddcc4's comment is a good point but I think the question of requested vs. provided is outside the intended scope of this PR, right?

Right. I talked to Sanniti about this yesterday. I'm OK with the implementation here for now. (The original goal was that a protocol written for p300_single would be able to run without change if a p300_single_gen2 was loaded, but there are already other reasons why that wouldn't work. This PR would mean that a liquid class written exclusively for the p300_single also wouldn't work with a p300_single_gen2 loaded, but the user already has other problems at that point.)

Co-authored-by: Max Marrone <[email protected]>

added InstrumentContext.get_load_name(), updated LiquidClass.get_for()

24310c3

sanni-t requested a review from a team as a code owner January 16, 2025 17:43

sanni-t requested review from SyntaxColoring and jbleon95 January 16, 2025 17:44

ddcc4 reviewed Jan 17, 2025

View reviewed changes

SyntaxColoring requested changes Jan 17, 2025

View reviewed changes

sanni-t added 4 commits January 29, 2025 14:12

Merge branch 'edge' into AUTH-1295-add_get_load_name

dc0bfae

removed get_load_name, moved the changes into .name as a bug fix

2284f95

change from 'tipracks' to 'tip_racks'

e548bdc

update tiprack validation logic to account for fixed Flex pip names

9181bc5

sanni-t changed the title ~~feat(api): add InstrumentContext.load_name and update LiquidClass.get_for()~~ feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() Jan 30, 2025

sanni-t and others added 2 commits January 30, 2025 14:17

Merge branch 'edge' into AUTH-1295-add_get_load_name

06c60c9

added fix note to internal release notes

9255061

sanni-t requested a review from SyntaxColoring January 30, 2025 19:21

SyntaxColoring reviewed Jan 30, 2025

View reviewed changes

SyntaxColoring approved these changes Jan 30, 2025

View reviewed changes

sanni-t and others added 3 commits January 30, 2025 16:01

Update api/src/opentrons/protocol_api/instrument_context.py

09e9765

Co-authored-by: Max Marrone <[email protected]>

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

c252cc2

Co-authored-by: Max Marrone <[email protected]>

linting

c9b96ec

sanni-t merged commit 3c23793 into edge Jan 30, 2025
21 of 25 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() #17289

feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() #17289

sanni-t commented Jan 16, 2025 •

edited

Loading

ddcc4 commented Jan 17, 2025

ddcc4 Jan 17, 2025

jbleon95 Jan 17, 2025

sanni-t Jan 17, 2025 •

edited

Loading

SyntaxColoring left a comment

SyntaxColoring Jan 17, 2025

SyntaxColoring Jan 17, 2025

sanni-t Jan 19, 2025

SyntaxColoring Jan 21, 2025

ddcc4 Jan 21, 2025

sanni-t Jan 29, 2025

ddcc4 Jan 29, 2025

SyntaxColoring Jan 30, 2025 •

edited

Loading

SyntaxColoring Jan 30, 2025

sanni-t Jan 30, 2025

SyntaxColoring Jan 30, 2025 •

edited

Loading

SyntaxColoring left a comment

ddcc4 commented Jan 30, 2025

		"""Get liquid class transfer properties for the specified pipette and tip."""
		from . import InstrumentContext, Labware

feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() #17289

feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() #17289

Conversation

sanni-t commented Jan 16, 2025 • edited Loading

Overview

Test Plan and Hands on Testing

Review requests

Risk assessment

ddcc4 commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t Jan 17, 2025 • edited Loading

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

SyntaxColoring Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

ddcc4 commented Jan 30, 2025

sanni-t commented Jan 16, 2025 •

edited

Loading

sanni-t Jan 17, 2025 •

edited

Loading

SyntaxColoring Jan 30, 2025 •

edited

Loading

SyntaxColoring Jan 30, 2025 •

edited

Loading