Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(api): Ensure stack of labware on Staging Area Slot properly resolves ancestor slot #16681

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

CaseyBatten
Copy link
Contributor

@CaseyBatten CaseyBatten commented Nov 4, 2024

Overview

Covers PLAT-538

Previously attempting to stack TC lids in the staging area slots would result in incorrect "Off-Deck" errors. Any and all labware could not be properly stacked in this location due to the Protocol Engine not being able to properly resolve the 'ancestor slot' of staging area slots.

In order to fix this, we now allow Staging Area Slot locations to be resolved as an ancestor, and raise errors where this would cause a conflict (due to staging area slots, as a location, being tied to the fixture location of column 3, rather than column 4). Fortunately, only pipetting actions would encounter these cases. Since we do not allow pipetting in column 4, we should theoretically never run into these kinds of problems.

Test Plan and Hands on Testing

  • The following protocol should pass analysis and perform as expected during execution:
from typing import List, Dict, Any, Optional
from opentrons.protocol_api import ProtocolContext, Labware

metadata = {"protocolName": "Opentrons Tough Auto-Sealing Lid Test"}
requirements = {"robotType": "Flex", "apiLevel": "2.20"}

LID_STARTING_SLOT = "D4"
LID_ENDING_SLOT = "C1"
LID_COUNT = 5
LID_DEFINITION = "opentrons_tough_pcr_auto_sealing_lid"
LID_BOTTOM_DEFINITION = "opentrons_tough_pcr_auto_sealing_lid"

USING_THERMOCYCLER = False

OFFSET_DECK = {
    "pick-up": {"x": 0, "y": 0, "z": 0},
    "drop": {"x": 0, "y": 0, "z": 0},
}
OFFSET_THERMOCYCLER = {
    "pick-up": {"x": 0, "y": 0, "z": 0},
    "drop": {"x": 0, "y": 0, "z": 0},
}


def _move_labware_with_offset_and_pause(
    protocol: ProtocolContext,
    labware: Labware,
    destination: Any,
    pick_up_offset: Optional[Dict[str, float]] = None,
    drop_offset: Optional[Dict[str, float]] = None,
) -> None:
    protocol.move_labware(
        labware,
        destination,
        use_gripper=True,
        pick_up_offset=pick_up_offset,
        drop_offset=drop_offset,
    )


def run(protocol: ProtocolContext):
    # SETUP
    lids: List[Labware] = [protocol.load_labware(LID_BOTTOM_DEFINITION, 'B4')]
    for i in range(LID_COUNT - 1):
        lids.append(lids[-1].load_labware(LID_DEFINITION))
    lids.reverse()  # NOTE: reversing to more easily loop through lids from top-to-bottom

    # RUN
    prev_moved_lid: Optional[Labware] = None
    for lid in lids:
        _move_labware_with_offset_and_pause(
            protocol,
            lid,
            prev_moved_lid if prev_moved_lid else LID_ENDING_SLOT,
            pick_up_offset=OFFSET_DECK["pick-up"],
            drop_offset=OFFSET_DECK["drop"],
        )
        prev_moved_lid = lid

Changelog

Review requests

Do the changes here seem to make sense? Any issues that could occur from the error-raising I'm doing to prevent our conflict cases?

What should we do about the PAPI core get_deck_slot(...) function? We have historically had it return an optional DeckSlotName object, should we change that to an optional Union including StagingAreaSlotName? Would that have any negative downstream effects on things using that function like loaded_labwares()? Right now I just have it mutating the slot to a DeckSlotName for an equivalent slot in column 3, which is a lie but technically is the deck cutout that both columns originate from. Probably want to replace this with something more sensible before merging.

Risk assessment

Low - I believe we're covering the conflict cases pretty aggressively here, and such states should be unreachable. As a result, resolving an ancestor to column 4 should be unblocked for the gripper.

@CaseyBatten CaseyBatten requested a review from a team as a code owner November 4, 2024 19:01
Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than the get_deck_slot change I requested (and some spelling). Would like to see a unit test or two to cover the major changes made.

self.labware_id
)
if isinstance(ancestor, StagingSlotName):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like the wrong thing to return with, since the deck slot it's on is not actually on the third column. But pulling back further, this function is only used by ProtocolContext.loaded_labwares which is a function that is now wildly out of date with how we load labware (it uses OT-2 style deck slot ints, it ignores anything off-deck). So I would return to this returning None for staging slots for now, then relook into this if we decide to introduce a new, similar function.

Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the changes!

@CaseyBatten CaseyBatten merged commit 7669fc2 into chore_release-8.2.0 Nov 5, 2024
21 checks passed
@CaseyBatten CaseyBatten deleted the tc_lid_in_staging_area_slot branch November 5, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants