diff --git a/doc/lsst.verify/index.rst b/doc/lsst.verify/index.rst index dcc63cd..bb331ca 100644 --- a/doc/lsst.verify/index.rst +++ b/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 `_ component. +You can find Jira issues for this module under the `verify `_ component. .. _lsst.verify-command-line-taskref: diff --git a/python/lsst/verify/tasks/apdbMetricTask.py b/python/lsst/verify/tasks/apdbMetricTask.py index 6537877..58b9178 100644 --- a/python/lsst/verify/tasks/apdbMetricTask.py +++ b/python/lsst/verify/tasks/apdbMetricTask.py @@ -23,8 +23,11 @@ "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 @@ -32,6 +35,9 @@ 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)) diff --git a/python/lsst/verify/tasks/testUtils.py b/python/lsst/verify/tasks/testUtils.py index 994f472..3b3dc7c 100644 --- a/python/lsst/verify/tasks/testUtils.py +++ b/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() diff --git a/tests/test_apdbMetricTask.py b/tests/test_apdbMetricTask.py index d254070..9afae9d 100644 --- a/tests/test_apdbMetricTask.py +++ b/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) diff --git a/tests/test_configApdbLoader.py b/tests/test_configApdbLoader.py index 7350912..aa0dd91 100644 --- a/tests/test_configApdbLoader.py +++ b/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)