Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-43416: Migrate AP code to external APDB configs #224

Merged
merged 5 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/lsst.ap.verify/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Contributing
============

``lsst.ap.verify`` is developed at https://github.com/lsst/ap_verify.
You can find Jira issues for this module under the `ap_verify <https://jira.lsstcorp.org/issues/?jql=project%20%3D%20DM%20AND%20component%20%3D%20ap_verify>`_ component.
You can find Jira issues for this module under the `ap_verify <https://rubinobs.atlassian.net/issues/?jql=project%20%3D%20DM%20AND%20component%20%3D%20ap_verify>`_ component.

.. _lsst.ap.verify-pyapi:

Expand Down
5 changes: 5 additions & 0 deletions pipelines/DECam/ApVerify.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,8 @@ tasks:
config:
doPackageAlerts: True
alertPackager.doWriteAlerts: True
contracts:
# Contracts removed by excluding apPipe
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
3 changes: 3 additions & 0 deletions pipelines/DECam/ApVerifyCalibrate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ tasks:
contracts:
# Must re-declare contracts that cross apPipe and metrics boundary, as
# these were removed on import.
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
# Use of ConnectionsClass for templated fields is a workaround for DM-30210
- detectAndMeasure.connections.ConnectionsClass(config=detectAndMeasure).diaSources.name ==
fracDiaSourcesToSciSources.connections.ConnectionsClass(config=fracDiaSourcesToSciSources).diaSources.name
5 changes: 5 additions & 0 deletions pipelines/HSC/ApVerify.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,8 @@ tasks:
config:
doPackageAlerts: True
alertPackager.doWriteAlerts: True
contracts:
# Contracts removed by excluding apPipe
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
5 changes: 5 additions & 0 deletions pipelines/HSC/ApVerifyCalibrate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,8 @@ tasks:
class: lsst.ap.association.DiaPipelineTask
config:
doPackageAlerts: True
contracts:
# Contracts removed by excluding apPipe
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
5 changes: 5 additions & 0 deletions pipelines/LSSTCam-imSim/ApVerify.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,8 @@ tasks:
config:
doPackageAlerts: True
alertPackager.doWriteAlerts: True
contracts:
# Contracts removed by excluding apPipe
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
3 changes: 3 additions & 0 deletions pipelines/LSSTCam-imSim/ApVerifyCalibrate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ tasks:
contracts:
# Must re-declare contracts that cross apPipe and metrics boundary, as
# these were removed on import.
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
# Use of ConnectionsClass for templated fields is a workaround for DM-30210
- detectAndMeasure.connections.ConnectionsClass(config=detectAndMeasure).diaSources.name ==
fracDiaSourcesToSciSources.connections.ConnectionsClass(config=fracDiaSourcesToSciSources).diaSources.name
4 changes: 4 additions & 0 deletions pipelines/_ingredients/ApVerify.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,7 @@ tasks:
# we have an alternative.
doPackageAlerts: True
alertPackager.doWriteAlerts: True
contracts:
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
3 changes: 3 additions & 0 deletions pipelines/_ingredients/ApVerifyCalibrate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ tasks:
# we have an alternative.
doPackageAlerts: True
contracts:
- contract: diaPipe.doConfigureApdb or not totalUnassociatedDiaObjects.doReadMarker
msg: "totalUnassociatedDiaObjects.doReadMarker requires diaPipe.doConfigureApdb"
- (totalUnassociatedDiaObjects.doReadMarker) or (diaPipe.apdb_config_url == totalUnassociatedDiaObjects.apdb_config_url)
# Metric inputs must match pipeline outputs
# Use of ConnectionsClass for templated fields is a workaround for DM-30210
- detectAndMeasure.connections.ConnectionsClass(config=detectAndMeasure).diaSources.name ==
Expand Down
3 changes: 3 additions & 0 deletions pipelines/_ingredients/MetricsMisc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,6 @@ tasks:
connections.labelName: diaPipe
totalUnassociatedDiaObjects:
class: lsst.ap.association.metrics.TotalUnassociatedDiaObjectsMetricTask
config:
doReadMarker: False # Impossible if diaPipe uses new-style config
apdb_config_url: parameters.apdb_config
3 changes: 3 additions & 0 deletions pipelines/_ingredients/MetricsMiscCalibrate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ tasks:
connections.labelName: diaPipe
totalUnassociatedDiaObjects:
class: lsst.ap.association.metrics.TotalUnassociatedDiaObjectsMetricTask
config:
doReadMarker: False # Impossible if diaPipe uses new-style config
apdb_config_url: parameters.apdb_config
numSciSources:
class: lsst.ip.diffim.metrics.NumberSciSourcesMetricTask
fracDiaSourcesToSciSources:
Expand Down
43 changes: 27 additions & 16 deletions python/lsst/ap/verify/pipeline_driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@
import logging

import lsst.ctrl.mpexec.execFixupDataId # not part of lsst.ctrl.mpexec
import lsst.ctrl.mpexec.cli.pipetask
from lsst.ap.pipe.make_apdb import makeApdb
import lsst.dax.apdb as daxApdb

