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): Do not load the absorbance reader lid with loadLabware #16734

Merged
merged 26 commits into from
Nov 12, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Nov 7, 2024

Overview

Closes RQA-3471 and its linked tickets.

Test plan

Manual testing

  • Analysis should always match the actual run: neither should ever have any loadLabware command for the plate reader.
  • Plate reader open_lid() and close_lid() commands should still work, physically.
    • It should not, for example, raise an error relating to the lid labware being missing.
    • The gripper offset should still be correct.
  • Plate reader initialize() and read() commands should still work. They should not, for example, erroneously complain that the lid is missing.
  • You should not be able to move a labware onto the dock.
  • E-stop behavior should still work as developed and tested in PR feat(api, robot-server, app): Handle the Plate Reader lid while held by the gripper when the Estop is pressed. #16574:
    • Make sure the plate reader lid is returned to its dock (staging area) after the stop is physically and logically disengaged.
    • Make sure the run is canceled when the estop is pressed.
    • Make sure the estop modal is shown while placing the plate reader lid back in its dock.
    • Make sure that we only call placeLabware when the estop is pressed
    • Make sure all the above works on the ODD and Desktop app.
  • Plate readers should still be able to be added or moved around before the run starts.
    1. Start off with a deck configuration with no plate reader,
    2. Then start setting up a plate reader protocol.
    3. Then add the plate reader during run setup.
    4. Then start the run.

Covered by automated tests

  • You should not be able to move a labware onto or off of the plate reader while the lid is closed.

Changelog

As discussed with @vegano1 and @CaseyBatten (see RQA-3471 for links to the conversation history), this mostly "de-labware-ifies" the absorbance reader lid.

Formerly, the lid was a labware that was supposed to be loaded implicitly from the robot's deck configuration, with no loadLabware command involved, neither from the protocol nor from the system. There were a few cracks where this did not quite work, and we tried to cover those cracks by having the system also implicitly emit loadLabware commands, in some cases.

With this PR, the lid is no longer a labware: it does not have a labware ID, it does not occupy a location in LabwareState, and there is never a loadLabware command for it. Instead, the lid is just part of the module, similar to how the Thermocycler lid is just part of the Thermocycler.

The reason we didn't do this in the first place was that our gripper code, and our geometry code in general, is tightly coupled to loaded labware. The bulk of this PR is to unwind some of that so it can work on other things.

As an implementation detail and as a compromise for the amount of work, the geometry of the lid is still specified as a labware definition, even though it should never be loaded as a labware. The one place that this implementation detail leaks outside of opentrons.protocol_engine is the way that we get the gripper to gracefully drop the lid if a run is E-stopped. That currently involves running the unsafe/placeLabware command in a maintenance run, and that command is based on the gripper payload having a labware definition.

Incidentally, this also fixes a bug where the system would let you load a lid into the absorbance reader after an .open_lid()/close_lid() cycle. The intent is to always disallow moving or loading labware into it while the lid is closed.

Review requests

  • Any other dead code we can eliminate now?
  • Is any of this going too far?
  • There were some "get ___ offset" methods that had names that I found ambiguous. Do my new names seem correct and less ambiguous?

Risk assessment

High risk to behavior related to the absorbance reader. This unfortunately upheaves a lot of it, hence the big test plan.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

This looks like what we want modulo figuring out some better way to link the lid def and the module. Honestly I think it's ok to have the two linked in code - sometimes weird stuff is just weird, and it's not worth schematizing every single possible weirdness - but we should probably source it from the module code somehow.


