CI: mypy&pyright in manual pre-commit stage #47075
Conversation
| name: mypy | ||
| entry: mypy | ||
| # note: assumes python env is setup and activated | ||
| language: system |
There was a problem hiding this comment.
would it work to use the mypy hook, and add numpy (and whatever else is needed for types) in additional_dependencies? I tried this a while ago and there some were inconsistencies with how mypy runs on the whole project versus as a hook, but if we were able to get it to pass as a hook, then it'd solve a whole bunch of typing headaches
I've seen a bunch of other projects do this, and it seems pretty reliable
There was a problem hiding this comment.
I would like that! I think the only downside is that the current pre-commit run is very fast as it doesn't need to install all the dependencies from environment.yml. I'm not familiar with pre-commit, is there a nice way of sharing the exact dependency versions (basically everything in environment.yml)?
I guess to get pyright working, we would need to use its venv argument to point it to pre-commit/mypy's environment (I don't think pre-commit allows installing python dependencies for a node program).
There was a problem hiding this comment.
does it need to be everything in environment.yml? I would've thought that only a few packages would be necessary, like numpy and maybe pytest
is there a nice way of sharing the exact dependency versions (basically everything in environment.yml)?
I don't think so, other than writing our own script (like the one that checks flake8 versions match)
There was a problem hiding this comment.
does it need to be everything in
environment.yml?
Probably almost everything: imported libraries might have type annotations (and pyright explicitly checks that it can find all imported modules).
I think I will just see what a mess it will be :) then we can decide on whether it is worth it or not
There was a problem hiding this comment.
😄 no worries then, system is fine, as long as the stage is manual (it used to not be system, and then there were a tonne of false-positives, and people would tend to just turn off pre-commit)
There was a problem hiding this comment.
Okay, let's try system for this PR! Two questions:
- Does github action create a new shell when executing pre-commit: the system hook doesn't find mypy from the activated conda env?
- It would be great to have only pyright and mypy in
manual. By default all hooks are in all stages. Could changedefault_stages, then we need to call pre-commit only once for mypy+pyright (might reduce some overhead). Do you think that would be helpful? If so, what value shoulddefault_stageshave?
There was a problem hiding this comment.
It is actually not that bad listing all dependencies for mypy, see the latest commit.
The issue is to programmatically find the python env that pre-commit created for mypy so that pyright can use it as well.
While it is possible to move mypy and later move pyright, I would really like to make sure that pyright uses the same env as mypy, as pyright errors when it encounters any missing modules. It is easy to forget a module, which is then resolved as Any (can hide errors).
There was a problem hiding this comment.
I thought I found an ugly way that should work on linux but it seems that pyright (and probably mypy too?) needs pandas to be compiled for some of its checks, see the error in pre-commit now.
Compiling pandas in the pre-commit is a little bit too much (more dependencies and it takes much longer). Let's go back to the system+manual approach.
There was a problem hiding this comment.
@MarcoGorelli I'm mostly pattern matching when it comes to github action+pre-commit but I found a working system+manual version :)
jreback
added
CI
.github/workflows/code-checks.yml
Outdated
| - name: Mypy | ||
| uses: pre-commit/[email protected] | ||
| with: | ||
| extra_args: --hook-stage manual --all-files mypy |
There was a problem hiding this comment.
Is it possible to both run mypy and pyright in this step and have this be a - name: Typing check?
There was a problem hiding this comment.
As far as I know, that is possible if mypy&pyright are in the same stage (that is the case) AND if no other hook is in this stage.
I'm not sure how people use per-commit, could reserve manual for mypy&pyright.
There was a problem hiding this comment.
That sounds good to me (for now). We could reserve manual for more expensive code checks like these
There was a problem hiding this comment.
Somehow two of the hooks are not affected by default_stages, so I explicitly set their stages.
I had a run, but then I rebased&force pushed :( Happy to wait until the doc-issue is fixed. |
|
I rebased but removed #47018 should hopefully work now |
Seems like there is another doc issue. I included the commit again. I'll rebase when main is green. |
Co-authored-by: Marco Edward Gorelli <[email protected]>
|
Sweet thanks @twoertwein! Yeah this should be fine despite the failure on main |
* CI: mypy&pyright in manual pre-commit stage * move mypy+pyright to pre-commit * Revert "move mypy+pyright to pre-commit" * system+manual * Update .pre-commit-config.yaml Co-authored-by: Marco Edward Gorelli <[email protected]> Co-authored-by: Marco Edward Gorelli <[email protected]>
xref #47067 (comment)