DM-36303: Replace RFC-878/879 deprecations with removals #891
Conversation
Codecov ReportAttention:
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. |
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm fine with handling components here like this.
| @@ -281,7 +281,7 @@ def queryDatasetTypes( | |||
| self, | |||
| expression: Any = ..., | |||
| *, | |||
| components: bool | None = None, | |||
| components: bool = False, | |||
There was a problem hiding this comment.
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 = _DefaultSentineland then warning if components is False ?
There was a problem hiding this comment.
👍, but we should instead warn if components is not _DefaultSentinal in case somebody is still passing None and isn't running MyPy.
There was a problem hiding this comment.
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.
timj
force-pushed
the
tickets/DM-36303
branch
2 times, most recently
from
cbbeb84
to
85ce59c
Compare
There was a problem hiding this comment.
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.
| @@ -281,7 +281,7 @@ def queryDatasetTypes( | |||
| self, | |||
| expression: Any = ..., | |||
| *, | |||
| components: bool | None = None, | |||
| components: bool = False, | |||
There was a problem hiding this comment.
👍, but we should instead warn if components is not _DefaultSentinal in case somebody is still passing None and isn't running MyPy.
| @@ -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) | |||
There was a problem hiding this comment.
I'm fine with handling components here like this.
|
I've pushed three new commits to deal with |
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.
Checklist
doc/changes