search

cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
63 stars 266 forks source link

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

Closed mexanick closed 2 weeks ago

mexanick commented 2 weeks ago

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 ```shell > 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 , use: --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=... Default: [] Equivalent to: [--Tool.config_files] --log-level= 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= Filename for the log Default: None Equivalent to: [--Tool.log_file] --log-file-level= 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= Default: traitlets.Undefined Equivalent to: [--Tool.provenance_log] -d, --workdir= working directory where configuration files should be written Default: './Work' Equivalent to: [--QuickStartTool.workdir] -n, --name= Contact name Default: '' Equivalent to: [--QuickStartTool.contact_name] -e, --email= Contact email Default: '' Equivalent to: [--QuickStartTool.contact_email] -o, --org= 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 "me@thing.com" --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[bot] commented 2 weeks ago

Passed

Analysis Details

1 Issue

Coverage and Duplications

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

ctao-dpps-sonarqube[bot] commented 2 weeks ago

Failed

Analysis Details

1 Issue

Coverage and Duplications

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

Tobychev commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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?).