DM-45657 Add tests for consdb-hinfo and consdb-pqserver +200-65 #33

bbrondel · 2024-08-08T17:25:01Z

More testing still desirable for consdb, but this begins to cover hinfo.

Aside from code I'm adding two files to the test directory: updated cdb_latiss.sql freshly generated from felis, and a yaml file pulled from s3.

Vebop · 2024-08-08T17:37:05Z

tests/test_hinfo.py

+
+
+@pytest.fixture
+def tmpdir(scope="module"):


Does this get cleaned up, or should we yield/finally the passing of tmpdir?

I understand this is not cleaned up automatically

Vebop · 2024-08-08T17:39:51Z

tests/test_hinfo.py

+
+
+@pytest.fixture
+def engine(tmpdir, scope="module"):


Just for my own context & learning, this looks like it could be relevant in other test files where we need to create an sqlite engine. Do other test files exist yet that need that, should we generalize this one, should we just keep an eye on it for further test development?

Vebop · 2024-08-08T17:45:41Z

tests/test_hinfo.py

+        print(f"{row=}")
+        print(f"{row.exposure_name=}")
+
+    assert _header_lookup(header, "OBSID") == row.exposure_name


The two other asserts look straight forward for names, but because I'm still fresh here, do you mind showing me how OBSID and exposure_name correlate? I can't seem to line them up by searching

The metadata transaltor populates exposure_name with OBSID - the mapping dict is in hinfo.py

womullan · 2024-08-08T19:51:39Z

tests/test_hinfo.py

+
+
+@pytest.fixture
+def tmpdir(scope="module"):


I understand this is not cleaned up automatically

womullan · 2024-08-08T20:01:00Z

tests/test_hinfo.py

+        print(f"{row=}")
+        print(f"{row.exposure_name=}")
+
+    assert _header_lookup(header, "OBSID") == row.exposure_name


The metadata transaltor populates exposure_name with OBSID - the mapping dict is in hinfo.py

ktlim · 2024-08-12T19:57:30Z

.github/workflows/pytest.yaml

-        python-version: "3.11"
-        cache: "pip"
+    - name: Build docker image
+      run: docker build -f Dockerfile.pytest -t pytest_image .


Wondering if this could be done in a container in the workflow: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idcontainer
instead of building a new container and running in it.

The advantage would be that more of the test code is in the workflow file, where it's more visible, than in another Dockerfile.

ktlim · 2024-08-12T19:58:23Z

Dockerfile.hinfo

 RUN source loadLSST.bash && mamba install aiokafka httpx
-RUN source loadLSST.bash && pip install kafkit
-COPY python/lsst/consdb/hinfo.py python/lsst/consdb/utils.py ./hinfo/
+RUN source loadLSST.bash && pip install kafkit aiokafka httpx


We generally try to avoid pip installing into conda environments whenever possible. Although it works pretty well nowadays, there's still some iffiness about the integration.

I tried to do this using only mamba, but it wouldn't cooperate. I think it's because we have to rely on newer versions of aiokafka that have not made their way to conda. My understanding is generally you can get away with mixing pip and conda as long as you never use conda again once you've used pip.

ktlim · 2024-08-12T19:59:10Z

Dockerfile.hinfo

+WORKDIR /home/lsst/
+COPY --chown=lsst .  ./consdb/
+WORKDIR /home/lsst/consdb/
+RUN source /opt/lsst/software/stack/loadLSST.bash && pip install -e .


I'm not sure why this as well as the install above.

ktlim · 2024-08-12T19:59:21Z

Dockerfile.hinfo

+RUN source loadLSST.bash && pip install kafkit aiokafka httpx
+
+WORKDIR /home/lsst/
+COPY --chown=lsst .  ./consdb/


This seems like it copies too much for the hinfo service.

ktlim · 2024-08-12T20:48:44Z

python/lsst/consdb/hinfo.py

