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-46599: Deprecate old query system behavior #1096

Merged
merged 19 commits into from
Oct 16, 2024
Merged

DM-46599: Deprecate old query system behavior #1096

merged 19 commits into from
Oct 16, 2024

Conversation

dhirving
Copy link
Contributor

@dhirving dhirving commented Oct 11, 2024

Deprecate behavior from the old query system that will no longer be supported when we switch to the new implementation.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes
  • (if changing dimensions.yaml) make a copy of dimensions.yaml in configs/old_dimensions

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 91.79104% with 11 lines in your changes missing coverage. Please review.

Project coverage is 89.74%. Comparing base (6616209) to head (84cf62c).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
python/lsst/daf/butler/registry/tests/_registry.py 87.87% 6 Missing and 2 partials ⚠️
python/lsst/daf/butler/registry/sql_registry.py 0.00% 1 Missing and 1 partial ⚠️
python/lsst/daf/butler/cli/cmd/commands.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1096   +/-   ##
=======================================
  Coverage   89.73%   89.74%           
=======================================
  Files         361      361           
  Lines       47324    47393   +69     
  Branches     5709     5723   +14     
=======================================
+ Hits        42466    42532   +66     
- Misses       3506     3508    +2     
- Partials     1352     1353    +1     

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

@dhirving dhirving marked this pull request as ready for review October 15, 2024 18:33
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.

Looks great.


The `offset` argument to `limit()` for `Registry.queryDataIds` and `Registry.queryDimensionRecords` result objects is now deprecated.

The `--offset` option for `butler query-data-ids` and `butler-query-datasets` is now deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like butler query-data-ids raises when --offset is used and query-dimension-records ignores it. Maybe we add a raise to the latter as well? Or remove the raise from the former and add deprecation warnings instead noting that the option is a no-op? I think this happened during the migration to the new query system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize that these had been switched over to the new query system... theoretically shouldn't have happened until after this deprecation.

Probably better to raise than silently ignore the option, so I'll add an exception to query-dimension-records.

The --offset parameter to query-dimension-records was changed to be silently ignored in DM-45556.  It can't be brought back without going back to the old query system.

We are removing this entirely soon, and it's better to flag the problem to the user than silently ignore what they asked for.

Also added a little more information to a similar information that was being thrown from query-data-ids.
@dhirving dhirving merged commit 3abf212 into main Oct 16, 2024
18 checks passed
@dhirving dhirving deleted the tickets/DM-46599 branch October 16, 2024 16:23
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.

2 participants