_LOG = logging.getLogger(__name__)

Expand All @@ -60,7 +59,7 @@ def __init__(self):
help="A custom version of the ap_verify pipeline (e.g., with different metrics). "
"Defaults to the ApVerify.yaml within --dataset.")
self.add_argument("--db", "--db_url", default=None,
help="A location for the AP database, formatted as if for ApdbConfig.db_url. "
help="A location for the AP database, formatted as if for apdb-cli create-sql. "
"Defaults to an SQLite file in the --output directory.")
self.add_argument("--skip-pipeline", action="store_true",
help="Do not run the AP pipeline itself. This argument is useful "
Expand Down Expand Up @@ -91,7 +90,7 @@ def runApPipeGen3(workspace, parsedCmdLine, processes=1):
"""
log = _LOG.getChild('runApPipeGen3')

makeApdb(_getApdbArguments(workspace, parsedCmdLine))
_makeApdb(workspace, _getApdbArguments(workspace, parsedCmdLine))

pipelineFile = _getPipelineFile(workspace, parsedCmdLine)
pipelineArgs = ["pipetask", "--long-log", "run",
Expand Down Expand Up @@ -197,8 +196,8 @@ def _getPipelineFile(workspace, parsed):


def _getApdbArguments(workspace, parsed):
"""Return the config options for running make_apdb.py on this workspace,
as command-line arguments.
"""Return the arguments for running apdb-cli create-sql on this workspace,
as key-value pairs.

Parameters
----------
Expand All @@ -210,14 +209,14 @@ def _getApdbArguments(workspace, parsed):

Returns
-------
args : `list` of `str`
Command-line arguments calling ``--config`` or ``--config-file``,
following the conventions of `sys.argv`.
args : mapping [`str`]
Arguments to `lsst.dax.apdb.sql.Apdb.init_database`.
"""
if not parsed.db:
parsed.db = "sqlite:///" + workspace.dbLocation

args = ["--config", "db_url=" + parsed.db]
args = {"db_url": parsed.db,
}

return args

Expand All @@ -239,14 +238,12 @@ def _getConfigArgumentsGen3(workspace, parsed):
Command-line arguments calling ``--config`` or ``--config-file``,
following the conventions of `sys.argv`.
"""
# Translate APDB-only arguments to work as a sub-config
args = [("diaPipe:apdb." + arg if arg != "--config" else arg)
for arg in _getApdbArguments(workspace, parsed)]
args.extend([
return [
# APDB config should have been stored in the workspace.
"--config", "parameters:apdb_config=" + workspace.dbConfigLocation,
# Put output alerts into the workspace.
"--config", "diaPipe:alertPackager.alertWriteLocation=" + workspace.alertLocation,
])
return args
]


def _getCollectionArguments(workspace, reuse):
Expand Down Expand Up @@ -284,3 +281,17 @@ def _getCollectionArguments(workspace, reuse):
if reuse and oldRuns:
args.extend(["--extend-run", "--skip-existing"])
return args


def _makeApdb(workspace, args):
"""Create an APDB and store its config for future use.

Parameters
----------
workspace : `lsst.ap.verify.workspace.Workspace`
A Workspace in which to store the database config.
args : mapping [`str`]
Arguments to `lsst.dax.apdb.sql.Apdb.init_database`.
"""
config = daxApdb.ApdbSql.init_database(**args)
config.save(workspace.dbConfigLocation)
13 changes: 13 additions & 0 deletions python/lsst/ap/verify/workspace.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,15 @@ def dbLocation(self):
Shall be a pathname to a database suitable for the backend of `Apdb`.
"""

@property
@abc.abstractmethod
def dbConfigLocation(self):
"""The absolute location of the config file for the source association
database to be created or updated by the pipeline (`str`, read-only).

The location is assumed to be a Python (`lsst.pex.config.Config`) file.
"""

@property
@abc.abstractmethod
def alertLocation(self):
Expand Down Expand Up @@ -189,6 +198,10 @@ def pipelineDir(self):
def dbLocation(self):
return os.path.join(self._location, 'association.db')

@property
def dbConfigLocation(self):
return os.path.join(self._location, 'apdb.py')

@property
def alertLocation(self):
return os.path.join(self._location, 'alerts')
Expand Down
3 changes: 3 additions & 0 deletions tests/MockApPipe.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ parameters:
coaddName: goodSeeing
# only refcat in ap_verify_testdata
refcat: gaia
apdb_config: dummy_path.yaml
tasks:
isr:
class: lsst.ap.verify.testPipeline.MockIsrTask
Expand Down Expand Up @@ -54,6 +55,8 @@ tasks:
class: lsst.ap.verify.testPipeline.MockDiaPipelineTask
config:
doWriteAssociatedSources: True
doConfigureApdb: False
apdb_config_url: parameters.apdb_config
connections.coaddName: parameters.coaddName
contracts:
# Inputs and outputs must match
Expand Down
36 changes: 13 additions & 23 deletions tests/test_driver.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def patchApPipeGen3(method):
"""
@functools.wraps(method)
def wrapper(self, *args, **kwargs):
dbPatcher = unittest.mock.patch("lsst.ap.verify.pipeline_driver.makeApdb")
dbPatcher = unittest.mock.patch("lsst.ap.verify.pipeline_driver._makeApdb")
patchedMethod = dbPatcher(method)
return patchedMethod(self, *args, **kwargs)
return wrapper
Expand Down Expand Up @@ -93,29 +93,24 @@ def testrunApPipeGen3Steps(self):
self.assertTrue(self.workspace.analysisButler.exists("apdb_marker", id))
self.assertTrue(self.workspace.analysisButler.exists("goodSeeingDiff_assocDiaSrc", id))

def _getCmdLineArgs(self, parseAndRunArgs):
if parseAndRunArgs[0]:
return parseAndRunArgs[0][0]
elif "args" in parseAndRunArgs[1]:
return parseAndRunArgs[1]["args"]
def _getArgs(self, call_args):
if call_args.args:
return call_args.args[1]
elif "args" in call_args.kwargs:
return call_args.kwargs["args"]
else:
self.fail("No command-line args passed to parseAndRun!")
self.fail(f"No APDB args passed to {call_args}!")

@patchApPipeGen3
def testrunApPipeGen3WorkspaceDb(self, mockDb):
"""Test that runApPipeGen3 places a database in the workspace location by default.
"""
pipeline_driver.runApPipeGen3(self.workspace, self.apPipeArgs)

# Test the call to make_apdb.py
mockDb.assert_called_once()
cmdLineArgs = self._getCmdLineArgs(mockDb.call_args)
self.assertIn("db_url=sqlite:///" + self.workspace.dbLocation, cmdLineArgs)

# Test the call to the AP pipeline
id = _getDataIds(self.workspace.analysisButler)[0]
apdbConfig = self.workspace.analysisButler.get("apdb_marker", id)
self.assertEqual(apdbConfig.db_url, "sqlite:///" + self.workspace.dbLocation)
dbArgs = self._getArgs(mockDb.call_args)
self.assertIn("db_url", dbArgs)
self.assertEqual(dbArgs["db_url"], "sqlite:///" + self.workspace.dbLocation)

@patchApPipeGen3
def testrunApPipeGen3WorkspaceCustom(self, mockDb):
Expand All @@ -124,15 +119,10 @@ def testrunApPipeGen3WorkspaceCustom(self, mockDb):
self.apPipeArgs.db = "postgresql://[email protected]/custom_db"
pipeline_driver.runApPipeGen3(self.workspace, self.apPipeArgs)

# Test the call to make_apdb.py
mockDb.assert_called_once()
cmdLineArgs = self._getCmdLineArgs(mockDb.call_args)
self.assertIn("db_url=" + self.apPipeArgs.db, cmdLineArgs)

# Test the call to the AP pipeline
id = _getDataIds(self.workspace.analysisButler)[0]
apdbConfig = self.workspace.analysisButler.get("apdb_marker", id)
self.assertEqual(apdbConfig.db_url, self.apPipeArgs.db)
dbArgs = self._getArgs(mockDb.call_args)
self.assertIn("db_url", dbArgs)
self.assertEqual(dbArgs["db_url"], self.apPipeArgs.db)

def testrunApPipeGen3Reuse(self):
"""Test that runApPipeGen3 does not run the pipeline at all (not even with
Expand Down
3 changes: 2 additions & 1 deletion tests/test_testPipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,8 @@ def testMockTransformDiaSourceCatalogTask(self):

def testMockDiaPipelineTask(self):
config = MockDiaPipelineTask.ConfigClass()
config.apdb.db_url = "testing_only"
config.doConfigureApdb = False
config.apdb_config_url = "testing_only"
task = MockDiaPipelineTask(config=config)
pipelineTests.assertValidInitOutput(task)
result = task.run(pandas.DataFrame(), pandas.DataFrame(), afwImage.ExposureF(),
Expand Down
9 changes: 9 additions & 0 deletions tests/test_workspace.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,15 @@ def testDatabase(self):
# Workspace spec allows these to be URIs or paths, whatever the Butler accepts
self._assertNotInDir(self._testbed.dbLocation, url2pathname(self._testbed.repo))

def testDbConfig(self):
"""Verify that a WorkspaceGen3 requests a database config in the target
directory, but not in any repository.
"""
root = self._testWorkspace
self._assertInDir(self._testbed.dbConfigLocation, root)
# Workspace spec allows these to be URIs or paths, whatever the Butler accepts
self._assertNotInDir(self._testbed.dbConfigLocation, url2pathname(self._testbed.repo))

def testAlerts(self):
"""Verify that a WorkspaceGen3 requests an alert dump in the target
directory, but not in any repository.
Expand Down
Loading