Execute mypy pre-commit hook in main virtualenv to have access to dependencies #714
Conversation
WalkthroughThe changes involve modifications to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
`pre-commit` will by default create an isolated virtualenv to run hooks such as `mypy`. This creates well-known issues because `mypy` won't have access to packages the code depends on. See python/mypy#13916 for recommendations by the `mypy` maintainer. By running `mypy` as a `system` hook as suggested, it will see the exact same packages as running `mypy` standalone (or through an IDE) will, which avoids discrepancies that appear only at commit time. The compromise is that running `mypy` through `pre-commit` will require a properly initialized virtualenv. However because we add a `files:` filter to the `mypy` hook, `pre-commit` will not run if no Python file has been changed; and if Python files have been changed it seems reasonable to expect a proper development environment has been set up. Also following the recommendation we set `pass_filenames: false` so that `pre-commit` does not pass only the changed files to `mypy`.
jonathanperret
force-pushed
the
fix-mypy-numpy
branch
from
95ed966
to
1263c05
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.pre-commit-config.yaml (1)
16-23: Minor formatting issues and missing newline at the end of file.There are a few minor formatting issues in the YAML file:
- The indentation for the mypy hook (line 18) should be 10 spaces instead of 6 to align with other hooks.
- There should be only one space after the hyphen on line 18.
- There's no newline character at the end of the file (line 23).
To address these issues, please apply the following changes:
repos: - repo: https://github.com/pre-commit/pre-commit-hooks rev: v3.1.0 hooks: - id: check-yaml - repo: https://github.com/psf/black-pre-commit-mirror rev: 24.3.0 hooks: - id: black language_version: python3.11 - repo: https://github.com/pycqa/flake8 rev: '7.0.0' hooks: - id: flake8 additional_dependencies: [flake8-bugbear,flake8-sfs] - repo: local hooks: - - id: mypy + - id: mypy name: check types with mypy language: system pass_filenames: false entry: mypy src - files: ^src/.*.py$ + files: ^src/.*.py$ +These changes will improve the consistency of the YAML file and address the missing newline at the end of the file.
🧰 Tools
🪛 yamllint
[warning] 18-18: too many spaces after hyphen
(hyphens)
[warning] 18-18: wrong indentation: expected 10 but found 6
(indentation)
[error] 23-23: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .pre-commit-config.yaml (1 hunks)
- .pre-commit.mypy.ini (0 hunks)
💤 Files with no reviewable changes (1)
- .pre-commit.mypy.ini
🧰 Additional context used
🪛 yamllint
.pre-commit-config.yaml
[warning] 18-18: too many spaces after hyphen
(hyphens)
[warning] 18-18: wrong indentation: expected 10 but found 6
(indentation)
[error] 23-23: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.pre-commit-config.yaml (1)
16-23: LGTM! The mypy hook configuration looks good and aligns with the PR objectives.The changes effectively move the mypy hook from an external repository to a local configuration, which will allow it to run in the main virtualenv and access all project dependencies. This addresses the issues mentioned in the PR summary.
Key benefits of this configuration:
pass_filenames: falseenables whole-project analysis.entry: mypy srcruns mypy on the entiresrcdirectory.files: ^src/.*.py$ensures the hook only runs on Python files in thesrcdirectory.language: systemallows the hook to use the system's Python installation (project's virtualenv).These changes should resolve the discrepancies between running mypy directly and through pre-commit, as well as the internal error when processing files that import numpy.
🧰 Tools
🪛 yamllint
[warning] 18-18: too many spaces after hyphen
(hyphens)
[warning] 18-18: wrong indentation: expected 10 but found 6
(indentation)
[error] 23-23: no new line character at the end of file
(new-line-at-end-of-file)
Problem
Currently,
pre-commitis configured to runmypyin an isolated virtualenv. This causesmypyto be unable to see dependencies listed inrequirements.txtunless they are duplicated intoadditional_dependenciesin thepre-commitconfiguration.Because most dependencies are unavailable in that environment,
mypyis configured to not follow imports (follow_imports=skip) when invoked bypre-commit.This cause
mypyto give different results on some files when run directly (as is done in the CI workflow) versus throughpre-commit.This can easily be demonstrated by running
pre-commit run -a mypy, which executes themypyhook on all files. Not only are the exclusion patterns written inmypy.iniignored, the differing configuration causesmypyto rejectsrc/main/python/main/ayab/engine/communication.pyfor example whereas it is accepted bymypyrun directly.Furthermore, that same invocation of
pre-commitcausesmypyto crash when invoked on a Python file that importsnumpy, hitting python/mypy#17396.To reproduce:
Proposed solution
The issues coming from running
mypyin an isolatedpre-commitenvironment are documented in python/mypy#13916 by themypymaintainer.Two solutions are suggested there: either duplicate all dependencies from
requirements[.build].txtasadditional_dependencies, or configurepre-committo executemypyin the project's default virtual environment.In this PR I opted for the latter option, mainly because it requires less maintenance when dependencies change in the future.
The compromise is that running
mypythroughpre-commitwill require a properly initialized virtualenv. However because we add afiles:filter to themypyhook,pre-commitwill not run if no Python file has been changed; and if Python files have been changed it seems reasonable to expect a proper development environment has been set up.The
pass_filenamespre-commitoption is set tofalseas well, as recommended, so thatmypycan do whole-project analysis.Summary by CodeRabbit