@@ -442,14 +443,16 @@ def get_kafka_config() -> KafkaConfig:

 logger = setup_logging("consdb.hinfo")

-instrument = os.environ["INSTRUMENT"]
-logger.info(f"Instrument = {instrument}")
+instrument = ""


The idea here is to have a more specific error than the KeyError that would be returned if the instrument is not defined? I'm not sure that an "Unrecognized instrument: " (with no instrument name) error is much better.

Also, instrument = os.environ.get("INSTRUMENT", "") may be more idiomatic.

ktlim · 2024-08-12T20:53:05Z

tests/test_hinfo.py

+    try:
+        yield tmpdir
+    finally:
+        shutil.rmtree(tmpdir)


It can often be useful to specify ignore_errors=True when using shutil.rmtree(), especially when working on NFS filesystems that sometimes leave behind .nfsNNNN files when removing things.

In this case, since it's for temporary test files that should be on local storage, this is not an issue.

ktlim · 2024-08-12T20:55:07Z

tests/test_hinfo.py

+            conn.execute(f"INSERT INTO schemas VALUES ('{schema}', '{schema_path}')")
+            with sqlite3.connect(schema_path) as schema_conn:
+                schema_conn.executescript(sql.read_text())
+            schema_conn.close()


Shouldn't this be handled by the context manager?

ktlim · 2024-08-12T20:55:47Z

tests/test_hinfo.py

+            schema_conn.close()
+
+    os.environ["POSTGRES_URL"] = f"sqlite:///{db_path}"
+    hinfo.engine = utils.setup_postgres()


Hm. Maybe we should rename this method, since it's obviously not setting up postgres...

ktlim · 2024-08-12T20:58:08Z

tests/test_hinfo.py

+
+
+def _header_lookup(header, key):
+    for line in header:


This is fine for tests, but for production code it might be better to load the array of key/value pairs into a Python dict for faster lookups.

ktlim · 2024-08-12T20:59:19Z

tests/test_hinfo.py

+    os.environ["INSTRUMENT"] = "LATISS"
+    tmpdir = Path(tempfile.mkdtemp())
+    try:
+        yield tmpdir


Can the yield actually ever fail? I think you could remove the try: finally: (here and in the other test).

bbrondel added 5 commits August 7, 2024 14:41

Set up hinfo for unit testing

09cb45f

Implement a test for hinfo

a1c5a95

Add an assert for one of the computed functions

d809865

Modify git workflow to use Docker container

a38ae4b

Lint and isort

909e797

bbrondel requested review from ktlim, Vebop and womullan August 8, 2024 17:25

Vebop approved these changes Aug 8, 2024

View reviewed changes

womullan approved these changes Aug 8, 2024

View reviewed changes

bbrondel added 2 commits August 9, 2024 10:03

Implement cleanup of tmpdir per suggestion by @Vebop

bae092b

isort

192f896

ktlim approved these changes Aug 12, 2024

View reviewed changes

ktlim reviewed Aug 12, 2024

View reviewed changes

More idiomatic use of getenv suggested by @ktlim

b3e5cbb

bbrondel merged commit 04b2b2b into main Aug 15, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-45657 Add tests for consdb-hinfo and consdb-pqserver +200-65 #33

DM-45657 Add tests for consdb-hinfo and consdb-pqserver +200-65 #33

bbrondel commented Aug 8, 2024

Vebop Aug 8, 2024

womullan Aug 8, 2024

Vebop Aug 8, 2024

Vebop Aug 8, 2024

womullan Aug 8, 2024

womullan Aug 8, 2024

womullan Aug 8, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

bbrondel Aug 13, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

ktlim Aug 12, 2024

DM-45657 Add tests for consdb-hinfo and consdb-pqserver +200-65 #33

DM-45657 Add tests for consdb-hinfo and consdb-pqserver +200-65 #33

Conversation

bbrondel commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment