Repository: cta-observatory/ctapipe
Pull request #2688 (2025-01-21 14:35:17)

Make sure config is fully serialized to provenance json

maxnoe: With the changes made here, we are no down to the quantity.

I can test if storing the string works now.

Concerning the enums, it might be better to not rely on the implicit `IntEnum` or `StrEnum` but add an explicit case that stores `<Enum>.name` into the json file.
kosack: > can test if storing the string works now.

What is allowed if these are specified on the command-line or in a YAML config? I guess it's just the string?  There would in any case only be two options: a single string `"option"="12 TeV"` or separating it into value and unit, e.g. `"option"={value="12",unit="TeV"}`
ctao-dpps-sonarqube[bot]: ![Passed](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/QualityGateBadge/passed.svg?sanitize=true)

# Analysis Details
## 0 Issues
- ![Bug](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/bug.svg?sanitize=true) 0 Bugs
- ![Vulnerability](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/vulnerability.svg?sanitize=true) 0 Vulnerabilities
- ![Code Smell](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/common/code_smell.svg?sanitize=true) 0 Code Smells

## Coverage and Duplications
- ![Coverage](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/CoverageChart/90.svg?sanitize=true) 97.80% Coverage (94.20% Estimated after merge)
- ![Duplications](https://sonar-cta-dpps.zeuthen.desy.de/static/communityBranchPlugin/checks/Duplications/3.svg?sanitize=true) 0.00% Duplicated Code (0.70% Estimated after merge)

**Project ID:** cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

[View in SonarQube](https://sonar-cta-dpps.zeuthen.desy.de/dashboard?id=cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs&pullRequest=2688)



Patch:
diff --git a/docs/changes/2688.bugfix.rst b/docs/changes/2688.bugfix.rst
new file mode 100644
index 00000000..70b19144
--- /dev/null
+++ b/docs/changes/2688.bugfix.rst
@@ -0,0 +1,3 @@
+Make sure that the configuration stored in the provenance
+information actually contains the full configuration
+and can be used to re-create the application.
diff --git a/src/ctapipe/core/provenance.py b/src/ctapipe/core/provenance.py
index 71835b70..519ad843 100644
--- a/src/ctapipe/core/provenance.py
+++ b/src/ctapipe/core/provenance.py
@@ -14,6 +14,7 @@ import uuid
 import warnings
 from collections import UserList
 from contextlib import contextmanager
+from enum import Enum
 from functools import cache
 from importlib import import_module
 from importlib.metadata import Distribution, distributions
@@ -21,6 +22,7 @@ from os.path import abspath
 from pathlib import Path
 from types import ModuleType
 
+import astropy.units as u
 import psutil
 from astropy.time import Time
 
@@ -29,7 +31,10 @@ from .support import Singleton
 
 log = logging.getLogger(__name__)
 
-__all__ = ["Provenance"]
+__all__ = [
+    "Provenance",
+    "json_config_handler",
+]
 
 _interesting_env_vars = [
     "CONDA_DEFAULT_ENV",
@@ -47,6 +52,47 @@ _interesting_env_vars = [
 ]
 
 
+def json_config_handler(obj):
+    """
+    Handle ctapipe configuration objects for json serialization.
+
+    Example
+    -------
+    >>> config = {"quantity": 5 * u.m}
+    >>> json.dumps(config, default=json_config_handler)
+    '{"quantity": {"value": 5.0, "unit": "m"}}'
+    """
+    from ctapipe.io.metadata import Contact, Instrument, Reference, _to_dict
+
+    if isinstance(obj, (set, UserList)):
+        return list(obj)
+
+    if isinstance(obj, Enum):
+        return obj.value
+
+    if isinstance(obj, Path):
+        return str(obj)
+
+    if isinstance(obj, Reference):
+        return obj.to_dict()
+
+    if isinstance(obj, Time):
+        # Time(iso_string) defaults to UTC, so we need to convert everything to
+        # UTC here.
+        return obj.utc.isot
+
+    if isinstance(obj, u.Quantity):
+        return {
+            "value": obj.value.tolist(),
+            "unit": obj.unit.to_string("vounit"),
+        }
+
+    if isinstance(obj, (Contact, Instrument)):
+        return _to_dict(obj)
+
+    raise TypeError(f"{obj!r} cannot be serialized to json")
+
+
 @cache
 def modules_of_distribution(distribution: Distribution):
     modules = distribution.read_text("top_level.txt")
@@ -370,18 +416,9 @@ class Provenance(metaclass=Singleton):
 
     def as_json(self, **kwargs):
         """return all finished provenance as JSON.  Kwargs for `json.dumps`
-        may be included, e.g. ``indent=4``"""
-
-        def set_default(obj):
-            """handle sets (not part of JSON) by converting to list"""
-            if isinstance(obj, set):
-                return list(obj)
-            if isinstance(obj, UserList):
-                return list(obj)
-            if isinstance(obj, Path):
-                return str(obj)
-
-        return json.dumps(self.provenance, default=set_default, **kwargs)
+        may be included, e.g. ``indent=4``
+        """
+        return json.dumps(self.provenance, default=json_config_handler, **kwargs)
 
     @property
     def active_activity_names(self):
diff --git a/src/ctapipe/core/traits.py b/src/ctapipe/core/traits.py
index b787c3fa..35d9e04a 100644
--- a/src/ctapipe/core/traits.py
+++ b/src/ctapipe/core/traits.py
@@ -3,6 +3,7 @@ Traitlet implementations for ctapipe
 """
 import os
 import pathlib
+from collections.abc import Mapping
 from urllib.parse import urlparse
 
 import astropy.units as u
@@ -120,7 +121,10 @@ class AstroQuantity(TraitType):
 
     def validate(self, obj, value):
         try:
-            quantity = u.Quantity(value)
+            if isinstance(value, Mapping):
+                quantity = u.Quantity(**value)
+            else:
+                quantity = u.Quantity(value)
         except TypeError:
             self.error(obj, value)
         except ValueError:
@@ -178,7 +182,9 @@ class Path(TraitType):
         file_ok=True,
         **kwargs,
     ):
-        super().__init__(default_value=default_value, **kwargs)
+        if default_value is not Undefined:
+            kwargs["default_value"] = default_value
+        super().__init__(**kwargs)
         self.exists = exists
         self.directory_ok = directory_ok
         self.file_ok = file_ok


Test patch:
diff --git a/src/ctapipe/core/tests/test_tool.py b/src/ctapipe/core/tests/test_tool.py
index de1bcb12..5e394118 100644
--- a/src/ctapipe/core/tests/test_tool.py
+++ b/src/ctapipe/core/tests/test_tool.py
@@ -7,11 +7,13 @@ import tempfile
 from multiprocessing import Barrier, Process
 from pathlib import Path
 
+import astropy.units as u
 import pytest
+from astropy.time import Time
 from traitlets import Dict, Float, Int, TraitError
 from traitlets.config import Config
 
-from .. import Component, Tool
+from .. import Component, TelescopeComponent, Tool, traits
 from ..tool import (
     ToolConfigurationError,
     export_tool_config_to_commented_yaml,
@@ -474,18 +476,48 @@ def test_exit_stack():
     assert tool.manager.exit_called
 
 
-def test_activity(tmp_path):
+def test_activity(tmp_path, example_subarray):
     """check that the config is correctly in the provenance"""
 
+    class Comp(Component):
+        quantity = traits.AstroQuantity(u.m).tag(config=True)
+
+    class TelComp(TelescopeComponent):
+        energy = traits.TelescopeParameter(
+            traits.AstroQuantity(u.TeV),
+        ).tag(config=True)
+
     class MyTool(Tool):
         name = "test_prov_log"
         description = "test"
-        userparam = Float(5.0, help="parameter").tag(config=True)
 
-    tool = MyTool()
+        value = Float(5.0, help="parameter").tag(config=True)
+        path = traits.Path(default_value=None, allow_none=True).tag(config=True)
+        time = traits.AstroTime(default_value=None, allow_none=True).tag(config=True)
+
+        def setup(self):
+            self.comp = Comp(parent=self)
+            self.tel_comp = TelComp(parent=self, subarray=example_subarray)
 
+    tool = MyTool()
+    config = {
+        "MyTool": {
+            "value": 10.0,
+            "path": str(tmp_path / "foo.txt"),
+            "time": "2025-01-01T00:00:00",
+            "Comp": {
+                "quantity": {"value": 5.0, "unit": "m"},
+            },
+            "TelComp": {
+                "energy": [
+                    ("type", "*", {"value": 2.0, "unit": "TeV"}),
+                    ("id", 1, {"value": 1.0, "unit": "TeV"}),
+                ]
+            },
+        }
+    }
     config_path = tmp_path / "config.json"
-    config_path.write_text(json.dumps({"MyTool": {"userparam": 10.0}}))
+    config_path.write_text(json.dumps(config))
     provenance_path = tmp_path / "provenance.json"
 
     run_tool(
@@ -508,6 +540,22 @@ def test_activity(tmp_path):
     assert inputs[0]["role"] == "Tool Configuration"
     assert inputs[0]["url"] == str(config_path)
 
+    # check we re-create the same tool from the config stored in provenance
+    provenance_config = provlog["config"]
+    prov_config_path = tmp_path / "config_from_prov.json"
+    prov_config_path.write_text(json.dumps(provenance_config))
+
+    tool = MyTool()
+    tool.initialize(["-c", str(prov_config_path)])
+    tool.setup()
+    assert tool.value == 10.0
+    assert tool.path == tmp_path / "foo.txt"
+    assert tool.time == Time("2025-01-01T00:00:00")
+    assert tool.comp.quantity == 5 * u.m
+    assert tool.tel_comp.energy.tel[1] == 1 * u.TeV
+    assert tool.tel_comp.energy.tel[2] == 2 * u.TeV
+    assert tool.tel_comp.energy.tel[None] == 2 * u.TeV
+
 
 @pytest.mark.parametrize(
     ("exit_code", "expected_status"),
diff --git a/src/ctapipe/core/tests/test_traits.py b/src/ctapipe/core/tests/test_traits.py
index c84b0da1..77cfa838 100644
--- a/src/ctapipe/core/tests/test_traits.py
+++ b/src/ctapipe/core/tests/test_traits.py
@@ -5,8 +5,10 @@ from abc import ABCMeta, abstractmethod
 from subprocess import CalledProcessError
 from unittest import mock
 
+import astropy.units as u
 import pytest
 from traitlets import CaselessStrEnum, HasTraits, Int
+from traitlets.config import Config, json
 
 from ctapipe.core import Component, Tool, run_tool
 from ctapipe.core.traits import (
@@ -337,6 +339,39 @@ def test_quantity():
             )
 
 
+def test_quantity_from_config(tmp_path):
+    from ctapipe.core import Component, Tool
+    from ctapipe.core.traits import AstroQuantity
+
+    class QuantityComponent(Component):
+        distance = AstroQuantity(u.cm, default_value=1 * u.m).tag(config=True)
+
+    config = Config()
+    config.QuantityComponent.distance = "5 cm"
+    q = QuantityComponent(config=config)
+    assert q.distance.value == 5.0
+    assert q.distance.unit == u.cm
+
+    config = Config()
+    config.QuantityComponent.distance = dict(value=10, unit="m")
+    q = QuantityComponent(config=config)
+    assert q.distance.value == 10.0
+    assert q.distance.unit == u.m
+
+    class QTool(Tool):
+        distance = AstroQuantity(u.cm, default_value=1 * u.m).tag(config=True)
+
+    config_file = tmp_path / "config.json"
+    config_file.write_text(
+        json.dumps({"QTool": {"distance": {"value": 10.0, "unit": "cm"}}})
+    )
+    tool = QTool()
+    tool.config_files = [config_file]
+    tool.initialize([])
+    assert tool.distance.value == 10
+    assert tool.distance.unit == u.cm
+
+
 def test_quantity_tool(capsys):
     import astropy.units as u