Skip to content

Commit

Permalink
refactor(protocol-engine): Rename stop() and pause() -> request_stop(…
Browse files Browse the repository at this point in the history
…) and request_pause() (#14879)

# Overview

This fixes something that keeps confusing me as I work on EXEC-382.

Various things state that `ProtocolEngine.stop()` takes effect
immediately—meaning, to me, that the robot's motion is stopped
immediately, the protocol exits immediately, and the HTTP run is marked
as `stopped` immediately. This does not seem true. It merely puts the
run into a `stop-requested` state, which only later settles into a
`stopped` state.

This PR adjusts some docstrings and renames `stop()` to
~~`stop_soon()`~~ `request_stop()`. ~~The name `stop_soon()` is inspired
by asyncio and anyio's `call_soon()`.~~ `pause()` has the same caveat,
so it's renamed to `request_pause()` for consistency.

# Test plan

None needed.

# Review requests

* ~~Taking for granted, for a moment, that the `ProtocolEngine`
interface has to work like this: is `stop_soon()` a good name? Maybe
`request_stop()` would be better?~~ Done.
* ~~`pause()` has the same caveat. Do we want to rename that too, for
consistency?~~ Done.


# Risk assessment

No risk.
  • Loading branch information
SyntaxColoring authored and Carlos-fernandez committed May 20, 2024
1 parent 37bf7f4 commit 8fb95ca
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 17 deletions.
5 changes: 1 addition & 4 deletions api/src/opentrons/protocol_engine/actions/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,7 @@ class PauseAction:

@dataclass(frozen=True)
class StopAction:
"""Stop the current engine execution.
After a StopAction, the engine status will be marked as stopped.
"""
"""Request engine execution to stop soon."""

from_estop: bool = False

Expand Down
21 changes: 15 additions & 6 deletions api/src/opentrons/protocol_engine/protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,12 @@ def play(self, deck_configuration: Optional[DeckConfigurationType] = None) -> No
else:
self._hardware_api.resume(HardwarePauseType.PAUSE)

def pause(self) -> None:
"""Pause executing commands in the queue."""
def request_pause(self) -> None:
"""Make command execution pause soon.
This will try to pause in the middle of the ongoing command, if there is one.
Otherwise, whenever the next command begins, the pause will happen then.
"""
action = self._state_store.commands.validate_action_allowed(
PauseAction(source=PauseSource.CLIENT)
)
Expand Down Expand Up @@ -371,12 +375,17 @@ def estop(
else:
_log.info("estop pressed before protocol was started, taking no action.")

async def stop(self) -> None:
"""Stop execution immediately, halting all motion and cancelling future commands.
async def request_stop(self) -> None:
"""Make command execution stop soon.
This will try to interrupt the ongoing command, if there is one. Future commands
are canceled. However, by the time this method returns, things may not have
settled by the time this method returns; the last command may still be
running.
After an engine has been `stop`'ed, it cannot be restarted.
After a stop has been requested, the engine cannot be restarted.
After a `stop`, you must still call `finish` to give the engine a chance
After a stop request, you must still call `finish` to give the engine a chance
to clean up resources and propagate errors.
"""
action = self._state_store.commands.validate_action_allowed(StopAction())
Expand Down
4 changes: 2 additions & 2 deletions api/src/opentrons/protocol_runner/protocol_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,12 @@ def play(self, deck_configuration: Optional[DeckConfigurationType] = None) -> No

def pause(self) -> None:
"""Pause the run."""
self._protocol_engine.pause()
self._protocol_engine.request_pause()

async def stop(self) -> None:
"""Stop (cancel) the run."""
if self.was_started():
await self._protocol_engine.stop()
await self._protocol_engine.request_stop()
else:
await self._protocol_engine.finish(
drop_tips_after_run=False,
Expand Down
6 changes: 3 additions & 3 deletions api/tests/opentrons/protocol_engine/test_protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ def test_pause(
state_store.commands.validate_action_allowed(expected_action),
).then_return(expected_action)

subject.pause()
subject.request_pause()

decoy.verify(
action_dispatcher.dispatch(expected_action),
Expand Down Expand Up @@ -810,7 +810,7 @@ async def test_stop(
state_store.commands.validate_action_allowed(expected_action),
).then_return(expected_action)

await subject.stop()
await subject.request_stop()

decoy.verify(
action_dispatcher.dispatch(expected_action),
Expand All @@ -836,7 +836,7 @@ async def test_stop_for_legacy_core_protocols(

decoy.when(hardware_api.is_movement_execution_taskified()).then_return(True)

await subject.stop()
await subject.request_stop()

decoy.verify(
action_dispatcher.dispatch(expected_action),
Expand Down
4 changes: 2 additions & 2 deletions api/tests/opentrons/protocol_runner/test_protocol_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ def test_pause(
"""It should pause a protocol run with pause."""
subject.pause()

decoy.verify(protocol_engine.pause(), times=1)
decoy.verify(protocol_engine.request_pause(), times=1)


@pytest.mark.parametrize(
Expand All @@ -261,7 +261,7 @@ async def test_stop(
subject.play()
await subject.stop()

decoy.verify(await protocol_engine.stop(), times=1)
decoy.verify(await protocol_engine.request_stop(), times=1)


@pytest.mark.parametrize(
Expand Down

0 comments on commit 8fb95ca

Please sign in to comment.