fix(api): discount labware in system location/hopper location when getting the deck height #18188

ahiuchingau · 2025-04-25T20:47:37Z

This PR adds a new geometry method to determine if labware is a deck obstacle and updates the logic for calculating the highest Z-point on the deck.

While refactoring the tests, I discovered that the we're also using the wrong z for modules. We should only consider the module height within the robot's deck, and not the overall height of the module.

codecov · 2025-04-25T21:12:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.48%. Comparing base (5c861e5) to head (e4b829b).
Report is 8 commits behind head on edge.

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #18188      +/-   ##
==========================================
- Coverage   57.48%   57.48%   -0.01%     
==========================================
  Files        3043     3043              
  Lines      255276   255275       -1     
  Branches    30517    30516       -1     
==========================================
- Hits       146756   146755       -1     
  Misses     108333   108333              
  Partials      187      187

Flag	Coverage Δ
app	`47.72% <ø> (-0.01%)`	⬇️
protocol-designer	`18.93% <ø> (ø)`
step-generation	`4.37% <ø> (-0.01%)`	⬇️

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

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sfoster1

I think we can make the check catch a little more

sfoster1 · 2025-04-28T13:19:27Z

api/src/opentrons/protocol_engine/state/geometry.py

+        ) or lw_data.location in [OFF_DECK_LOCATION, SYSTEM_LOCATION]:
+            return False
+        # is it a lid?
+        if self._labware.get_labware_by_lid_id(lw_data.id) is not None:


wait why wouldn't it be a deck obstacle if it's a lid?

sfoster1 · 2025-04-28T13:21:07Z

api/src/opentrons/protocol_engine/state/geometry.py

