From e02efee96bb1dd2e00ab754ba7657044e790a076 Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Wed, 19 Jun 2024 16:39:08 +0200 Subject: [PATCH 1/9] Add SystemExit handling in Tool.run() --- src/ctapipe/core/tool.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py index 9221ae9fb6e..795569560d2 100644 --- a/src/ctapipe/core/tool.py +++ b/src/ctapipe/core/tool.py @@ -1,4 +1,5 @@ """Classes to handle configurable command-line user interfaces.""" + import html import logging import logging.config @@ -445,6 +446,12 @@ def run(self, argv=None, raises=False): exit_status = 1 # any other error if raises: raise + except SystemExit as err: + exit_status = err.code + self.log.exception("Caught SystemExit with exit code %s", exit_status) + Provenance().finish_activity( + activity_name=self.name, status="SystemExit" + ) finally: if not {"-h", "--help", "--help-all"}.intersection(self.argv): self.write_provenance() From ac232fb802ee64444607ce8d666adac9618656f3 Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Wed, 19 Jun 2024 16:44:48 +0200 Subject: [PATCH 2/9] Fix logging laziness in tool.py --- src/ctapipe/core/tool.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py index 795569560d2..12579573803 100644 --- a/src/ctapipe/core/tool.py +++ b/src/ctapipe/core/tool.py @@ -256,7 +256,7 @@ def initialize(self, argv=None): self.update_config(self.cli_config) self.update_logging_config() - self.log.info(f"ctapipe version {self.version_string}") + self.log.info("ctapipe version %s", self.version_string) def load_config_file(self, path: str | pathlib.Path) -> None: """ @@ -406,7 +406,7 @@ def run(self, argv=None, raises=False): with self._exit_stack: try: - self.log.info(f"Starting: {self.name}") + self.log.info("Starting: %s", self.name) Provenance().start_activity(self.name) self.initialize(argv) @@ -414,7 +414,7 @@ def run(self, argv=None, raises=False): self.setup() self.is_setup = True - self.log.debug(f"CONFIG: {self.get_current_config()}") + self.log.debug("CONFIG: %s", self.get_current_config()) Provenance().add_config(self.get_current_config()) # check for any traitlets warnings using our custom handler @@ -426,7 +426,7 @@ def run(self, argv=None, raises=False): self.start() self.finish() - self.log.info(f"Finished: {self.name}") + self.log.info("Finished: %s", self.name) Provenance().finish_activity(activity_name=self.name) except (ToolConfigurationError, TraitError) as err: self.log.error("%s", err) @@ -441,7 +441,7 @@ def run(self, argv=None, raises=False): ) exit_status = 130 # Script terminated by Control-C except Exception as err: - self.log.exception(f"Caught unexpected exception: {err}") + self.log.exception("Caught unexpected exception: %s", err) Provenance().finish_activity(activity_name=self.name, status="error") exit_status = 1 # any other error if raises: From 628e81feead99e0fdb3a5bd674e36d1be2f2e672 Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Wed, 19 Jun 2024 17:07:52 +0200 Subject: [PATCH 3/9] Add test for SystemExit handling --- src/ctapipe/core/tests/test_run_tool.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/ctapipe/core/tests/test_run_tool.py b/src/ctapipe/core/tests/test_run_tool.py index 4f13676da98..35ff81b1fa9 100644 --- a/src/ctapipe/core/tests/test_run_tool.py +++ b/src/ctapipe/core/tests/test_run_tool.py @@ -1,4 +1,5 @@ from subprocess import CalledProcessError +import sys import pytest @@ -15,5 +16,16 @@ def start(self): ret = run_tool(ErrorTool(), ["--non-existing-alias"], raises=False) assert ret == 2 + + class SysExitTool(Tool): + def setup(self): + pass + + def start(self): + sys.exit(4) + + ret = run_tool(SysExitTool(), raises=False) + assert ret == 4 + with pytest.raises(CalledProcessError): run_tool(ErrorTool(), ["--non-existing-alias"], raises=True) From 2663085af22472eed288ef7b2caeb01937cc228f Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Wed, 19 Jun 2024 17:11:38 +0200 Subject: [PATCH 4/9] Fix ruff error --- src/ctapipe/core/tests/test_run_tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctapipe/core/tests/test_run_tool.py b/src/ctapipe/core/tests/test_run_tool.py index 35ff81b1fa9..5b584818704 100644 --- a/src/ctapipe/core/tests/test_run_tool.py +++ b/src/ctapipe/core/tests/test_run_tool.py @@ -1,5 +1,5 @@ -from subprocess import CalledProcessError import sys +from subprocess import CalledProcessError import pytest From 8fef3fd6c66e8f3c57849a2856dc52fc9da94adc Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Thu, 20 Jun 2024 11:02:24 +0200 Subject: [PATCH 5/9] Add exit code to provenance - Add "exit_code" field to provenance dictionary - Update the possible provenance status values to ["success", "interrupted", "error", "partial_success"] - Making the "partial_success" status default - It is overridden in finish_activity with a default "success" --- src/ctapipe/core/provenance.py | 10 +++++++--- src/ctapipe/core/tool.py | 15 +++++++++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/ctapipe/core/provenance.py b/src/ctapipe/core/provenance.py index 19be14dd278..51b6d773f70 100644 --- a/src/ctapipe/core/provenance.py +++ b/src/ctapipe/core/provenance.py @@ -4,6 +4,7 @@ TODO: have this register whenever ctapipe is loaded """ + import json import logging import os @@ -145,7 +146,7 @@ def add_config(self, config): activity.name, ) - def finish_activity(self, status="completed", activity_name=None): + def finish_activity(self, status="completed", exit_code=0, activity_name=None): """end the current activity""" activity = self._activities.pop() if activity_name is not None and activity_name != activity.name: @@ -155,7 +156,7 @@ def finish_activity(self, status="completed", activity_name=None): ) ) - activity.finish(status) + activity.finish(status, exit_code) self._finished_activities.append(activity) log.debug(f"finished activity: {activity.name}") @@ -221,11 +222,13 @@ def __init__(self, activity_name=sys.executable): self._prov = { "activity_name": activity_name, "activity_uuid": str(uuid.uuid4()), + "status": "partial_success", "start": {}, "stop": {}, "system": {}, "input": [], "output": [], + "exit_code": None, } self.name = activity_name @@ -268,7 +271,7 @@ def register_config(self, config): """add a dictionary of configuration parameters to this activity""" self._prov["config"] = config - def finish(self, status="completed"): + def finish(self, status="success", exit_code=0): """record final provenance information, normally called at shutdown.""" self._prov["stop"].update(_sample_cpu_and_memory()) @@ -276,6 +279,7 @@ def finish(self, status="completed"): t_start = Time(self._prov["start"]["time_utc"], format="isot") t_stop = Time(self._prov["stop"]["time_utc"], format="isot") self._prov["status"] = status + self._prov["exit_code"] = exit_code self._prov["duration_min"] = (t_stop - t_start).to("min").value @property diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py index 12579573803..5f9164beb60 100644 --- a/src/ctapipe/core/tool.py +++ b/src/ctapipe/core/tool.py @@ -432,25 +432,32 @@ def run(self, argv=None, raises=False): 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 Provenance().finish_activity( - activity_name=self.name, status="interrupted" + activity_name=self.name, status="interrupted", exit_code=exit_status ) - exit_status = 130 # Script terminated by Control-C except Exception as err: self.log.exception("Caught unexpected exception: %s", err) - Provenance().finish_activity(activity_name=self.name, status="error") 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 self.log.exception("Caught SystemExit with exit code %s", exit_status) Provenance().finish_activity( - activity_name=self.name, status="SystemExit" + activity_name=self.name, + status="partial_success", + exit_code=exit_status, ) finally: if not {"-h", "--help", "--help-all"}.intersection(self.argv): From 636d4511cf7f7029312edafa6b7a6b9c47aaa343 Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Thu, 20 Jun 2024 13:39:06 +0200 Subject: [PATCH 6/9] Fix tests (do not re-intercept re-raised exceptions) --- src/ctapipe/core/tool.py | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py index 5f9164beb60..3ad0ceb3282 100644 --- a/src/ctapipe/core/tool.py +++ b/src/ctapipe/core/tool.py @@ -452,13 +452,18 @@ def run(self, argv=None, raises=False): if raises: raise except SystemExit as err: - exit_status = err.code - self.log.exception("Caught SystemExit with exit code %s", exit_status) - Provenance().finish_activity( - activity_name=self.name, - status="partial_success", - exit_code=exit_status, - ) + if raises: + raise # do not re-intercept in tests + else: + exit_status = err.code + self.log.exception( + "Caught SystemExit with exit code %s", exit_status + ) + Provenance().finish_activity( + activity_name=self.name, + status="partial_success", + exit_code=exit_status, + ) finally: if not {"-h", "--help", "--help-all"}.intersection(self.argv): self.write_provenance() From bbe607cfdb3cbadad3ea96fb8aa489d83246e688 Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Fri, 21 Jun 2024 10:32:54 +0200 Subject: [PATCH 7/9] Change status "partial_success" to "error" --- src/ctapipe/core/tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py index 3ad0ceb3282..4bb93dbeccc 100644 --- a/src/ctapipe/core/tool.py +++ b/src/ctapipe/core/tool.py @@ -461,7 +461,7 @@ def run(self, argv=None, raises=False): ) Provenance().finish_activity( activity_name=self.name, - status="partial_success", + status="error", exit_code=exit_status, ) finally: From 6925244ff317b0fc482d0a93c6d883b32d822058 Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Fri, 21 Jun 2024 10:37:04 +0200 Subject: [PATCH 8/9] Change default status (at init) to "running" --- src/ctapipe/core/provenance.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctapipe/core/provenance.py b/src/ctapipe/core/provenance.py index 51b6d773f70..02ee888ab75 100644 --- a/src/ctapipe/core/provenance.py +++ b/src/ctapipe/core/provenance.py @@ -222,7 +222,7 @@ def __init__(self, activity_name=sys.executable): self._prov = { "activity_name": activity_name, "activity_uuid": str(uuid.uuid4()), - "status": "partial_success", + "status": "running", "start": {}, "stop": {}, "system": {}, From 42ecb4cb4b9295fcd74e292cb3151c57b1fb97f6 Mon Sep 17 00:00:00 2001 From: Mykhailo Dalchenko Date: Fri, 21 Jun 2024 10:48:26 +0200 Subject: [PATCH 9/9] Add changes doc --- docs/changes/2566.feature.rst | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 docs/changes/2566.feature.rst diff --git a/docs/changes/2566.feature.rst b/docs/changes/2566.feature.rst new file mode 100644 index 00000000000..f9f32ae2b54 --- /dev/null +++ b/docs/changes/2566.feature.rst @@ -0,0 +1,7 @@ +Add ``SystemExit`` handling at the ``ctapipe.core.Tool`` level + +If a ``SystemExit`` with a custom error code is generated during the tool execution, +the tool will be terminated gracefully and the error code will be preserved and propagated. + +The ``Activity`` statuses have been updated to ``["running", "success", "interrupted", "error"]``. +The ``"running"`` status is assigned at init.