Feature/custom exit codes in tools

[mexanick]

Following a need in custom exit codes in ctapipe.core.Tool, I've implemented an extra catch of SystemExit that can be generated during the tool's execution phase that also performs a clean up in the same fashion as in general Exception interception and propagates the exit code further down.

[kosack]

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

[mexanick]

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?

[maxnoe]

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

[mexanick]

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

[maxnoe]

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

[mexanick]

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)

[ctao-dpps-sonarqube]

Analysis Details

1 Issue

• 0 Bugs
• 0 Vulnerabilities
• 1 Code Smell

Coverage and Duplications

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

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube