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

feat(api): return machine appropriate deck slot names in PAPIv2 #12595

Merged
merged 16 commits into from
May 5, 2023

Conversation

jbleon95
Copy link
Contributor

@jbleon95 jbleon95 commented Apr 28, 2023

Overview

Closes RLAB-337.

This PR makes it so that any call to a public facing Python method that previously would return a string for the deck slot (e.g. "1", "2", "3", etc) now will return a deck coordinate ("A1", "A2" "A3", etc) when analyzing/running on a Flex machine. When on an OT-2, previous behavior will be kept.

In practice this only covers the two parent properties, Labware.parent and ModuleContext.parent. The Deck class also has methods that return slot definitions that at reference slot ids, but those are reliant on the deck definition, which is covered in PR #12571.

Test Plan

Tested the following protocol passes analysis and runs on a robot

metadata = {
    'protocolName': 'Deck Coordinate PAPIv2 Test',
}

requirements = {
    "robotType": "OT-3",
    "apiLevel": "2.14"
}

LABWARE_LOCATIONS = ['2', 6, 'C1', 'b3']
OT3_COORDINATE_NAMES = ['D2', 'C3', 'C1', 'B3']

MOVE_LABWARE_LOCATION = 'C2'

THERMOCYCLER_LOCATION = 'B1'
HEATER_SHAKER_LOCATION = 'D3'
TEMP_MODULE_LOCATION_OT2 = 1
TEMP_MODULE_LOCATION_OT3 = 'D1'


def run(context):
    # Protocol Context Load Labware
    labwares = []
    for loc in LABWARE_LOCATIONS:
        labwares.append(context.load_labware("armadillo_96_wellplate_200ul_pcr_full_skirt", loc))

    # Labware Context (parent)
    for coordinate, labware in zip(OT3_COORDINATE_NAMES, labwares):
        assert labware.parent == coordinate, f"Labware parent {labware.parent} does not equal {coordinate}"

    # Protocol Context Move Labware
    context.move_labware(labware=labwares[0], new_location=MOVE_LABWARE_LOCATION, use_gripper=True)
    assert labwares[0].parent == MOVE_LABWARE_LOCATION, f"Labware parent {labwares[0].parent} does not equal {MOVE_LABWARE_LOCATION}"

    # Protocol Context Load Module
    thermocycler = context.load_module("thermocyclerModuleV2", THERMOCYCLER_LOCATION)
    heater_shaker = context.load_module("heaterShakerModuleV1", HEATER_SHAKER_LOCATION)
    temp_module = context.load_module("temperatureModuleV2", TEMP_MODULE_LOCATION_OT2)

    # Module Context (parent)
    assert thermocycler.parent == THERMOCYCLER_LOCATION, f"Thermocycler parent does not equal {THERMOCYCLER_LOCATION}"
    assert heater_shaker.parent == HEATER_SHAKER_LOCATION, f"Heater-Shaker parent does not equal {HEATER_SHAKER_LOCATION}"
    assert temp_module.parent == TEMP_MODULE_LOCATION_OT3, f"Temperature Module parent does not equal {TEMP_MODULE_LOCATION_OT3}"

Changelog

  • Updated Labware.parent and ModuleContext.parent to return deck coordinates when running on Flex
  • Removed some implicit DeckSlotName to str conversions that were unclear
  • Added robot_type property to protocol cores
  • Added OT-3 integration test to robot server for analyzing a protocol returning deck coordinates
  • Fixed workflow oversight where tests marked as ot3_only were never getting run on CI

Review requests

Risk assessment

Medium. This PR does not change behavior on the OT-2, but any protocol that relies on any of the above methods may not function as they did previous to this PR.

@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #12595 (abdc608) into edge (00d7430) will increase coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12595      +/-   ##
==========================================
+ Coverage   73.31%   73.56%   +0.24%     
==========================================
  Files        1506     2271     +765     
  Lines       49367    62467   +13100     
  Branches     2998     6644    +3646     
==========================================
+ Hits        36192    45952    +9760     
- Misses      12719    14931    +2212     
- Partials      456     1584    +1128     
Flag Coverage Δ
notify-server 89.13% <ø> (ø)

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

Impacted Files Coverage Δ
api/src/opentrons/protocol_api/module_contexts.py 88.31% <ø> (ø)
api/src/opentrons/protocol_api/labware.py 90.80% <100.00%> (+0.03%) ⬆️

... and 776 files with indirect coverage changes

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Lookin' good so far!

api/src/opentrons/protocol_api/module_contexts.py Outdated Show resolved Hide resolved
robot-server/tests/integration/conftest.py Outdated Show resolved Hide resolved
@jbleon95 jbleon95 marked this pull request as ready for review May 5, 2023 14:27
@jbleon95 jbleon95 requested review from a team as code owners May 5, 2023 14:27
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Nice. Thanks for figuring out how to split this into an individually mergeable piece. Here are a couple of small things, but other than these, this looks good to merge.

robot-server/tests/integration/conftest.py Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Recapping from an in-person between me and @jbleon95, for posterity:

We're basically integration-testing the Python Protocol API through robot-server here. We're doing this because I don't know what the proper way to do this is, these days. But this is almost certainly not it. We should keep thinking about how to do this better.

.github/workflows/robot-server-lint-test.yaml Outdated Show resolved Hide resolved
@jbleon95 jbleon95 merged commit 94d367d into edge May 5, 2023
SyntaxColoring added a commit that referenced this pull request May 8, 2023
@SyntaxColoring SyntaxColoring deleted the RLAB-337_return_deck_coordinates branch May 26, 2023 19:31
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