Repository: cta-observatory/ctapipe
Pull request #2213 (2023-01-12 15:17:49)

Consistent overwrite
- Remove all -f aliases
- Check for conflict on setup
- rename infile/outfile to input_path/output_path
- Use ToolConfigurationError everywhere

I guess that would be an API change since we remove a flag from some tools?


Patch:
diff --git a/ctapipe/core/tool.py b/ctapipe/core/tool.py
index 4ccb2fb5..39d1ac1e 100644
--- a/ctapipe/core/tool.py
+++ b/ctapipe/core/tool.py
@@ -174,6 +174,7 @@ class Tool(Application):
     ).tag(config=True)
 
     quiet = Bool(default_value=False).tag(config=True)
+    overwrite = Bool(default_value=False).tag(config=True)
 
     _log_formatter_cls = ColoredFormatter
 
@@ -203,8 +204,12 @@ class Tool(Application):
                 {"Tool": {"log_level": "DEBUG"}},
                 "Set log level to DEBUG",
             ),
+            "overwrite": (
+                {"Tool": {"overwrite": True}},
+                "Overwrite existing output files without asking",
+            ),
         }
-        self.flags.update(flags)
+        self.flags = {**flags, **self.flags}
 
         self.is_setup = False
         self.version = version
@@ -324,6 +329,32 @@ class Tool(Application):
         self._registered_components.append(component_instance)
         return component_instance
 
+    def check_output(self, *output_paths):
+        """
+        Test if output files exist and if they do, throw an error
+        unless ``self.overwrite`` is set to True.
+        This should be checked during tool setup to avoid having a tool only
+        realize the output can not be written after some long-running calculations
+        (e.g. training of ML-models).
+        Because we currently do not collect all created output files in the tool
+        (they can be attached to some component), the output files need
+        to be given and can not easily be derived from ``self``.
+
+        Parameters
+        ----------
+        output_paths: Path
+            One or more output path to check.
+
+        """
+        for output in output_paths:
+            if output is not None and output.exists():
+                if self.overwrite:
+                    self.log.warning("Overwriting %s", output)
+                else:
+                    raise ToolConfigurationError(
+                        f"Output path {output} exists, but overwrite=False"
+                    )
+
     @abstractmethod
     def setup(self):
         """Set up the tool.
diff --git a/ctapipe/tools/apply_models.py b/ctapipe/tools/apply_models.py
index ccb1501d..cf408604 100644
--- a/ctapipe/tools/apply_models.py
+++ b/ctapipe/tools/apply_models.py
@@ -9,7 +9,7 @@ from astropy.table.operations import hstack, vstack
 from tqdm.auto import tqdm
 
 from ctapipe.core.tool import Tool
-from ctapipe.core.traits import Bool, Integer, Path, flag
+from ctapipe.core.traits import Integer, Path
 from ctapipe.io import TableLoader, write_table
 from ctapipe.io.astropy_helpers import read_table
 from ctapipe.io.tableio import TelListToMaskTransform
@@ -48,8 +48,6 @@ class ApplyModels(Tool):
         --output gamma_applied.dl2.h5
     """
 
-    overwrite = Bool(default_value=False).tag(config=True)
-
     input_url = Path(
         default_value=None,
         allow_none=False,
@@ -104,19 +102,6 @@ class ApplyModels(Tool):
         "chunk-size": "ApplyModels.chunk_size",
     }
 
-    flags = {
-        **flag(
-            "overwrite",
-            "ApplyModels.overwrite",
-            "Overwrite tables in output file if it exists",
-            "Don't overwrite tables in output file if it exists",
-        ),
-        "f": (
-            {"ApplyModels": {"overwrite": True}},
-            "Overwrite output file if it exists",
-        ),
-    }
-
     classes = [
         TableLoader,
         EnergyRegressor,
@@ -129,10 +114,8 @@ class ApplyModels(Tool):
         """
         Initialize components from config
         """
+        self.check_output(self.output_path)
         self.log.info("Copying to output destination.")
-        if self.output_path.exists() and not self.overwrite:
-            raise IOError(f"Output path {self.output_path} exists, but overwrite=False")
-
         shutil.copy(self.input_url, self.output_path)
 
         self.h5file = self.enter_context(tables.open_file(self.output_path, mode="r+"))
diff --git a/ctapipe/tools/dump_triggers.py b/ctapipe/tools/dump_triggers.py
index 2e2077f9..af03cdd8 100644
--- a/ctapipe/tools/dump_triggers.py
+++ b/ctapipe/tools/dump_triggers.py
@@ -7,10 +7,9 @@ import numpy as np
 from astropy import units as u
 from astropy.table import Table
 
+from ..core import Provenance, Tool
+from ..core.traits import Dict, Path, Unicode
 from ..io import EventSource
-from ..core import Provenance, ToolConfigurationError
-from ..core.traits import Unicode, Dict, Bool, Path, flag
-from ..core import Tool
 
 MAX_TELS = 1000
 
@@ -23,42 +22,30 @@ class DumpTriggersTool(Tool):
     # configuration parameters:
     # =============================================
 
-    infile = Path(exists=True, directory_ok=False, help="input simtelarray file").tag(
-        config=True
-    )
+    input_path = Path(
+        exists=True, directory_ok=False, help="input simtelarray file", allow_none=False
+    ).tag(config=True)
 
-    outfile = Path(
+    output_path = Path(
         default_value="triggers.fits",
         directory_ok=False,
         help="output filename (*.fits, *.h5)",
     ).tag(config=True)
 
-    overwrite = Bool(False, help="overwrite existing output file").tag(config=True)
-
     # =============================================
     # map low-level options to high-level command-line options
     # =============================================
 
     aliases = Dict(
-        {"infile": "DumpTriggersTool.infile", "outfile": "DumpTriggersTool.outfile"}
+        {
+            "input": "DumpTriggersTool.input_path",
+            "output": "DumpTriggersTool.output_path",
+        }
     )
 
-    flags = {
-        "f": (
-            {"DumpTriggersTool": {"overwrite": True}},
-            "Enable overwriting of output file",
-        ),
-        **flag(
-            "overwrite"
-            "DumpTriggersTool.overwrite"
-            "Enable overwriting of output file.",
-            "Disable overwriting of output file.",
-        ),
-    }
-
     examples = (
-        "ctapipe-dump-triggers --infile gamma.simtel.gz "
-        "--outfile trig.fits --overwrite"
+        "ctapipe-dump-triggers --input gamma.simtel.gz "
+        "--output trig.fits --overwrite"
         "\n\n"
         "If you want to see more output, use --log_level=DEBUG"
     )
@@ -103,11 +90,9 @@ class DumpTriggersTool(Tool):
         )
 
     def setup(self):
-        """ setup function, called before `start()` """
-
-        if self.infile == "":
-            raise ToolConfigurationError("No 'infile' parameter was specified. ")
+        """setup function, called before `start()`"""
 
+        self.check_output(self.output_path)
         self.events = Table(
             names=["EVENT_ID", "T_REL", "DELTA_T", "N_TRIG", "TRIGGERED_TELS"],
             dtype=[np.int64, np.float64, np.float64, np.int32, np.uint8],
@@ -117,15 +102,15 @@ class DumpTriggersTool(Tool):
         self.events["T_REL"].unit = u.s
         self.events["T_REL"].description = "Time relative to first event"
         self.events["DELTA_T"].unit = u.s
-        self.events.meta["INPUT"] = str(self.infile)
+        self.events.meta["INPUT"] = str(self.input_path)
 
         self._current_trigpattern = np.zeros(MAX_TELS)
         self._current_starttime = None
         self._prev_time = None
 
     def start(self):
-        """ main event loop """
-        with EventSource(self.infile) as source:
+        """main event loop"""
+        with EventSource(self.input_path) as source:
             for event in source:
                 self.add_event_to_table(event)
 
@@ -136,16 +121,16 @@ class DumpTriggersTool(Tool):
         """
         # write out the final table
         try:
-            if ".fits" in self.outfile.suffixes:
-                self.events.write(self.outfile, overwrite=self.overwrite)
-            elif self.outfile.suffix in (".hdf5", ".h5", ".hdf"):
+            if ".fits" in self.output_path.suffixes:
+                self.events.write(self.output_path, overwrite=self.overwrite)
+            elif self.output_path.suffix in (".hdf5", ".h5", ".hdf"):
                 self.events.write(
-                    self.outfile, path="/events", overwrite=self.overwrite
+                    self.output_path, path="/events", overwrite=self.overwrite
                 )
             else:
-                self.events.write(self.outfile)
+                self.events.write(self.output_path)
 
-            Provenance().add_output_file(self.outfile)
+            Provenance().add_output_file(self.output_path)
         except IOError as err:
             self.log.warning("Couldn't write output (%s)", err)
 
diff --git a/ctapipe/tools/merge.py b/ctapipe/tools/merge.py
index 9170171c..7fd6d1fd 100644
--- a/ctapipe/tools/merge.py
+++ b/ctapipe/tools/merge.py
@@ -129,7 +129,7 @@ class MergeTool(Tool):
         help="Input dl1-files",
     ).tag(config=True)
     output_path = traits.Path(
-        help="Merged-DL1 output filename", directory_ok=False
+        help="Merged-DL1 output filename", directory_ok=False, allow_none=False
     ).tag(config=True)
     skip_images = Bool(
         help="Skip DL1/Event/Telescope and Simulation/Event/Telescope images in output",
@@ -146,9 +146,6 @@ class MergeTool(Tool):
     skip_broken_files = Bool(
         help="Skip broken files instead of raising an error", default_value=False
     ).tag(config=True)
-    overwrite = Bool(
-        help="Overwrite output file if it exists", default_value=False
-    ).tag(config=True)
     progress_bar = Bool(
         help="Show progress bar during processing", default_value=False
     ).tag(config=True)
@@ -177,13 +174,6 @@ class MergeTool(Tool):
     }
 
     flags = {
-        "f": ({"MergeTool": {"overwrite": True}}, "Overwrite output file if it exists"),
-        **flag(
-            "overwrite",
-            "MergeTool.overwrite",
-            "Overwrite output file if it exists",
-            "Don't overwrite output file if it exists",
-        ),
         "progress": (
             {"MergeTool": {"progress_bar": True}},
             "Show a progress bar for all given input files",
@@ -221,16 +211,7 @@ class MergeTool(Tool):
             sys.exit(1)
 
         self.output_path = self.output_path.expanduser()
-        if self.output_path.exists():
-            if self.overwrite:
-                self.log.warning(f"Overwriting {self.output_path}")
-                self.output_path.unlink()
-            else:
-                self.log.critical(
-                    f"Output file {self.output_path} exists, "
-                    "use `--overwrite` to overwrite"
-                )
-                sys.exit(1)
+        self.check_output(self.output_path)
 
         PROV.add_output_file(str(self.output_path))
 
diff --git a/ctapipe/tools/muon_reconstruction.py b/ctapipe/tools/muon_reconstruction.py
index b61617a5..f9666226 100644
--- a/ctapipe/tools/muon_reconstruction.py
+++ b/ctapipe/tools/muon_reconstruction.py
@@ -14,8 +14,7 @@ from ctapipe.image.muon import (
 from ..calib import CameraCalibrator
 from ..containers import MuonParametersContainer, TelEventIndexContainer
 from ..coordinates import CameraFrame, TelescopeFrame
-from ..core import Provenance, Tool, ToolConfigurationError, traits
-from ..core.traits import flag
+from ..core import Provenance, Tool, traits
 from ..image.cleaning import TailcutsImageCleaner
 from ..instrument import CameraGeometry
 from ..io import EventSource, HDF5TableWriter
@@ -33,9 +32,9 @@ class MuonAnalysis(Tool):
     name = "ctapipe-reconstruct-muons"
     description = traits.Unicode(__doc__)
 
-    output = traits.Path(directory_ok=False, help="HDF5 output file name").tag(
-        config=True
-    )
+    output = traits.Path(
+        directory_ok=False, allow_none=False, help="HDF5 output file name"
+    ).tag(config=True)
 
     completeness_threshold = traits.FloatTelescopeParameter(
         default_value=30.0, help="Threshold for calculating the ``ring_completeness``"
@@ -49,10 +48,6 @@ class MuonAnalysis(Tool):
         ),
     ).tag(config=True)
 
-    overwrite = traits.Bool(
-        default_value=False, help="If true, overwrite outputfile without asking"
-    ).tag(config=True)
-
     min_pixels = traits.IntTelescopeParameter(
         help=(
             "Minimum number of pixels after cleaning and ring finding"
@@ -80,25 +75,8 @@ class MuonAnalysis(Tool):
         ("t", "allowed-tels"): "EventSource.allowed_tels",
     }
 
-    flags = {
-        "f": ({"MuonAnalysis": {"overwrite": True}}, "Overwrite output file"),
-        **flag(
-            "overwrite",
-            "MuonAnalysis.overwrite",
-            "Overwrite output file",
-            "Don't overwrite output file",
-        ),
-    }
-
     def setup(self):
-        if self.output is None:
-            raise ToolConfigurationError("You need to provide an --output file")
-
-        if self.output.exists() and not self.overwrite:
-            raise ToolConfigurationError(
-                "Outputfile {self.output} already exists, use `--overwrite` to overwrite"
-            )
-
+        self.check_output(self.output)
         self.source = EventSource(parent=self)
         subarray = self.source.subarray
 
diff --git a/ctapipe/tools/process.py b/ctapipe/tools/process.py
index 5ceaa584..da86c012 100644
--- a/ctapipe/tools/process.py
+++ b/ctapipe/tools/process.py
@@ -81,16 +81,10 @@ class ProcessorTool(Tool):
     }
 
     flags = {
-        "f": (
+        "overwrite": (
             {"DataWriter": {"overwrite": True}},
             "Overwrite output file if it exists",
         ),
-        **flag(
-            "overwrite",
-            "DataWriter.overwrite",
-            "Overwrite output file if it exists",
-            "Don't overwrite output file if it exists",
-        ),
         **flag(
             "progress",
             "ProcessorTool.progress_bar",
diff --git a/ctapipe/tools/train_disp_reconstructor.py b/ctapipe/tools/train_disp_reconstructor.py
index 10ffa7c6..36eb8099 100644
--- a/ctapipe/tools/train_disp_reconstructor.py
+++ b/ctapipe/tools/train_disp_reconstructor.py
@@ -2,7 +2,7 @@ import astropy.units as u
 import numpy as np
 
 from ctapipe.core import Tool
-from ctapipe.core.traits import Bool, Int, IntTelescopeParameter, Path, TraitError, flag
+from ctapipe.core.traits import Bool, Int, IntTelescopeParameter, Path
 from ctapipe.io import TableLoader
 from ctapipe.reco import CrossValidator, DispReconstructor
 from ctapipe.reco.preprocessing import check_valid_rows, horizontal_to_telescope
@@ -46,8 +46,6 @@ class TrainDispReconstructor(Tool):
         ),
     ).tag(config=True)
 
-    overwrite = Bool(help="overwrite existing output files").tag(config=True)
-
     random_seed = Int(
         default_value=0, help="Random seed for sampling and cross validation"
     ).tag(config=True)
@@ -63,11 +61,12 @@ class TrainDispReconstructor(Tool):
     ).tag(config=True)
 
     flags = {
-        **flag(
-            "overwrite",
-            "TrainDispReconstructor.overwrite",
-            "Overwrite output existing output files",
-            "Don't overwrite existing output files",
+        "overwrite": (
+            {
+                "TrainDispReconstructor": {"overwrite": True},
+                "CrossValidator": {"overwrite": True},
+            },
+            "Overwrite existing output",
         ),
     }
 
@@ -98,17 +97,7 @@ class TrainDispReconstructor(Tool):
         self.models = DispReconstructor(self.loader.subarray, parent=self)
         self.cross_validate = CrossValidator(parent=self, model_component=self.models)
         self.rng = np.random.default_rng(self.random_seed)
-
-        if self.output_path.suffix != ".pkl":
-            self.log.warning(
-                "Expected .pkl extension for output_path, got %s",
-                self.output_path.suffix,
-            )
-
-        if self.output_path.exists() and not self.overwrite:
-            raise TraitError(
-                f"output_path '{self.output_path}' exists and overwrite=False"
-            )
+        self.check_output(self.output_path, self.cross_validate.output_path)
 
     def start(self):
         """
diff --git a/ctapipe/tools/train_energy_regressor.py b/ctapipe/tools/train_energy_regressor.py
index a4ca36a5..35aa7ade 100644
--- a/ctapipe/tools/train_energy_regressor.py
+++ b/ctapipe/tools/train_energy_regressor.py
@@ -4,7 +4,7 @@ Tool for training the EnergyRegressor
 import numpy as np
 
 from ctapipe.core import Tool
-from ctapipe.core.traits import Bool, Int, IntTelescopeParameter, Path, TraitError, flag
+from ctapipe.core.traits import Int, IntTelescopeParameter, Path
 from ctapipe.io import TableLoader
 from ctapipe.reco import CrossValidator, EnergyRegressor
 from ctapipe.reco.preprocessing import check_valid_rows
@@ -52,18 +52,17 @@ class TrainEnergyRegressor(Tool):
         ),
     ).tag(config=True)
 
-    overwrite = Bool(help="overwrite existing output files").tag(config=True)
-
     random_seed = Int(
         default_value=0, help="Random seed for sampling and cross validation"
     ).tag(config=True)
 
     flags = {
-        **flag(
-            "overwrite",
-            "TrainEnergyRegressor.overwrite",
-            "Overwrite output existing output files",
-            "Don't overwrite existing output files",
+        "overwrite": (
+            {
+                "TrainEnergyReconstructor": {"overwrite": True},
+                "CrossValidator": {"overwrite": True},
+            },
+            "Overwrite existing output",
         ),
     }
 
@@ -98,18 +97,7 @@ class TrainEnergyRegressor(Tool):
         self.cross_validate = CrossValidator(
             parent=self, model_component=self.regressor
         )
-        self.rng = np.random.default_rng(self.random_seed)
-
-        if self.output_path.suffix != ".pkl":
-            self.log.warning(
-                "Expected .pkl extension for output_path, got %s",
-                self.output_path.suffix,
-            )
-
-        if self.output_path.exists() and not self.overwrite:
-            raise TraitError(
-                f"output_path '{self.output_path}' exists and overwrite=False"
-            )
+        self.check_output(self.output_path, self.cross_validate.output_path)
 
     def start(self):
         """
diff --git a/ctapipe/tools/train_particle_classifier.py b/ctapipe/tools/train_particle_classifier.py
index 46f73dd8..95d905e9 100644
--- a/ctapipe/tools/train_particle_classifier.py
+++ b/ctapipe/tools/train_particle_classifier.py
@@ -5,7 +5,7 @@ import numpy as np
 from astropy.table import vstack
 
 from ctapipe.core.tool import Tool
-from ctapipe.core.traits import Bool, Int, IntTelescopeParameter, Path, TraitError, flag
+from ctapipe.core.traits import Int, IntTelescopeParameter, Path
 from ctapipe.io import TableLoader
 from ctapipe.reco import CrossValidator, ParticleClassifier
 from ctapipe.reco.preprocessing import check_valid_rows
@@ -77,19 +77,18 @@ class TrainParticleClassifier(Tool):
         ),
     ).tag(config=True)
 
-    overwrite = Bool(help="overwrite existing output files").tag(config=True)
-
     random_seed = Int(
         default_value=0,
         help="Random number seed for sampling and the cross validation splitting",
     ).tag(config=True)
 
     flags = {
-        **flag(
-            "overwrite",
-            "TrainParticleClassifier.overwrite",
-            "Overwrite output existing output files",
-            "Don't overwrite existing output files",
+        "overwrite": (
+            {
+                "TrainParticleClassifier": {"overwrite": True},
+                "CrossValidator": {"overwrite": True},
+            },
+            "Overwrite existing output",
         ),
     }
 
@@ -147,17 +146,7 @@ class TrainParticleClassifier(Tool):
         self.cross_validate = CrossValidator(
             parent=self, model_component=self.classifier
         )
-
-        if self.output_path.suffix != ".pkl":
-            self.log.warning(
-                "Expected .pkl extension for output_path, got %s",
-                self.output_path.suffix,
-            )
-
-        if self.output_path.exists() and not self.overwrite:
-            raise TraitError(
-                f"output_path '{self.output_path}' exists and overwrite=False"
-            )
+        self.check_output(self.output_path, self.cross_validate.output_path)
 
     def start(self):
         """
diff --git a/docs/changes/2213.api.rst b/docs/changes/2213.api.rst
new file mode 100644
index 00000000..181fbd26
--- /dev/null
+++ b/docs/changes/2213.api.rst
@@ -0,0 +1,1 @@
+Remove ``-f`` flag as alias for ``--overwrite`` and fail early if output exists, but overwrite is not set


Test patch:
diff --git a/ctapipe/core/tests/test_tool.py b/ctapipe/core/tests/test_tool.py
index f2a25095..8db62479 100644
--- a/ctapipe/core/tests/test_tool.py
+++ b/ctapipe/core/tests/test_tool.py
@@ -329,6 +329,20 @@ def test_tool_logging_quiet(capsys):
     assert len(log) == 0
 
 
+def test_tool_overwrite_output(capsys, tmp_path):
+    path = tmp_path / "overwrite_dummy"
+    tool = Tool()
+    # path does not exist
+    tool.check_output(path)
+    # path exists and no overwrite
+    path.touch()
+    with pytest.raises(ToolConfigurationError):
+        tool.check_output(path)
+    # path exists and overwrite is True
+    tool.overwrite = True
+    tool.check_output(path)
+
+
 def test_invalid_traits(tmp_path, caplog):
     caplog.set_level(logging.INFO, logger="ctapipe")
 
diff --git a/ctapipe/tools/tests/test_tools.py b/ctapipe/tools/tests/test_tools.py
index 7d84f8fe..747e3d78 100644
--- a/ctapipe/tools/tests/test_tools.py
+++ b/ctapipe/tools/tests/test_tools.py
@@ -138,12 +138,12 @@ def test_dump_triggers(tmp_path):
     from ctapipe.tools.dump_triggers import DumpTriggersTool
 
     sys.argv = ["dump_triggers"]
-    outfile = tmp_path / "triggers.fits"
-    tool = DumpTriggersTool(infile=PROD5B_PATH, outfile=str(outfile))
+    output_path = tmp_path / "triggers.fits"
+    tool = DumpTriggersTool(input_path=PROD5B_PATH, output_path=str(output_path))
 
     assert run_tool(tool, cwd=tmp_path) == 0
 
-    assert outfile.exists()
+    assert output_path.exists()
     assert run_tool(tool, ["--help-all"]) == 0