-
Notifications
You must be signed in to change notification settings - Fork 180
Add type-checking #2174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add type-checking #2174
Conversation
szabgab
commented
Oct 25, 2025
- Add .mypy.ini
- Add mypy to pre-commit
- Add mypy to GitHub Actioons
- Starts working on Add type-annotation, enable mypy to check it #2173
- Tests added
- Release note added (or unnecessary)
.github/workflows/mypy.yml
Outdated
| @@ -0,0 +1,26 @@ | |||
| name: Run mypy | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have a pre-commit bot that already runs on PRs, so in theory should be running the added mypy to the .pre-commit-config.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, Now I can see it is triggered in .github/workflows/check-pr.yml.
What is unclear to me why is it failing on the pre-commit.ci server. It seems that it does not take in account the .mypy.ini that excludes the duplicate files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I run pre-commit run -a locally, I get the same error as we can see on pre-commit.ci
mypy.....................................................................Failed
- hook id: mypy
- exit code: 2
src/anndata/_settings.pyi: error: Duplicate module named "anndata._settings" (also at "src/anndata/_settings.py")
Found 1 error in 1 file (errors prevented further checking)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've commented out mypy in the pre-commit hook to make ti pass on the pre-commit.ci server.
We still have it on GitHub Actions.
Either someone will figure out what's the problem on the pre-commit.ci server or this can be revisited later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: what Lukas is saying in the review, it should probably be the reverse. We should figure out why things aren't working on the pre-commit ci instead of trying to use github actions.
|
I don't understand why we would add a GHA workflow instead of just adding mypy to the pre-commit config. The latter should suffice. I also suggest that we put the mypy config into the pyproject.toml file. Thanks @szabgab for getting this started! |
|
I've moved the mypy configuration to I still could not figure out why does mypy fail on the pre-commit.ci while passes on GHA. That's the reason I kept GHA. |
Because pre-commit creates isolated environments. You’d need to duplicate all dependencies in |
* Add .mypy.ini * Add mypy to pre-commit * Add mypy to GitHub Actioons See scverse#2173
|
It seems that rebasing to |
|
Starting with version 1.12, mypy supports this syntax: https://mypy.readthedocs.io/en/latest/changelog.html#support-python-3-12-syntax-for-generics-pep-695 I see an internal error, so it’s for sure a mypy bug though! |
|
Seems like it’s either fixed on master or the “49 files” mypy checks are |
|
So maybe you could go ahead and merge this now. |
| - id: codespell | ||
| additional_dependencies: | ||
| - tomli | ||
| # See https://github.com/scverse/anndata/pull/2174 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also exclude them like this:
https://github.com/laminlabs/lamindb/blob/main/.pre-commit-config.yaml#L71
| @@ -0,0 +1,27 @@ | |||
| name: mypy | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that this whole workflow shouldn't exist and we can make it work with pre-commit. See below for the hammer - just exclude the unimportant conftest files.
| python-version: ${{ env.max_python_version }} | ||
| - name: Install mypy | ||
| run: | | ||
| pip install 'mypy @ git+https://github.com/python/mypy.git' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we were to use this workflow, we shouldn't install from Github. We can just use a release.
| @@ -1,5 +1,6 @@ | |||
| ci: | |||
| autoupdate_commit_msg: "ci: pre-commit autoupdate" | |||
| skip: [mypy] # too big | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| skip: [mypy] # too big |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Zethson did you test that it works? pre-commit.ci often cries when the mypy environment contains a halfway useful subset of a package’s dependencies, so I basically have to add this skip to almost everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I did not but I have never had to use it anywhere. But you're probably right then