From 165a607607226e0a9149d3700f2c4eb590a60fbd Mon Sep 17 00:00:00 2001 From: CaseyBatten Date: Tue, 12 Dec 2023 18:24:48 -0500 Subject: [PATCH] refactor(api, robot-server): removal of automatic drop tip from Flex while maintaining OT2 behavior (#14182) Removes the automatic drop tip behavior from do_stop_and_recover for Flex protocols while retaining OT2 behavior --- .../execution/hardware_stopper.py | 83 ++++++++++++++----- .../protocol_runner/protocol_runner.py | 33 +++++++- .../opentrons/protocol_runner/task_queue.py | 29 +++++-- .../execution/test_hardware_stopper.py | 31 ++----- .../protocol_runner/test_task_queue.py | 21 +++-- .../robot_server/runs/engine_store.py | 9 ++ 6 files changed, 142 insertions(+), 64 deletions(-) diff --git a/api/src/opentrons/protocol_engine/execution/hardware_stopper.py b/api/src/opentrons/protocol_engine/execution/hardware_stopper.py index 11f753b0ee4..2b45431fb57 100644 --- a/api/src/opentrons/protocol_engine/execution/hardware_stopper.py +++ b/api/src/opentrons/protocol_engine/execution/hardware_stopper.py @@ -15,6 +15,8 @@ from .tip_handler import TipHandler, HardwareTipHandler from ...hardware_control.types import OT3Mount +from opentrons.protocol_engine.types import AddressableOffsetVector + log = logging.getLogger(__name__) # TODO(mc, 2022-03-07): this constant dup'd from opentrons.protocols.geometry.deck @@ -68,25 +70,38 @@ async def _drop_tip(self) -> None: axes=[MotorAxis.X, MotorAxis.Y, MotorAxis.LEFT_Z, MotorAxis.RIGHT_Z] ) - for pipette_id, tip in attached_tips: - try: - await self._tip_handler.add_tip(pipette_id=pipette_id, tip=tip) - # TODO: Add ability to drop tip onto custom trash as well. - await self._movement_handler.move_to_well( - pipette_id=pipette_id, - labware_id=FIXED_TRASH_ID, - well_name="A1", - ) - await self._tip_handler.drop_tip( - pipette_id=pipette_id, - home_after=False, + # OT-2 Will only ever use the Fixed Trash Addressable Area + if self._state_store.config.robot_type == "OT-2 Standard": + for pipette_id, tip in attached_tips: + try: + await self._tip_handler.add_tip(pipette_id=pipette_id, tip=tip) + # TODO: Add ability to drop tip onto custom trash as well. + # if API is 2.15 and below aka is should_have_fixed_trash + + await self._movement_handler.move_to_addressable_area( + pipette_id=pipette_id, + addressable_area_name="fixedTrash", + offset=AddressableOffsetVector(x=0, y=0, z=0), + force_direct=False, + speed=None, + minimum_z_height=None, + ) + + await self._tip_handler.drop_tip( + pipette_id=pipette_id, + home_after=False, + ) + + except HwPipetteNotAttachedError: + # this will happen normally during protocol analysis, but + # should not happen during an actual run + log.debug(f"Pipette ID {pipette_id} no longer attached.") + + else: + log.debug( + "Flex protocols do not support automatic tip dropping at this time." ) - except HwPipetteNotAttachedError: - # this will happen normally during protocol analysis, but - # should not happen during an actual run - log.debug(f"Pipette ID {pipette_id} no longer attached.") - async def do_halt(self, disengage_before_stopping: bool = False) -> None: """Issue a halt signal to the hardware API. @@ -102,12 +117,36 @@ async def do_stop_and_recover( post_run_hardware_state: PostRunHardwareState, drop_tips_after_run: bool = False, ) -> None: - """Stop and reset the HardwareAPI, optionally dropping tips and homing.""" - if drop_tips_after_run: - await self._drop_tip() - + """Stop and reset the HardwareAPI, homing and dropping tips independently if specified.""" home_after_stop = post_run_hardware_state in ( PostRunHardwareState.HOME_AND_STAY_ENGAGED, PostRunHardwareState.HOME_THEN_DISENGAGE, ) - await self._hardware_api.stop(home_after=home_after_stop) + if drop_tips_after_run: + await self._drop_tip() + await self._hardware_api.stop(home_after=home_after_stop) + + elif home_after_stop: + if len(self._state_store.pipettes.get_all_attached_tips()) == 0: + await self._hardware_api.stop(home_after=home_after_stop) + else: + try: + ot3api = ensure_ot3_hardware(hardware_api=self._hardware_api) + if ( + not self._state_store.config.use_virtual_gripper + and ot3api.has_gripper() + ): + await ot3api.home_z(mount=OT3Mount.GRIPPER) + except HardwareNotSupportedError: + pass + + await self._movement_handler.home( + axes=[ + MotorAxis.X, + MotorAxis.Y, + MotorAxis.LEFT_Z, + MotorAxis.RIGHT_Z, + ] + ) + else: + await self._hardware_api.stop(home_after=home_after_stop) diff --git a/api/src/opentrons/protocol_runner/protocol_runner.py b/api/src/opentrons/protocol_runner/protocol_runner.py index eb4225a02b9..ec0b576b442 100644 --- a/api/src/opentrons/protocol_runner/protocol_runner.py +++ b/api/src/opentrons/protocol_runner/protocol_runner.py @@ -121,6 +121,8 @@ def __init__( legacy_file_reader: Optional[LegacyFileReader] = None, legacy_context_creator: Optional[LegacyContextCreator] = None, legacy_executor: Optional[LegacyExecutor] = None, + post_run_hardware_state: PostRunHardwareState = PostRunHardwareState.HOME_AND_STAY_ENGAGED, + drop_tips_after_run: bool = True, ) -> None: """Initialize the PythonAndLegacyRunner with its dependencies.""" super().__init__(protocol_engine) @@ -133,7 +135,14 @@ def __init__( self._legacy_executor = legacy_executor or LegacyExecutor() # TODO(mc, 2022-01-11): replace task queue with specific implementations # of runner interface - self._task_queue = task_queue or TaskQueue(cleanup_func=protocol_engine.finish) + self._task_queue = ( + task_queue or TaskQueue() + ) # cleanup_func=protocol_engine.finish)) + self._task_queue.set_cleanup_func( + func=protocol_engine.finish, + drop_tips_after_run=drop_tips_after_run, + post_run_hardware_state=post_run_hardware_state, + ) async def load( self, protocol_source: ProtocolSource, python_parse_mode: PythonParseMode @@ -214,6 +223,8 @@ def __init__( task_queue: Optional[TaskQueue] = None, json_file_reader: Optional[JsonFileReader] = None, json_translator: Optional[JsonTranslator] = None, + post_run_hardware_state: PostRunHardwareState = PostRunHardwareState.HOME_AND_STAY_ENGAGED, + drop_tips_after_run: bool = True, ) -> None: """Initialize the JsonRunner with its dependencies.""" super().__init__(protocol_engine) @@ -223,7 +234,15 @@ def __init__( self._json_translator = json_translator or JsonTranslator() # TODO(mc, 2022-01-11): replace task queue with specific implementations # of runner interface - self._task_queue = task_queue or TaskQueue(cleanup_func=protocol_engine.finish) + self._task_queue = ( + task_queue or TaskQueue() + ) # cleanup_func=protocol_engine.finish)) + self._task_queue.set_cleanup_func( + func=protocol_engine.finish, + drop_tips_after_run=drop_tips_after_run, + post_run_hardware_state=post_run_hardware_state, + ) + self._hardware_api.should_taskify_movement_execution(taskify=False) async def load(self, protocol_source: ProtocolSource) -> None: @@ -312,7 +331,9 @@ def __init__( # TODO(mc, 2022-01-11): replace task queue with specific implementations # of runner interface self._hardware_api = hardware_api - self._task_queue = task_queue or TaskQueue(cleanup_func=protocol_engine.finish) + self._task_queue = task_queue or TaskQueue() + self._task_queue.set_cleanup_func(func=protocol_engine.finish) + self._hardware_api.should_taskify_movement_execution(taskify=False) def prepare(self) -> None: @@ -348,6 +369,8 @@ def create_protocol_runner( legacy_file_reader: Optional[LegacyFileReader] = None, legacy_context_creator: Optional[LegacyContextCreator] = None, legacy_executor: Optional[LegacyExecutor] = None, + post_run_hardware_state: PostRunHardwareState = PostRunHardwareState.HOME_AND_STAY_ENGAGED, + drop_tips_after_run: bool = True, ) -> AnyRunner: """Create a protocol runner.""" if protocol_config: @@ -361,6 +384,8 @@ def create_protocol_runner( json_file_reader=json_file_reader, json_translator=json_translator, task_queue=task_queue, + post_run_hardware_state=post_run_hardware_state, + drop_tips_after_run=drop_tips_after_run, ) else: return PythonAndLegacyRunner( @@ -370,6 +395,8 @@ def create_protocol_runner( legacy_file_reader=legacy_file_reader, legacy_context_creator=legacy_context_creator, legacy_executor=legacy_executor, + post_run_hardware_state=post_run_hardware_state, + drop_tips_after_run=drop_tips_after_run, ) return LiveRunner( diff --git a/api/src/opentrons/protocol_runner/task_queue.py b/api/src/opentrons/protocol_runner/task_queue.py index e79dc097aa1..a24fb0c7c64 100644 --- a/api/src/opentrons/protocol_runner/task_queue.py +++ b/api/src/opentrons/protocol_runner/task_queue.py @@ -5,14 +5,16 @@ from typing import Any, Awaitable, Callable, Optional from typing_extensions import Protocol as Callback - log = logging.getLogger(__name__) class CleanupFunc(Callback): """Expected cleanup function signature.""" - def __call__(self, error: Optional[Exception]) -> Any: + def __call__( + self, + error: Optional[Exception], + ) -> Any: """Cleanup, optionally taking an error thrown. Return value will not be used. @@ -26,19 +28,35 @@ class TaskQueue: Once started, a TaskQueue may not be re-used. """ - def __init__(self, cleanup_func: CleanupFunc) -> None: + def __init__( + self, + # cleanup_func: CleanupFunc, + ) -> None: """Initialize the TaskQueue. Args: cleanup_func: A function to call at run function completion with any error raised by the run function. """ - self._cleanup_func: CleanupFunc = cleanup_func + self._cleanup_func: Optional[ + Callable[[Optional[Exception]], Any] + ] = None # CleanupFunc = cleanup_func self._run_func: Optional[Callable[[], Any]] = None self._run_task: Optional["asyncio.Task[None]"] = None self._ok_to_join_event: asyncio.Event = asyncio.Event() + def set_cleanup_func( + self, + func: Callable[..., Awaitable[Any]], + **kwargs: Any, + ) -> None: + """Add the protocol cleanup task to the queue. + + The "cleanup" task will be run after the "run" task. + """ + self._cleanup_func = partial(func, **kwargs) + def set_run_func( self, func: Callable[..., Awaitable[Any]], @@ -74,4 +92,5 @@ async def _run(self) -> None: log.exception("Exception raised by protocol") error = e - await self._cleanup_func(error=error) + if self._cleanup_func is not None: + await self._cleanup_func(error) diff --git a/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py b/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py index 96e0cc3ea88..891c7e4d3ae 100644 --- a/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py +++ b/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py @@ -91,7 +91,7 @@ async def test_hardware_stopping_sequence( post_run_hardware_state: PostRunHardwareState, expected_home_after: bool, ) -> None: - """It should stop the hardware, home the robot and perform drop tip if required.""" + """It should stop the hardware, and home the robot. Flex no longer performs automatic drop tip..""" decoy.when(state_store.pipettes.get_all_attached_tips()).then_return( [ ("pipette-id", TipGeometry(length=1.0, volume=2.0, diameter=3.0)), @@ -99,7 +99,8 @@ async def test_hardware_stopping_sequence( ) await subject.do_stop_and_recover( - drop_tips_after_run=True, post_run_hardware_state=post_run_hardware_state + drop_tips_after_run=True, + post_run_hardware_state=post_run_hardware_state, ) decoy.verify( @@ -107,16 +108,6 @@ async def test_hardware_stopping_sequence( await movement.home( axes=[MotorAxis.X, MotorAxis.Y, MotorAxis.LEFT_Z, MotorAxis.RIGHT_Z] ), - await mock_tip_handler.add_tip( - pipette_id="pipette-id", - tip=TipGeometry(length=1.0, volume=2.0, diameter=3.0), - ), - await movement.move_to_well( - pipette_id="pipette-id", - labware_id="fixedTrash", - well_name="A1", - ), - await mock_tip_handler.drop_tip(pipette_id="pipette-id", home_after=False), await hardware_api.stop(home_after=expected_home_after), ) @@ -210,7 +201,7 @@ async def test_hardware_stopping_sequence_with_gripper( movement: MovementHandler, mock_tip_handler: TipHandler, ) -> None: - """It should stop the hardware, home the robot and perform drop tip if required.""" + """It should stop the hardware, and home the robot. Flex no longer performs automatic drop tip.""" subject = HardwareStopper( hardware_api=ot3_hardware_api, state_store=state_store, @@ -224,6 +215,7 @@ async def test_hardware_stopping_sequence_with_gripper( ) decoy.when(state_store.config.use_virtual_gripper).then_return(False) decoy.when(ot3_hardware_api.has_gripper()).then_return(True) + await subject.do_stop_and_recover( drop_tips_after_run=True, post_run_hardware_state=PostRunHardwareState.HOME_AND_STAY_ENGAGED, @@ -235,18 +227,5 @@ async def test_hardware_stopping_sequence_with_gripper( await movement.home( axes=[MotorAxis.X, MotorAxis.Y, MotorAxis.LEFT_Z, MotorAxis.RIGHT_Z] ), - await mock_tip_handler.add_tip( - pipette_id="pipette-id", - tip=TipGeometry(length=1.0, volume=2.0, diameter=3.0), - ), - await movement.move_to_well( - pipette_id="pipette-id", - labware_id="fixedTrash", - well_name="A1", - ), - await mock_tip_handler.drop_tip( - pipette_id="pipette-id", - home_after=False, - ), await ot3_hardware_api.stop(home_after=True), ) diff --git a/api/tests/opentrons/protocol_runner/test_task_queue.py b/api/tests/opentrons/protocol_runner/test_task_queue.py index 0359a0eb50b..92cd5f52765 100644 --- a/api/tests/opentrons/protocol_runner/test_task_queue.py +++ b/api/tests/opentrons/protocol_runner/test_task_queue.py @@ -9,14 +9,15 @@ async def test_set_run_func(decoy: Decoy) -> None: run_func = decoy.mock(is_async=True) cleanup_func = decoy.mock(is_async=True) - subject = TaskQueue(cleanup_func=cleanup_func) + subject = TaskQueue() # cleanup_func=cleanup_func) + subject.set_cleanup_func(func=cleanup_func) subject.set_run_func(func=run_func) subject.start() await subject.join() decoy.verify( await run_func(), - await cleanup_func(error=None), + await cleanup_func(None), ) @@ -25,7 +26,8 @@ async def test_passes_args(decoy: Decoy) -> None: run_func = decoy.mock(is_async=True) cleanup_func = decoy.mock(is_async=True) - subject = TaskQueue(cleanup_func=cleanup_func) + subject = TaskQueue() # cleanup_func=cleanup_func) + subject.set_cleanup_func(func=cleanup_func) subject.set_run_func(func=run_func, hello="world") subject.start() await subject.join() @@ -41,18 +43,20 @@ async def test_cleanup_gets_run_error(decoy: Decoy) -> None: decoy.when(await run_func()).then_raise(error) - subject = TaskQueue(cleanup_func=cleanup_func) + subject = TaskQueue() # cleanup_func=cleanup_func) + subject.set_cleanup_func(func=cleanup_func) subject.set_run_func(func=run_func) subject.start() await subject.join() - decoy.verify(await cleanup_func(error=error)) + decoy.verify(await cleanup_func(error)) async def test_join_waits_for_start(decoy: Decoy) -> None: """It should wait until the queue is started when join is called.""" cleanup_func = decoy.mock(is_async=True) - subject = TaskQueue(cleanup_func=cleanup_func) + subject = TaskQueue() # cleanup_func=cleanup_func) + subject.set_cleanup_func(func=cleanup_func) join_task = asyncio.create_task(subject.join()) await asyncio.sleep(0) @@ -67,11 +71,12 @@ async def test_start_runs_stuff_once(decoy: Decoy) -> None: run_func = decoy.mock(is_async=True) cleanup_func = decoy.mock(is_async=True) - subject = TaskQueue(cleanup_func=cleanup_func) + subject = TaskQueue() # leanup_func=cleanup_func) + subject.set_cleanup_func(func=cleanup_func) subject.set_run_func(func=run_func) subject.start() subject.start() await subject.join() decoy.verify(await run_func(), times=1) - decoy.verify(await cleanup_func(error=None), times=1) + decoy.verify(await cleanup_func(None), times=1) diff --git a/robot-server/robot_server/runs/engine_store.py b/robot-server/robot_server/runs/engine_store.py index 056f8d55259..b3adf76306a 100644 --- a/robot-server/robot_server/runs/engine_store.py +++ b/robot-server/robot_server/runs/engine_store.py @@ -185,10 +185,19 @@ async def create( load_fixed_trash=load_fixed_trash, deck_configuration=deck_configuration, ) + + post_run_hardware_state = PostRunHardwareState.HOME_AND_STAY_ENGAGED + drop_tips_after_run = True + if self._robot_type == "OT-3 Standard": + post_run_hardware_state = PostRunHardwareState.HOME_AND_STAY_ENGAGED + drop_tips_after_run = False + runner = create_protocol_runner( protocol_engine=engine, hardware_api=self._hardware_api, protocol_config=protocol.source.config if protocol else None, + post_run_hardware_state=post_run_hardware_state, + drop_tips_after_run=drop_tips_after_run, ) if self._runner_engine_pair is not None: