DM-44850: Add utility to create empty tables #223

isullivan · 2024-06-14T23:54:25Z

No description provided.

kfindeisen

I'm worried that ApdbSchema is, like _make_empty_catalog, an implementation detail that may break at any time. I'd like feedback from @andy-slac before approving.

python/lsst/ap/association/utils.py

kfindeisen · 2024-06-15T00:02:13Z

tests/test_diaPipe.py

@@ -35,6 +35,7 @@
 from lsst.pipe.base.testUtils import assertValidOutput

 from lsst.ap.association import DiaPipelineTask
+from lsst.ap.association.utils import make_empty_catalog, convertTableToSdmSchema


I'd suggest moving this to a test_utils.py.

kfindeisen · 2024-06-15T00:03:10Z

tests/test_diaPipe.py

+ def test_make_empty_catalog(self):
+ """Check that an empty catalog has the correct format.
+ """
+ tableName = "DiaObject"


Does it also work with other tables, e.g. DiaForcedSource?

Yes, it should work with any of the tables. I'll add a for loop over a few tables.

kfindeisen · 2024-06-15T00:04:46Z

tests/test_diaPipe.py

+ tableName = "DiaObject"
+ config = self._makeDefaultConfig(config_file=self.config_file.name)
+ diaPipeTask = DiaPipelineTask(config=config)
+ emptyDiaObjects = make_empty_catalog(diaPipeTask.schema, tableName=tableName)


If you provide a test schema directly, then there's no need to mix DiaPipelineTask into the test...

That is true, though here I wanted to also test that diaPipeTask.schema is getting set correctly. That's why I added this test to test_diaPipe instead of creating a test_utils, but I am happy to move it out and just make the schema directly.

I'm not sure that's actually being tested here -- the only thing you compare the schema to is itself.

OK, I will move it to test_utils, and also add a check of a few of the values.

kfindeisen · 2024-06-15T00:06:12Z

tests/test_diaPipe.py

+ emptyDf = pd.DataFrame(columns=["diaObjectId",])
+ emptyDf.set_index("diaObjectId")
+ convertedEmptyDiaObjects = convertTableToSdmSchema(diaPipeTask.schema, emptyDf, tableName=tableName)
+ self.assertSetEqual(set(convertedEmptyDiaObjects.columns), set(emptyDiaObjects.columns))


If you're comparing two set, then assertSetEqual is redundant.

Suggested change

self.assertSetEqual(set(convertedEmptyDiaObjects.columns), set(emptyDiaObjects.columns))

self.assertEqual(set(convertedEmptyDiaObjects.columns), set(emptyDiaObjects.columns))

andy-slac

I agree with Krzysztof's sentiment - ApdbSchema presently is not in public API. And I think that you do not actually have to use it, as Apdb has a method to return table schema which wraps access to ApdbSchema. I'd probably re-implement readSchemaFromApdb like this:

from lsst.dax.apdb import ApdbTables, schema_model

def readSchemaFromApdb(apdb: Apdb) -> dict[str, schema_model.Table | None]:
    """...."""
    return {table.table_name(): apdb.tableDef(table) for table in ApdbTables}

OTOH, if there is something else in ApdbSchema that you want to reuse, we can definitely extend Apdb's public interface.

andy-slac · 2024-06-17T16:01:43Z

python/lsst/ap/association/utils.py

@@ -141,7 +142,7 @@ def convertTableToSdmSchema(apdbSchema, sourceTable, tableName):

 Parameters
 ----------
- apdbSchema : `lsst.dax.apdb.apdbSchema.ApdbSchema`
+ apdbSchema : 'dict' of `lsst.dax.apdb.apdbSchema.ApdbSchema`


This should be lsst.dax.apdb.schema_model.Table instead of lsst.dax.apdb.apdbSchema.ApdbSchema. Same on the line 128 above.

andy-slac · 2024-06-17T16:09:21Z

python/lsst/ap/association/utils.py

@@ -182,7 +183,7 @@ def dropEmptyColumns(apdbSchema, sourceTable, tableName):

 Parameters
 ----------
- apdbSchema : `lsst.dax.apdb.apdbSchema.ApdbSchema`
+ apdbSchema : 'dict' of `lsst.dax.apdb.apdbSchema.ApdbSchema`


lsst.dax.apdb.schema_model.Table here too.

andy-slac · 2024-06-17T16:09:29Z

python/lsst/ap/association/utils.py

+
+ Parameters
+ ----------
+ apdbSchema : 'dict' of `lsst.dax.apdb.apdbSchema.ApdbSchema`


isullivan · 2024-06-17T21:42:04Z

Great! Your code snippet for readSchemaFromApdb works for my tests, so I have switched over to using it.

kfindeisen

Looks good, some documentation and usage comments.

kfindeisen · 2024-06-18T17:22:21Z

python/lsst/ap/association/utils.py

@@ -124,13 +126,10 @@ def readSchemaFromApdb(apdb):

 Returns
 -------
- schemaTable : 'dict' of `lsst.dax.apdb.apdbSchema.ApdbSchema`
+ schemaTable : dict[str, schema_model.Table | None]


Would it make more sense to not make an entry instead of returning None? I assume that's for tables that aren't in a particular DB instance, but it would be good to document that explicitly.

kfindeisen · 2024-06-18T17:25:42Z

