DM-34340: implement RFC-834 deprecations #903

TallJimbo · 2023-11-06T17:38:48Z

Checklist

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

codecov · 2023-11-10T20:38:06Z

Codecov Report

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

Comparison is base (f22234c) 87.72% compared to head (4d005f7) 87.45%.

Files	Patch %	Lines
...daf/butler/dimensions/_data_coordinate_iterable.py	44.04%	31 Missing and 16 partials ⚠️
python/lsst/daf/butler/dimensions/_graph.py	66.03%	34 Missing and 2 partials ⚠️
python/lsst/daf/butler/dimensions/_coordinate.py	87.11%	21 Missing and 8 partials ⚠️
python/lsst/daf/butler/dimensions/_group.py	90.50%	13 Missing and 4 partials ⚠️
python/lsst/daf/butler/_dataset_type.py	48.00%	9 Missing and 4 partials ⚠️
python/lsst/daf/butler/dimensions/_universe.py	87.50%	6 Missing and 1 partial ⚠️
python/lsst/daf/butler/registry/sql_registry.py	66.66%	5 Missing and 1 partial ⚠️
...ython/lsst/daf/butler/registry/queries/_structs.py	76.19%	3 Missing and 2 partials ⚠️
python/lsst/daf/butler/_named.py	75.00%	4 Missing ⚠️
python/lsst/daf/butler/dimensions/_packer.py	63.63%	3 Missing and 1 partial ⚠️
... and 12 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #903      +/-   ##
==========================================
- Coverage   87.72%   87.45%   -0.27%     
==========================================
  Files         285      286       +1     
  Lines       36760    37144     +384     
  Branches     7698     7828     +130     
==========================================
+ Hits        32246    32485     +239     
- Misses       3341     3474     +133     
- Partials     1173     1185      +12

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

timj

Looks great. DimensionGroup is much better name than DimensionGraph and I see that many isinstance checks have been removed.

doc/changes/DM-34340.api.md

timj · 2023-11-16T20:12:45Z

python/lsst/daf/butler/_dataset_type.py

@@ -355,7 +362,7 @@ def dimensions(self) -> DimensionGraph:
        The dimensions label and relate instances of this
        `DatasetType` (`DimensionGraph`).
        """
-        return self._dimensions
+        return self._dimensions._as_graph()


This property will change to DimensionGroup at some point? Also typo in doc string.

This property will change to DimensionGroup at some point?

Yes, when DimensionGraph is removed this will become DimensionGroup, and I think it should be transparently to code that isn't doing deprecated things with it.

Also typo in doc string.

Rewrote it completely, since Dimension instances are no longer in play and the parenthetical type somehow had gotten bumped down to the second paragraph.

timj · 2023-11-16T20:13:07Z

python/lsst/daf/butler/_dataset_type.py

@@ -661,7 +668,7 @@ def to_simple(self, minimal: bool = False) -> SerializedDatasetType:
                "name": self.name,
                "storageClass": self._storageClassName,
                "isCalibration": self._isCalibration,
-                "dimensions": self.dimensions.to_simple(),
+                "dimensions": list(self.dimensions.names),


Not self._dimensions ?

timj · 2023-11-16T20:23:07Z

python/lsst/daf/butler/dimensions/_coordinate.py

    functions for private concrete implementations that should be sufficient
    for most purposes.  `standardize` is the most flexible and safe of these;
-    the others (`makeEmpty`, `fromRequiredValues`, and `fromFullValues`) are
-    more specialized and perform little or no checking of inputs.
+    the others (`makeEmpty`, `from_required_values`, and `from_full_values`)


Now the makeEmpty looks incongruous in that it's not make_empty...

Should I add make_empty, but leave makeEmpty un-deprecated (for now)?

We could start aliasing things so that makeEmpty redirects to make_empty. Then we can use the consistent form internally at least.

timj · 2023-11-20T16:13:44Z

python/lsst/daf/butler/dimensions/_graph.py

-        }
+    @property
+    @cached_getter
+    def dimensions(self) -> NamedValueAbstractSet[Dimension]:


This is now a deprecated getter?

Good catch. Will deprecate and fix any fallout.

timj · 2023-11-20T16:49:18Z

python/lsst/daf/butler/dimensions/_group.py

+        return super().__eq__(other)
+
+    @classmethod
+    def _from_iterable(cls, iterable: Iterable[str]) -> set[str]:


What calls this private method?

~collections.abc.Set, in mixin methods like __and__. Will add a comment.

timj · 2023-11-20T16:56:46Z

python/lsst/daf/butler/dimensions/_group.py

+        return self.intersection(other)
+
+    @property
+    def data_coordinate_keys(self) -> Set[str]:


It seems odd that this is talking about returning an ordered entity that is a Set but this is really a KeysView.

KeysView implements Set, and Set does not specify order one way or the other, so annotating this as Set is a sort of encapsulation: the user should not care that data_coordinate_keys is implemented as the keys of a dict.

In fact, I actually think it's a huge shame that KeysView, ValuesView, and ItemsView are defined collections.abc at all, let alone included in the type annotation for Mapping, as they are concrete classes that add nothing in terms of interfaces. Mapping.keys() should have been annotated to return Set, and the other two should have been annotated to return Collection.

python/lsst/daf/butler/registry/datasets/byDimensions/_manager.py

timj · 2023-11-20T17:14:48Z

python/lsst/daf/butler/script/queryDatasets.py

-        dimensions = list(refInfo.datasetRef.dataId.full.keys())
+        dimensions = [
+            refInfo.datasetRef.dataId.universe.dimensions[k]
+            for k in refInfo.datasetRef.dataId.dimensions.data_coordinate_keys


This seems a bit more convoluted than the original. I'm trying to understand why the data_coordinate_keys aren't enough given that they are converted to strings in the next line.

The list of actual Dimension instances is passed to sortAstropyTable, down below.

This adds a lot of warnings; DimensionGraph usage will be migrated in later commits. Due to those warnings, one test in test_obscore.py has been temporarily disabled, because it counts warnings, and I couldn't figure out how to make it only count the kind of warning it's actually looking for.

The confusingly-named DimensionUniverse.getStatic* methods are no longer needed after this, but while I think the middleware team has informally agreed we should drop them and go back to attributes, I don't think that's been RFC'd, so they stay for now. The new skypix attributes help with the other goal here: using set-membership tests rather than isinstance checks in places where we care about different types of dimensions. This will work better when DimensionElement objects appear more rarely, in favor of str names.

This includes replacing '.graph' with '.dimensions' (to go along with replacing DimensionGraph with DimensionGroup), adding '.mapping' and '.required' to replace '.full', and deprecating Dimension-instance lookup and iteration (which effectively deprecates the Mapping interface).

DatasetType.dimensions needs to continue to return DimensionGraph during the deprecation period, since that's the name we want to use long-term (unlike e.g. DataCoordinate.graph). That means we rely on the fact that we've also deprecated all of the things DimensionGraph can do that DimensionGroup can't do (like iteration), and we rely on those warnings instead of making DatasetType.dimensions itself warn. That should let a lot of usage just blindly pass DatasetType.dimensions to something else and not care about what type it is throughout the deprecation period.

Only usage was in DimensionGraph.

The old DataCoordinate.makeEmpty is not being deprecated because that wasn't included on RFC-834 and we don't have a good reason to get rid it, but we've now got a consistent suite of DataCoordinate factory methods with from_full_values and from_required_values (whose camelCase forms _did_ have a good reason to be retired, as this aided with the transition from DimensionGraph to DimensionGroup).

TallJimbo force-pushed the tickets/DM-34340 branch 4 times, most recently from b5a773a to d34e126 Compare November 10, 2023 20:25

TallJimbo force-pushed the tickets/DM-34340 branch 3 times, most recently from 3e6b773 to 95299b5 Compare November 16, 2023 15:10

timj approved these changes Nov 20, 2023

View reviewed changes

TallJimbo added 14 commits November 20, 2023 15:50

Use typing.overload to clean up DimensionUniverse.sorted annotations.

98141ff

Use typing.overload to clean up Named container annotations.

b39a323

Modify TopologicalFamily.choose to avoid sets of dimensions.

699fd11

Add DimensionElement.minimal_group to replace .graph.

71e786a

Use DimensionGroup in DataCoordinateIterables and query system.

ed29a9b

Switch to DimensionGroup in DimensionPackers.

f4b7fa5

Fix usage of now-deprecated interfaces.

371110a

Accept DimensionGroup in Registry.queryDataIds.

0d9f226

Rewrap some docstring text.

a81634d

Deprecate DimensionUniverse.expandDimensionNameSet.

f4833e1

Only usage was in DimensionGraph.

TallJimbo force-pushed the tickets/DM-34340 branch from 3e0306f to 0de720a Compare November 20, 2023 20:59

TallJimbo added 2 commits November 21, 2023 14:00

Add extensive changelog entry.

6519a1e

TallJimbo force-pushed the tickets/DM-34340 branch from 2d5f464 to 4d005f7 Compare November 21, 2023 19:00

TallJimbo merged commit 8ca025c into main Nov 22, 2023
15 of 17 checks passed

TallJimbo deleted the tickets/DM-34340 branch November 22, 2023 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-34340: implement RFC-834 deprecations #903

DM-34340: implement RFC-834 deprecations #903

TallJimbo commented Nov 6, 2023 •

edited

codecov bot commented Nov 10, 2023 •

edited

timj left a comment

timj Nov 16, 2023

TallJimbo Nov 20, 2023

timj Nov 16, 2023

timj Nov 16, 2023

TallJimbo Nov 20, 2023

timj Nov 20, 2023

timj Nov 20, 2023

TallJimbo Nov 20, 2023

timj Nov 20, 2023

TallJimbo Nov 20, 2023

timj Nov 20, 2023

TallJimbo Nov 20, 2023

timj Nov 20, 2023

TallJimbo Nov 20, 2023

DM-34340: implement RFC-834 deprecations #903

DM-34340: implement RFC-834 deprecations #903

Conversation

TallJimbo commented Nov 6, 2023 • edited

Checklist

codecov bot commented Nov 10, 2023 • edited

Codecov Report

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo commented Nov 6, 2023 •

edited

codecov bot commented Nov 10, 2023 •

edited