DM-55980: Bring mypy coverage to 100% and gate on it

.github/workflows/mypy-coverage.yaml

@@ -1,18 +1,5 @@
 name: "mypy coverage"
 
-# Runs https://github.com/mfisherlevine/mypy_coverage on every PR and on
-# pushes to main. Currently informational only (no threshold), so it can
-# never block a merge -- the value is the inline annotations on the PR
-# diff for unannotated / partially annotated definitions, plus the
-# markdown coverage report posted as a sticky PR comment.
-#
-# This is complementary to mypy_check.yaml: that one is a pass/fail mypy
-# run; this one quantifies how much of the codebase is annotated at all.
-#
-# To gate on a coverage floor in the future, add ``threshold: 85`` (or
-# whatever) under ``with:`` and add the job's name to the required
-# status checks for ``main`` in the branch protection settings.
-
 "on":
   pull_request:
     branches:
@@ -28,6 +15,7 @@ concurrency:
 
 jobs:
   coverage:
+    # Required status check on main; renaming it strands every open PR.
     name: "Annotate coverage gaps"
     runs-on: ubuntu-latest
     timeout-minutes: 5
@@ -37,21 +25,17 @@ jobs:
     steps:
       - uses: actions/checkout@v4
       - id: mc
-        uses: mfisherlevine/mypy_coverage@v0.2.3
+        uses: mfisherlevine/mypy_coverage@v0.2.4
         with:
-          version: "0.2.3"
+          version: "0.2.4"
           format: github
-          # Use the repo's mypy.ini directly so we pick up the same
-          # exclude pattern, files=, and mypy_path= as the local mypy run.
           config: mypy.ini
-          # Drop the walled-off "Excluded files" section: it's noise in
-          # the PR comment and reviewers don't act on it.
           include-excluded: "false"
+          threshold: 100
+          threshold-metric: fully-typed
       - name: "Build markdown report"
         if: always()
-        # The action above pip-installs mypy-coverage, so the CLI is on
-        # PATH for the rest of the job. Re-run it here in markdown mode
-        # to produce a body we can post as a sticky PR comment.
+        # mypy-coverage is on PATH from the action above.
         run: |
           mypy-coverage \
             --color never \
@@ -64,11 +48,8 @@ jobs:
         if: always()
         run: cat "$RUNNER_TEMP/mypy-coverage.md" >> "$GITHUB_STEP_SUMMARY"
       - name: "Post sticky PR comment (recreated each run)"
-        # ``recreate: true`` deletes the previous comment and posts a
-        # fresh one each run, so the new comment gets a current
-        # timestamp and the PR conversation always renders it at the
-        # bottom (after every commit) instead of stranded at the top
-        # where it first appeared.
+        # recreate: true re-posts at the bottom of the conversation rather
+        # than leaving it stranded where it first appeared.
         if: always() && github.event_name == 'pull_request'
         uses: marocchino/sticky-pull-request-comment@v2
         with:

.github/workflows/mypy_check.yaml

@@ -11,4 +11,6 @@ jobs:
     uses: lsst/rubin_workflows/.github/workflows/mypy.yaml@main
     with:
       # https://github.com/python/mypy/issues/17002
-      mypy_package: mypy!=1.9.0
+      mypy_package: mypy!=1.9.0
+      # The shared workflow checks python/ only unless told otherwise.
+      folders: "python tests"

mypy.ini

@@ -5,10 +5,18 @@ warn_unused_configs = False
 warn_redundant_casts = False
 plugins = pydantic.mypy
 enable_error_code = possibly-undefined
+disallow_untyped_defs = True
 
 # Check all of summit_utils...
 [mypy-lsst.summit.utils.*]
 disallow_untyped_defs = False
+# An untyped decorator erases the signature of the function it wraps, so
+# every caller goes unchecked while the definition still looks fully
+# annotated - that is how an untyped @deprecated hid all of
+# ButlerUtilsTestCase. Scoped to the library rather than global because the
+# tests still carry untyped @vcr.use_cassette() decorators; promote it to
+# [mypy] once DM-55972 has replaced those with @pytest.mark.vcr.
+disallow_untyped_decorators = True
 disallow_incomplete_defs = False
 strict_equality = True
 
@@ -126,9 +134,6 @@ ignore_missing_imports = True
 [mypy-lsst.obs.lsst.cameraTransforms.*]
 ignore_missing_imports = True
 