python/lsst/ap/association/utils.py

@@ -89,7 +91,7 @@ def readSdmSchemaFile(schemaFile: str,

 Returns
 -------
- schemaTable : 'dict' of `lsst.dax.apdb.apdbSchema.ApdbSchema`
+ schemaTable : 'dict' of `lsst.dax.apdb.schema_model.Table`


Suggested change

schemaTable : 'dict' of `lsst.dax.apdb.schema_model.Table`

schemaTable : `dict` [`str`, `lsst.dax.apdb.schema_model.Table`]

Same for all the other places that say "'dict' of" instead of "dict of"

kfindeisen · 2024-06-18T17:42:01Z

tests/test_utils.py

+ def test_make_empty_catalog(self):
+ """Check that an empty catalog has the correct format.
+ """
+ apdb_config = daxApdb.ApdbSql.init_database(db_url="sqlite://")


IIRC, in-memory APDBs will be removed in a future version. But I think @andy-slac would need to deprecate them first?

In-memory SQLite support is already removed since DM-43414. It might work for this particular unit test, but you still should not use it.

In that case, I recommend using NamedTemporaryFile as a context manager instead of using addCleanup (which is more appropriate for stuff created in setUp):

Suggested change

apdb_config = daxApdb.ApdbSql.init_database(db_url="sqlite://")

with tempfile.NamedTemporaryFile() as apdb_file:

apdb_config = daxApdb.ApdbSql.init_database(db_url="sqlite://" + apdb_file)

apdb = daxApdb.Apdb.from_config(apdb_config)

I was struggling to get your suggestion to work, and realized I only need the schema here, not the full APDB. I will use readSdmSchemaFile instead of readSchemaFromApdb to get the schema.

kfindeisen · 2024-06-18T17:45:00Z

tests/test_utils.py

+ config_file = tempfile.NamedTemporaryFile()
+ self.addCleanup(config_file.close)
+ apdb_config.save(config_file.name)
+ apdb = daxApdb.Apdb.from_uri(config_file.name)


When writing this kind of code, I recommend reading the API docs instead of cargo-culting:

Suggested change

config_file = tempfile.NamedTemporaryFile()

self.addCleanup(config_file.close)

apdb_config.save(config_file.name)

apdb = daxApdb.Apdb.from_uri(config_file.name)

apdb = daxApdb.Apdb.from_config(apdb_config)

isullivan requested a review from kfindeisen June 14, 2024 23:55

kfindeisen reviewed Jun 15, 2024

View reviewed changes

isullivan force-pushed the tickets/DM-44850 branch from a54f713 to ea8881b Compare June 15, 2024 00:42

andy-slac reviewed Jun 17, 2024

View reviewed changes

kfindeisen approved these changes Jun 18, 2024

View reviewed changes

isullivan force-pushed the tickets/DM-44850 branch from 74d70b9 to dcd3631 Compare June 18, 2024 23:17

Add utility to create empty tables.

899a360

isullivan force-pushed the tickets/DM-44850 branch from dcd3631 to 899a360 Compare June 18, 2024 23:18

isullivan merged commit 639f7d8 into main Jun 19, 2024
2 checks passed

isullivan deleted the tickets/DM-44850 branch June 19, 2024 05:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-44850: Add utility to create empty tables #223

DM-44850: Add utility to create empty tables #223

isullivan commented Jun 14, 2024

kfindeisen left a comment

kfindeisen Jun 15, 2024

kfindeisen Jun 15, 2024

isullivan Jun 15, 2024

kfindeisen Jun 15, 2024

isullivan Jun 15, 2024

kfindeisen Jun 15, 2024

isullivan Jun 15, 2024

kfindeisen Jun 15, 2024

andy-slac left a comment •

edited

Loading

andy-slac Jun 17, 2024

andy-slac Jun 17, 2024

andy-slac Jun 17, 2024

isullivan commented Jun 17, 2024

kfindeisen left a comment

kfindeisen Jun 18, 2024

kfindeisen Jun 18, 2024

kfindeisen Jun 18, 2024

andy-slac Jun 18, 2024

kfindeisen Jun 18, 2024 •

edited

Loading

isullivan Jun 18, 2024

kfindeisen Jun 18, 2024

	self.assertSetEqual(set(convertedEmptyDiaObjects.columns), set(emptyDiaObjects.columns))
	self.assertEqual(set(convertedEmptyDiaObjects.columns), set(emptyDiaObjects.columns))

	schemaTable : 'dict' of `lsst.dax.apdb.schema_model.Table`
	schemaTable : `dict` [`str`, `lsst.dax.apdb.schema_model.Table`]

- apdb_config = daxApdb.ApdbSql.init_database(db_url="sqlite://")
+ with tempfile.NamedTemporaryFile() as apdb_file:
+ apdb_config = daxApdb.ApdbSql.init_database(db_url="sqlite://" + apdb_file)
+ apdb = daxApdb.Apdb.from_config(apdb_config)

DM-44850: Add utility to create empty tables #223

DM-44850: Add utility to create empty tables #223

Conversation

isullivan commented Jun 14, 2024

kfindeisen left a 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

andy-slac left a comment • edited Loading

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

isullivan commented Jun 17, 2024

kfindeisen left a 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

kfindeisen Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-slac left a comment •

edited

Loading

kfindeisen Jun 18, 2024 •

edited

Loading