diff --git a/api/mypy.ini b/api/mypy.ini index bd85666af02..56b8435855c 100644 --- a/api/mypy.ini +++ b/api/mypy.ini @@ -14,8 +14,8 @@ warn_untyped_fields = True # TODO(mc, 2021-09-08): fix and remove any / all of the # overrides below whenever able -# ~125 errors -[mypy-opentrons.protocols.advanced_control.*,opentrons.protocols.api_support.*,opentrons.protocols.duration.*,opentrons.protocols.execution.*,opentrons.protocols.geometry.*,opentrons.protocols.models.*,opentrons.protocols.labware.*] +# ~115 errors +[mypy-opentrons.protocols.advanced_control.*,opentrons.protocols.api_support.*,opentrons.protocols.duration.*,opentrons.protocols.execution.dev_types,opentrons.protocols.execution.execute_json_v4,opentrons.protocols.execution.execute_python,opentrons.protocols.geometry.*,opentrons.protocols.models.*,opentrons.protocols.labware.*] disallow_any_generics = False disallow_untyped_defs = False disallow_untyped_calls = False diff --git a/api/src/opentrons/protocol_engine/protocol_engine.py b/api/src/opentrons/protocol_engine/protocol_engine.py index 032325013f9..fa851ec29e7 100644 --- a/api/src/opentrons/protocol_engine/protocol_engine.py +++ b/api/src/opentrons/protocol_engine/protocol_engine.py @@ -329,13 +329,28 @@ async def finish( after the run is over. """ if self._state_store.commands.state.stopped_by_estop: + # This handles the case where the E-stop was pressed while we were *not* in the middle + # of some hardware interaction that would raise it as an exception. For example, imagine + # we were paused between two commands, or imagine we were executing a very long run of + # comment commands. drop_tips_after_run = False post_run_hardware_state = PostRunHardwareState.DISENGAGE_IN_PLACE if error is None: error = EStopActivatedError(message="Estop was activated during a run") + if error: - if isinstance(error, EnumeratedError) and self._code_in_exception_stack( - error=error, code=ErrorCodes.E_STOP_ACTIVATED + # If the run had an error, check if that error indicates an E-stop. + # This handles the case where the run was in the middle of some hardware control + # method and the hardware controller raised an E-stop error from it. + # + # To do this, we need to scan all the way through the error tree. + # By the time E-stop error has gotten to us, it may have been wrapped in other errors, + # so we need to unwrap them to uncover the E-stop error's inner beauty. + # + # We don't use self._hardware_api.get_estop_state() because the E-stop may have been + # released by the time we get here. + if isinstance(error, EnumeratedError) and self._code_in_error_tree( + root_error=error, code=ErrorCodes.E_STOP_ACTIVATED ): drop_tips_after_run = False post_run_hardware_state = PostRunHardwareState.DISENGAGE_IN_PLACE @@ -494,34 +509,34 @@ async def use_attached_modules( # TODO(tz, 7-12-23): move this to shared data when we dont relay on ErrorOccurrence @staticmethod - def _code_in_exception_stack( - error: Union[EnumeratedError, ErrorOccurrence], code: ErrorCodes + def _code_in_error_tree( + root_error: Union[EnumeratedError, ErrorOccurrence], code: ErrorCodes ) -> bool: - if isinstance(error, ErrorOccurrence): + if isinstance(root_error, ErrorOccurrence): # ErrorOccurrence is not the same as the enumerated error exceptions. Check the # code by a string value. - if error.errorCode == code.value.code: + if root_error.errorCode == code.value.code: return True return any( - ProtocolEngine._code_in_exception_stack(wrapped, code) - for wrapped in error.wrappedErrors + ProtocolEngine._code_in_error_tree(wrapped, code) + for wrapped in root_error.wrappedErrors ) # From here we have an exception, can just check the code + recurse to wrapped errors. - if error.code == code: + if root_error.code == code: return True if ( - isinstance(error, ProtocolCommandFailedError) - and error.original_error is not None + isinstance(root_error, ProtocolCommandFailedError) + and root_error.original_error is not None ): # For this specific EnumeratedError child, we recurse on the original_error field # in favor of the general error.wrapping field. - return ProtocolEngine._code_in_exception_stack(error.original_error, code) + return ProtocolEngine._code_in_error_tree(root_error.original_error, code) - if len(error.wrapping) == 0: + if len(root_error.wrapping) == 0: return False return any( - ProtocolEngine._code_in_exception_stack(wrapped_error, code) - for wrapped_error in error.wrapping + ProtocolEngine._code_in_error_tree(wrapped_error, code) + for wrapped_error in root_error.wrapping ) diff --git a/api/src/opentrons/protocols/execution/errors.py b/api/src/opentrons/protocols/execution/errors.py index 84d28d84643..aa77070a289 100644 --- a/api/src/opentrons/protocols/execution/errors.py +++ b/api/src/opentrons/protocols/execution/errors.py @@ -1,3 +1,6 @@ +from types import TracebackType +from typing import Optional + from opentrons_shared_data.errors.exceptions import GeneralError @@ -7,19 +10,31 @@ class ExceptionInProtocolError(GeneralError): we can properly figure out formatting """ - def __init__(self, original_exc, original_tb, message, line): + def __init__( + self, + original_exc: Exception, + original_tb: Optional[TracebackType], + message: str, + line: Optional[int], + ) -> None: self.original_exc = original_exc self.original_tb = original_tb - self.message = message self.line = line super().__init__( wrapping=[original_exc], - message="{}{}: {}".format( - self.original_exc.__class__.__name__, - " [line {}]".format(self.line) if self.line else "", - self.message, + message=_build_message( + exception_class_name=self.original_exc.__class__.__name__, + line_number=self.line, + message=message, ), ) - def __str__(self): + def __str__(self) -> str: return self.message + + +def _build_message( + exception_class_name: str, line_number: Optional[int], message: str +) -> str: + line_number_part = f" [line {line_number}]" if line_number is not None else "" + return f"{exception_class_name}{line_number_part}: {message}" diff --git a/api/src/opentrons/protocols/execution/execute.py b/api/src/opentrons/protocols/execution/execute.py index e5a062a89ae..ea8ef6163e9 100644 --- a/api/src/opentrons/protocols/execution/execute.py +++ b/api/src/opentrons/protocols/execution/execute.py @@ -16,7 +16,7 @@ MODULE_LOG = logging.getLogger(__name__) -def run_protocol(protocol: Protocol, context: ProtocolContext): +def run_protocol(protocol: Protocol, context: ProtocolContext) -> None: """Run a protocol. :param protocol: The :py:class:`.protocols.types.Protocol` to execute diff --git a/api/src/opentrons/protocols/execution/execute_python.py b/api/src/opentrons/protocols/execution/execute_python.py index 940767fdb03..1d01a7120cd 100644 --- a/api/src/opentrons/protocols/execution/execute_python.py +++ b/api/src/opentrons/protocols/execution/execute_python.py @@ -10,7 +10,6 @@ from opentrons.protocols.execution.errors import ExceptionInProtocolError from opentrons.protocols.types import PythonProtocol, MalformedPythonProtocolError from opentrons.hardware_control import ExecutionCancelledError -from opentrons.protocol_engine.errors import ProtocolCommandFailedError MODULE_LOG = logging.getLogger(__name__) @@ -63,7 +62,6 @@ def run_python(proto: PythonProtocol, context: ProtocolContext): SmoothieAlarm, asyncio.CancelledError, ExecutionCancelledError, - ProtocolCommandFailedError, ): # this is a protocol cancel and shouldn't have special logging raise diff --git a/api/src/opentrons/protocols/execution/json_dispatchers.py b/api/src/opentrons/protocols/execution/json_dispatchers.py index a48bba1f97a..398aee14a0e 100644 --- a/api/src/opentrons/protocols/execution/json_dispatchers.py +++ b/api/src/opentrons/protocols/execution/json_dispatchers.py @@ -66,7 +66,7 @@ } -def tc_do_nothing(module: ThermocyclerContext, params) -> None: +def tc_do_nothing(module: ThermocyclerContext, params: object) -> None: pass diff --git a/api/tests/opentrons/cli/test_cli.py b/api/tests/opentrons/cli/test_cli.py index 51ab62d6ae8..eae5aa31ccc 100644 --- a/api/tests/opentrons/cli/test_cli.py +++ b/api/tests/opentrons/cli/test_cli.py @@ -180,3 +180,65 @@ def run(protocol): "You may only put apiLevel in the metadata dict or the requirements dict" ) assert expected_message in result.stdout_stderr + + +@pytest.mark.parametrize( + ("python_protocol_source", "expected_detail"), + [ + ( + textwrap.dedent( + # Raises an exception from outside of Opentrons code, + # in between two PAPI functions. + """\ + requirements = {"apiLevel": "2.14"} # line 1 + # line 2 + def run(protocol): # line 3 + protocol.comment(":^)") # line 4 + raise RuntimeError(">:(") # line 5 + protocol.comment(":D") # line 6 + """ + ), + "RuntimeError [line 5]: >:(", + ), + ( + textwrap.dedent( + # Raises an exception from inside a Protocol Engine command. + # https://opentrons.atlassian.net/browse/RSS-317 + """\ + requirements = {"apiLevel": "2.14"} # line 1 + # line 2 + def run(protocol): # line 3 + tip_rack = protocol.load_labware( # line 4 + "opentrons_96_tiprack_300ul", 1 # line 5 + ) # line 6 + pipette = protocol.load_instrument( # line 7 + "p300_single", "left" # line 8 + ) # line 9 + pipette.pick_up_tip(tip_rack["A1"]) # line 10 + pipette.pick_up_tip(tip_rack["A2"]) # line 11 + """ + ), + ( + # TODO(mm, 2023-09-12): This is an overly verbose concatenative Frankenstein + # message. We should simplify our error propagation to trim out the noise. + "ProtocolCommandFailedError [line 11]:" + " Error 4000 GENERAL_ERROR (ProtocolCommandFailedError):" + " TipAttachedError: Pipette should not have a tip attached, but does." + ), + ), + # TODO: PAPIv<2.15? + ], +) +def test_python_error_line_numbers( + tmp_path: Path, python_protocol_source: str, expected_detail: str +) -> None: + """Test that error messages from Python protocols have line numbers.""" + protocol_source_file = tmp_path / "protocol.py" + protocol_source_file.write_text(python_protocol_source, encoding="utf-8") + + result = _get_analysis_result([protocol_source_file]) + + assert result.exit_code == 0 + assert result.json_output is not None + [error] = result.json_output["errors"] + assert error["detail"] == expected_detail