Update exception handling in tools

[mexanick]

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.

[maxnoe]

I think we should still catch the SystemExit with error code != 0 case here, I think this was the cause of the issue for #1881

[mexanick]

ok, SystemExit treatment is added back

[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 no different than sys.exit(), so raise doesn't make sense for it

[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

[mexanick]

@maxnoe @Tobychev can you please take a look?

[maxnoe]

Please add some documentation on this to the docstring of Tool.

[ctao-dpps-sonarqube]

Analysis Details

0 Issues

• 0 Bugs
• 0 Vulnerabilities
• 0 Code Smells

Coverage and Duplications

• 100.00% Coverage (93.60% Estimated after merge)
• 0.00% Duplicated Code (0.60% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube