Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature/custom exit codes in tools #2566

Merged
merged 9 commits into from
Jun 21, 2024
Merged

Feature/custom exit codes in tools #2566

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e02efee
Add SystemExit handling in Tool.run()
mexanick Jun 19, 2024
ac232fb
Fix logging laziness in tool.py
mexanick Jun 19, 2024
628e81f
Add test for SystemExit handling
mexanick Jun 19, 2024
2663085
Fix ruff error
mexanick Jun 19, 2024
8fef3fd
Add exit code to provenance
mexanick Jun 20, 2024
636d451
Fix tests (do not re-intercept re-raised exceptions)
mexanick Jun 20, 2024
bbe607c
Change status "partial_success" to "error"
mexanick Jun 21, 2024
6925244
Change default status (at init) to "running"
mexanick Jun 21, 2024
42ecb4c
Add changes doc
mexanick Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/ctapipe/core/tests/test_run_tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import sys
from subprocess import CalledProcessError

import pytest
Expand All @@ -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)
17 changes: 12 additions & 5 deletions src/ctapipe/core/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Classes to handle configurable command-line user interfaces."""

import html
import logging
import logging.config
Expand Down Expand Up @@ -255,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:
"""
Expand Down Expand Up @@ -405,15 +406,15 @@ 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)

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
Expand All @@ -425,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)
Expand All @@ -440,11 +441,17 @@ 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:
raise
except SystemExit as err:
maxnoe marked this conversation as resolved.
Show resolved Hide resolved
exit_status = err.code
self.log.exception("Caught SystemExit with exit code %s", exit_status)
Provenance().finish_activity(
activity_name=self.name, status="SystemExit"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the original plan was to not allow any random exit code, only a pre-defined set. If we need to have arbitrary exit codes (I guess the use case here is for workflow handling?), at the very least the code that was used should be stored in the provenance. Just recording "SystemExit" loses the reason for the exit happening. So we should either add a field to the provenance, or include the code in the status. The latter is less ideal since it would require parsing, and means the sets of possible status values is not finite

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the use case here is workflow handling. And the exit codes should not be completely randomly, but rather controlled by the corresponding ICD in our case. However, given that ctapipe is a community product, I'm a bit hesitant to introduce such a dependency here. On the other hand, I fully agree with your point that the exit code should be stored in provenance. Here before making a move, I'd like to discuss options.

So far, the possible statuses are ["completed", "interrupted", "error"] plus "SystemExit" I've added. I'm not sure what the subsequent purpose of this field. Is it just a human-readable label? If so, I would add another field, exit_code or just code where an actual exit code will be stored and change the list of possible statuses to ["success", "interrupted", "error", "partial_success"] or similar. On the other hand, if the status field is supposed to be machine-readable, shall it just keep the exit code value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, the possible statuses are ["completed", "interrupted", "error"] plus "SystemExit"

I think SystemExit is not needed. The only reason to have a non-zero exit code is by definition some kind of "error".

If so, I would add another field, exit_code

Yes, I think adding the exit_code field is the way to go.

The problem is a bit that in the current scheme, Acitivity is not mapped one-to-one to a command-line tool.

"partial_success"

I think that's a bit hard to define at the Tool logic-level, however, what we could do is to let the implementation set this status already

Provenance().current_acitvity.status = "partial_success" and in that case we don't override it in Tool.run

Copy link
Contributor Author

@mexanick mexanick Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, this should be default in the _ActivityProvenance helper class constructor:

ctapipe/src/ctapipe/core/provenance.py

Lines 220 to 230 in 2b70207

def __init__(self, activity_name=sys.executable):
self._prov = {
"activity_name": activity_name,
"activity_uuid": str(uuid.uuid4()),
"start": {},
"stop": {},
"system": {},
"input": [],
"output": [],
}
self.name = activity_name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sys.executable

That's always the python interpreter, right? It won't be the cli tool name even if used via an entry point installed script

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the default value in the current code. It is overridden in the Tool.run() function:

ctapipe/src/ctapipe/core/tool.py

Line 409 in 2b70207

Provenance().start_activity(self.name)

)
finally:
if not {"-h", "--help", "--help-all"}.intersection(self.argv):
self.write_provenance()
Expand Down
Loading