Add optional mypy check

[shwina]

No description provided.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[shwina]

/ok to test

[github-actions]

Doc Preview CI
🚀 View preview at https://nvidia.github.io/cuda-python/pr-preview/pr-505/
https://nvidia.github.io/cuda-python/pr-preview/pr-505/cuda-core/
https://nvidia.github.io/cuda-python/pr-preview/pr-505/cuda-bindings/
Preview will be ready when the GitHub Pages deployment is complete.

[shwina]

/ok to test

[rwgk]

I developed the feeling that mypy is losing momentum. So I asked like this:

We want to add Python type checking to pre-commit and .github/workflows. mypy is one option. Are there other options we should consider?

• https://chatgpt.com/share/67cf1139-82b0-8008-b41c-848d9047e207

Picking out one point from the response:

Pyright is often used for editor-level feedback since it's fast and lightweight, but it works well for CI too.

I believe supporting "editor-level feedback" is the single most valuable motivation for investing time into typing. — Other than that, in my experience, typing is only a moderate win overall. (ruff gives us a lot of protections already.)

I see from the CI output that we have a sizable cleanup job. Do we want to target mypy, or pyright? There is probably a big overlap, but I expect that we'll spend a significant fraction of time dealing with idiosyncrasies of one or the other.

[shwina]

I developed the feeling that mypy is losing momentum. So I asked like this

Are there examples of projects using pyright for enforcing type checks in CI? I looked at some existing "big" Python projects to see what they were using:

• PyTorch uses MyPy
• NumPy uses MyPy
• scikit-learn uses MyPy

[kkraus14]

Would it make sense to integrate this with pre-commit and then make the CI job call just that task in pre-commit for now? Would maybe be nice to just make this part of the developer workflow to start getting fixes in?

[shwina]

Here's how the pre-commit results get reported:

https://results.pre-commit.ci/run/github/381173759/1741612446.WIQqJeAMSMy0Q8RPtY3MbA

If we made the mypy check "unconditionally" pass, I couldn't figure out if/whether we could actually inspect its output from that result page.

[shwina]

But yes, ultimately I would like for type checking to be part of the pre-commit step (not as a standalone check)

[leofang]

Are there examples of projects using pyright for enforcing type checks in CI? I looked at some existing "big" Python projects to see what they were using:

I think mypy is really unfriendly to array libraries like NumPy and it's being discussed to move away to pyright or basedpyright. @seberg was involved in long discussions like numpy/numpy#27957 and numpy/numpy#28076. But it is unclear to me why we wouldn't like mypy in cuda.core. I guess we need to go over the list of all failures and decide.

[leofang]

If we made the mypy check "unconditionally" pass, I couldn't figure out if/whether we could actually inspect its output from that result page.

Yeah my limited understanding about pre-commit.ci is the same. We can't make certain pre-commit steps optional. So moving this check to pre-commit in the future SGTM.

[rwgk]

But it is unclear to me why we wouldn't not like mypy in cuda.core.

What I had in mind is optimizing for what the user community cares about the most. I'm assuming that's type hints in code editors. I could be wrong.

[seberg]

Yeah, it could be that NumPy will use pyright more if we run into issues. For now, we only had in release notes that users annoyed by regressions should try pyright (its behavior is currently better).[1]

There was also sentiment that mypy has more bugs (I suspect there is reason for it). But unless you run into those, I am not aware that mypy is problematic, and I think it is still wider used in our projects.

---

