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-42637: Keep copies of old dimension universes #977

Merged
merged 1 commit into from Mar 19, 2024
Merged

Conversation

dhirving
Copy link
Contributor

@dhirving dhirving commented Mar 15, 2024

Store the dimensions.yaml configuration for universes 5 and 6.

This will be used by daf_butler_migrate to verify that the dimension universe stored in the DB is as expected before performing the migration.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.91%. Comparing base (2967610) to head (6c3cf4d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #977   +/-   ##
=======================================
  Coverage   88.91%   88.91%           
=======================================
  Files         329      329           
  Lines       42330    42330           
  Branches     8703     8703           
=======================================
  Hits        37639    37639           
  Misses       3441     3441           
  Partials     1250     1250           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

universe : `str`
Dimension universe configuration encoded as a JSON string.
"""
# This is used by daf_butler_migrate during dimension universe migrations.
Copy link
Contributor

Choose a reason for hiding this comment

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

In general for daf_butler_migrate I try to avoid depending on daf_butler interfaces. daf_butler_migrate is very special in that it may need to work with different daf_butler versions, not just most recent (would be better to avoid this dependency completely, but some things are too expensive to reimplement).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So would you prefer that I put the historical dimension universe YAMLs in daf_butler_migrate instead then? (I actually slightly prefer that but Tim wanted them in daf_butler.)

Copy link
Member

Choose a reason for hiding this comment

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

My argument for putting them in daf_butler is that I have been asked by people in the past about old universes and it seems easier to point them at a file in daf_butler than try to explain to them what daf_butler_migrate is. If the consensus is that people want them in daf_butler_migrate I will go along with that (and we have to remember to add them there when we create new ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like them better in daf_butler, but the code that reads them can go to daf_butler_migrate. I'm not sure that load_historical_dimension_universe_json is even generally useful in daf_butler.

Copy link
Contributor Author

@dhirving dhirving Mar 15, 2024

Choose a reason for hiding this comment

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

I thought it would be a good idea to have a stable function that daf_butler_migrate could call to insulate it from changes in daf_butler's package structure, and to make it obvious to readers in daf_butler that there is code elsewhere using these files, but I don't feel that strongly about it -- I'll put the code in daf_butler_migrate and the YAMLs in daf_butler.

@timj
Copy link
Member

timj commented Mar 15, 2024

As a general comment, I think I'd like the file name to include the namespace and the version number for completeness. I realize that daf_butler won't really have universes in it other than for daf_butler namespace but it would make it more explicit for people looking in from the outside if both were included.

@dhirving dhirving changed the title DM-42637: Allow loading of old dimension universes DM-42637: Keep copies of old dimension universes Mar 15, 2024
@dhirving dhirving marked this pull request as ready for review March 19, 2024 17:38
@dhirving dhirving requested a review from timj March 19, 2024 17:38
Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

This looks okay. I'll see about copying in earlier ones at some point.

These files are used by
[daf_butler_migrate](https://github.com/lsst-dm/daf_butler_migrate) to verify
and update the dimension universe stored in the database when upgrading the
database schema.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note here that the naming convention is <namespace>_universe<version>.yaml ?

@timj
Copy link
Member

timj commented Mar 19, 2024

Also consider adding a new PR check list item to remind people to copy universes in here whenever there is a new universe.

Store the dimensions.yaml configuration for universes 5 and 6.

This will be used by daf_butler_migrate to verify that the dimension universe stored in the DB is as expected before performing the migration.
@dhirving dhirving merged commit fd4a59f into main Mar 19, 2024
18 checks passed
@dhirving dhirving deleted the tickets/DM-42637 branch March 19, 2024 19:46
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