state_update.set_labware_location(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we not want to update the location of the labware to keep track of it?

Right now we are using this command for the absorbance reader lid in a temporary maintenance run, however, it can also be used for any other labware in either a maintenance run or a regular run. Which means we might care about keeping track of the labware location.

  • For regular labware we can update the location with this state_update.set_labware_location function
  • For the absorbance reader lid, we should update the is_lid_on substate how we do so for close_lid and open_lid commands.
state_update.set_absorbance_reader_lid(
    module_id=mod_substate.module_id, is_lid_on=False
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is one way that this could turn out, yeah. I don't want to anticipate too much at this point, though.

I think if we get to the point where we want to run other commands after this one, we should probably think about what it would take for us to graduate this command from unsafe.

Like, that would effectively be adding a new way to load a labware into a run. It's nice that all labware loading currently gets funneled through loadLabware, in my opinion. So maybe it could work by allowing location: "gripper" in loadLabware, and then placeLabware would drop whatever is currently loaded into the gripper.

async def move_labware_with_gripper(
self,
*,
labware_definition: LabwareDefinition,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice
Using labware_definition instead of labware_id makes moving geometry around simple for things like lids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something else that I've been noticing here is that it's actually kind of misleading for many of these methods to take labware_ids in the first place.

Like, a labware_id is a pointer to information about a concrete, loaded labware. It has a location and an LPC offset. But then many of these methods don't take advantage of that information, and they only return something from the labware definition. So when we're computing a complicated stackup of offsets, it's hard to tell which methods add the LPC offset and which methods don't (for example).

@@ -71,43 +69,6 @@ async def get_deck_fixed_labware(
slot = cast(Optional[str], fixture.get("slot"))

if (
deck_configuration is not None
Copy link
Contributor

Choose a reason for hiding this comment

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

Adieu

"opentrons/opentrons_flex_lid_absorbance_plate_reader_module/1"
]

@overload
Copy link
Contributor

Choose a reason for hiding this comment

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

good use of overloads to change the shape of the function only where needed

Copy link
Contributor

@vegano1 vegano1 left a comment

Choose a reason for hiding this comment

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

Great approach overall, I left a few comments.

We spoke irl about It yesterday, but one case to keep in mind is

  • loading in the plate reader lid definition when connecting a module after the protocol engine has been created, like in the module setup flow before starting a protocol run.

Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (chore_release-8.2.0@2fda991). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@                  Coverage Diff                   @@
##             chore_release-8.2.0   #16734   +/-   ##
======================================================
  Coverage                       ?   79.32%           
======================================================
  Files                          ?      120           
  Lines                          ?     4459           
  Branches                       ?        0           
======================================================
  Hits                           ?     3537           
  Misses                         ?      922           
  Partials                       ?        0           
Flag Coverage Δ
g-code-testing 92.43% <ø> (?)
shared-data 74.02% <ø> (?)

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

@SyntaxColoring SyntaxColoring marked this pull request as ready for review November 8, 2024 20:58
@SyntaxColoring SyntaxColoring requested review from a team as code owners November 8, 2024 20:58
Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

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

Very nice work! Thank you for picking this up!

@@ -93,6 +96,20 @@ async def create_protocol_engine(
file_provider=file_provider,
)

# todo(mm, 2024-11-08): This is a quick hack to support the absorbance reader, which
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@vegano1 vegano1 left a comment

Choose a reason for hiding this comment

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

Thank you, I went through the test suite and Its all working as expected. Here is specifically what I tested.

  • Make sure the plate reader lid is placed back in the dock when held by the gripper after an e-stop event.
  • Make sure we can LPC labware in and out of the plate reader
  • Run the following protocol successfully
from typing import cast
from opentrons import protocol_api
from opentrons.protocol_api.module_contexts import AbsorbanceReaderContext

# metadata
metadata = {
    'protocolName': 'Absorbance Reader Multi read/csv test',
    'author': 'Platform Expansion',
}

requirements = {
    "robotType": "Flex",
    "apiLevel": "2.21",
}

# protocol run function
def run(protocol: protocol_api.ProtocolContext):
    mod = cast(AbsorbanceReaderContext, protocol.load_module("absorbanceReaderV1", "C3"))
    plate = protocol.load_labware("nest_96_wellplate_200ul_flat", "C2")
    tiprack_1000 = protocol.load_labware(load_name='opentrons_flex_96_tiprack_1000ul', location="B2")
    trash_labware = protocol.load_trash_bin("B3")
    instrument = protocol.load_instrument("flex_8channel_1000", "right", tip_racks=[tiprack_1000])
    instrument.trash_container = trash_labware

    # pick up tip and perform action
    instrument.pick_up_tip(tiprack_1000.wells_by_name()['A1'])
    instrument.aspirate(100, plate.wells_by_name()['A1'])
    instrument.dispense(100, plate.wells_by_name()['B1'])
    instrument.return_tip()
   
    # Initialize to a single wavelength with reference wavelength
    mod.close_lid()
    mod.initialize('single', [600], 450)

    # Remove the Plate Reader lid using the Gripper.
    mod.open_lid()
    protocol.move_labware(plate, mod, use_gripper=True)
    mod.close_lid()

    # Take a reading and show the resulting absorbance values.
    result = mod.read()
    msg = f"single: {result}"
    protocol.comment(msg=msg)
    protocol.pause(msg=msg)

    # Initialize to multiple wavelengths
    protocol.pause(msg="Perform Multi Read")
    mod.open_lid()
    protocol.move_labware(plate, "C2", use_gripper=True)
    mod.close_lid()
    mod.initialize('multi', [450, 570, 600])

    # Open the lid and move the labware into the reader
    mod.open_lid()
    protocol.move_labware(plate, mod, use_gripper=True)

    # pick up tip and perform action on labware inside plate reader
    instrument.pick_up_tip(tiprack_1000.wells_by_name()['A1'])
    instrument.aspirate(100, plate.wells_by_name()['A1'])
    instrument.dispense(100, plate.wells_by_name()['B1'])
    instrument.return_tip()

    mod.close_lid()

    # Take reading
    result = mod.read()
    msg = f"multi: {result}"
    protocol.comment(msg=msg)
    protocol.pause(msg=msg)

    # Take a reading and save to csv
    protocol.pause(msg="Perform Read and Save to CSV")
    result = mod.read(export_filename="plate_reader_csv.csv")
    msg = f"csv: {result}"
    protocol.pause(msg=msg)

    # Place the Plate Reader lid back on using the Gripper.
    mod.open_lid()
    protocol.move_labware(plate, "C2", use_gripper=True)
    mod.close_lid()

@SyntaxColoring SyntaxColoring merged commit 60dca54 into chore_release-8.2.0 Nov 12, 2024
48 checks passed
@SyntaxColoring SyntaxColoring deleted the no_loadlabware_for_lid branch November 12, 2024 16:52
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.

4 participants