fix(api): Report Python protocol line numbers for errors from Protocol Engine

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

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
         )

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}"

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

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

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
 
 

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