Bugfix: don't intercept normal SystemExit(0) in Tool

[mexanick]

The interception of SystemExit introduced harmless, but annoying bug: the error message was printed while calling any tool with help option, i.e.:

2024-07-08 11:21:59,498 ERROR [ctapipe.ctapipe-quickstart] (tool.run): Caught SystemExit with exit code 0
Traceback (most recent call last):
  File "/Users/mdalchen/work/dpps/code/ctapipe/src/ctapipe/core/tool.py", line 412, in run
    self.initialize(argv)
  File "/Users/mdalchen/work/dpps/code/ctapipe/src/ctapipe/core/tool.py", line 242, in initialize
    self.parse_command_line(argv)
  File "/Users/mdalchen/miniforge3/envs/cta-dev/lib/python3.11/site-packages/traitlets/config/application.py", line 118, in inner
    return method(app, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mdalchen/miniforge3/envs/cta-dev/lib/python3.11/site-packages/traitlets/config/application.py", line 870, in parse_command_line
    self.exit(0)
  File "/Users/mdalchen/miniforge3/envs/cta-dev/lib/python3.11/site-packages/traitlets/config/application.py", line 1062, in exit

Full log

> ctapipe-quickstart -h
Generate quick start files and directory structure.

Options
=======
The options below are convenience aliases to configurable class-options,
as listed in the "Equivalent to" description-line of the aliases.
To see all configurable class-options for some <cmd>, use:
    <cmd> --help-all

-q, --quiet
    Disable console logging.
    Equivalent to: [--Tool.quiet=True]
-v, --verbose
    Set log level to DEBUG
    Equivalent to: [--Tool.log_level=DEBUG]
--overwrite
    Overwrite existing output files without asking
    Equivalent to: [--Tool.overwrite=True]
--debug
    Set log-level to debug, for the most verbose logging.
    Equivalent to: [--Application.log_level=10]
--show-config
    Show the application's configuration (human-readable format)
    Equivalent to: [--Application.show_config=True]
--show-config-json
    Show the application's configuration (json format)
    Equivalent to: [--Application.show_config_json=True]
-c, --config=<list-item-1>...
    Default: []
    Equivalent to: [--Tool.config_files]
--log-level=<Enum>
    Set the log level by value or name.
    Choices: any of [0, 10, 20, 30, 40, 50, 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL']
    Default: 30
    Equivalent to: [--Tool.log_level]
"""Classes to handle configurable command-line user interfaces."""

import html
import logging
import logging.config
import os
import pathlib
import re
import textwrap
from abc import abstractmethod
from contextlib import ExitStack
from inspect import cleandoc
from subprocess import CalledProcessError
from tempfile import mkdtemp

import yaml
from docutils.core import publish_parts
from traitlets import TraitError

try:
    import tomli as toml

    HAS_TOML = True
except ImportError:
    HAS_TOML = False

from traitlets import List, default
from traitlets.config import Application, Config, Configurable

from .. import __version__ as version
from . import Provenance
from .component import Component
from .logging import ColoredFormatter, create_logging_config
from .traits import Bool, Dict, Enum, Path

__all__ = ["Tool", "ToolConfigurationError"]


class CollectTraitWarningsHandler(logging.NullHandler):
    regex = re.compile(".*Config option.*not recognized")

    def __init__(self):
        super().__init__()
        self.errors = []

    def handle(self, record):
        if self.regex.match(record.msg) and record.levelno == logging.WARNING:
            self.errors.append(record.msg)


class ToolConfigurationError(Exception):
    def __init__(self, message):
        # Call the base class constructor with the parameters it needs
        self.message = message


class Tool(Application):
"src/ctapipe/core/tool.py" 669L, 23974B

-l, --log-file=<Path>
    Filename for the log
    Default: None
    Equivalent to: [--Tool.log_file]
--log-file-level=<Enum>
    Logging Level for File Logging
    Choices: any of [0, 10, 20, 30, 40, 50, 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL']
    Default: 'INFO'
    Equivalent to: [--Tool.log_file_level]
--provenance-log=<Path>
    Default: traitlets.Undefined
    Equivalent to: [--Tool.provenance_log]
-d, --workdir=<Path>
    working directory where configuration files should be written
    Default: './Work'
    Equivalent to: [--QuickStartTool.workdir]
-n, --name=<Unicode>
    Contact name
    Default: ''
    Equivalent to: [--QuickStartTool.contact_name]
-e, --email=<Unicode>
    Contact email
    Default: ''
    Equivalent to: [--QuickStartTool.contact_email]
-o, --org=<Unicode>
    Contact organization
    Default: ''
    Equivalent to: [--QuickStartTool.contact_organization]

Examples
--------

    To be prompted for contact info:

            ctapipe-quickstart --workdir MyProduction

        Or specify it all in the command-line:

            ctapipe-quickstart --name "my name" --email "[email protected]" --org "My Organization" --workdir Work

To see all available configurables, use `--help-all`.

2024-07-08 11:21:59,498 ERROR [ctapipe.ctapipe-quickstart] (tool.run): Caught SystemExit with exit code 0
Traceback (most recent call last):
  File "/Users/mdalchen/work/dpps/code/ctapipe/src/ctapipe/core/tool.py", line 412, in run
    self.initialize(argv)
  File "/Users/mdalchen/work/dpps/code/ctapipe/src/ctapipe/core/tool.py", line 242, in initialize
    self.parse_command_line(argv)
  File "/Users/mdalchen/miniforge3/envs/cta-dev/lib/python3.11/site-packages/traitlets/config/application.py", line 118, in inner
    return method(app, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/mdalchen/miniforge3/envs/cta-dev/lib/python3.11/site-packages/traitlets/config/application.py", line 870, in parse_command_line
    self.exit(0)
  File "/Users/mdalchen/miniforge3/envs/cta-dev/lib/python3.11/site-packages/traitlets/config/application.py", line 1062, in exit

With this fix in case a SystemExit with exit code 0 is intercepted, nothing will be done (finally clause is still honored)

[ctao-dpps-sonarqube]

• B Maintainability Rating on New Code (is worse than A)

Analysis Details

1 Issue

• 0 Bugs
• 0 Vulnerabilities
• 1 Code Smell

Coverage and Duplications

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

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

[Tobychev]

I am somewhat surprised this bugfix ended up being needed as there was a discussion about catching sys.exit(0) in the original PR and an existing test that checked the behaviour of --help.

Looking at the pre-existing test it simply checked the return value, which the buggy version didn't change, and forgetting --help could short circuit is easy enough. My guess is that since @mexanick put his tests in a separate file, neither he nor the reviewers saw the existing test and thus forgot about --help.

My conclusion is that we need some better guidance on where tests should go, because I don't see a reason why the test code for SystemExit should be in a separate file instead of inside test_tool_exit_code() in test_run_tool.py, and if I hadn't wondered why this bug fix didn't come with a change to the tests I wouldn't have opened test_run_tool.py by mistake trying to find the existing tests.

[kosack]

I think it's just that before this fix, --help emitted an ugly systemexit traceback, but also returned 0 (success), so it's hard to check the output for that case. Manually running it of course immediately shows the problem.

[Tobychev]

@kosack I think the original PR also included a write to the provenance log that didn't happen before the catch to SystemExit was added, and if the test had checked that the problem would have been caught earlier.

But I with the Provenance log not getting a lot of attention it is understandably why nobody bothered to have a check of its status (is there even easy ways to check its status programmatically?).