feat(api): implement pipetting restrictions around heater-shaker #11218

sanni-t · 2022-07-25T21:24:56Z

Overview

Closes #9915

Adds pipetting restrictions to PAPIv2 as specified in the ticket.

Changelog

Added flag_unsafe_move method to HeaterShakerContext and HeaterShakerGeometry
Moved all adjacent slot getter functions out of ../protocols/geometry/deck_conflict and into opentrons/utils since it's now being used by both PAPIv2 & PE. Tests for those functions upcoming.

Review requests

Question:
- The method that checks for unsafe moves in the api is almost the same as that implemented in PE. Do we want to create a common, pure data 'restrictions checker' that can be used in both PAPI & PE to avoid having two sources of truth?
The use of Location vs LabwareLike vs Labware in the API can be confusing. Please make sure I have't mis-converted any types.

Risk assessment

Medium. Affects pipetting ability.

…stub for h/s

codecov · 2022-07-25T21:35:55Z

Codecov Report

Merging #11218 (93b2128) into edge (9774e64) will not change coverage.
The diff coverage is 90.00%.

@@           Coverage Diff           @@
##             edge   #11218   +/-   ##
=======================================
  Coverage   73.81%   73.81%           
=======================================
  Files        2086     2086           
  Lines       57677    57677           
  Branches     5845     5845           
=======================================
  Hits        42574    42574           
  Misses      13826    13826           
  Partials     1277     1277

Flag	Coverage Δ
notify-server	`89.17% <ø> (ø)`

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.36% <ø> (ø)`
...rc/opentrons/protocols/geometry/module_geometry.py	`83.87% <87.50%> (ø)`
...i/src/opentrons/protocol_api/instrument_context.py	`88.66% <100.00%> (ø)`

mcous

Code looks mostly good, just a couple organizational thoughts. I'm good to 🚢 as longs as we've tested this on hardware

mcous · 2022-07-26T20:34:47Z

api/src/opentrons/util/adjacent_slots_getters.py

@@ -0,0 +1,47 @@
+"""Getters for different types of adjacent slots."""


I usually treat anything named util as a yellow flag. In my experience, you usually end up with a weird grab-bag of unrelated stuff that becomes difficult to find, test, and/or mock.

For example, looking at what's in opentrons.util right now, we have a four different things (plug an entirely empty file) that have nothing to do with each other:

Labware definition fetching plumbing

Datetime functions

Linear algebra functions

Logging config

Could we put these functions in opentrons.motion_planning instead? I think that's a way more descriptive home

mcous · 2022-07-26T20:39:31Z

api/src/opentrons/util/adjacent_slots_getters.py

+        return location - 1
+
+
+def get_east_west_locations(location: int) -> List[int]:


While we're moving things around, location is a bit of an overloaded term for us. Should we shift this interface/language to something like get_east_west_slots(slot: int) -> List[int]?

mcous · 2022-07-26T20:39:44Z

api/tests/opentrons/protocol_api/test_context.py

@@ -156,6 +161,7 @@ def fake_plan_move(
    # Once we have a location cache, that should be our from_loc
    right.move_to(lw.wells()[1].top())
    assert test_args[0].labware.as_well() == lw.wells()[0]  # type: ignore[index]
+    print(f"#### Well Location= {lw.wells()[1].top()} ####")


Remove debugging code

mcous · 2022-07-26T20:40:50Z

api/tests/opentrons/protocol_api/test_context.py

+    # TODO (spp. 2022-07-20): use the ctx fixture once h/s ff is removed
+    ctx = papi.ProtocolContext(
+        implementation=ProtocolContextImplementation(sync_hardware=hardware.sync),
+    )


Is this necessary? IIRC the flag doesn't have to be on when the context is created, just when the load_module is issued

sanni-t · 2022-07-27T14:57:17Z

Tested this python protocol on a dev app and it raised errors correctly during analysis.

metadata = {
    'protocolName': 'PAPIv2 Pipetting restrictions checking protocol',
    'author': 'Opentrons <[email protected]>',
    'apiLevel': '2.12'
}

def run(context):
	# Load Heater-shaker module
	hs_mod = context.load_module("heaterShakerModuleV1", 4)

	# Load adapter + labware on module
	hs_plate = hs_mod.load_labware("opentrons_96_flat_bottom_adapter_nest_wellplate_200ul_flat")

	# p300rack = [context.load_labware('opentrons_96_tiprack_300ul', 9)]
	p300rack = [context.load_labware('opentrons_96_tiprack_300ul', 1)]  # Multi-channel can access tipracks on NS of H/S

	p300_multi = context.load_instrument('p300_multi', 'right', tip_racks=p300rack)

	p10rack = [context.load_labware('opentrons_96_tiprack_10ul', 2)]
	p20 = context.load_instrument('p10_single', 'left', tip_racks= p10rack)

	# slot_1_lw = context.load_labware('agilent_1_reservoir_290ml', 1)
	slot_5_lw = context.load_labware('nest_96_wellplate_200ul_flat', 5)
	slot_7_lw = context.load_labware('nest_12_reservoir_15ml', 7)
	slot_8_lw = context.load_labware('biorad_96_wellplate_200ul_pcr', 8)
	slot_9_lw = context.load_labware('agilent_1_reservoir_290ml', 9)

	p20.pick_up_tip()
	p20.aspirate(10, slot_9_lw['A1'])
	
	p300_multi.pick_up_tip()
	p300_multi.aspirate(30, slot_9_lw['A1'])

	hs_mod.close_labware_latch()
	hs_mod.set_and_wait_for_shake_speed(rpm=500)	# Waits until H/S has reached 500rpm speed

	context.delay(minutes=1)
	
	########## Move to EWNS + h/s while shaking ##########

	# p20.dispense(10, hs_plate['A1'])	# Should raise error
	# p20.dispense(10, slot_1_lw['A1'])		# Should raise error
	# p20.dispense(10, slot_5_lw['A1'])		# Should raise error
	# p20.dispense(10, slot_7_lw['A1'])		# Should raise error
	p20.dispense(10, slot_8_lw['A1'])		# Should NOT raise error

	hs_mod.deactivate_shaker()

	########## Move to EW or h/s while latch open ##########

	hs_mod.open_labware_latch()

	# p20.dispense(10, hs_plate['A1'])	# Should raise error
	# p20.dispense(10, slot_5_lw['A1'])		# Should raise error
	p20.dispense(10, slot_8_lw['A1'])		# Should NOT raise error

	########## No multi-channel access to any labware on EW of H/S ##########
	
	hs_mod.close_labware_latch()

	p300_multi.dispense(30, hs_plate['A1'])		# Should NOT raise error
	# p300_multi.dispense(30, slot_5_lw['A1'])	# Should raise error
	p300_multi.dispense(30, slot_9_lw['A1'])	# Should NOT raise error

	########## Multichannels can access only tipracks on NS of H/S ##########
	
	# p300_multi.dispense(30, slot_7_lw['A1'])	# Should raise error


	p20.drop_tip()

	hs_mod.deactivate_shaker()
	hs_mod.deactivate_heater()

SyntaxColoring

Nice! Left some review suggestions.

Also:

The method that checks for unsafe moves in the api is almost the same as that implemented in PE. Do we want to create a common, pure data 'restrictions checker' that can be used in both PAPI & PE to avoid having two sources of truth?

I would definitely prefer that, but that doesn't have to happen here if you've already done things the other way.

SyntaxColoring · 2022-07-27T14:57:06Z

api/src/opentrons/protocol_api/module_contexts.py

+        """
+        Raise an error if attempting to perform a move that's deemed unsafe due to
+        the presence of the heater-shaker.
+        """


This is not intended to be part of the public Python Protocol API, right?

Let's hide it from docs.opentrons.com:

Suggested change

"""

Raise an error if attempting to perform a move that's deemed unsafe due to

the presence of the heater-shaker.

"""

"""

Raise an error if attempting to perform a move that's deemed unsafe due to

the presence of the heater-shaker.

For Opentrons internal use only.

:meta private:

"""

SyntaxColoring · 2022-07-27T15:01:07Z

api/src/opentrons/protocol_api/module_contexts.py

+                "Cannot determine pipette movement safety."
+            )
+
+        cast(HeaterShakerGeometry, self.geometry).flag_unsafe_move(


I think we can easily avoid this cast by changing this in the superclass:

opentrons/api/src/opentrons/protocol_api/module_contexts.py

Line 212 in f3ac4d9

def geometry(self) -> ModuleGeometry:

To this:

def geometry(self) -> GeometryType:

(GeometryType is a TypeVar defined in that file.)

Added a TODO for this

api/src/opentrons/protocol_api/module_contexts.py

SyntaxColoring · 2022-07-27T15:18:47Z

api/tests/opentrons/protocol_api/test_module_context.py

+@pytest.fixture
+def mock_heater_shaker_geometry() -> mock.MagicMock:
+    return mock.MagicMock()
+
+


Is this used for anything? Cmd+F isn't finding anything.

Woops. Leftover from deleted code

Co-authored-by: Max Marrone <[email protected]>

sanni-t added 5 commits July 21, 2022 16:57

added latch and speed caching in geometry, creating flag_unsafe_move …

97d51cc

…stub for h/s

added restrictions

d928385

remove unused geometry properties

59bacb7

linting

d916fed

fix all mypy errors

384d2a6

sanni-t requested a review from a team as a code owner July 25, 2022 21:24

sanni-t requested review from jbleon95 and mcous July 25, 2022 21:25

sanni-t added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Jul 25, 2022

Merge branch 'edge' into papiv2_hs_pipetting_restrictions

5dde26f

mcous approved these changes Jul 26, 2022

View reviewed changes

sanni-t added 7 commits July 26, 2022 17:52

move adjacent_slot_getter to motion_planning

7399596

rename 'location's to 'slot's

f753e22

address comment about ff in test_context

1ca4219

update signature too

0660668

no debug comments

18ff9e0

linter

1fe6c39

added tests for adjacent_slots_getters

b56aa04

SyntaxColoring reviewed Jul 27, 2022

View reviewed changes

sanni-t and others added 3 commits July 27, 2022 12:44

Apply suggestions from code review

8c08d07

Co-authored-by: Max Marrone <[email protected]>

linter fixes

0c85927

more review suggestions

93b2128

sanni-t merged commit d9caa3d into edge Jul 27, 2022

sanni-t deleted the papiv2_hs_pipetting_restrictions branch July 27, 2022 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): implement pipetting restrictions around heater-shaker #11218

feat(api): implement pipetting restrictions around heater-shaker #11218

sanni-t commented Jul 25, 2022

codecov bot commented Jul 25, 2022 •

edited

Loading

mcous left a comment

mcous Jul 26, 2022

mcous Jul 26, 2022

mcous Jul 26, 2022

sanni-t Jul 26, 2022

mcous Jul 26, 2022

sanni-t Jul 26, 2022

sanni-t commented Jul 27, 2022

SyntaxColoring left a comment

SyntaxColoring Jul 27, 2022

SyntaxColoring Jul 27, 2022

sanni-t Jul 27, 2022

SyntaxColoring Jul 27, 2022

sanni-t Jul 27, 2022

		@@ -0,0 +1,47 @@
		"""Getters for different types of adjacent slots."""

		return location - 1


		def get_east_west_locations(location: int) -> List[int]:

feat(api): implement pipetting restrictions around heater-shaker #11218

feat(api): implement pipetting restrictions around heater-shaker #11218

Conversation

sanni-t commented Jul 25, 2022

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Jul 25, 2022 • edited Loading

Codecov Report

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t commented Jul 27, 2022

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 25, 2022 •

edited

Loading