DM-27689: extend static type checking and fix errors #441

TallJimbo · 2020-11-23T20:22:24Z

No description provided.

A single __getattr__ replaces a lot of "type: ignore".

timj

Looks great. Standard comment about error messages on asserts 😄

timj · 2020-11-24T16:27:17Z

python/lsst/daf/butler/_butler.py

@@ -105,14 +107,14 @@ class PurgeWithoutUnstorePruneCollectionsError(PruneCollectionsArgsError):
    purge is True but unstore is False.
    """

-    def __init__(self):
+    def __init__(self) -> None:


How come this return type is needed is it?

Yeah, MyPy special-cases __init__ with type-annotated arguments to not need a return type annotation, but it needs the annotation here to be able to tell whether it should treat this as a typed method it should look at more closely or one it should ignore. And in strict mode it demands that everything be a typed method.

timj · 2020-11-24T16:39:32Z

python/lsst/daf/butler/_butler.py


            # Keep track of whether an item is associated with multiple
            # dimensions.
-            counter = Counter()
-            assigned: dict[Any, Set[str]] = defaultdict(set)
+            counter: Any = Counter()


I'm not sure what this type is doing. We know it's not Any -- it should be Counter shouldn't it?

I don't know why I assumed typing.Counter didn't exist, but of course it does (first I tried Dict, but that failed because most_common isn't a member, and then I got lazy). Fixed.

timj · 2020-11-24T16:46:15Z

python/lsst/daf/butler/_butler.py

@@ -778,7 +783,11 @@ def quote(s):
                                       f" records when constrained by {values}")

                # Get the primary key from the real dimension object
-                dimension = self.registry.dimensions[dimensionName]
+                dimension = self.registry.dimensions.getStaticDimensions()[dimensionName]
+                if not isinstance(dimension, Dimension):


How does this ever trigger given that the lookup above would KeyError if it wasn't. Is this just a mypy hint?

Yes, just a mypy hint - indexing DimensionUniverse can return Dimension or DimensionElement, so it was this or an assert isinstance check.

timj · 2020-11-24T16:47:10Z

python/lsst/daf/butler/_butler.py

@@ -1367,6 +1376,7 @@ def pruneDatasets(self, refs: Iterable[DatasetRef], *,
            if purge:
                self.registry.removeDatasets(refs)
            elif disassociate:
+                assert tags


Can I have an error text as well please?

timj · 2020-11-24T16:48:59Z

python/lsst/daf/butler/_butler.py

@@ -1613,14 +1623,14 @@ def import_(self, *, directory: Optional[str] = None,
            if filename is None:
                raise TypeError("At least one of 'filename' or 'format' must be provided.")
            else:
-                _, format = os.path.splitext(filename)
+                _, format = os.path.splitext(filename)  # type: ignore


Now I want filename to be a ButlerURI

I'll let you take care of that one 🙂

timj · 2020-11-24T16:53:16Z

python/lsst/daf/butler/core/_butlerUri/http.py

@@ -329,6 +329,8 @@ def session(self) -> requests.Session:
        if isWebdavEndpoint(baseURL):
            log.debug("%s looks like a Webdav endpoint.", baseURL)
            s = getHttpSession()
+        else:


@bgounon I'm not entirely sure what you wanted to happen in this else -- can you confirm this is okay?

@TallJimbo I'm fine with this merging as is -- it's clearly fixing a problem and if @bgounon comes back with a different fix that can be a different ticket.

timj · 2020-11-24T16:54:01Z

python/lsst/daf/butler/core/composites.py

@@ -137,6 +137,8 @@ def shouldBeDisassembled(self, entity: Union[DatasetRef, DatasetType, StorageCla
                matchName = key
                break

+        assert isinstance(disassemble, bool)


Error message? Would this be needed if we declared the type at line 132 above?

I should probably just upgrade this to a real exception because it's essentially external config validation. Of course, it's only the tip of that iceberg, but I figure every little bit helps.

timj · 2020-11-24T16:56:51Z

python/lsst/daf/butler/core/config.py

@@ -873,7 +875,9 @@ def dumpToUri(self, uri: Union[ButlerURI, str], updateFile: bool = True,
        ext = uri.getExtension()
        format = ext[1:].lower()

-        uri.write(self.dump(format=format).encode(), overwrite=overwrite)
+        output = self.dump(format=format)
+        assert output is not None


Error message? 😄

timj · 2020-11-24T16:59:50Z

python/lsst/daf/butler/formatters/parquet.py

@@ -178,8 +180,7 @@ def read(self, component: Optional[str] = None) -> Any:

        return loader.read(**self.fileDescriptor.parameters)

-    def write(self, inMemoryDataset: Any) -> str:
+    def write(self, inMemoryDataset: Any) -> None:


Sorry about that one. I thought I had caught them all.

This one wasn't being type-checked before, so you're absolved 🙂

timj · 2020-11-24T17:05:18Z

python/lsst/daf/butler/_butler.py

@@ -600,8 +605,8 @@ def _findDatasetRef(self, datasetRefOrType: Union[DatasetRef, DatasetType, str],

        # Process dimension records that are using record information
        # rather than ids
-        newDataId: dict[Any, Any] = {}
-        byRecord: dict[Any, dict[str, Any]] = defaultdict(dict)
+        newDataId: Dict[Any, Any] = {}


Oh, I think you mentioned something about proper types for data Id keys and values?

This was a bit trickier than I expected, due to the messiness of Mapping[Union[str, Dimension], ...] vs. Union[Mapping[str, ...], Mapping[Dimension, ...]]. Rather than fight that I just made these dicts use str keys and coerced the one place where they might have been given Dimension instances.

At least a couple of real bugs in this bunch, as well as a lot of annotation problems and missing annotations.

MyPy didn't spot these, but the pylance language server in VSCode did. These are the ones where it's pretty clearly correct and the fix is simple. There were many more where the change wasn't a clear improvement (I'm not sure it's more strict or less intelligent than MyPy, or just differently strict and intelligent).

TallJimbo force-pushed the tickets/DM-27689 branch 2 times, most recently from 09e8743 to e23528a Compare November 23, 2020 20:30

Simplify mypy handling of DimensionRecord's dynamic type creation.

a9f4ff7

A single __getattr__ replaces a lot of "type: ignore".

TallJimbo force-pushed the tickets/DM-27689 branch 2 times, most recently from f3fd1c9 to 3a1fca0 Compare November 24, 2020 04:34

timj approved these changes Nov 24, 2020

View reviewed changes

timj reviewed Nov 24, 2020

View reviewed changes

Extend mypy config to more modules, fix revealed errors.

1cf5b35

At least a couple of real bugs in this bunch, as well as a lot of annotation problems and missing annotations.

TallJimbo force-pushed the tickets/DM-27689 branch from 3a1fca0 to 5bc0660 Compare November 24, 2020 19:32

TallJimbo added 2 commits November 24, 2020 14:43

Use cached_getter in DimensionUniverse.primaryKeyTraversalOrder.

556b985

TallJimbo force-pushed the tickets/DM-27689 branch from 5bc0660 to 556b985 Compare November 24, 2020 19:43

TallJimbo merged commit df93a10 into master Nov 24, 2020

TallJimbo deleted the tickets/DM-27689 branch November 24, 2020 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-27689: extend static type checking and fix errors #441

DM-27689: extend static type checking and fix errors #441

TallJimbo commented Nov 23, 2020

timj left a comment

timj Nov 24, 2020

TallJimbo Nov 24, 2020

timj Nov 24, 2020

TallJimbo Nov 24, 2020

timj Nov 24, 2020

TallJimbo Nov 24, 2020

timj Nov 24, 2020

timj Nov 24, 2020

TallJimbo Nov 24, 2020

timj Nov 24, 2020

timj Nov 24, 2020

timj Nov 24, 2020

TallJimbo Nov 24, 2020

timj Nov 24, 2020

timj Nov 24, 2020

TallJimbo Nov 24, 2020

timj Nov 24, 2020

TallJimbo Nov 24, 2020

DM-27689: extend static type checking and fix errors #441

DM-27689: extend static type checking and fix errors #441

Conversation

TallJimbo commented Nov 23, 2020

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

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