-[mypy-lsst.obs.base]
-ignore_missing_imports = True
-
 [mypy-astropy.time]
 ignore_missing_imports = True
 
@@ -212,6 +217,7 @@ ignore_errors = False
 
 [mypy-lsst.obs.base.*]
 ignore_errors = True
+ignore_missing_imports = True
 
 [mypy-lsst.pipe.base.*]
 ignore_errors = True
@@ -222,5 +228,7 @@ ignore_errors = True
 [mypy-config.*]
 ignore_errors = True
 
-[mypy-tests.*]
-ignore_errors = True
+# Test modules (named ``test_*`` - the tests/ dir is not a package) are type-
+# checked under the base config above, which requires annotations. The library
+# source is exempted from that requirement by the disallow_untyped_defs = False
+# in the [mypy-lsst.summit.utils.*] section near the top of this file.

python/lsst/summit/utils/butlerUtils.py

@@ -467,7 +467,9 @@ def getExpIdFromDayObsSeqNum(butler: dafButler.Butler, dataId: dafButler.DataId)
     return {"exposure": expRecord.id}
 
 
-def updateDataIdOrDataCord(dataId: dafButler.DataId, **updateKwargs: Any) -> Mapping[str, Any]:
+def updateDataIdOrDataCord(
+    dataId: dafButler.DataId | dafButler.DimensionRecord, **updateKwargs: Any
+) -> Mapping[str, Any]:
     """Add key, value pairs to a dataId or data coordinate.
 
     Parameters
@@ -493,7 +495,9 @@ def updateDataIdOrDataCord(dataId: dafButler.DataId, **updateKwargs: Any) -> Map
     return newId
 
 
-def fillDataId(butler: DirectButler, dataId: dafButler.DataId) -> Mapping[str, Any]:
+def fillDataId(
+    butler: DirectButler, dataId: dafButler.DataId | dafButler.DimensionRecord
+) -> Mapping[str, Any]:
     """Given a dataId, fill it with values for all available dimensions.
 
     Parameters