+        """Check if the labware is a deck obstacle."""
+        # is it offdeck?
+        if isinstance(
+            lw_data.location, InStackerHopperLocation


you probably want to use self.get_location_sequence(lw_data.id)[-1] rather than lw_data.location; the check here won't find a labware on an adapter that's off-deck, for instance

SyntaxColoring

Agreed with Seth's comments, plus one more.

api/src/opentrons/protocol_engine/state/geometry.py

SyntaxColoring

Changes requested for avoiding mocking out other parts of subject. (I see this is targeted into edge, so we aren't rushing to get this into chore_release_8.4.0, right?)

api/src/opentrons/protocol_engine/state/geometry.py

api/tests/opentrons/protocol_engine/state/test_geometry_view.py

sfoster1

agree with max but i'm happy with the changes you made to my suggestions

vegano1 · 2025-04-29T18:49:50Z

api/src/opentrons/protocol_engine/state/geometry.py

+                return False
+        # TODO: (AA 4/28/25) re-evaluate this logic, this doesn't make sense
+        # to me but keeping it to preserve existing behavior
+        if self._labware.get_labware_by_lid_id(labware_id) is not None:


I agree with this, lids should be considered in height checks.

OK i got rid of it!

SyntaxColoring

Thank you! Approving so we can fix the bug and move on.

The testing practices continue to concern me. I think almost always, we should avoid writing tests like this, that mess with private details of the subject. At a glance, nothing that we're doing here strikes me as warranting an exception to that rule.

If any of this is controversial, we can keep talking about it in Slack.

SyntaxColoring · 2025-05-01T15:42:09Z

api/src/opentrons/protocol_engine/state/geometry.py

-        # Fixme (spp, 2023-12-04): the overall height is not the true highest z of modules
-        #  on a Flex.


I don't understand what this old fixme comment was talking about, do you?

The overall height is the physical height of the module. This does not account for how the module sits on the deck, so its overall height is usually much taller than the highest point of the module when it's loaded into the robot.

Ohh—are you talking about how, like, how a module installed on a Flex is mostly sunken below deck? So we were over-conservatively saying that, say, a Temperature Module was 8 cm tall (or however tall it is), when in fact most of those cm shouldn't have counted because they were below deck?

api/tests/opentrons/protocol_engine/state/test_geometry_view.py

sfoster1

Looks pretty good, I agree with max that the tests or the code they're testing should change to allow for better adherence to well-defined APIs. Some of the tests can be changed to provide fixed numbers as inputs and then check that the eventual output matches the fixed numbers (like, I don't think we need those asserts about the labware definition dimensions - that's not what we're testing, after all).

Would love to see some of that tuned up before merging but if you have to do it now then ok

api/tests/opentrons/protocol_engine/state/test_geometry_view.py

sfoster1

Thank you! Looks great!

SyntaxColoring

Looks great, thank you!

SyntaxColoring · 2025-05-01T20:50:07Z

api/src/opentrons/protocol_engine/state/geometry.py

-        # Fixme (spp, 2023-12-04): the overall height is not the true highest z of modules
-        #  on a Flex.


Ohh—are you talking about how, like, how a module installed on a Flex is mostly sunken below deck? So we were over-conservatively saying that, say, a Temperature Module was 8 cm tall (or however tall it is), when in fact most of those cm shouldn't have counted because they were below deck?

This avoids a problem where the protocol_engine_lid_stack object can be in SYSTEM_LOCATION and we were asking for its ancestor slot. Also avoids a problem where asking for the ancestor slot of a SYSTEM_LOCATION would give you a bad kind of exception instead of a nice kind of exception. Closes RQA-4164 ## Reviews and TODOs This issue would also be addressed by #18188 but that includes stacker stuff that isn't in the release branch, so it does just a much lighter version of that. ## testing Check that the protocol in the ticket passes analysis

ahiuchingau requested a review from a team as a code owner April 25, 2025 20:47

ahiuchingau force-pushed the fix_high-deck-height branch from 92b7141 to 4b5a57c Compare April 25, 2025 20:47

ahiuchingau changed the title ~~fix(api): discount labware in system location/hopper location from getting the deck height~~ fix(api): discount labware in system location/hopper location when getting the deck height Apr 25, 2025

sfoster1 requested changes Apr 28, 2025

View reviewed changes

SyntaxColoring reviewed Apr 28, 2025

View reviewed changes

api/src/opentrons/protocol_engine/state/geometry.py Outdated Show resolved Hide resolved

ahiuchingau force-pushed the fix_high-deck-height branch 2 times, most recently from 5776f18 to de7721d Compare April 29, 2025 15:33

ahiuchingau requested a review from sfoster1 April 29, 2025 15:33

SyntaxColoring requested changes Apr 29, 2025

View reviewed changes

api/src/opentrons/protocol_engine/state/geometry.py Outdated Show resolved Hide resolved

api/src/opentrons/protocol_engine/state/geometry.py Outdated Show resolved Hide resolved

api/tests/opentrons/protocol_engine/state/test_geometry_view.py Outdated Show resolved Hide resolved

sfoster1 approved these changes Apr 29, 2025

View reviewed changes

vegano1 reviewed Apr 29, 2025

View reviewed changes

vegano1 approved these changes Apr 29, 2025

View reviewed changes

ahiuchingau requested review from SyntaxColoring, sfoster1 and vegano1 May 1, 2025 14:30

SyntaxColoring approved these changes May 1, 2025

View reviewed changes

sfoster1 approved these changes May 1, 2025

View reviewed changes

api/tests/opentrons/protocol_engine/state/test_geometry_view.py Show resolved Hide resolved

ahiuchingau added 7 commits May 1, 2025 15:08

discount system location/hopper location from getting the deck highest z

3c4b5f4

add is_deck_obstacle check

76d8e8a

wow

85ecbaf

update

af62470

make load_x fixtures

ed4f1c4

integration tests

34592ca

better tests

e4b829b

ahiuchingau force-pushed the fix_high-deck-height branch from b925e19 to e4b829b Compare May 1, 2025 19:24

sfoster1 approved these changes May 1, 2025

View reviewed changes

ahiuchingau merged commit 8f9bb0d into edge May 1, 2025
55 checks passed

ahiuchingau deleted the fix_high-deck-height branch May 1, 2025 20:49

SyntaxColoring approved these changes May 1, 2025

View reviewed changes

sfoster1 mentioned this pull request May 2, 2025

fix(api): dont check SYSTEM lid stacks z #18241

Merged

		# Fixme (spp, 2023-12-04): the overall height is not the true highest z of modules
		# on a Flex.

fix(api): discount labware in system location/hopper location when getting the deck height #18188

fix(api): discount labware in system location/hopper location when getting the deck height #18188

Uh oh!

Conversation

ahiuchingau commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sfoster1 left a comment

Choose a reason for hiding this comment

Uh oh!

sfoster1 Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

sfoster1 Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SyntaxColoring left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfoster1 left a comment

Choose a reason for hiding this comment

Uh oh!

vegano1 Apr 29, 2025

Choose a reason for hiding this comment

Uh oh!

ahiuchingau May 1, 2025

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring left a comment

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring May 1, 2025

Choose a reason for hiding this comment

Uh oh!

ahiuchingau May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring May 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfoster1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sfoster1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SyntaxColoring left a comment

Choose a reason for hiding this comment

Uh oh!

SyntaxColoring May 1, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ahiuchingau commented Apr 25, 2025 •

edited

Loading

codecov bot commented Apr 25, 2025 •

edited

Loading

SyntaxColoring left a comment •

edited

Loading

ahiuchingau May 1, 2025 •

edited

Loading