Repository: cta-observatory/ctapipe
Pull request #2594 (2024-07-17 13:28:01)

Update exception handling in tools
Following some real-world experience with my own changes proposed before, I believe this would be a better way to handle custom runtime exceptions with the custom exit codes.
- ~~Remove SystemExit handling in `Tool.run()`~~
  - decided to keep
- Add a possibility to register custom exceptions and handle them in `Tool.run()` with the preservation of the exit code.

ctao-dpps-sonarqube[bot]: ![Passed](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/QualityGateBadge/passed.svg?sanitize=true)

# Analysis Details
## 0 Issues
- ![Bug](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/bug.svg?sanitize=true) 0 Bugs
- ![Vulnerability](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/vulnerability.svg?sanitize=true) 0 Vulnerabilities
- ![Code Smell](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/code_smell.svg?sanitize=true) 0 Code Smells

## Coverage and Duplications
- ![Coverage](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/CoverageChart/100.svg?sanitize=true) 100.00% Coverage (94.20% Estimated after merge)
- ![Duplications](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/Duplications/3.svg?sanitize=true) 0.00% Duplicated Code (0.60% Estimated after merge)

**Project ID:** cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

[View in SonarQube](https://sonar-cta-dpps.zeuthen.desy.de/dashboard?id=cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs&pullRequest=2594)


maxnoe: btw.: I simplified the `SystemExit` part here:
https://github.com/cta-observatory/ctapipe/pull/2591/files#diff-73435d23e062b16d391308f70039eb8d6bc9c40c1b12d03e9737eb4ec91b36f3R456
after realizing that `raise SystemExit` is now different than `sys.exit()`, so `raise ` doesn't make sense for it
ctao-dpps-sonarqube[bot]: ![Passed](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/QualityGateBadge/passed.svg?sanitize=true)

# Analysis Details
## 1 Issue
- ![Bug](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/bug.svg?sanitize=true) 0 Bugs
- ![Vulnerability](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/vulnerability.svg?sanitize=true) 0 Vulnerabilities
- ![Code Smell](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/code_smell.svg?sanitize=true) 1 Code Smell

## Coverage and Duplications
- ![Coverage](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/CoverageChart/100.svg?sanitize=true) 100.00% Coverage (94.20% Estimated after merge)
- ![Duplications](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/Duplications/3.svg?sanitize=true) 0.00% Duplicated Code (0.60% Estimated after merge)

**Project ID:** cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

[View in SonarQube](https://sonar-cta-dpps.zeuthen.desy.de/dashboard?id=cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs&pullRequest=2594)


ctao-dpps-sonarqube[bot]: ![Passed](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/QualityGateBadge/passed.svg?sanitize=true)

# Analysis Details
## 1 Issue
- ![Bug](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/bug.svg?sanitize=true) 0 Bugs
- ![Vulnerability](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/vulnerability.svg?sanitize=true) 0 Vulnerabilities
- ![Code Smell](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/code_smell.svg?sanitize=true) 1 Code Smell

## Coverage and Duplications
- ![Coverage](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/CoverageChart/100.svg?sanitize=true) 100.00% Coverage (94.20% Estimated after merge)
- ![Duplications](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/Duplications/3.svg?sanitize=true) 0.00% Duplicated Code (0.60% Estimated after merge)

**Project ID:** cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

[View in SonarQube](https://sonar-cta-dpps.zeuthen.desy.de/dashboard?id=cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs&pullRequest=2594)


maxnoe: How about introducing a member variable `exit_code` on the `Tool`, so that the need for custom exceptions is not there, but tools can just do:

```
self.exit_code = 5
raise ValueError(...)
```

However, I don't really see why this is different to just doing this in the tool:

```
log.critical("Error condition foo")
self.exit(5)
```
mexanick: The exceptions can be raised in the components that tool uses, in this case what you propose is still possible, but requires additional boiler plate try-except code in every tool. Here, for the price of a bit more complex exception handling, we can properly catch all the exceptions raised in the Tool itself or in the components it uses, and also keep the definition of exit codes tightly coupled to exceptions themselves. Adding an `exit_code` attribute to the tool seems to be redundant to me. 
maxnoe: That means a component down the line would already specify the exit code?

What if a component is used in more than one tool?
maxnoe: I think it would be much more straight forward to catch the exception where it appears in your tool:

```
try:
    self.comp.foo()
except Exception:
     self.exception("comp failed doing foo")   
     self.exit(5)
```
mexanick: > That means a component down the line would already specify the exit code?

I'm in favor of defining custom exceptions specifying the exit code, i.e. one exception can be reused between the components or tools if needed and the exit code is tied to this particular exception.

> What if a component is used in more than one tool?

I don't see a problem here... can you elaborate?

> I think it would be much more straight forward to catch the exception where it appears in your tool:
> 
```
try:
    self.comp.foo()
except Exception:
     self.exception("comp failed doing foo")   
     self.exit(5)
```

This is an option, but as I wrote above, would require more boiler plate code above.  Since I'm still in favor of having exit code being tied to the custom exception, this should become like this:
```
try:
    self.comp.foo()
except MyCustomException as exc:
    self.log.exception(".{}", exc)
    self.exit(exc.exit_code)\
```

which is basically the same that's added to the `Tool`'s exception handling in the PR.

mexanick: CI failed because of broken provenance, will rebase as soon as #2595 is merged

Patch:
diff --git a/docs/changes/2594.optimization.rst b/docs/changes/2594.optimization.rst
new file mode 100644
index 00000000..c1045f4d
--- /dev/null
+++ b/docs/changes/2594.optimization.rst
@@ -0,0 +1,4 @@
+Update exception handling in tools
+
+- Add a possibility to handle custom exception in ``Tool.run()``
+  with the preservation of the exit code.
diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py
index e1f7c2e2..cdc005a5 100644
--- a/src/ctapipe/core/tool.py
+++ b/src/ctapipe/core/tool.py
@@ -87,6 +87,9 @@ class Tool(Application):
     executing. This happens after the ``finish`` method has run or
     in case of exceptions.
 
+    User-defined code can raise custom exceptions both in the components
+    or in the tool methods. If these custom exceptions have an ``exit_code`` attribute,
+    it will be propagated to the final exit code of the tool.
 
     .. code:: python
 
@@ -403,6 +406,7 @@ class Tool(Application):
         #  https://tldp.org/LDP/abs/html/exitcodes.html
 
         exit_status = 0
+        current_exception = None
 
         with self._exit_stack:
             try:
@@ -429,14 +433,13 @@ class Tool(Application):
                 self.log.info("Finished: %s", self.name)
                 Provenance().finish_activity(activity_name=self.name)
             except (ToolConfigurationError, TraitError) as err:
+                current_exception = err
                 self.log.error("%s", err)
                 self.log.error("Use --help for more info")
                 exit_status = 2  # wrong cmd line parameter
                 Provenance().finish_activity(
                     activity_name=self.name, status="error", exit_code=exit_status
                 )
-                if raises:
-                    raise
             except KeyboardInterrupt:
                 self.log.warning("WAS INTERRUPTED BY CTRL-C")
                 exit_status = 130  # Script terminated by Control-C
@@ -444,31 +447,33 @@ class Tool(Application):
                     activity_name=self.name, status="interrupted", exit_code=exit_status
                 )
             except Exception as err:
+                current_exception = err
+                exit_status = getattr(err, "exit_code", 1)
                 self.log.exception("Caught unexpected exception: %s", err)
-                exit_status = 1  # any other error
                 Provenance().finish_activity(
                     activity_name=self.name, status="error", exit_code=exit_status
                 )
-                if raises:
-                    raise
             except SystemExit as err:
                 exit_status = err.code
-                # Do nothing if SystemExit was called with the exit code 0 (e.g. with -h option)
-                if exit_status != 0:
-                    if raises:
-                        raise  # do not re-intercept in tests
-                    else:
-                        self.log.exception(
-                            "Caught SystemExit with exit code %s", exit_status
-                        )
-                        Provenance().finish_activity(
-                            activity_name=self.name,
-                            status="error",
-                            exit_code=exit_status,
-                        )
+                if exit_status == 0:
+                    # Finish normally
+                    Provenance().finish_activity(activity_name=self.name)
+                else:
+                    # Finish with error
+                    current_exception = err
+                    self.log.critical(
+                        "Caught SystemExit with exit code %s", exit_status
+                    )
+                    Provenance().finish_activity(
+                        activity_name=self.name,
+                        status="error",
+                        exit_code=exit_status,
+                    )
             finally:
                 if not {"-h", "--help", "--help-all"}.intersection(self.argv):
                     self.write_provenance()
+                if raises and current_exception:
+                    raise current_exception
 
         self.exit(exit_status)
 


Test patch:
diff --git a/src/ctapipe/core/tests/test_tool.py b/src/ctapipe/core/tests/test_tool.py
index 53dc5a18..a9322eb5 100644
--- a/src/ctapipe/core/tests/test_tool.py
+++ b/src/ctapipe/core/tests/test_tool.py
@@ -381,13 +381,40 @@ def test_tool_raises():
         def start(self):
             raise ValueError("1 does not equal 0.")
 
+    class CustomErrorNoExitCode(Exception):
+        pass
+
+    class CustomErrorWithExitCode(Exception):
+        exit_code = 42
+
+    class ToolCustomExceptionNoExitCode(Tool):
+        name = "CustomException"
+        description = "This tool raises a custom exception without an exit code."
+
+        def start(self):
+            raise CustomErrorNoExitCode("This is a custom exception.")
+
+    class ToolCustomExceptionWithExitCode(Tool):
+        name = "CustomException"
+        description = "This tool raises a custom exception with a custom exit code."
+
+        def start(self):
+            raise CustomErrorWithExitCode("This is a custom exception.")
+
     assert run_tool(ToolGood(), raises=True) == 0
 
     assert run_tool(ToolBad(), raises=False) == 1
 
+    assert run_tool(ToolCustomExceptionNoExitCode(), raises=False) == 1
+
+    assert run_tool(ToolCustomExceptionWithExitCode(), raises=False) == 42
+
     with pytest.raises(ValueError):
         run_tool(ToolBad(), raises=True)
 
+    with pytest.raises(CustomErrorNoExitCode):
+        run_tool(ToolCustomExceptionNoExitCode(), raises=True)
+
 
 def test_exit_stack():
     """Test that components that are context managers are properly handled"""
diff --git a/src/ctapipe/core/tests/test_traits.py b/src/ctapipe/core/tests/test_traits.py
index 06a7e450..c84b0da1 100644
--- a/src/ctapipe/core/tests/test_traits.py
+++ b/src/ctapipe/core/tests/test_traits.py
@@ -351,11 +351,11 @@ def test_quantity_tool(capsys):
         run_tool(tool, ["--MyTool.energy=5 m"], raises=True)
 
     captured = capsys.readouterr()
-    assert (
-        captured.err.split(":")[-1]
-        == f" Given quantity is of physical type {u.get_physical_type(5 * u.m)}."
-        + f" Expected {u.physical.energy}.\n"
+    expected = (
+        f" Given quantity is of physical type {u.get_physical_type(5 * u.m)}."
+        f" Expected {u.physical.energy}.\n"
     )
+    assert expected in captured.err
 
 
 def test_quantity_none():