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-13353: Add support for Formatter writeRecipes #305

Merged
merged 16 commits into from Jun 9, 2020
Merged

Conversation

timj
Copy link
Member

@timj timj commented Jun 5, 2020

No description provided.

Copy link
Contributor

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

Many of these butler-specific changes are over my head, so you may want to seek review from someone more familiar with the butler for those. The writeRecipes-related changes look fine to me.

python/lsst/daf/butler/core/formatter.py Outdated Show resolved Hide resolved
python/lsst/daf/butler/tests/testFormatters.py Outdated Show resolved Hide resolved
tests/test_formatter.py Show resolved Hide resolved
@timj timj force-pushed the tickets/DM-13353 branch 2 times, most recently from aacd636 to b25bab2 Compare June 8, 2020 18:17
Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of line comments, and also:

  • I had been trying to keep all Registry configuration keys snake_case (at least dimensions config; I haven't paid quite as much attention to other registry config). If Datastore configuration is consistently camelCase, maybe we should just accept that boundary - if not, we'll need to standardize on one or the other soon.

  • You've got a typo in one of your commit messages ("Consitently").

python/lsst/daf/butler/core/formatter.py Outdated Show resolved Hide resolved
config/datastores/formatters.yaml Show resolved Hide resolved
@timj
Copy link
Member Author

timj commented Jun 8, 2020

I'm happy to change the key to write_recipes. As for obs_base -- we haven't really solved the problem of how obs_base can introduce config items that can merge with daf_butler repo config items or override certain entries. Formatters are the sort of thing where it's clearly going to get annoying if every time some new stack high level object needs to be serialized we require that daf_butler gets a new StorageClass and new default Formatter entry.

timj added 5 commits June 8, 2020 16:11
This was added in for mypy but it changes the default
behavior.  Remove it and ignore the mypy warning on
makeUpdatedLocation.  Also adds a note explaining
the issue with extension being optional for
a formatter.
This simplifies some assumptions that datastores are
making about formatter.extension.
timj added 11 commits June 8, 2020 16:13
formatter.extension is mostly going to work fine but it
is safer to go through the Location object.
Config might be confusing since we only used Config
for a specific purpose
These come from obs_base originally in the gen2 implementation.
Almost no-one using the top level Butler interface will be
ingesting files in-place. Change the default to auto
so that there is more chance of a correct default being
used.
This makes it more consistent with other Butler YAML.
@timj timj merged commit dd1ad65 into master Jun 9, 2020
@timj timj deleted the tickets/DM-13353 branch June 9, 2020 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants