-
Notifications
You must be signed in to change notification settings - Fork 179
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(api): Allow treating errors as false-positives (ignore them and continue with the run) #16556
Changes from all commits
a99b012
3cf5acf
783d6af
4c4191c
36d54c3
4c3334f
a2b7628
b59f51c
da72776
6e46203
d872bc2
6803390
4ccc92b
e37a589
e64eded
d9d237c
7fc9d08
aafc99d
a3c0c35
db850fa
d3c28c9
e88c8c6
28650c5
40bbcc5
2a4d086
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,35 @@ | ||
# noqa: D100 | ||
|
||
|
||
from .actions import Action, SucceedCommandAction, FailCommandAction | ||
from .actions import ( | ||
Action, | ||
ResumeFromRecoveryAction, | ||
SucceedCommandAction, | ||
FailCommandAction, | ||
) | ||
from ..commands.command import DefinedErrorData | ||
from ..error_recovery_policy import ErrorRecoveryType | ||
from ..state.update_types import StateUpdate | ||
|
||
|
||
def get_state_update(action: Action) -> StateUpdate | None: | ||
"""Extract the StateUpdate from an action, if there is one.""" | ||
def get_state_updates(action: Action) -> list[StateUpdate]: | ||
"""Extract all the StateUpdates that the StateStores should apply when they apply an action.""" | ||
if isinstance(action, SucceedCommandAction): | ||
return action.state_update | ||
return [action.state_update] | ||
|
||
elif isinstance(action, FailCommandAction) and isinstance( | ||
action.error, DefinedErrorData | ||
): | ||
return action.error.state_update | ||
if action.type == ErrorRecoveryType.ASSUME_FALSE_POSITIVE_AND_CONTINUE: | ||
return [ | ||
action.error.state_update, | ||
action.error.state_update_if_false_positive, | ||
] | ||
else: | ||
return [action.error.state_update] | ||
|
||
elif isinstance(action, ResumeFromRecoveryAction): | ||
return [action.state_update] | ||
|
||
else: | ||
return None | ||
return [] |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is kind of an unrelated refactor. Protocol Engine has a bunch of injected dependencies that, themselves, need to be wired up to each other. This is usually straightforward, but it is, you know, code, and we can make mistakes. Especially when the dependencies' constructors do defaulting that can permit us to forget to wire something up. The wire-up was split between this |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,20 @@ class ErrorRecoveryType(enum.Enum): | |
""" | ||
|
||
WAIT_FOR_RECOVERY = enum.auto() | ||
"""Stop and wait for the error to be recovered from manually.""" | ||
"""Enter interactive error recovery mode.""" | ||
|
||
IGNORE_AND_CONTINUE = enum.auto() | ||
"""Continue with the run, as if the command never failed.""" | ||
CONTINUE_WITH_ERROR = enum.auto() | ||
"""Continue without interruption, carrying on from whatever error state the failed | ||
command left the engine in. | ||
|
||
This is like `ProtocolEngine.resume_from_recovery(reconcile_false_positive=False)`. | ||
""" | ||
Comment on lines
+31
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any better names than Would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
ASSUME_FALSE_POSITIVE_AND_CONTINUE = enum.auto() | ||
"""Continue without interruption, acting as if the underlying error was a false positive. | ||
|
||
This is like `ProtocolEngine.resume_from_recovery(reconcile_false_positive=True)`. | ||
""" | ||
|
||
|
||
class ErrorRecoveryPolicy(Protocol): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
# noqa: D100 | ||
|
||
|
||
from opentrons.hardware_control import HardwareControlAPI | ||
from opentrons.protocol_engine.actions.action_handler import ActionHandler | ||
from opentrons.protocol_engine.actions.actions import ( | ||
Action, | ||
FailCommandAction, | ||
ResumeFromRecoveryAction, | ||
) | ||
from opentrons.protocol_engine.commands.command import DefinedErrorData | ||
from opentrons.protocol_engine.error_recovery_policy import ErrorRecoveryType | ||
from opentrons.protocol_engine.execution.tip_handler import HardwareTipHandler | ||
from opentrons.protocol_engine.state import update_types | ||
from opentrons.protocol_engine.state.state import StateView | ||
|
||
|
||
class ErrorRecoveryHardwareStateSynchronizer(ActionHandler): | ||
"""A hack to keep the hardware API's state correct through certain error recovery flows. | ||
|
||
BACKGROUND: | ||
|
||
Certain parts of robot state are duplicated between `opentrons.protocol_engine` and | ||
`opentrons.hardware_control`. Stuff like "is there a tip attached." | ||
|
||
Normally, Protocol Engine command implementations (`opentrons.protocol_engine.commands`) | ||
mutate hardware API state when they execute; and then when they finish executing, | ||
the Protocol Engine state stores (`opentrons.protocol_engine.state`) update Protocol | ||
Engine state accordingly. So both halves are accounted for. This generally works fine. | ||
|
||
However, we need to go out of our way to support | ||
`ProtocolEngine.resume_from_recovery(reconcile_false_positive=True)`. | ||
It wants to apply a second set of state updates to "fix things up" with the | ||
new knowledge that some error was a false positive. The Protocol Engine half of that | ||
is easy for us to apply the normal way, through the state stores; but the | ||
hardware API half of that cannot be applied the normal way, from the command | ||
implementation, because the command in question is no longer running. | ||
|
||
THE HACK: | ||
|
||
This listens for the same error recovery state updates that the state stores do, | ||
figures out what hardware API state mutations ought to go along with them, | ||
and then does those mutations. | ||
|
||
The problem is that hardware API state is now mutated from two different places | ||
(sometimes the command implementations, and sometimes here), which are bound | ||
to grow accidental differences. | ||
|
||
TO FIX: | ||
|
||
Make Protocol Engine's use of the hardware API less stateful. e.g. supply | ||
tip geometry every time we call a hardware API movement method, instead of | ||
just once when we pick up a tip. Use Protocol Engine state as the single source | ||
of truth. | ||
""" | ||
|
||
def __init__(self, hardware_api: HardwareControlAPI, state_view: StateView) -> None: | ||
self._hardware_api = hardware_api | ||
self._state_view = state_view | ||
|
||
def handle_action(self, action: Action) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LOVELY! |
||
"""Modify hardware API state in reaction to a Protocol Engine action.""" | ||
state_update = _get_state_update(action) | ||
if state_update: | ||
self._synchronize(state_update) | ||
|
||
def _synchronize(self, state_update: update_types.StateUpdate) -> None: | ||
tip_handler = HardwareTipHandler(self._state_view, self._hardware_api) | ||
|
||
if state_update.pipette_tip_state != update_types.NO_CHANGE: | ||
pipette_id = state_update.pipette_tip_state.pipette_id | ||
tip_geometry = state_update.pipette_tip_state.tip_geometry | ||
if tip_geometry is None: | ||
tip_handler.remove_tip(pipette_id) | ||
else: | ||
tip_handler.cache_tip(pipette_id=pipette_id, tip=tip_geometry) | ||
|
||
|
||
def _get_state_update(action: Action) -> update_types.StateUpdate | None: | ||
"""Get the mutations that we need to do on the hardware API to stay in sync with an engine action. | ||
|
||
The mutations are returned in Protocol Engine terms, as a StateUpdate. | ||
They then need to be converted to hardware API terms. | ||
""" | ||
match action: | ||
case ResumeFromRecoveryAction(state_update=state_update): | ||
return state_update | ||
|
||
case FailCommandAction( | ||
error=DefinedErrorData( | ||
state_update_if_false_positive=state_update_if_false_positive | ||
) | ||
): | ||
return ( | ||
state_update_if_false_positive | ||
if action.type == ErrorRecoveryType.ASSUME_FALSE_POSITIVE_AND_CONTINUE | ||
else None | ||
) | ||
|
||
case _: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know why the false positive is so confusing to me but can we change it?
state_update_if_command_failed
if its only me we can leave as itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed a bit in-person: It sounds like we're keeping the "false positive" terminology for now, but I'm definitely open to better ideas. We can change the terminology relatively easily before end of day Monday.