DM-55980: Bring mypy coverage to 100% and gate on it - #196
Merged
Conversation
The tests were never actually type-checked: the [mypy-tests.*] ignore_errors section never matched, because the tests/ dir is not a package so mypy names the modules test_<name> (top-level), not tests.<name>. They were therefore checked under the base config, which neither required annotations nor looked inside unannotated function bodies, so nothing surfaced. Enable it properly by setting disallow_untyped_defs = True in the base [mypy] section; the existing [mypy-lsst.summit.utils.*] section already turns it off for the library source, so only the (top-level) test modules are newly required to be annotated. The dead [mypy-tests.*] sections are removed. This commit does the purely mechanical part: annotating every test function (mostly -> None) and declaring the class-level attributes that the unittest TestCases populate in setUpClass/setUp (so mypy knows self.foo exists). The EFD client and TMAEventMaker._data come from untyped libraries, so those attributes are typed Any. Real type errors that these newly-checked bodies reveal are fixed in the following commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With the test suite now type-checked, mypy sees inside the previously-skipped function bodies. These are the fixes that are genuine and self-contained: - Guard values that are statically optional before use: assert-not-None for a visit's start seqNum and a dataId seq_num, for getBlockInfo results (as the truth-value writer already does), and for request bodies before json.loads (requests types .body as bytes | str | None). - Annotate variables that are deliberately rebound to a different type across a test (a dayObs as str then int; a dataId as a DataCoordinate then a dict; ra/dec lists then their np.deg2rad arrays). - type: ignore the handful of lines mypy cannot know about: intentionally malformed obs_id tuples and a bad dayObs passed to assert they are rejected, dafButler.Butler(...) which the stubs mark abstract, and the dynamically-added PipelineTaskConnections descriptors. - Initialise expTime before the loop that may not run, and stop discarding the None return of _ipython_display_ / the unused assertNoLogs watcher. Three remaining errors are not mechanical at all - they point at a vacuous assertion and two annotations that are simply wrong - so each is fixed, with its rationale, in its own following commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
self.assertTrue(all(isinstance(n, str)) for n in blockNums) puts the "for" outside the all() call, so the argument to assertTrue is a generator object (always truthy) that is never iterated: all(isinstance(...)) is never even evaluated, and the check silently passed regardless of the block/seqNum types. Moving the closing paren makes all() consume the generator, so the assertion actually tests that every blockNum is a str and every seqNum an int, matching the correctly-parenthesised assertion just below it. This is EFD-gated so it does not run off-summit; the change is a pure parenthesisation fix that mypy now accepts (all() gets an iterable, not a lone bool). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The blockInfos field was annotated as list, but its own docstring says "or None", it is compared against None in __repr__ and in getEvent, and the EFD tests construct a TMAEvent with blockInfos=None. Widen the annotation to list | None to match reality (the default_factory=list default is unchanged, so nothing that relies on the empty-list default is affected). Widening exposed one place, TMAEvent.associatedWith, that iterated self.blockInfos with no None guard, unlike the guarded access in __repr__; iterate over `self.blockInfos or []` there so a None is treated the same as no block info. The test that checks an empty blockInfos is narrowed through a local so mypy is satisfied it is non-None at the len() call. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
quickSmooth was annotated NDArray[np.float64] on both the parameter and the return, but it is a thin wrapper over scipy.ndimage.gaussian_filter, which preserves the input dtype and happily accepts float32. Real callers already pass float32 (e.g. ImageExaminer on an afw image.array), and the unit test deliberately exercises the float32 interface. Widen both to NDArray[np.floating[Any]] so the annotation matches what the function actually accepts and returns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
requests types HTTPError.response as Response | None, so mypy rejects e.response.json(). raise_for_status() always attaches the response it was called on, so read the body from that response directly instead.
offset_time_aware() called utc.localize() on a zoneinfo.ZoneInfo, which has no such method - that is pytz's API. Any naive tick label reaching this branch would have raised AttributeError rather than being converted to Chilean time.
mfisherlevine
force-pushed
the
tickets/DM-55980
branch
3 times, most recently
from
August 28, 2026 14:29
94412da to
cd8aa58
Compare
Fills in every remaining unannotated and partially annotated definition the mypy-coverage report listed, so the whole package is body-checked by mypy rather than silently skipped. GuiderData.__getitem__ gets @overload signatures: a single return union of Stamps | ndarray | list[ndarray] would have forced narrowing at every gd[det, i] call site across plotting, detection and tracking, whereas the overloads give each key form its real type. black collapses the overload stub bodies onto the def line, so E704 joins the other black-disagrees- with-flake8 codes already ignored in setup.cfg.
Three holes let the typing work on this branch regress silently: The shared mypy workflow defaults to checking python/ only, so the test suite mypy.ini names in files= was never checked in CI at all - the [mypy-tests.*] ignore_errors section it used to rely on had never matched anything either, since tests/ is not a package. Checking tests/ turns up one config gap: [mypy-lsst.obs.base] matched only the exact module, not the makeRawVisitInfoViaObsInfo submodule test_utils imports, so that is folded into the [mypy-lsst.obs.base.*] section which already existed. CI installed no requests stubs, so mypy.ini's per-module ignore_missing_imports degraded every requests object to Any. That is why the HTTPError.response and request.body errors fixed earlier on this branch passed CI while failing locally. types.txt, which the shared workflow installs when present, now pins the stubs plus the pytest and responses imports the test modules need. The mypy-coverage workflow ran without a threshold and could never fail. It now gates fully-typed at 100%, which pins body-checked to 100% as well: body-checked counts the partially annotated definitions that fully-typed excludes, so leaving no gap in the latter leaves none in the former. "call-workflow / mypy" was already a required status check on main, so it gated considerably less than it looked like it did. "Annotate coverage gaps" has now been added to that list alongside it.
mfisherlevine
force-pushed
the
tickets/DM-55980
branch
from
August 28, 2026 14:35
cd8aa58 to
3d118ea
Compare
__str__ is a dunder: Python calls it with no arguments on every print(), str() and f-string, so its expid parameter could only ever be filled by an explicit self.__str__(expid) call - which is what summary() did, and the only such call there was. That forced a default, and any default is a lie: "" and None both render a blank exposure id line, and 0 renders a fictitious one. A DriftResult does not carry an exposure id, so __str__ no longer claims to know one and summary() takes a plain required int, printing the header itself. Output from summary(expid) is byte-identical; print() on a DriftResult now omits the vestigial blank "Exposure summary: " line.
fred3m
approved these changes
Aug 28, 2026
fred3m
left a comment
fred3m
left a comment
Contributor
There was a problem hiding this comment.
LGTM. My only suggestion is that the LLM generated commit descriptions are (as expected) verbose. It's getting better but as long as for this package you're still using the premise that only AI will actually look at them, it's fine. If you expect a person to read them then they should be simplified a bit.
mypy sees @deprecated from deprecated.sphinx as untyped here: the Deprecated package ships no py.typed, and mypy.ini silences the resulting import-untyped warning. An untyped decorator erases the type of everything it decorates, so makeDefaultLatissButler() returned Any, self.butler was Any, and the whole of ButlerUtilsTestCase went unchecked. Environments that do have type information for Deprecated (USDF's shared stack, for one) see the real signature and report 16 errors that CI never did. types.txt now pins types-Deprecated so every environment agrees. All 16 were real. Three annotations were narrower than their own bodies: updateDataIdOrDataCord and fillDataId both coerce with _assureDict(), which handles DimensionRecord, so both now accept one. getDatasetRefForDataId went the other way - it mutates the dataId with .update(), so passing it the DataCoordinate the test passes would have raised AttributeError had the expid been absent; it now coerces with _assureDict() first rather than widen a promise it cannot keep. In the tests, the butler is pinned to DirectButler where DirectButler -only APIs are used, the heterogeneous dataId lists are annotated so they do not join to object, and the materialised list of dimension records gets its own name rather than being assigned back over the query result it came from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An untyped decorator gives mypy an Any-returning callable, which erases the signature of the function it wraps: the definition still reads as fully annotated, and mypy still checks its body, but every caller of it goes unchecked. That is how an untyped @deprecated on makeDefaultLatissButler hid the whole of ButlerUtilsTestCase, and why mypy-coverage reported 100% while a test class went unexamined. Scoped to lsst.summit.utils rather than set globally: with the types-Deprecated stub the library is already clean, whereas the tests still carry 32 untyped @vcr.use_cassette() decorators. DM-55972 replaces those with @pytest.mark.vcr, after which this can move to [mypy]. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mypy-coverage report✅ All 796 definitions are fully annotated.
Summary
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.