@@ -651,7 +655,7 @@ def getDayObsSeqNumFromExposureId(butler: dafButler.Butler, dataId: Mapping[str,
 
 
 def getDatasetRefForDataId(
-    butler: dafButler.Butler, datasetType: str | dafButler.DatasetType, dataId: dict[str, Any]
+    butler: dafButler.Butler, datasetType: str | dafButler.DatasetType, dataId: dafButler.DataId
 ) -> dafButler.DatasetRef | None:
     """Get the datasetReference for a dataId.
 
@@ -661,19 +665,20 @@ def getDatasetRefForDataId(
         The butler.
     datasetType : `str` or `datasetType`
         The dataset type.
-    dataId : `dict[str, Any]`
+    dataId : `dafButler.DataId`
         The dataId.
 
     Returns
     -------
     datasetRef : `lsst.daf.butler.dimensions.DatasetReference`
         The dataset reference.
     """
-    if not _expid_present(dataId):
-        assert _dayobs_present(dataId) and _seqnum_present(dataId)
-        dataId.update(getExpIdFromDayObsSeqNum(butler, dataId))
+    dictId = _assureDict(dataId)
+    if not _expid_present(dictId):
+        assert _dayobs_present(dictId) and _seqnum_present(dictId)
+        dictId.update(getExpIdFromDayObsSeqNum(butler, dictId))
 
-    dRef = butler.find_dataset(datasetType, dataId)
+    dRef = butler.find_dataset(datasetType, dictId)
     return dRef
 
 

python/lsst/summit/utils/consdbClient.py

@@ -78,7 +78,9 @@ def _check_status(r: requests.Response) -> None:
         r.raise_for_status()
     except requests.HTTPError as e:
         try:
-            json_data = e.response.json()
+            # Use ``r`` rather than ``e.response``: raise_for_status always
+            # attaches this response, but its type is Response | None.
+            json_data = r.json()
             e.add_note(str(json_data))
             if "message" in json_data:
                 e.add_note(f"\n\n{json_data['message']}")
@@ -87,7 +89,7 @@ def _check_status(r: requests.Response) -> None:
         raise e
 
 
-def clean_url(resp: requests.Response, *args, **kwargs) -> requests.Response:
+def clean_url(resp: requests.Response, *args: Any, **kwargs: Any) -> requests.Response:
     """Parse url from response and remove netloc portion.
 
     Set new url in response and return response
@@ -456,7 +458,7 @@ def insert_flexible_metadata(
         values: dict[str, Any] | None = None,
         *,
         allow_update: bool = False,
-        **kwargs,
+        **kwargs: Any,
     ) -> requests.Response:
         """Set flexible metadata values for an observation.
 
@@ -516,7 +518,7 @@ def insert(
         values: Mapping[str, Any],
         *,
         allow_update: bool = False,
-        **kwargs,
+        **kwargs: Any,
     ) -> requests.Response:
         """Insert values into a single ConsDB fixed metadata table.
 
@@ -603,7 +605,7 @@ def insert_multiple(
         table: str,
         obs_dict: dict[int, dict[str, Any]],
         *,
-        allow_update=False,
+        allow_update: bool = False,
     ) -> requests.Response:
         """Insert values into a single ConsDB fixed metadata table.
 

python/lsst/summit/utils/guiders/plotting.py

@@ -352,7 +352,7 @@ def stripPlot(
         sigma = mad_std(yvals, ignore_nan=True)
         ylims = (p16 - 2.5 * sigma, p84 + 2.5 * sigma)
 
-        def _zero(ax, c):
+        def _zero(ax: plt.Axes, c: str) -> None:
             label = {
                 "daz": f"Az: {az:0.5f} deg",
                 "dalt": f"Alt: {alt:0.5f} deg",

python/lsst/summit/utils/guiders/reading.py

@@ -21,7 +21,8 @@
 from __future__ import annotations
 
 import logging
-from typing import TYPE_CHECKING, Any
+from collections.abc import Iterator
+from typing import TYPE_CHECKING, Any, overload
 
 __all__ = [
     "GuiderReader",
@@ -59,7 +60,7 @@
     from lsst.geom import SkyWcs
 
 
-def make_subplot(nrows=1, ncols=1, **fig_kwargs):
+def make_subplot(nrows: int = 1, ncols: int = 1, **fig_kwargs: Any) -> tuple[plt.Figure, Any]:
     """Return (fig, axs) using LSST's make_figure."""
     fig = make_figure(**fig_kwargs)
     axs = fig.subplots(nrows=nrows, ncols=ncols, squeeze=True)
@@ -533,25 +534,42 @@ def az(self) -> float:
         return float(raw)
 
     # Iterable / dict-like helpers
-    def __iter__(self):
+    def __iter__(self) -> Iterator[str]:  # type: ignore[override]
         """Iterate over detector names in guiderNames order."""
         return iter(self.guiderNames)
 
-    def items(self):
+    def items(self) -> Iterator[tuple[str, Stamps]]:
         """Yield (detName, stamps) pairs like dict.items()."""
         for det in self.guiderNames:
             yield det, self.stampsMap[det]
 
-    def keys(self):
+    def keys(self) -> Iterator[str]:
         """Iterate over detector names (dict-like .keys())."""
         return iter(self.guiderNames)
 
-    def values(self):
+    def values(self) -> Iterator[Stamps]:
         """Iterate over Stamps objects in guiderNames order."""
         for det in self.guiderNames:
             yield self.stampsMap[det]
 
-    def __getitem__(self, key):
+    @overload
+    def __getitem__(self, key: str) -> Stamps: ...
+
+    @overload
+    def __getitem__(self, key: int) -> np.ndarray: ...
+
+    @overload
+    def __getitem__(self, key: slice) -> list[np.ndarray]: ...
+
+    @overload
+    def __getitem__(self, key: tuple[str, int]) -> np.ndarray: ...
+
+    @overload
+    def __getitem__(self, key: tuple[str, slice]) -> list[np.ndarray]: ...
+
+    def __getitem__(
+        self, key: str | int | slice | tuple[str, int | slice]
+    ) -> Stamps | np.ndarray | list[np.ndarray]:
         """
         Direct stamp access helper.
 
@@ -625,7 +643,12 @@ def plotter(self) -> GuiderPlotter:
         return GuiderPlotter(self)
 
     def plotStamp(
-        self, detName: str, stampNum: int, plo: float = 90, phi: float = 99.5, figsize=(10, 8)
+        self,
+        detName: str,
+        stampNum: int,
+        plo: float = 90,
+        phi: float = 99.5,
+        figsize: tuple[float, float] = (10, 8),
     ) -> plt.Figure:
         """
         Plot a single guider stamp.