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-31698: Move ConvertReferenceCatalog classes out of ingestIndex file #297

Merged
merged 5 commits into from Oct 17, 2022

Conversation

parejkoj
Copy link
Contributor

No description provided.

The useful methods in "ingestIndex" now live in
convertReferenceCatalog.py, which is where they are used. "ingest" in
gen3 is an entirely separate process.

As part of this, I removed the convert base class, because it was only
there for gen2/gen3 compatibility. I refactored the interface as part
of that, to remove methods that only were split to modularize the
gen2/gen3 differences.
This was ticketed for DM-31817, but it makes sense to do it here.
Have to add that field back to the `bad_config.py` test, as it is still
a necessary field (so that the name of the refcat is tracked in
something other than the path it is written to).
This should make it more clear why these tests both matter: a naive
coverage comparison might suggest one could be removed.
Having only one file for convert tests, and one file for load tests (plus the
nopytest file for the parallel convert tests) should help reduce confusion
about what the various tests are for.
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all looks good, and I appreciate that we won't have both test_loadReferenceObjects.py and test_referenceObjectLoader.py which tripped me up roughly eleventy billion times.

)
ref_dataset_name = pexConfig.Field(
dtype=str,
doc="Name of this reference catalog; this should match the name used during butler ingest.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an optional or required field? Do the default config settings make sure this is required with a default of None so it must be set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required: we may want to revamp this at some point in the future, but other than this field, the only thing tracking the refcat name is the directory it's written to (and then the butler datasetType it's registered as, once it's added to a butler). I'd much rather have the name the author expected written to a config file.

ConvertReferenceCatalogTask is a Task, so it will validate the config on init (thanks, DM-971!), including when this is run on the commandline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though your comment made me check on, and clean up, a couple validate()-related things.

Post-DM-971, Tasks always validate their configs on init, so we don't need to
validate when running from the commandline via main().
@parejkoj parejkoj merged commit ee093ee into main Oct 17, 2022
@parejkoj parejkoj deleted the tickets/DM-31698 branch October 17, 2022 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants