fix(api): OT2 fixed trash load fix and API 2,15 support for new trash container structure #14145

CaseyBatten · 2023-12-07T22:05:07Z

Overview

This seeks to close: RSS-417

Also seeks to add coverage for API 2.15 protocols expected fixed trash behavior.

Test Plan

Ensure no analysis failures!
Test a 2.16 Protocol on OT2, it should execute correctly
Test a 2,16 protocol on Flex with multiple trashes
Test a 2.15 protocol with trashing behavior on OT2 and Flex, legacy trash behavior should be expected.

…tion_behavior

codecov · 2023-12-07T22:13:35Z

Codecov Report

Merging #14145 (38a30b5) into chore_release-7.1.0 (e0ac68e) will increase coverage by 0.00%.
Report is 3 commits behind head on chore_release-7.1.0.
The diff coverage is n/a.

Additional details and impacted files

@@                 Coverage Diff                  @@
##           chore_release-7.1.0   #14145   +/-   ##
====================================================
  Coverage                70.41%   70.42%           
====================================================
  Files                     2483     2483           
  Lines                    70339    70339           
  Branches                  8778     8778           
====================================================
+ Hits                     49531    49533    +2     
+ Misses                   18653    18652    -1     
+ Partials                  2155     2154    -1

Flag	Coverage Δ
g-code-testing	`96.44% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`

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

Files	Coverage Δ
api/src/opentrons/protocol_api/protocol_context.py	`92.30% <ø> (ø)`
api/src/opentrons/protocol_engine/state/labware.py	`100.00% <ø> (ø)`

... and 1 file with indirect coverage changes

ecormany

Thanks for the quick fix!

I pulled the branch and simulated the protocol included in RSS-417. Works as expected. 🚀

Will let others test the rest and inspect the code.

SyntaxColoring · 2023-12-07T23:29:06Z

This seeks to close [...] https://opentrons.atlassian.net/browse/RQA-2022

Are we sure that this closes that? That looks like something from within Protocol Engine, unrelated to PAPI. Notice how the error message complains about a missing fixedTrash (camelCase, like it appears in Protocol Engine commands and the deck definition), not a missing fixed_trash (snake_case, as it appears in the Python Protocol API).

[Update] Yeah, after digging into the error from that ticket, I'm pretty sure it's unrelated to this. I'm removing that ticket from this PR for now.

SyntaxColoring

Unfortunately I don't think this fully resolves RSS-417. Try this:

requirements = {
    # "apiLevel": "2.15",  # Works
    "apiLevel": "2.16",  # Does not work
    "robotType": "OT-2",
}


def run(protocol):
    # Note: ProtocolContext.fixed_trash, not InstrumentContext.trash_container.
    # 
    # This raises NoTrashDefinedError.
    protocol.fixed_trash

I caught this by running this against the tests in #14142. I'm going to pull those into this PR.

Merging to benefit from new tests.

Transition to use of should_load_fixed_trash on API check, fixes to support all current tests, updates to old tests

CaseyBatten · 2023-12-08T18:18:41Z

api/src/opentrons/protocol_api/protocol_context.py

        """
+        if self._api_version >= APIVersion(2, 16):


This newly added section to should provide handling for the issue Max identified and has tests implemented for. The following behavior is to be expected as a result:
v2.16 OT2 Protocol will identify protocol.fixed_trash as a TrashBin object
v2.16 Flex Protocol will raise an error on protocol.fixed_trash, notifying that for the Flex fixed trash does not exist on API version 2.16 and above.

CaseyBatten

Changes integrated to cover requested issue regarding protocol context fixed trash references.

SyntaxColoring

This looks like it'll work and I'm satisfied with our test coverage now. Thanks for seeing this through!

Here are some code organizational suggestions. ~~I'd like us to resolve the NoTrashDefinedError vs. APIVersionError thing before merging, because it seems easy enough. We can leave the harder ones to later, if we need to move on to other things.~~ [EDIT: Ehh we can leave it all to later if we need to.]

SyntaxColoring · 2023-12-08T20:43:19Z

api/src/opentrons/protocol_api/protocol_context.py

+        # With the addition of Moveable Trashes and Waste Chute support, it is not necessary
+        # to ensure that the list of "disposal locations", essentially the list of trashes,
+        # is initialized correctly on protocols utilizing former API versions prior to 2.16
+        # and also to ensure that any protocols after 2.16 intialize a Fixed Trash for OT-2
+        # protocols so that no load trash bin behavior is required within the protocol itself.
+        # Protocols prior to 2.16 expect the Fixed Trash to exist as a Labware object, while
+        # protocols after 2.16 expect trash to exist as either a TrashBin or WasteChute object.


Can this comment be broken up, or shortened, for readability? I'm having trouble parsing it.

SyntaxColoring · 2023-12-08T20:49:56Z

api/src/opentrons/protocol_api/protocol_context.py

+        # protocols so that no load trash bin behavior is required within the protocol itself.
+        # Protocols prior to 2.16 expect the Fixed Trash to exist as a Labware object, while
+        # protocols after 2.16 expect trash to exist as either a TrashBin or WasteChute object.
+
        self._load_fixed_trash()


It looks like some trash initialization code already existed in self._load_fixed_trash(). Organizationally, it seems like we should be adding our new code there.

SyntaxColoring · 2023-12-08T20:56:19Z

api/src/opentrons/protocol_api/protocol_context.py

+        if should_load_fixed_trash_for_python_protocol(self._api_version):
+            self._core.append_disposal_location(self.fixed_trash)
+        elif (
+            self._api_version >= APIVersion(2, 16)
+            and self._core.robot_type == "OT-2 Standard"
+        ):
+            _fixed_trash_trashbin = TrashBin(
+                location=DeckSlotName.FIXED_TRASH, addressable_area_name="fixedTrash"
+            )
+            self._core.append_disposal_location(_fixed_trash_trashbin)


I think this is showing us that we need to rename or rework should_load_fixed_trash_for_python_protocol().

I'm not super familiar with that code, but I'm inferring from this usage that it currently works like "should we load a fixed trash labware," not "should we load any form of fixed trash at all."

So:

We call should_load_fixed_trash_for_python_protocol().

If it returns yes: Load a fixed trash as a Labware.

If it returns no:

Do our own check for whether we should load a fixed trash.

If yes: Load a fixed trash as a TrashBin.

This is what's confusing to me. It reads like we just asked should_load_fixed_trash_for_python_protocol() a question, then ignored its answer and did things our own way anyway.

@jbleon95 Any opinions?

SyntaxColoring · 2023-12-08T21:05:40Z

api/src/opentrons/protocol_api/protocol_context.py

+        if should_load_fixed_trash_for_python_protocol(self._api_version):
+            self._core.append_disposal_location(self.fixed_trash)
+        elif (
+            self._api_version >= APIVersion(2, 16)


As far as I can tell, this self._api_version >= APIVersion(2, 16) check is not doing anything. We can only reach here if self._api_version >= APIVersion(2, 16), because there's a similar version check in the call to should_load_fixed_trash_for_python_protocol(), above.

I see where this is coming from, but I think we should leave this _api_version check out. I'm worried about accidentally leaving a gap between should_load_fixed_trash_for_python_protocol()'s maximum APIVersion, which defines the latest version where the fixed trash is a Labware, and this APIVersion, which defines the earliest version where the fixed trash is a TrashBin. If there's a gap, we'd be left with versions where OT-2 protocols accidentally have no fixed trash.

I would just write this as elif self._core.robot_type == "OT-2 Standard".

SyntaxColoring · 2023-12-08T21:10:35Z

api/src/opentrons/protocol_api/protocol_context.py

+                raise APIVersionError(
+                    "Fixed Trash is not supported on Flex protocols in API Version 2.16 and above."
+                )


Let's raise NoTrashDefinedError instead of APIVersionError. Whether protocol.fixed_trash is available is a question of more than just the apiLevel; it has to do with the robot type, too.

Suggested change

raise APIVersionError(

"Fixed Trash is not supported on Flex protocols in API Version 2.16 and above."

)

raise NoTrashDefinedError(

"Fixed Trash is not supported on Flex protocols in API Version 2.16 and above."

)

And we'll have to update the except that's catching it, too.

SyntaxColoring · 2023-12-08T21:50:12Z

api/src/opentrons/protocol_api/protocol_context.py

+                )
+            if isinstance(disposal_locations[0], TrashBin):
+                return disposal_locations[0]
+
        fixed_trash = self._core_map.get(self._core.fixed_trash)
        if fixed_trash is None:
            raise NoTrashDefinedError(
                "No trash container has been defined in this protocol."
            )
+


This is a bit confusing to me because it looks like we now have two competing ways of keeping track of the fixed trash: AbstractProtocol.fixed_trash, and AbstractProtocol.get_disposal_locations().

AbstractProtocol.append_disposal_location()/.get_disposal_locations() were really only added specifically to keep track of the user's explicit calls to ProtocolContext.load_waste_chute() and ProtocolContext.load_trash_bin(). They could also keep track of the fixed trash, like you have here, but if we do that, we need to do it consistently.

In other words, I think we need to choose between either of these two strategies:

AbstractProtocol.get_disposal_locations() returns the fixed trash or the user's explicitly-loaded trashes.

Then, we would delete self._core.fixed_trash from our codebase.

AbstractProtocol.get_disposal_locations() returns only the user's explicitly-loaded trashes. AbstractProtocol.fixed_trash returns only the Opentrons standard fixed trash, if there is one.

Then, when a user accesses ProtocolContext.fixed_trash, we would not do self._core.get_disposal_locations()[0]. Instead, we would do self._core.fixed_trash.

CaseyBatten added 7 commits December 6, 2023 16:37

Addition of trash bins and waste chutes to appropriate commands

1dacc49

Merge branch 'chore_release-7.1.0' into trash_wastechute_command_loca…

ba0f1c8

…tion_behavior

Addition of blow out to transfer, analysis error fix

9933c3d

linting fixes

d3313f4

Addition of trash logic for 215 and OT2

21432b4

Adjustment to support trash labware

6fd0245

Testing fixes

d9d03f2

CaseyBatten requested a review from a team as a code owner December 7, 2023 22:05

CaseyBatten changed the base branch from edge to chore_release-7.1.0 December 7, 2023 22:09

Merge branch 'chore_release-7.1.0' into OT2_and_2_15_trash_fix

d1e2c7d

ecormany reviewed Dec 7, 2023

View reviewed changes

SyntaxColoring requested changes Dec 7, 2023

View reviewed changes

SyntaxColoring added 3 commits December 7, 2023 18:44

Merge branch 'chore_release-7.1.0' into OT2_and_2_15_trash_fix

7703efb

Merging to benefit from new tests.

Remove a pytest.mark.ot3_only that was on the wrong test.

82ea645

Remove the pytest.mark.xfail for the bug we're trying to fix.

0147462

y3rsh mentioned this pull request Dec 8, 2023

changes so far found in v7.1.0-alpha.2 vs v7.0.2 #14148

Closed

1 task

Abandon all trash ye who enter here

38a30b5

Transition to use of should_load_fixed_trash on API check, fixes to support all current tests, updates to old tests

CaseyBatten commented Dec 8, 2023

View reviewed changes

SyntaxColoring approved these changes Dec 8, 2023

View reviewed changes

CaseyBatten merged commit d5d3968 into chore_release-7.1.0 Dec 8, 2023
25 checks passed

SyntaxColoring deleted the OT2_and_2_15_trash_fix branch February 29, 2024 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): OT2 fixed trash load fix and API 2,15 support for new trash container structure #14145

fix(api): OT2 fixed trash load fix and API 2,15 support for new trash container structure #14145

CaseyBatten commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

ecormany left a comment

SyntaxColoring commented Dec 7, 2023 •

edited

Loading

SyntaxColoring left a comment

CaseyBatten Dec 8, 2023

CaseyBatten left a comment

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring Dec 8, 2023

SyntaxColoring Dec 8, 2023

SyntaxColoring Dec 8, 2023

SyntaxColoring Dec 8, 2023

SyntaxColoring Dec 8, 2023 •

edited

Loading

SyntaxColoring Dec 8, 2023 •

edited

Loading

fix(api): OT2 fixed trash load fix and API 2,15 support for new trash container structure #14145

fix(api): OT2 fixed trash load fix and API 2,15 support for new trash container structure #14145

Conversation

CaseyBatten commented Dec 7, 2023 • edited Loading

Overview

Test Plan

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

ecormany left a comment

Choose a reason for hiding this comment

SyntaxColoring commented Dec 7, 2023 • edited Loading

SyntaxColoring left a comment

Choose a reason for hiding this comment

CaseyBatten Dec 8, 2023

Choose a reason for hiding this comment

CaseyBatten left a comment

Choose a reason for hiding this comment

SyntaxColoring left a comment • edited Loading

Choose a reason for hiding this comment

SyntaxColoring Dec 8, 2023

Choose a reason for hiding this comment

SyntaxColoring Dec 8, 2023

Choose a reason for hiding this comment

SyntaxColoring Dec 8, 2023

Choose a reason for hiding this comment

SyntaxColoring Dec 8, 2023

Choose a reason for hiding this comment

SyntaxColoring Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

CaseyBatten commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

SyntaxColoring commented Dec 7, 2023 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading

SyntaxColoring Dec 8, 2023 •

edited

Loading

SyntaxColoring Dec 8, 2023 •

edited

Loading