Enforce mypy in CI behind a ratchet - #757
Conversation
Nothing in CI type-checks today. The lint job runs `scripts/check` and nothing else, and the mypy line in that script has been commented out since the file was added in df20a17 -- it never ran. `tox -e typecheck` and `make typecheck` do invoke `mypy -p faust`, but no workflow invokes tox or make, so they only fire if a contributor runs them by hand. The `[tool.mypy]` block in pyproject.toml has been configured the whole time with nothing reading it in anger. flake8 cannot cover for it: pycodestyle and pyflakes do no type inference, so implicit-Optional defaults and missing annotations are invisible to the checks that do run. `mypy -p faust` reports 692 errors across 78 modules, so it cannot simply be switched on. Instead, silence exactly those 78 modules with `ignore_errors` and enforce the other 86. Everything outside the list is checked, including every module added from here on, and the list is documented to only ever shrink. Two things had to be nailed down first, or the check would be red on one machine and green on another: * `python_version` was unset, so mypy targeted whichever interpreter ran it. Pin it to 3.12, the lint job's interpreter -- typeshed differs enough between versions to change the verdict (`asyncio.wait` in faust/app/_attached.py is clean at 3.10 and an error from 3.11 on). * `mypy>=0.750` floats to whatever is newest, and the enforced set moves with it -- 1.19.1 and 2.3.0 disagree about 6 modules, largely because 2.x reads inline types from py.typed packages that 1.x ignored. Pin it, for the reason test.txt already pins the formatters, and pull typecheck.txt into test.txt so the lint job installs it. Verified that `scripts/check` exits zero both with and without confluent-kafka installed -- the lint job does not install that extra, and the silenced set is identical either way -- and that adding an unannotated function to an enforced module fails the check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012TbXZ1ATm7RvvDsqgZ3Xy6
58c506a to
ad3091c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #757 +/- ##
=======================================
Coverage 96.06% 96.06%
=======================================
Files 103 103
Lines 11072 11072
Branches 1191 1191
=======================================
Hits 10636 10636
Misses 345 345
Partials 91 91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note on the red This PR changes no source files ( Project coverage oscillates in a narrow band across commits on
This PR reports 96.04% — the same figure Two things make that fail rather than pass:
Worth noting this check is advisory here: the branch-protection gate is the Separately, the result this PR was really testing: If the coverage threshold is worth tightening up, adding a Generated by Claude Code |
|
Correcting my comment above: all checks are now green, and my explanation for the red Once the last two legs uploaded, codecov re-reported So the whole thing was the incomplete upload set from start to finish — first 5 of 16 uploads (32.55%), then 14 of 16 (-0.02%), then all 16 (+0.00%). There was never any coverage delta. My "run-to-run jitter" reasoning was a bad inference: I cited 96.04% and 95.98% from earlier What stands from the earlier comment: this PR changes no source files, and Generated by Claude Code |
There was a problem hiding this comment.
🟢 Ready to approve
The changes consistently wire mypy into CI with deterministic pins and a clear per-module ratchet, without altering runtime code paths.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR wires mypy into the existing CI lint job via scripts/check, but “behind a ratchet” so only already-clean modules are enforced while known-dirty modules are silenced via per-module mypy overrides. This makes type-checking effective immediately without requiring a large, blocking cleanup of the current backlog of typing errors.
Changes:
- Enable
mypy -p faustinscripts/check(CI lint job), relying on per-moduleignore_errorsoverrides as a ratchet. - Pin mypy to a specific version to keep CI verdicts stable over time.
- Pin mypy’s target
python_versionto avoid interpreter-dependent results.
File summaries
| File | Description |
|---|---|
| scripts/check | Runs mypy -p faust as part of the lint script, enforcing typing for modules not in the ratchet ignore list. |
| requirements/typecheck.txt | Pins mypy to a specific version to prevent CI drift as mypy evolves. |
| requirements/test.txt | Ensures CI installs the pinned mypy by including typecheck.txt in the lint/test requirements set. |
| pyproject.toml | Adds python_version pin and the per-module ignore_errors ratchet list under [[tool.mypy.overrides]]. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Description
Follow-up to #756, which slipped through because nothing in CI type-checks.
scripts/checkand nothing else, and the mypy line in that script has been commented out since the file was added indf20a17— it never ran.tox -e typecheckandmake typecheckdo invokemypy -p faust, but no workflow invokes tox or make, so they only fire if a contributor runs them by hand.[tool.mypy]inpyproject.tomlhas been configured the whole time with nothing reading it in anger.Optionaldefaults and missing annotations are invisible to the checks that do run.mypy -p faustreports 692 errors across 78 modules, so it can't simply be switched on. This adds a ratchet instead.How it works
[[tool.mypy.overrides]]withignore_errors = truesilences exactly the 78 modules that still have errors. The other 86 are enforced — as is every module added to the package from here on, since anything not on the list is checked by default. The list is documented to only ever shrink: cleaning up a module means deleting its line in the same PR, so it can't regress.Using mypy's own per-module config rather than a shell allowlist or grep-filtered output means
mypy -p faustjust exits zero, andtox -e typecheck/make typecheckbecome useful again for free.Two things had to be pinned first
Without these the check is red on one machine and green on another:
python_versionwas unset, so mypy targeted whichever interpreter ran it. Pinned to3.12, the lint job's interpreter. Typeshed differs enough between versions to change the verdict —faust/app/_attached.pyis clean targeting 3.10 but reportsasyncio.waitreceiving a bareAwaitablefrom 3.11 on, which is a real hazard given the CI matrix runs through 3.14.mypy>=0.750floats to whatever is newest, and the enforced set moves with it. 1.19.1 and 2.3.0 disagree about 6 modules, largely because 2.x reads inline types frompy.typedpackages that 1.x ignored. Pinned to2.3.0for the same reasontest.txtalready pins the formatters, andtypecheck.txtis now pulled intotest.txtso the lint job installs it.Verification
Against merged
master(aeec5f3):scripts/checkexits0with confluent-kafka installed and without it — the lint job doesn't install that extra, and the silenced set is identical either way, so the check can't pass locally and fail in CI over an optional dependency.def _ratchet_probe(x): return xtofaust/utils/urls.py(an enforced module) fails the check witherror: Function is missing a type annotation, exit1. Reverted after testing.pyproject.toml,scripts/check, and two requirements files.Note on scope
faust/transport/drivers/confluent.pyis on the silenced list despite #756. Under the pinned mypy that PR takes it from 53 errors to 17, not to zero: annotatingAsyncConsumer.__init__lets mypy resolveself.consumerto the realconfluent_kafka.Consumerinstead ofAny, which surfaces pre-existing bugs (AsyncConsumerhas noseekorseek_to_beginning; sync callables passed tocall_thread, which wants awaitables). Those are real defects worth their own PR, and removing that line from the ratchet list is the natural way to close it out.