DM-15588: Remove home-brewed SQLite PPDB #53
Conversation
| @@ -36,10 +36,10 @@ | |||
| from .dataset import Dataset | |||
| from .ingestion import ingestDataset | |||
| from .metrics import MetricsParser, checkSquashReady, AutoJob | |||
| from .pipeline_driver import ApPipeParser, runApPipe | |||
| from .pipeline_driver import ApPipeParser, runApPipe, _getConfig | |||
There was a problem hiding this comment.
I'd like to avoid forcing ap_verify.py to know about implementation details in pipeline_driver. My intent was that the measurements code could just load the config from the Butler, but I'm willing to do that myself on DM-15806.
There was a problem hiding this comment.
Okay, should I leave this as in for now then and let DM-15806 take care of it or is there is there something I can do in the mean time?
There was a problem hiding this comment.
It's up to you. I think the hardest part of that ticket would be adding a line(s) in pipeline_driver that saves the config to the Butler as if we'd called parseAndRun (much like what I do in #49).
There was a problem hiding this comment.
I think this is worth just making another ticket to do the work. It feels like it will take long enough to warrant it.
| @@ -159,22 +159,19 @@ def _convertDataIdString(dataId): | |||
| return dataIdDict | |||
|
|
|||
|
|
|||
| def measureFromL1DbSqlite(dbName): | |||
| def measureFromPpdb(configurable): | |||
| """Make measurements on an sqlite database containing the results of | |||
| source association. | |||
|
|
|||
| dbName : `str` | |||
| Name of the sqlite database created from a previous run of | |||
| `lsst.ap.association.AssociationDBSqliteTask` to load. | |||
There was a problem hiding this comment.
Out-of-date documentation. (Also, at present configurable is not a ConfigurableInstance, but a Config, so the name is a bit misleading.)
There was a problem hiding this comment.
Oops, thought I had fixed this but it turned out the commit was still sitting on a local copy. Also changed to config.
| measurement = measureTotalUnassociatedDiaObjects( | ||
| dbCursor, "ap_association.totalUnassociatedDiaObjects") | ||
| ppdb, "association.totalUnassociatedDiaObjects") |
There was a problem hiding this comment.
I think you missed a rebase, because the correct namespace is ap_association.
There was a problem hiding this comment.
Okay, I'm guessing that's what happened. I'll change it to the correct namespace.
| config.associator.level1_db.db_name = workspace.dbLocation | ||
|
|
||
| # ApVerify will use the sqlite hooks for the Ppdb. | ||
| config.ppdb.db_url = "sqlite:///" + workspace.dbLocation |
There was a problem hiding this comment.
I assume this is that part that made you ask about whether the path was absolute or relative. I take it something like "sqlite:///workspace/output/association.db" gets resolved as a relative and not an absolute path?
There was a problem hiding this comment.
Yes. sqlite:/// is always relative path so the above example works. If workspace is an absolute path with the added / at the beginning it "should" resolve to the correct absolute path.
| if subSchema.getField().getTypeString() == "Angle": | ||
| continue | ||
| elif subSchema.getField().getTypeString() == "String": | ||
| src[subSchema.getField().getName()] = 'g' |
There was a problem hiding this comment.
It took me a while to figure out that you're assuming this is a filter field; might want to explicitly say that.
| for diaObject in self.diaObjects: | ||
| diaObject['nDiaSources'] = 1 | ||
|
|
||
| self.butler = NonCallableMock(spec=dafPersist.Butler, get=mockGet) |
There was a problem hiding this comment.
Can you please keep the NonCallableMock and mockGet definitions together? It's more confusing with the unrelated self.diaObjects definition in between.
tests/test_association.py
Outdated
| config=self.ppdbCfg, | ||
| afw_schemas=dict(DiaObject=make_dia_object_schema(), | ||
| DiaSource=make_dia_source_schema())) | ||
| self.ppdb._schema.makeSchema() |
There was a problem hiding this comment.
| self.ppdb._schema.makeSchema() | |
| self.ppdb.makeSchema(drop=True) |
tests/test_association.py
Outdated
| @@ -184,7 +185,8 @@ def testValidFromMetadata(self): | |||
| meas.metric_name, | |||
| lsst.verify.Name( | |||
| metric='association.fracUpdatedDIAObjects')) | |||
| self.assertEqual(meas.quantity, nUnassociatedDiaObjects * u.count) | |||
| self.assertEqual(meas.quantity, | |||
| nUnassociatedDiaObjects * u.count) | |||
There was a problem hiding this comment.
Why add a line break here? The original line was only 75 characters...
| @@ -308,11 +306,9 @@ def testNoMetric(self): | |||
| metricName='foo.bar.FooBar') | |||
|
|
|||
| # Fake DB handle to avoid DB initialization overhead | |||
| cursor = NonCallableMock(spec=sqlite3.Cursor) | |||
There was a problem hiding this comment.
This comment is obsolete, since you deleted the fake.
Fix typo in Ppdb import Fix import bug
Fix measement unit type.
Fix linting error. Fix call to db config field. Fix custom config test. Add READ_UNCOMMITTED to ap_verify defaults. Fix docstring Remove old comment. Remove line break.
Change ordering of mockGet and mock butler.
Change name from configurable to config. Correct metric namespace.
morriscb
force-pushed
the
tickets/DM-15588
branch
from
01be669
to
2560d47
Compare
No description provided.