DM-43416: Migrate AP code to external APDB configs

doc/lsst.verify/index.rst

@@ -30,7 +30,7 @@ Contributing
 ============
 
 ``lsst.verify`` is developed at https://github.com/lsst/verify.
-You can find Jira issues for this module under the `verify <https://jira.lsstcorp.org/issues/?jql=project%20%3D%20DM%20AND%20component%20%3D%20verify>`_ component.
+You can find Jira issues for this module under the `verify <https://rubinobs.atlassian.net/issues/?jql=project%20%3D%20DM%20AND%20component%20%3D%20verify>`_ component.
 
 .. _lsst.verify-command-line-taskref:
 

python/lsst/verify/tasks/apdbMetricTask.py

@@ -23,15 +23,21 @@
            "DirectApdbLoader", "ApdbMetricConnections"]
 
 import abc
+import warnings
 
-from lsst.pex.config import Config, ConfigurableField, ConfigurableInstance, \
+from deprecated.sphinx import deprecated
+
+from lsst.pex.config import Config, ConfigurableField, Field, ConfigurableInstance, \
     ConfigDictField, ConfigChoiceField, FieldValidationError
 from lsst.pipe.base import NoWorkFound, Task, Struct, connectionTypes
 from lsst.dax.apdb import Apdb, ApdbConfig
 
 from lsst.verify.tasks import MetricTask, MetricConfig, MetricConnections
 
 
+@deprecated(reason="APDB loaders have been replaced by ``ApdbMetricConfig.apdb_config_url``. "
+                   "Will be removed after v28.",
+            version="v28.0", category=FutureWarning)
 class ConfigApdbLoader(Task):
     """A Task that takes a science task config and returns the corresponding
     Apdb object.
@@ -151,6 +157,7 @@ def run(self, config):
         return Struct(apdb=self._getApdb(config))
 
 
+# TODO: remove on DM-43419
 class DirectApdbLoader(Task):
     """A Task that takes a Apdb config and returns the corresponding
     Apdb object.
@@ -206,9 +213,9 @@ class ApdbMetricConnections(
     """
     dbInfo = connectionTypes.Input(
         name="apdb_marker",
-        doc="The dataset from which an APDB instance can be constructed by "
-            "`dbLoader`. By default this is assumed to be a marker produced "
-            "by AP processing.",
+        doc="The dataset(s) indicating that AP processing has finished for a "
+            "given data ID. If ``config.doReadMarker`` is set, the datasets "
+            "are also used by ``dbLoader`` to construct an Apdb object.",
         storageClass="Config",
         multiple=True,
         minimum=1,
@@ -227,13 +234,55 @@ class ApdbMetricConfig(MetricConfig,
                        pipelineConnections=ApdbMetricConnections):
     """A base class for APDB metric task configs.
     """
-    dbLoader = ConfigurableField(
+    dbLoader = ConfigurableField(  # TODO: remove on DM-43419
         target=DirectApdbLoader,
-        doc="Task for loading a database from `dbInfo`. Its run method must "
-        "take one object of the dataset type indicated by `dbInfo` and return "
-        "a Struct with an 'apdb' member."
+        doc="Task for loading a database from ``dbInfo``. Its run method must "
+        "take one object of the dataset type indicated by ``dbInfo`` and return "
+        "a Struct with an 'apdb' member. Ignored if ``doReadMarker`` is unset.",
+        deprecated="This field has been replaced by ``apdb_config_url``; set "
+                   "``doReadMarker=False`` to use it. Will be removed after v28.",
+    )
+    apdb_config_url = Field(
+        dtype=str,
+        default=None,
+        optional=False,
+        doc="A config file specifying the APDB and its connection parameters, "
+            "typically written by the apdb-cli command-line utility.",
+    )
+    doReadMarker = Field(  # TODO: remove on DM-43419
+        dtype=bool,
+        default=True,
+        doc="Use the ``dbInfo`` input to set up the APDB, instead of the new "
+            "config (``apdb_config_url``). This field is provided for "
+            "backward-compatibility ONLY and will be removed without notice "
+            "after v28.",
     )
 
+    # TODO: remove on DM-43419
+    def validate(self):
+        # Sidestep Config.validate to avoid validating uninitialized
+        # fields we're not using.
+        skip = {"apdb_config_url"} if self.doReadMarker else set()
+        for name, field in self._fields.items():
+            if name not in skip:
+                field.validate(self)
+
+        # Copied from MetricConfig.validate
+        if "." in self.connections.package:
+            raise ValueError(f"package name {self.connections.package} must "
+                             "not contain periods")
+        if "." in self.connections.metric:
+            raise ValueError(f"metric name {self.connections.metric} must "
+                             "not contain periods; use connections.package "
+                             "instead")
+
+        if self.doReadMarker:
+            warnings.warn("The encoding of config information in apdbMarker is "
+                          "deprecated, replaced by ``apdb_config_url``; set "
+                          "``doReadMarker=False`` to use it. ``apdb_config_url`` "
+                          "will be required after v28.",
+                          FutureWarning)
+
 
 class ApdbMetricTask(MetricTask):
     """A base class for tasks that compute metrics from an alert production
@@ -261,7 +310,8 @@ class ApdbMetricTask(MetricTask):
     def __init__(self, **kwargs):
         super().__init__(**kwargs)
 
-        self.makeSubtask("dbLoader")
+        if self.config.doReadMarker:
+            self.makeSubtask("dbLoader")
 
     @abc.abstractmethod
     def makeMeasurement(self, dbHandle, outputDataId):
@@ -331,7 +381,10 @@ def run(self, dbInfo, outputDataId={}):
         ``outputDataId`` to `makeMeasurement`. The result of `makeMeasurement`
         is returned to the caller.
         """
-        db = self.dbLoader.run(dbInfo[0]).apdb
+        if self.config.doReadMarker:
+            db = self.dbLoader.run(dbInfo[0]).apdb
+        else:
+            db = Apdb.from_uri(self.config.apdb_config_url)
 
         if db is not None:
             return Struct(measurement=self.makeMeasurement(db, outputDataId))

python/lsst/verify/tasks/testUtils.py

@@ -27,6 +27,7 @@
 from unittest.mock import patch
 
 import lsst.utils.tests
+import lsst.pex.config as pexConfig
 from lsst.pipe.base import TaskMetadata
 from lsst.dax.apdb import ApdbConfig
 
@@ -216,3 +217,28 @@ def testPassThroughRun(self):
             info = self.makeDbInfo()
             with self.assertRaises(MetricComputationError):
                 self.task.run([info])
+
+    # TODO: remove on DM-43419
+    def testConfigApdbRead(self):
+        config = self.taskClass.ConfigClass()
+        with self.assertWarns(FutureWarning):
+            config.doReadMarker = True
+            config.freeze()
+            config.validate()
+
+    # TODO: remove on DM-43419
+    def testConfigApdbFileOk(self):
+        config = self.taskClass.ConfigClass()
+        config.doReadMarker = False
+        config.apdb_config_url = "some/file/path.yaml"
+        config.freeze()
+        config.validate()
+
+    # TODO: remove on DM-43419
+    def testConfigApdbFileInvalid(self):
+        config = self.taskClass.ConfigClass()
+        config.doReadMarker = False
+        # Don't set apdb_config_url
+        config.freeze()
+        with self.assertRaises(pexConfig.FieldValidationError):
+            config.validate()

tests/test_apdbMetricTask.py

@@ -27,8 +27,9 @@
 
 import lsst.utils.tests
 from lsst.pex.config import Config
+import lsst.dax.apdb as daxApdb
 import lsst.daf.butler.tests as butlerTests
-from lsst.pipe.base import Task, Struct, testUtils
+from lsst.pipe.base import Struct, testUtils
 
 from lsst.verify import Measurement
 from lsst.verify.tasks import ApdbMetricTask
@@ -51,14 +52,9 @@ def makeMeasurement(self, _dbHandle, outputDataId):
 class Gen3ApdbTestSuite(ApdbMetricTestCase):
     @classmethod
     def makeTask(cls):
-        class MockDbLoader(Task):
-            ConfigClass = Config
-
-            def run(self, _):
-                return Struct(apdb=unittest.mock.Mock())
-
         config = DummyTask.ConfigClass()
-        config.dbLoader.retarget(MockDbLoader)
+        config.doReadMarker = False
+        config.apdb_config_url = cls.config_file.name
         config.connections.package = "verify"
         config.connections.metric = "DummyApdb"
         return DummyTask(config=config)
@@ -67,12 +63,18 @@ def run(self, _):
     def setUpClass(cls):
         super().setUpClass()
 
+        apdb_config = daxApdb.ApdbSql.init_database(db_url="sqlite://")
+        cls.config_file = tempfile.NamedTemporaryFile()
+        cls.addClassCleanup(cls.config_file.close)
+        apdb_config.save(cls.config_file.name)
+
         cls.CAMERA_ID = "NotACam"
         cls.VISIT_ID = 42
         cls.CHIP_ID = 5
 
         # makeTestRepo called in setUpClass because it's *very* slow
         cls.root = tempfile.mkdtemp()
+        cls.addClassCleanup(shutil.rmtree, cls.root, ignore_errors=True)
         cls.repo = butlerTests.makeTestRepo(cls.root, {
             "instrument": [cls.CAMERA_ID],
             "visit": [cls.VISIT_ID],
@@ -94,11 +96,6 @@ def setUpClass(cls):
             connections.dbInfo.dimensions,
             connections.dbInfo.storageClass)
 
-    @classmethod
-    def tearDownClass(cls):
-        shutil.rmtree(cls.root, ignore_errors=True)
-        super().tearDownClass()
-
     def setUp(self):
         super().setUp()
 
@@ -116,9 +113,7 @@ def _prepareQuantum(self, task):
         }
 
         butler = butlerTests.makeTestCollection(self.repo, uniqueId=self.id())
-        # task.config not persistable if it refers to a local class
-        # We don't actually use the persisted config, so just make a new one
-        info = task.ConfigClass()
+        info = Config()
         butler.put(info, "apdb_marker", detectorId)
 
         quantum = testUtils.makeQuantum(
@@ -146,6 +141,8 @@ def run(self, *args, **kwargs):
                 return Struct(measurement=None)
 
         config = NoneTask.ConfigClass()
+        config.doReadMarker = False
+        config.apdb_config_url = self.config_file.name
         config.connections.package = "verify"
         config.connections.metric = "DummyApdb"
         task = NoneTask(config=config)

tests/test_configApdbLoader.py

@@ -49,7 +49,8 @@ def _dummyApdbConfig(self):
     def setUp(self):
         self.tempdir = tempfile.mkdtemp()
         self.db_url = f"sqlite:///{self.tempdir}/apdb.sqlite3"
-        self.task = ConfigApdbLoader()
+        with self.assertWarns(FutureWarning):
+            self.task = ConfigApdbLoader()
 
     def tearDown(self):
         shutil.rmtree(self.tempdir, ignore_errors=True)