[1] The main specific issues in NumPy revolved around issues related to shape/dtype typing (e.g. ndarray[float64] can't be assigned to a ndarray[1d, float64]; mypy tended to be less forgiving in these cases creating issues/regressions).
MyPy actually reacted to some related bugs and is slowly fixing the behavior of the later things.

[kkraus14]

Here's how the pre-commit results get reported:

https://results.pre-commit.ci/run/github/381173759/1741612446.WIQqJeAMSMy0Q8RPtY3MbA

If we made the mypy check "unconditionally" pass, I couldn't figure out if/whether we could actually inspect its output from that result page.

We could exclude the mypy task from pre-commit in the CI run for now and then have a separate optional job that runs only the mypy task? This way developers using pre-commit locally get mypy as part of their workflow unless they opt out of it, but it remains optional to get PRs through until we hit a later point.

[rwgk]

We could exclude the mypy task from pre-commit in the CI run for now and then have a separate optional job that runs only the mypy task? This way developers using pre-commit locally get mypy as part of their workflow unless they opt out of it, but it remains optional to get PRs through until we hit a later point.

I'm running pre-commit all the time while developing, so that I don't have to manually format my code or watch out for obsolete imports, etc. If pre-commit becomes noisy, it'll slow me down. I'll probably comment out the mypy section locally in .pre-commit.yaml.

I believe it will be most productive to make a clear decision: When do we want to spend the time making our code mypy compatible? As soon as pre-commit is clean, we're done, without any extra work on the CI side.

To add in my estimated effort for making cuda.core mypy compatible: couple days to one week.

For me specifically: I'd (finally) jump the barrier installing Cursor and ask it to help me. Then it might be even faster.

[shwina]

How do we want to proceed here? There are two questions here that are unresolved:

Should we even use mypy for type checking?

My 2c is that we should. There's a lot of collective experience using MyPy both within NVIDIA and the wider open-source community. While it can absolutely be fiddly at times, I've almost always been able to quickly find resolutions on StackOverflow or GitHub issues. Also tools like https://mypy-play.net/ are great for debugging.

Note that using mypy to enforce type checks doesn't preclude using pyright with LSP or your IDE locally.

Should we invoke mypy via pre-commit?

I don't have a very strong opinion here. I definitely think we should eventually invoke mypy via pre-commit. In the interim, I just don't know how exactly to achieve what @kkraus14 is suggesting here:

We could exclude the mypy task from pre-commit in the CI run for now and then have a separate optional job that runs only the mypy task?

1. How do I configure pre-commit.ci to skip a specific pre-commit check?
2. How do I make it so that a specific pre-commit check is "optional"? I guess I could wrap the mypy invocation in a script that always succeeds, and have pre-commit call that script. If we do this, (1) becomes a non-issue.

[rwgk]

Should we even use mypy for type checking?

I don't have strong opinions here. It's definitely a good start.

I definitely think we should eventually invoke mypy via pre-commit.

Assuming we have pre-commit type checking, and we've made the code compatible (no errors), would we want to keep the type checking CI job?

[shwina]

Assuming we have pre-commit type checking, and we've made the code compatible (no errors), would we want to keep the type checking CI job?

Nope, at that point it goes away. The point of this PR is just to have somewhere that the results of type checking are visible

[rwgk]

TBH, just thinking about what I'd do:

• The cuda_bindings/pyproject.toml change you figured out looks very useful.
• The CI job, why not, but I'd probably not look at the CI logs.
• Assuming I'd decide to jump on cleaning up, I'd figure out the .pre-commit-config.yaml addition we need, then roll up my sleeves working through the pre-commit errors until I got them all.

[rwgk]

LGTM, mainly for the cuda_bindings/pyproject.toml change.

[shwina]

OK - thanks. I opened this PR for review so that it's mergeable. I'd defer to you or @leofang if/whether to merge this - no objections either way from me

[shwina]

/ok to test

[shwina]

/ok to test

[shwina]

/ok to test

[leofang]

what does this overrides section do?

[kkraus14]

1. How do I configure pre-commit.ci to skip a specific pre-commit check?

You'd use the skip option as described here: https://pre-commit.ci/#configuration-skip. I'm not clear on if its possible to add another pre-commit.ci job that skips everything but mypy and isn't required to pass.

2. How do I make it so that a specific pre-commit check is "optional"? I guess I could wrap the mypy invocation in a script that always succeeds, and have pre-commit call that script. If we do this, (1) becomes a non-issue.

It would be optional in the sense that it wouldn't be required to pass in CI to merge PRs, but would be opt-out for developers where they'd need to add an environment variable of SKIP=mypy to avoid it from running when they run pre-commit locally.

[leofang]

btw let's hold off merging until code freeze is lifted (we're still wrapping up v0.2.0).

Another question that just occurred to me is: If we add py.typed now, but don't fix the typing errors, would it impact downstream projects running mypy? Do they have to also waive the failures?

[rwgk]

Cross-referencing #527 — A very quick experiment, trying to use Cursor to add typing annotations.