From a435d84e65091a4389f6bcb4a1d1fb547ce0262a Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Mon, 21 Mar 2022 17:18:18 -0500 Subject: [PATCH 1/9] Add unit tests for Visit. --- python/activator/visit.py | 2 +- tests/test_visit.py | 57 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) create mode 100644 tests/test_visit.py diff --git a/python/activator/visit.py b/python/activator/visit.py index b994b8d5..b6065fac 100644 --- a/python/activator/visit.py +++ b/python/activator/visit.py @@ -5,7 +5,7 @@ @dataclass(frozen=True) class Visit: - # elements should use built-in types that are hashable and JSON-persistable + # elements must be hashable and JSON-persistable; built-in types recommended instrument: str detector: int group: str diff --git a/tests/test_visit.py b/tests/test_visit.py new file mode 100644 index 00000000..2daa5688 --- /dev/null +++ b/tests/test_visit.py @@ -0,0 +1,57 @@ +# This file is part of prompt_prototype. +# +# Developed for the LSST Data Management System. +# This product includes software developed by the LSST Project +# (https://www.lsst.org). +# See the COPYRIGHT file at the top-level directory of this distribution +# for details of code ownership. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +import json +import unittest + +from activator.visit import Visit + + +class VisitTest(unittest.TestCase): + """Test the Visit class's functionality. + """ + def setUp(self): + super().setUp() + + self.testbed = Visit( + instrument="NotACam", + detector=42, + group="2022032100001", + snaps=2, + filter="k2022", + ra=134.5454, + dec=-65.3261, + rot=135.0, + kind="IMAGINARY", + ) + + def test_hash(self): + # Strictly speaking should test whether Visit fulfills the hash + # contract, but it's not clear what kinds of differences the default + # __hash__ might be insensitive to. So just test that the object + # is hashable. + value = hash(self.testbed) + self.assertNotEqual(value, 0) + + def test_json(self): + serialized = json.dumps(self.testbed.__dict__).encode("utf-8") + deserialized = Visit(**json.loads(serialized)) + self.assertEqual(deserialized, self.testbed) From a0284a0d5f9864f287ecd4290bb42e96123827d1 Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Tue, 22 Mar 2022 15:12:32 -0500 Subject: [PATCH 2/9] Document next_visit_handler. --- python/activator/activator.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/python/activator/activator.py b/python/activator/activator.py index 14a97160..927446e0 100644 --- a/python/activator/activator.py +++ b/python/activator/activator.py @@ -98,6 +98,18 @@ def check_for_snap( @app.route("/next-visit", methods=["POST"]) def next_visit_handler() -> Tuple[str, int]: + """A Flask view function for handling next-visit events. + + Like all Flask handlers, this function accepts input through the + ``request`` global rather than parameters. + + Returns + ------- + message : `str` + The HTTP response reason to return to the client. + status : `int` + The HTTP response status code to return to the client. + """ if request.args.get("token", "") != verification_token: return "Invalid request", 400 subscription = subscriber.create_subscription( From f9ab36b0fea964237cb90cd610c370f4a413db44 Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Mon, 21 Mar 2022 17:54:32 -0500 Subject: [PATCH 3/9] Distinguish instrument names in activator.py. --- python/activator/activator.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/python/activator/activator.py b/python/activator/activator.py index 927446e0..bcf4c584 100644 --- a/python/activator/activator.py +++ b/python/activator/activator.py @@ -33,6 +33,7 @@ from google.cloud import pubsub_v1, storage from lsst.daf.butler import Butler +from lsst.obs.base.utils import getInstrument from .middleware_interface import MiddlewareInterface from .visit import Visit @@ -41,6 +42,7 @@ verification_token = os.environ["PUBSUB_VERIFICATION_TOKEN"] # The full instrument class name, including module path. config_instrument = os.environ["RUBIN_INSTRUMENT"] +active_instrument = getInstrument(config_instrument) calib_repo = os.environ["CALIB_REPO"] image_bucket = os.environ["IMAGE_BUCKET"] oid_regexp = re.compile(r"(.*?)/(\d+)/(.*?)/(\d+)/\1-\3-\4-.*?-.*?-\2\.f") @@ -50,7 +52,7 @@ # Use JSON format compatible with Google Cloud Logging format=( '{{"severity":"{levelname}", "labels":{{"instrument":"' - + config_instrument + + active_instrument.getName() + '"}}, "message":{message!r}}}' ), style="{", @@ -62,20 +64,23 @@ subscriber = pubsub_v1.SubscriberClient() topic_path = subscriber.topic_path( PROJECT_ID, - f"{config_instrument}-image", + f"{active_instrument.getName()}-image", ) subscription = None storage_client = storage.Client() -# Initialize middleware interface; TODO: we'll need one of these per detector. +# Initialize middleware interface. +# TODO: this should not be done in activator.py, which is supposed to have only +# framework/messaging support (ideally, it should not contain any LSST imports). +# However, we don't want MiddlewareInterface to need to know details like where +# the central repo is located, either, so perhaps we need a new module. repo = f"/tmp/butler-{os.getpid()}" central_butler = Butler(calib_repo, - # TODO: How do we get the appropriate instrument name - # here and for what we pass to MiddlewareInterface? - instrument=config_instrument, + # TODO: investigate whether these defaults, esp. skymap, slow down queries + instrument=active_instrument.getName(), skymap="deepCoadd_skyMap", - collections=[f"{config_instrument}/defaults"], + collections=[active_instrument.makeCollectionName("defaults")], writeable=False) butler = Butler(Butler.makeRepo(repo.name), writeable=True) mwi = MiddlewareInterface(central_butler, image_bucket, config_instrument, butler) @@ -132,7 +137,7 @@ def next_visit_handler() -> Tuple[str, int]: payload = base64.b64decode(envelope["message"]["data"]) data = json.loads(payload) expected_visit = Visit(**data) - assert expected_visit.instrument == config_instrument + assert expected_visit.instrument == active_instrument.getName() snap_set = set() # Copy calibrations for this detector/visit From c1478fee877f2ff2018c318fd9725dec297d24aa Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Mon, 21 Mar 2022 20:03:30 -0500 Subject: [PATCH 4/9] Turn off bad default skymap. --- python/activator/activator.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/python/activator/activator.py b/python/activator/activator.py index bcf4c584..b385bfb2 100644 --- a/python/activator/activator.py +++ b/python/activator/activator.py @@ -79,9 +79,12 @@ central_butler = Butler(calib_repo, # TODO: investigate whether these defaults, esp. skymap, slow down queries instrument=active_instrument.getName(), - skymap="deepCoadd_skyMap", + # NOTE: with inferDefaults=True, it's possible we don't need to hardcode this + # value from the real repository. + # skymap="hsc_rings_v1", collections=[active_instrument.makeCollectionName("defaults")], - writeable=False) + writeable=False, + inferDefaults=True) butler = Butler(Butler.makeRepo(repo.name), writeable=True) mwi = MiddlewareInterface(central_butler, image_bucket, config_instrument, butler) From c32d5e5e5d0a5df143a0ee77ef6cfed77ad10864 Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Tue, 22 Mar 2022 13:01:04 -0500 Subject: [PATCH 5/9] Fix bug in local repo initialization. --- python/activator/activator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/activator/activator.py b/python/activator/activator.py index b385bfb2..d85ec14f 100644 --- a/python/activator/activator.py +++ b/python/activator/activator.py @@ -75,7 +75,6 @@ # framework/messaging support (ideally, it should not contain any LSST imports). # However, we don't want MiddlewareInterface to need to know details like where # the central repo is located, either, so perhaps we need a new module. -repo = f"/tmp/butler-{os.getpid()}" central_butler = Butler(calib_repo, # TODO: investigate whether these defaults, esp. skymap, slow down queries instrument=active_instrument.getName(), @@ -85,7 +84,8 @@ collections=[active_instrument.makeCollectionName("defaults")], writeable=False, inferDefaults=True) -butler = Butler(Butler.makeRepo(repo.name), writeable=True) +repo = f"/tmp/butler-{os.getpid()}" +butler = Butler(Butler.makeRepo(repo), writeable=True) mwi = MiddlewareInterface(central_butler, image_bucket, config_instrument, butler) From ee3681439ddc9c66c9b3b10579e273d170083f63 Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Tue, 22 Mar 2022 14:00:23 -0500 Subject: [PATCH 6/9] Fix initialization bug in MiddlewareInterface. This change also fixes the unit tests to test the correct behavior instead of patching over it. --- python/activator/middleware_interface.py | 8 +++++++- tests/data/central_repo/gen3.sqlite3 | Bin 2469888 -> 2469888 bytes tests/data/export.yaml | 1 - 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/python/activator/middleware_interface.py b/python/activator/middleware_interface.py index 22c0deb1..077c7dba 100644 --- a/python/activator/middleware_interface.py +++ b/python/activator/middleware_interface.py @@ -86,7 +86,13 @@ def __init__(self, central_butler: Butler, image_bucket: str, instrument: str, self._init_local_butler(butler) self._init_ingester() # TODO DM-34098: note that we currently need to supply instrument here. - self.camera = self.central_butler.get("camera", instrument=self.instrument.getName()) + # HACK: explicit collection gets around the fact that we don't have any + # timestamp/exposure information in a form we can pass to the Butler. + # This code will break once cameras start being versioned. + self.camera = self.central_butler.get( + "camera", instrument=self.instrument.getName(), + collections=self.instrument.makeCalibrationCollectionName("unbounded") + ) self.skymap = self.central_butler.get("skyMap") # self.r = self.src.registry diff --git a/tests/data/central_repo/gen3.sqlite3 b/tests/data/central_repo/gen3.sqlite3 index 4a2e5bc8f108169efe727cf1c0717704e8bfec13..6b9f39505b2bd3c1d7d6ed093c9e52fe09a97bc6 100644 GIT binary patch delta 336 zcmW;DJyOFk5Qbr`wU+Ee@yZZ>69ObaL&0zcF2Ljh=mu~DOyQv=4Jp%b1;!mERM4gL z3^yQ6DjGgfJl|+$^;X}{RKe4LrnBJ{O&>xXs18XT^lRXk8*SvPv4P*@TqzYPWepQi zshv&#ABi{2lv622Gi6j3^VKc)kuDp1$M2FMI*a9edCwQlNUP*mT3O3a<17Aik(H-P{l6$OXj1UW(W25OtF zVREsn5vgOQn0)38Q;U33Eibr>*Y{d3`Aw_IcV>-esXngwEES!v>5u;DTz~7l;F(r0 z1IwdStRc!>Q?*SqZkm{;orn#^?Wd)FYUJf+F=epept)NRLjr9`p#vEx Date: Tue, 22 Mar 2022 14:22:07 -0500 Subject: [PATCH 7/9] Add diagnostic info to gatekeeper assert. --- python/activator/activator.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/activator/activator.py b/python/activator/activator.py index d85ec14f..7e1a0baa 100644 --- a/python/activator/activator.py +++ b/python/activator/activator.py @@ -140,7 +140,8 @@ def next_visit_handler() -> Tuple[str, int]: payload = base64.b64decode(envelope["message"]["data"]) data = json.loads(payload) expected_visit = Visit(**data) - assert expected_visit.instrument == active_instrument.getName() + assert expected_visit.instrument == active_instrument.getName(), \ + f"Expected {active_instrument.getName()}, received {expected_visit.instrument}." snap_set = set() # Copy calibrations for this detector/visit From 557c29c43b1270d2dcf684e65c60d5d494a72413 Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Tue, 22 Mar 2022 17:50:17 -0500 Subject: [PATCH 8/9] Generate pgpass file in service container. The script for generating the file is called from the activator rather than during container setup, because it needs to be in python/activator/ anyway. --- python/activator/activator.py | 8 +++++ python/activator/make_pgpass.py | 58 +++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100755 python/activator/make_pgpass.py diff --git a/python/activator/activator.py b/python/activator/activator.py index 7e1a0baa..7b0db789 100644 --- a/python/activator/activator.py +++ b/python/activator/activator.py @@ -34,6 +34,7 @@ from lsst.daf.butler import Butler from lsst.obs.base.utils import getInstrument +from .make_pgpass import make_pgpass from .middleware_interface import MiddlewareInterface from .visit import Visit @@ -59,6 +60,13 @@ ) _log = logging.getLogger("lsst." + __name__) _log.setLevel(logging.DEBUG) + + +# Write PostgreSQL credentials. +# This MUST be done before creating a Butler or accessing the APDB. +make_pgpass() + + app = Flask(__name__) subscriber = pubsub_v1.SubscriberClient() diff --git a/python/activator/make_pgpass.py b/python/activator/make_pgpass.py new file mode 100755 index 00000000..f06853df --- /dev/null +++ b/python/activator/make_pgpass.py @@ -0,0 +1,58 @@ +# This file is part of prompt_prototype. +# +# Developed for the LSST Data Management System. +# This product includes software developed by the LSST Project +# (https://www.lsst.org). +# See the COPYRIGHT file at the top-level directory of this distribution +# for details of code ownership. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + + +__all__ = ["make_pgpass"] + + +import os +import stat + + +IP_APDB = "10.229.96.4:5432" +IP_REGISTRY = "10.229.96.5:5432" +PSQL_DB = "postgres" +PSQL_USER = "postgres" + + +def make_pgpass(): + """Create a .pgpass file that contains the service's database credentials. + + This function is designed to work in the Prompt Processing Service's docker + container, and no other environment. + + Raises + ------ + RuntimeError + Raised if the database passwords cannot be found. + """ + try: + pass_apdb = os.environ["PSQL_APDB_PASS"] + pass_registry = os.environ["PSQL_REGISTRY_PASS"] + except KeyError as e: + raise RuntimeError("Passwords have not been configured") from e + + filename = os.path.join(os.environ["HOME"], ".pgpass") + with open(filename, mode="wt") as file: + file.write(f"{IP_APDB}:{PSQL_DB}:{PSQL_USER}:{pass_apdb}\n") + file.write(f"{IP_REGISTRY}:{PSQL_DB}:{PSQL_USER}:{pass_registry}\n") + # Only user may access the file + os.chmod(filename, stat.S_IRUSR) From 8ab663d3547505a3c2cd9ea68795db5b523113ab Mon Sep 17 00:00:00 2001 From: Krzysztof Findeisen Date: Wed, 23 Mar 2022 17:23:21 -0500 Subject: [PATCH 9/9] Move DB addresses to environment. --- python/activator/make_pgpass.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/python/activator/make_pgpass.py b/python/activator/make_pgpass.py index f06853df..ce135200 100755 --- a/python/activator/make_pgpass.py +++ b/python/activator/make_pgpass.py @@ -27,8 +27,6 @@ import stat -IP_APDB = "10.229.96.4:5432" -IP_REGISTRY = "10.229.96.5:5432" PSQL_DB = "postgres" PSQL_USER = "postgres" @@ -45,14 +43,16 @@ def make_pgpass(): Raised if the database passwords cannot be found. """ try: + ip_apdb = os.environ["IP_APDB"] pass_apdb = os.environ["PSQL_APDB_PASS"] + ip_registry = os.environ["IP_REGISTRY"] pass_registry = os.environ["PSQL_REGISTRY_PASS"] except KeyError as e: - raise RuntimeError("Passwords have not been configured") from e + raise RuntimeError("Addresses and passwords have not been configured") from e filename = os.path.join(os.environ["HOME"], ".pgpass") with open(filename, mode="wt") as file: - file.write(f"{IP_APDB}:{PSQL_DB}:{PSQL_USER}:{pass_apdb}\n") - file.write(f"{IP_REGISTRY}:{PSQL_DB}:{PSQL_USER}:{pass_registry}\n") + file.write(f"{ip_apdb}:{PSQL_DB}:{PSQL_USER}:{pass_apdb}\n") + file.write(f"{ip_registry}:{PSQL_DB}:{PSQL_USER}:{pass_registry}\n") # Only user may access the file os.chmod(filename, stat.S_IRUSR)