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): Fully stringify well names #13764

Merged

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Oct 11, 2023

Overview

Fixes RSS-368.

Test Plan

Run some PAPIv≥2.14 protocols through opentrons_simulate and check the run log. You shouldn't see the string None anywhere. The location strings should match PAPIv≤2.13.

Here's a test case demonstrating how we print a well of a labware on an adapter on a module on a deck slot. It also demonstrates how we prefer to print the user-defined labware label, but if that doesn't exist, we fall back to the display name specified by the labware definition.

from opentrons import protocol_api


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


def run(protocol: protocol_api.ProtocolContext) -> None:
    heater_shaker = protocol.load_module("heaterShakerModuleV1", "D1")
    source = heater_shaker.load_labware(
        "opentrons_96_wellplate_200ul_pcr_full_skirt",
        adapter="opentrons_96_pcr_adapter",
    )
    destination = protocol.load_labware(
        "opentrons_96_wellplate_200ul_pcr_full_skirt", "A2", label="destination"
    )

    tip_rack = protocol.load_labware("opentrons_flex_96_tiprack_50ul", "B1")

    pipette = protocol.load_instrument(
        "flex_1channel_50", mount="right", tip_racks=[tip_rack]
    )

    heater_shaker.close_labware_latch()
    pipette.transfer(50, source.wells()[0], destination.wells()[0])
$ pipenv run opentrons_simulate ~/Desktop/complicated_locations.py
/Users/maxmarrone/.opentrons/robot_settings.json not found. Loading defaults
Belt calibration not found.
Latching labware on Heater-Shaker
Transferring 50.0 from A1 of Opentrons Tough 96 Well Plate 200 µL PCR Full Skirt on Opentrons 96 PCR Adapter on Heater-Shaker Module GEN1 on D1 to A1 of destination on A2
	Picking up tip from A1 of Opentrons Flex 96 Tip Rack 50 µL on B1
	Aspirating 50.0 uL from A1 of Opentrons Tough 96 Well Plate 200 µL PCR Full Skirt on Opentrons 96 PCR Adapter on Heater-Shaker Module GEN1 on D1 at 8.0 uL/sec
	Dispensing 50.0 uL into A1 of destination on A2 at 8.0 uL/sec
	Dropping tip into A1 of Opentrons Fixed Trash on A3

Details

This fixes two problems with how PAPIv≥2.14 was stringifying well names when they were printed to the opentrons_simulate run log:

  1. We were only printing the user-specified labware label. That's it. If the user didn't specify a labware label, we were printing None, without making an attempt to fall back to the display name specified by the labware definition.
  2. We were just printing the name of the labware itself, without descending down to the labware's parents. So, the name of the module or deck slot that the labware was on wouldn't get printed.

We fix both of these by adding a helper function to opentrons.protocol_api.core.engine that queries the ProtocolEngine for full location information and builds a human-readable string out of it. It works recursively, so it supports deep locations like "well A1 of labware Foo on adapter Bar on module Baz on C2."

This brings PAPIv≥2.14's user-facing behavior in line with that of PAPIv≤2.13.

For comparison, PAPIv≤2.13 accomplished this through a combination of this helper function, and computing the human-readable strings eagerly as Well objects were constructed. That approach isn't sufficient for us anymore because labware can move around throughout the protocol.

Review requests

Any other stringifications that need to be fixed?

Risk assessment

Low.

@SyntaxColoring SyntaxColoring changed the base branch from edge to chore_release-7.0.1 October 11, 2023 19:19
@SyntaxColoring SyntaxColoring changed the base branch from chore_release-7.0.1 to stab_at_simulation October 11, 2023 19:19
@SyntaxColoring SyntaxColoring marked this pull request as ready for review October 11, 2023 19:25
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner October 11, 2023 19:25
@SyntaxColoring SyntaxColoring requested a review from a team October 11, 2023 19:26
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.

Curious about if performance is affected by the increased amount of engine client calls, but otherwise looks good outside of my couple suggestions below.

api/src/opentrons/protocol_api/core/engine/stringify.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/core/engine/stringify.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Merging #13764 (18bb36f) into chore_release-7.0.1 (e8ba506) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.1   #13764      +/-   ##
=======================================================
- Coverage                71.26%   71.25%   -0.01%     
=======================================================
  Files                     1588     1588              
  Lines                    52761    52744      -17     
  Branches                  3451     3451              
=======================================================
- Hits                     37602    37585      -17     
  Misses                   14624    14624              
  Partials                   535      535              
Flag Coverage Δ
app 43.57% <ø> (ø)
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

see 8 files with indirect coverage changes

@SyntaxColoring
Copy link
Contributor Author

Curious about if performance is affected by the increased amount of engine client calls

Hm, yeah.

It seems like ideally, we would maybe only compute the parts of the run log that are actually required by the caller. For robot-server, that's basically none of it, whereas for opentrons_simulate, it would include this stuff...

@SyntaxColoring
Copy link
Contributor Author

Waiting for #13709 to hit chore_release-7.0.1 before we merge this into chore_release-7.0.1.

Base automatically changed from stab_at_simulation to chore_release-7.0.1 October 11, 2023 21:41
@SyntaxColoring SyntaxColoring force-pushed the rss_368_fix_stringified_well_names branch from 545e52e to 18bb36f Compare October 11, 2023 21:42
@SyntaxColoring SyntaxColoring added api Affects the `api` project papi-v2 Python API V2 protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs labels Oct 11, 2023
@SyntaxColoring SyntaxColoring merged commit 3f52995 into chore_release-7.0.1 Oct 11, 2023
44 checks passed
@SyntaxColoring SyntaxColoring deleted the rss_368_fix_stringified_well_names branch October 11, 2023 21:58
SyntaxColoring added a commit that referenced this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project papi-v2 Python API V2 protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants