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(shared-data,api,robot-server): Use new OT-3 deck slot naming style #12571

Merged
merged 74 commits into from
Jun 2, 2023

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Apr 27, 2023

Overview

This is a chunk of back-end support for OT-3 style deck slot names, like D1 instead of 1.

Closes RLAB-250, RLAB-251, RLAB-253, RLAB-256, RLAB-257, RLAB-339, and RLAB-346.

Changelog

In general, the robot will now accept deck slots of either style (1 or D1) as input. It will normalize them to the device's preferred style (1 for OT-2s, D1 for OT-3s) in its internal state, and in its output. In protocol analyses, the style in the output will follow the protocol's declared robotType.

This affects:

  • loadLabware commands
  • moveLabware commands
  • loadModule commands
  • Labware offsets POSTed over HTTP
  • Run summaries visible over HTTP

The underlying OT-3 deck definition in shared-data is changed to use the OT-3-style names. The module definitions are updated to match.

Review requests

  • Scrutinize my updates in shared-data.
    • Make sure I'm updating things to the correct new values. Here's a handy quick reference table, but double-check this with a physical OT-3.
    • Make sure I didn't accidentally leave any of the OT-2-style numbers behind. It's very important that the module definitions and deck definitions are consistent with each other.
  • Can you think of any other places that might need to normalize deck slots before comparing them? For example, any conflict-checking code hiding out somewhere?

Risk assessment

High.

This unfortunately has to change a bunch of stuff all at once, because it's all cross-dependent on each other.

Code that does this is in danger of breaking silently:

if some_slot == some_other_slot:
    ...

We do this in places such as:

  • Determining the offset of a module. Modules have per-slot transformations, which are matched by slot ID.
  • Determining the Labware Position Check offset to apply to a labware. Labware Position Check offsets are defined by a labware URI and a location, and the location includes the slot ID.

Test plan

Here's the protocol file that I'm using.

After pushing this branch to an OT-3, you'll need to factory-reset runs history before the robot is usable. You'll also need to factory-reset runs history when pushing edge back.

  • OT-3-only: Make sure you can still detach and attach pipettes.
  • OT-3-only: Make sure you can still calibrate pipettes.
  • OT-2 and OT-3: Make sure the Labware Position Check flow continues to behave correctly.
  • OT-2 and OT-3: Make sure that offsets saved in Labware Position Check correctly apply to the actual protocol run.
  • OT-2 and OT-3: Make sure you can apply labware offsets from prior runs.
  • OT-2 and OT-3: Make sure that in a protocol run, the robot physically continues to go to the right place, especially when the labware is loaded atop a module.
  • OT-2 and OT-3: Make sure the robot continues to dodge Thermocyclers, in protocols that have them.
  • OT-2 and OT-3: In general, make sure the Opentrons App shows slot names like "1" everywhere it's dealing with an OT-2, and "D1" everywhere it's dealing with an OT-3.
    • RAUT-473 is one missed spot, but otherwise this seems good.
  • OT-2 and OT-3: In general, make sure the Opentrons App keeps displaying the deck map correctly.

@SyntaxColoring SyntaxColoring changed the base branch from edge to protocol_engine_threadmanager April 27, 2023 14:00
@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #12571 (0c52b1e) into edge (e65750c) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12571      +/-   ##
==========================================
- Coverage   73.35%   73.33%   -0.03%     
==========================================
  Files        2284     2307      +23     
  Lines       62736    63272     +536     
  Branches     6829     6949     +120     
==========================================
+ Hits        46022    46400     +378     
- Misses      15088    15239     +151     
- Partials     1626     1633       +7     
Flag Coverage Δ
app 71.46% <ø> (+0.01%) ⬆️
g-code-testing 96.44% <ø> (ø)
hardware-testing ∅ <ø> (∅)
notify-server 89.13% <ø> (ø)
protocol-designer 46.35% <ø> (ø)
shared-data 75.47% <ø> (-0.89%) ⬇️
step-generation 88.64% <ø> (ø)

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

Impacted Files Coverage Δ
...i/src/opentrons/protocol_engine/protocol_engine.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/geometry.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/modules.py 91.48% <ø> (-0.05%) ⬇️
api/src/opentrons/protocol_engine/state/state.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/types.py 97.72% <ø> (ø)
api/src/opentrons/types.py 86.17% <ø> (ø)
...data/python/opentrons_shared_data/deck/__init__.py 62.06% <ø> (+2.06%) ⬆️
api/src/opentrons/protocol_engine/state/labware.py 100.00% <100.00%> (ø)

... and 29 files with indirect coverage changes

@SyntaxColoring SyntaxColoring force-pushed the protocol_engine_threadmanager branch from 15c4918 to 8da6f39 Compare April 28, 2023 17:43
@SyntaxColoring SyntaxColoring changed the base branch from protocol_engine_threadmanager to edge May 4, 2023 13:55
@SyntaxColoring SyntaxColoring force-pushed the flex_deck_slot_names branch from 5013888 to 57722e0 Compare May 4, 2023 16:15
@SyntaxColoring SyntaxColoring added api Affects the `api` project robot server Affects the `robot-server` project protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs shared data Affects the `shared-data` project labels May 6, 2023
@SyntaxColoring SyntaxColoring requested a review from a team May 6, 2023 00:24
@SyntaxColoring SyntaxColoring removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label May 31, 2023
@SyntaxColoring SyntaxColoring marked this pull request as draft May 31, 2023 16:59
This is okay because it's outside of Protocol Engine, so Protocol Engine will normalize it when it ingests the loadModule command.
Per discussion with Tamar, this is going to change in the near future to be based on coordinates, not deck slots. It's also OT-3-only. So, do the simple thing for now, and don't worry about refactoring LabwareView to handle both kinds of deck.
We're
I think this is okay to leave alone because this should only be used by PAPIv<=2.13, which should never emit the new OT-3-style deck slot IDs.
@SyntaxColoring SyntaxColoring marked this pull request as ready for review June 1, 2023 19:05
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner June 1, 2023 19:05
@SyntaxColoring SyntaxColoring requested review from smb2268 and removed request for a team June 1, 2023 19:05
Resolve conflicts in api/release-notes-internal.md.
This test uses robotType: 'OT-3 Standard', so it should expect OT-3-style slots names.
]
}

_OT3_THERMOCYCLER_SLOT_TRANSITS_TO_DODGE = {
Copy link
Contributor

Choose a reason for hiding this comment

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

not for this PR but these changes are starting to make me think of moving all specific implementation details to a designated class. for example this will be a field in a class instead of a variable. same for _RIGHT_SIDE_SLOTS.

Copy link
Contributor

Choose a reason for hiding this comment

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

again, this is me thinking from a DB perspective. This will probably live in a db table related to each robot

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.

still looks great! Max tested this a lot!

@SyntaxColoring
Copy link
Contributor Author

There's one failing app front-end test, but I think that's an unrelated thing that's fixed in #12842.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs robot server Affects the `robot-server` project shared data Affects the `shared-data` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants