Conversation
`mypy openwpm` died in the conda env on numpy's own `__init__.pyi` -- "Type
statement is only supported in Python 3.12 and greater" -- before checking a
single OpenWPM file, while CI stayed green. Both halves of that were bugs.
`[tool.mypy] python_version` said 3.10 while environment.yaml pins Python
3.14.7. mypy applies that setting to the third-party stubs it follows, not just
to our code, so a stale value breaks on any dependency using newer syntax. It
also meant the project was type-checked at an older language level than it
actually ships. Set it to 3.14 and teach scripts/update.py to keep it there:
`sync_mypy_python_version` reads the conda env's Python and rewrites the setting
in the `[tool.mypy]` table, alongside the existing pre-commit-rev and
engines.node syncs, so a repin can no longer leave it behind.
CI missed it because the hook did not run in this environment. `mirrors-mypy`
installs mypy into its own venv, so it saw only the four packages named in
`additional_dependencies` and never resolved numpy at all. That list was a
second, hand-maintained copy of the dependency set -- exactly the thing that
invites drift, and the config comment already conceded it ("We may need to add
more and more dependencies here"). Everything not on it degraded to `Any`, so
the hook was passing largely by not looking.
Run mypy as a `language: system` hook instead. The pre-commit CI job already
sets up the conda env, so it now type-checks against the packages the project
actually installs and their bundled type information, and a local run matches
CI. The version is unchanged -- environment.yaml pins mypy=2.3.1, the same
release the removed `rev: v2.3.1` pointed at -- and it is now pinned in one
place instead of two, which is why mypy also comes out of update.py's
`_LINTER_MAP`: a system hook has no rev to sync, and leaving it there would
make the next repin raise. A regression test pins that.
`--scripts-are-modules` is carried over from the mirrors-mypy hook definition
and is load-bearing: without it a file with a shebang is named `__main__`, which
the `module = "test.*"` overrides would not match.
`--ignore-missing-imports`, also inherited, is dropped because pyproject.toml
already sets it.
No new errors surface: mypy in the conda env over all 86 tracked Python files is
clean.
There was a problem hiding this comment.
🔵 Needs a closer look
The moderate validation issue and both release-checklist documentation nits remain unresolved.
Pull request overview
This PR makes mypy type-check against the conda environment and Python 3.14.
Changes:
- Runs mypy through the system pre-commit hook.
- Synchronizes mypy’s Python version with conda.
- Updates synchronization tests and removes mypy revision syncing.
Open findings:
- Nit (1 vote):
.pre-commit-config.yaml:18references a no-longer-existent mypy revision in the release checklist. - Moderate (1 vote):
scripts/update.py:354should match active, non-commented assignments and add regression coverage. - Nit (1 vote):
scripts/update.py:30leaves the release checklist’s mypy revision description inaccurate.
File summaries
| File | Summary |
|---|---|
test/test_update_script.py |
Tests Python-version synchronization behavior. |
scripts/update.py |
Adds synchronization and removes mypy revision syncing. |
pyproject.toml |
Targets Python 3.14. |
.pre-commit-config.yaml |
Configures environment-backed mypy execution. |
Review details
Suppressed comments (3)
.pre-commit-config.yaml:20
- The release checklist still says
scripts/update.pysynchronizes mypy's pre-commit version (docs/Release-Checklist.md:7), but this change removes that rev and moves mypy to the conda-managed system hook. Please update that checklist in the same PR so release maintainers do not look for a non-existent mypy rev.
# set, free to drift from environment.yaml. Its version now comes from
# environment.yaml like every other tool, and `python_version` in
# pyproject.toml is kept in step by scripts/update.py.
scripts/update.py:354
- This search is not anchored to an active TOML key, so a table with only
# python_version = "..."(or a string containing that text) is treated as configured. The helper then rewrites the comment and returns successfully, leaving mypy without the intended setting instead of failing loudly; anchor the match to the start of a non-comment assignment (and add a regression test).
setting = re.search(r'python_version\s*=\s*"([^"]*)"', body)
scripts/update.py:34
- This change leaves
docs/Release-Checklist.md:7inaccurate: it still saysscripts/update.pysynchronizes black, isort, and mypy revisions, but mypy is now intentionally absent from_LINTER_MAPand has no pre-commitrev. Please update that checklist to describe only black/isort as rev-synced and note that mypy is sourced from the conda environment.
# Only tools pre-commit installs itself belong here. mypy is deliberately
# absent: it runs as a `language: system` hook so it can see the project's real
# dependencies, which means its version already comes from environment.yaml and
# there is no rev to pin. What does need syncing for mypy is `python_version`
# in pyproject.toml — see sync_mypy_python_version.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1236 +/- ##
==========================================
- Coverage 62.34% 62.31% -0.03%
==========================================
Files 40 40
Lines 3930 3930
==========================================
- Hits 2450 2449 -1
- Misses 1480 1481 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vringar
left a comment
There was a problem hiding this comment.
I feel like update.py should use actual parsers and not just string together regexes
The symptom
mypy openwpmdied in the conda env on numpy's own__init__.pyi— "Type statement is only supported in Python 3.12 and greater" — before checking a single OpenWPM file, while CI stayed green. Both halves of that were bugs.python_versionwas stale[tool.mypy] python_versionsaid3.10whileenvironment.yamlpins Python 3.14.7.mypy applies that setting to the third-party stubs it follows, not just to our own code, so a stale value breaks on any dependency using newer syntax. It also meant the project was being type-checked at an older language level than it actually ships.
Set to
3.14, andscripts/update.pygainssync_mypy_python_versionso a repin can't leave it behind again — alongside the existing pre-commit-rev andengines.nodesyncs. It rewrites only the[tool.mypy]table and fails loud if the section or the setting is missing, matching the convention of the other sync helpers.CI missed it because the hook didn't run in this environment
mirrors-mypyinstalls mypy into its own venv, so it saw only the four packages named inadditional_dependenciesand never resolved numpy at all. That list was a second, hand-maintained copy of the dependency set — and the config comment already conceded the treadmill ("We may need to add more and more dependencies here"). Everything not on it degraded toAny, so the hook was passing largely by not looking.mypy now runs as a
language: systemhook. Thepre-commitCI job already sets up the conda env, so no workflow change was needed, and it now type-checks against the packages the project actually installs and their bundled type information.environment.yamlpinsmypy=2.3.1, the same release the removedrev: v2.3.1pointed at. It's now pinned in one place instead of two.update.py's_LINTER_MAP: a system hook has no rev to sync, and leaving it there would make the next repin raise. A regression test pins that.--scripts-are-modulesis carried over from the mirrors-mypy hook definition and is load-bearing: without it a file with a shebang is named__main__, which themodule = "test.*"overrides would not match.test/test_http_instrumentation.pyhas one.--ignore-missing-imports, also inherited, is dropped becausepyproject.tomlalready sets it.Verification
No new errors surface: mypy in the conda env over all tracked Python files is clean.
test/test_update_script.pygrows from 9 to 15 tests, covering the new sync helper (update, no-op-without-rewriting, missing python, missing section, missing setting) plus a regression test asserting mypy stays out of_LINTER_MAP.Split out of #1230, where it was unrelated noise.