DM-36303: Replace RFC-878/879 deprecations with removals #891

timj · 2023-09-29T17:57:20Z

Checklist

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

codecov · 2024-01-05T22:03:38Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (d8d7886) 88.37% compared to head (8ee1e29) 88.34%.

Files	Patch %	Lines
python/lsst/daf/butler/registry/sql_registry.py	57.57%	10 Missing and 4 partials ⚠️
...ython/lsst/daf/butler/registry/queries/_results.py	60.00%	2 Missing and 2 partials ⚠️
python/lsst/daf/butler/cli/cmd/commands.py	25.00%	2 Missing and 1 partial ⚠️
python/lsst/daf/butler/direct_query_results.py	80.00%	2 Missing ⚠️
.../butler/registry/datasets/byDimensions/_manager.py	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #891      +/-   ##
==========================================
- Coverage   88.37%   88.34%   -0.03%     
==========================================
  Files         301      301              
  Lines       38854    38752     -102     
  Branches     8186     8165      -21     
==========================================
- Hits        34337    34237     -100     
+ Misses       3343     3334       -9     
- Partials     1174     1181       +7

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

timj

The rebase took a while because the first batch of changes turned out to already be applied on main but in slightly different ways. I think everything looks reasonable now and all tests pass and mypy is happy.

We do still issue a warning for components in the tests because we are calling _iter_by_dataset_type which calls the .components property on the result (and then calls withComponents. Presumably that code could be cleaned up as well.

timj · 2024-01-05T21:59:00Z

python/lsst/daf/butler/direct_butler.py

@@ -1229,8 +1237,11 @@ def find_dataset(
        )
        if ref is not None and dimension_records:
            ref = ref.expanded(self._registry.expandDataId(ref.dataId, dimensions=ref.datasetType.dimensions))
+        if ref is not None and component_name:
+            ref = ref.makeComponentRef(component_name)


This change is sufficient to make all the Butler.get() tests with components work. I do understand that doing this means that Butler.find_dataset is handling components but Butler.query_datasets() does not. If we don't like this the above code will have to move into _findDatasetRef.

I'm fine with handling components here like this.

timj · 2024-01-05T22:01:22Z

python/lsst/daf/butler/_registry_shim.py

@@ -281,7 +281,7 @@ def queryDatasetTypes(
        self,
        expression: Any = ...,
        *,
-        components: bool | None = None,
+        components: bool = False,


When I was working on #937, it occurred to me that we really want to be warning if people set the components flag to False. What do you think about having this be something like:

components : bool | _Sentinel = _DefaultSentinel

and then warning if components is False ?

👍, but we should instead warn if components is not _DefaultSentinal in case somebody is still passing None and isn't running MyPy.

Yes, I was trying to say that we should warn any time it's not _DefaultSentinel because that would imply someone actually tried to set it. We should likely raise if they set it to anything other than False.

TallJimbo

registry.queries.ParentDatasetQueryResults.withComponents seems to have escaped deprecation originally, so we should definitely add that deprecation now. I think that means we also have to defer removing the _components attribute until that's removed; after other changes on this ticket it should be possible for it to be constructed with anything other than components=[None] except for via withComponents.

And then I think we want to reimplement _iter_by_dataset_type to just use _components and duplicate the implementation of withComponents internally. That will keep it from emitting any warnings of its own, while the deprecation of withComponents will close the last remaining avenue for making it actually iterate over components without a warning.

There's also the issue that the "parent" in that class name and in byParentDatasetTypes is not so great anymore. I'm inclined to leave it on the old registry result types, but we should fix the Butler._query_results versions since those aren't public yet.

TallJimbo · 2024-01-09T14:56:56Z

python/lsst/daf/butler/_registry_shim.py

@@ -281,7 +281,7 @@ def queryDatasetTypes(
        self,
        expression: Any = ...,
        *,
-        components: bool | None = None,
+        components: bool = False,


👍, but we should instead warn if components is not _DefaultSentinal in case somebody is still passing None and isn't running MyPy.

TallJimbo · 2024-01-09T14:59:38Z

python/lsst/daf/butler/direct_butler.py

@@ -1229,8 +1237,11 @@ def find_dataset(
        )
        if ref is not None and dimension_records:
            ref = ref.expanded(self._registry.expandDataId(ref.dataId, dimensions=ref.datasetType.dimensions))
+        if ref is not None and component_name:
+            ref = ref.makeComponentRef(component_name)


I'm fine with handling components here like this.

TallJimbo · 2024-01-09T19:15:42Z

I've pushed three new commits to deal with ParentDatasetQueryResults.

See RFC-879.

This involves some guesses about how we'll update the RemoteRegistry server, which I'm not attempting to change right now since I don't know how to test it.

Dataset type wildcards are no longer permitted in queryDataIds and queryDimensionRecords, and missing dataset types are now an error in those contexts.

Do not forward.

Should have been deprecated earlier, but better late than never.

Since getting a component DatasetRef from a parent one is just a method call, there's no need for this complexity in the query system.

The components parameter is deprecated. Warn if False is specified explicitly in addition to raising if any other value is used.

timj force-pushed the tickets/DM-36303 branch from 11ce7f4 to 4280299 Compare January 5, 2024 21:53

timj commented Jan 5, 2024

View reviewed changes

timj marked this pull request as ready for review January 5, 2024 22:05

timj force-pushed the tickets/DM-36303 branch 2 times, most recently from cbbeb84 to 85ce59c Compare January 8, 2024 21:39

timj requested a review from TallJimbo January 8, 2024 23:13

TallJimbo approved these changes Jan 9, 2024

View reviewed changes

TallJimbo and others added 16 commits January 9, 2024 15:00

Deprecate component dataset support in Registry query methods.

acefb48

See RFC-879.

Remove CollectionSearch.

d9ed29a

This involves some guesses about how we'll update the RemoteRegistry server, which I'm not attempting to change right now since I don't know how to test it.

Remove Registry support for component dataset types.

3cb054a

Turn more RFC-879 deprecations into errors.

4a472c0

Dataset type wildcards are no longer permitted in queryDataIds and queryDimensionRecords, and missing dataset types are now an error in those contexts.

Force components parameter to False in registry shim

c173d5e

Fix exception type in test code

e2736ba

More fixes for removal of components

48a6c40

Fix API call now that component not returned

af41679

Fix tests now that components not allowed

ead414a

Allow Butler.find_dataset to return a component DatasetRef

0e8a205

Fix return type of base class

fa4caa7

Drop components option in query-dataset-types command line

ebfe7a4

Do not forward.

Issue warning from butler query-dataset-types if --[no-]components used

0e4372e

Deprecate ParentDatasetQueryResults.withComponents.

b5d07c0

Should have been deprecated earlier, but better late than never.

Avoid warnings in DatasetQueryResults._iter_by_dataset_type.

4f677dd

Drop component support and nomenclature from new query interface.

43d1db0

Since getting a component DatasetRef from a parent one is just a method call, there's no need for this complexity in the query system.

timj force-pushed the tickets/DM-36303 branch from 04dc549 to 43d1db0 Compare January 9, 2024 22:00

timj added 2 commits January 9, 2024 15:09

Add news fragment

4d10137

Add default value to query classes to allow usage to be detected

8ee1e29

The components parameter is deprecated. Warn if False is specified explicitly in addition to raising if any other value is used.

timj force-pushed the tickets/DM-36303 branch from fa2c67b to 8ee1e29 Compare January 9, 2024 23:37

timj merged commit eae7056 into main Jan 10, 2024
16 of 18 checks passed

timj deleted the tickets/DM-36303 branch January 10, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-36303: Replace RFC-878/879 deprecations with removals #891

DM-36303: Replace RFC-878/879 deprecations with removals #891

timj commented Sep 29, 2023 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

timj left a comment

timj Jan 5, 2024

TallJimbo Jan 9, 2024

timj Jan 5, 2024

TallJimbo Jan 9, 2024 •

edited

Loading

timj Jan 9, 2024

TallJimbo left a comment

TallJimbo Jan 9, 2024 •

edited

Loading

TallJimbo Jan 9, 2024

TallJimbo commented Jan 9, 2024

DM-36303: Replace RFC-878/879 deprecations with removals #891

DM-36303: Replace RFC-878/879 deprecations with removals #891

Conversation

timj commented Sep 29, 2023 • edited Loading

Checklist

codecov bot commented Jan 5, 2024 • edited Loading

Codecov Report

timj left a comment

Choose a reason for hiding this comment

timj Jan 5, 2024

Choose a reason for hiding this comment

TallJimbo Jan 9, 2024

Choose a reason for hiding this comment

timj Jan 5, 2024

Choose a reason for hiding this comment

TallJimbo Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

timj Jan 9, 2024

Choose a reason for hiding this comment

TallJimbo left a comment

Choose a reason for hiding this comment

TallJimbo Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

TallJimbo Jan 9, 2024

Choose a reason for hiding this comment

TallJimbo commented Jan 9, 2024

timj commented Sep 29, 2023 •

edited

Loading

codecov bot commented Jan 5, 2024 •

edited

Loading

TallJimbo Jan 9, 2024 •

edited

Loading

TallJimbo Jan 9, 2024 •

edited

Loading