From adf53e0e632e2edec08c3e7cbb7eae25ac2c3e23 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 4 Dec 2024 13:43:18 +0100 Subject: [PATCH 1/5] Fixed unknown configs being silently ignored --- src/ctapipe/core/tests/test_tool.py | 24 ++++++++++++++++++++++++ src/ctapipe/core/tool.py | 11 +++++++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/ctapipe/core/tests/test_tool.py b/src/ctapipe/core/tests/test_tool.py index 6a915cce935..d4afaf1291f 100644 --- a/src/ctapipe/core/tests/test_tool.py +++ b/src/ctapipe/core/tests/test_tool.py @@ -569,3 +569,27 @@ def test_exit_status_interrupted(tmp_path, provenance): provlog = activities[0] assert provlog["activity_name"] == InterruptTestTool.name assert provlog["status"] == "interrupted" + + +def test_no_ignore_bad_config_type(tmp_path: Path): + """Check that if an unknown type of config file is given, an error is raised + instead of silently ignoring the file (which is the default for + traitlets.config) + """ + + class SomeTool(Tool): + float_option = Float(1.0, help="An option to change") + + test_config_file = """ + SomeTool: + float_option: 200.0 + """ + + bad_conf_path = tmp_path / "test.conf" # note named "conf" not yaml. + bad_conf_path.write_text(test_config_file) + + tool = SomeTool() + + # here we should receive an error. + with pytest.raises(ToolConfigurationError): + tool.load_config_file(bad_conf_path) diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py index b46acac3321..b9763e5c36f 100644 --- a/src/ctapipe/core/tool.py +++ b/src/ctapipe/core/tool.py @@ -282,9 +282,16 @@ def load_config_file(self, path: str | pathlib.Path) -> None: with open(path, "rb") as infile: config = Config(toml.load(infile)) self.update_config(config) - else: - # fall back to traitlets.config.Application's implementation + elif path.suffix in ["json", "py"]: + # fall back to traitlets.config.Application's implementation. Note + # that if we don't specify the file suffixes here, traitlets seems + # to silently ignore unknown ones. super().load_config_file(str(path)) + else: + raise ToolConfigurationError( + f"The config file '{path}' is not in a known format. " + "The file should end in one: yml, yaml, toml, json, py" + ) Provenance().add_input_file(path, role="Tool Configuration", add_meta=False) From 930cd57200abdf221afb088975c31358bd18f12a Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 4 Dec 2024 13:49:09 +0100 Subject: [PATCH 2/5] add changelog entry --- docs/changes/2666.bugfix.rst | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changes/2666.bugfix.rst diff --git a/docs/changes/2666.bugfix.rst b/docs/changes/2666.bugfix.rst new file mode 100644 index 00000000000..47cf1fdc72c --- /dev/null +++ b/docs/changes/2666.bugfix.rst @@ -0,0 +1,6 @@ +Fixed a bug where if a configuration file with unknown file extension was passed +to a tool, e.g. ``--config myconf.conf`` instead of ``--config myconf.yaml``, it +was silently ignored, despite an info log saying "Loading config file +myconf.conf". Configuration files must now have one of the following extensions +to be recognized: yml, yaml, toml, json, py. If not a ``ToolConfigurationError`` +is raised. From 6f83bdde8dea466ccbc7bace4a5301a99e75c27d Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 4 Dec 2024 13:51:45 +0100 Subject: [PATCH 3/5] added missing config=true --- src/ctapipe/core/tests/test_tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctapipe/core/tests/test_tool.py b/src/ctapipe/core/tests/test_tool.py index d4afaf1291f..d25e1d716bc 100644 --- a/src/ctapipe/core/tests/test_tool.py +++ b/src/ctapipe/core/tests/test_tool.py @@ -578,7 +578,7 @@ def test_no_ignore_bad_config_type(tmp_path: Path): """ class SomeTool(Tool): - float_option = Float(1.0, help="An option to change") + float_option = Float(1.0, help="An option to change").tag(config=True) test_config_file = """ SomeTool: From a1c69a745ebc2bf1e8a3f6e8e006eb32fe08de03 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 4 Dec 2024 13:54:04 +0100 Subject: [PATCH 4/5] added positive test just in case --- src/ctapipe/core/tests/test_tool.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/ctapipe/core/tests/test_tool.py b/src/ctapipe/core/tests/test_tool.py index d25e1d716bc..d431304814f 100644 --- a/src/ctapipe/core/tests/test_tool.py +++ b/src/ctapipe/core/tests/test_tool.py @@ -588,8 +588,15 @@ class SomeTool(Tool): bad_conf_path = tmp_path / "test.conf" # note named "conf" not yaml. bad_conf_path.write_text(test_config_file) + good_conf_path = tmp_path / "test.yaml" + good_conf_path.write_text(test_config_file) + tool = SomeTool() # here we should receive an error. with pytest.raises(ToolConfigurationError): tool.load_config_file(bad_conf_path) + + # test correct case: + tool.load_config_file(good_conf_path) + assert tool.float_option > 1 From 491c94d6e096e1693f58551fa580ab3a50f0fd67 Mon Sep 17 00:00:00 2001 From: Karl Kosack Date: Wed, 4 Dec 2024 14:22:00 +0100 Subject: [PATCH 5/5] suffixes need to include the "." --- src/ctapipe/core/tool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctapipe/core/tool.py b/src/ctapipe/core/tool.py index b9763e5c36f..488dd1242b2 100644 --- a/src/ctapipe/core/tool.py +++ b/src/ctapipe/core/tool.py @@ -282,7 +282,7 @@ def load_config_file(self, path: str | pathlib.Path) -> None: with open(path, "rb") as infile: config = Config(toml.load(infile)) self.update_config(config) - elif path.suffix in ["json", "py"]: + elif path.suffix in [".json", ".py"]: # fall back to traitlets.config.Application's implementation. Note # that if we don't specify the file suffixes here, traitlets seems # to silently ignore unknown ones.