Add automatically-generated .pyi files to cuda_core#2061
Conversation
mdboom
added
enhancement
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
cc @ZzEeKkAa for vis |
|
Hello! The patch that @mdboom mentioned is merged, and is included in stubgen-pyx v0.2.6 which is now available via pip |
|
|
pre-commit.ci autofix |
Andy-Jost
left a comment
Andy-Jost
left a comment
There was a problem hiding this comment.
No issues here. I checked the .pyx files carefully but didn't read the generated .pyi files.
| if TYPE_CHECKING: | ||
| from cuda.core.graph import GraphBuilder |
There was a problem hiding this comment.
Previously @leofang mentioned he wanted to remove all if TYPE_CHECKING blocks (but I don't recall the reason). Is that no longer the guidance? It appears real imports are needed by the type checking tools.
There was a problem hiding this comment.
Yes. Even if you want to use the quoted syntax, i.e. def my_function() -> "GraphBuilder":, you need to import those things so the type checker knows what you are referring to. The if TYPE_CHECKING: block is just a way to prevent cyclical imports that this otherwise might cause.
| from __future__ import annotations | ||
|
|
||
| from collections.abc import Callable, Iterable, MutableSet | ||
| from collections.abc import Callable, Iterable, MutableSet, Set as AbstractSet |
There was a problem hiding this comment.
Does the distinct name AbstractSet buy anything?
There was a problem hiding this comment.
Probably not. The agent did this. On reflection it's probably clearer to not rename. I can update the PR.
There was a problem hiding this comment.
I think the agent did this because it's the convention in typeshed:
https://github.com/search?q=repo%3Apython%2Ftypeshed%20abstractset&type=code
Not an argument that we need go either way. It's functionality the same thing, and personally I don't like to rename things unless it's to workaround a clash or something.
| buf._size = new_size | ||
| # TODO: #2049 This is a real bug, accessing _size which doesn't exist. | ||
| # Fix bug and remove the "type: ignore[attr-defined]" comment. | ||
| buf._size = new_size # type: ignore[attr-defined] |
There was a problem hiding this comment.
This seems like a good enough reason to Cythonize even when we don't expect performance benefits.
There was a problem hiding this comment.
Or, conversely, it's a reason not to Cythonize because the type checker would not have been able to catch this bug if it were Cython 😄
But it's all tradeoffs. We tend to take runtime performance over developer productivity in this project, which is reasonable.
| # `from cuda import cuda as driver` etc. import path.) | ||
| # `as X` form is the PEP 484 explicit re-export marker, which type checkers | ||
| # need to treat these names as part of the public API of this module. | ||
| from cuda.bindings import driver as driver, nvrtc as nvrtc, runtime as runtime |
There was a problem hiding this comment.
Question for @leofang: Do we still need the fallback to the pre-cuda-bindings library?
from cuda import cuda as driver
from cuda import cudart as runtime
from cuda import nvrtc
It makes the type-checking a lot more complicated. There is probably a solution, but I didn't want to add it if we can just drop this because cuda_bindings has been out long enough by now.
4 participants
This adds automatically-generated
.pyifiles to cuda_core. This allows IDE auto-completion to work (which is also used by IDE-integrated coding agents). This has also found 2 real bugs in our code already (#2049 and #2050). The ability to catch a certain class of bugs with this will be really helpful going forward, especially since our linting abilities withcython-lintare a bit behind what they are in pure Python.(Marked as a draft because it requires a small patch to stubgen-pyx and is currently using my personal fork. We can update this after the patch, or something like it, is merged -- jon-edward/stubgen-pyx#19)
Reviewing
Three separate commits to make this easier to review:
Workflow
The workflow I chose here is that
.pyifiles are committed explicitly into the repo. A pre-commit hook regenerates them and then runs a type checker over the entire result.Alternative: An alternative approach would be to not store them in the repo: generate them at the time of wheel building, and then test them as part of the test suite.
I prefer the approach taken here, because it will make it obvious in a PR when our public API has changed, and we can validate that it is making an allowed transformation (adding, but not removing etc.) We could (as a future step), use something like griffe.
Type-checking
This adds mypy-based type checking as part of the pre-commit hook. I ran this code over all of the major type checkers (mypy, pyright, pyrefly and ty). mypy and pyright were very similar in behavior, we could use either, but given that pathfinder is already using mypy and I have more confidence in its long-term viability, I went with that. pyrefly seems worth following up on in a year, and ty was pretty limited at this point. It takes 0.25s to run mypy over cuda-core on my machine, so I don't think moving to a Rust-based checker matters much at this point.
Assessment
Overall, I was very impressed by how far
stubgen-pyxhas come since last I looked, and I think it's good enough to be relied on at this point.One minor rough edge is with code at the top-level. Code that calls
cdeffunctions at the top-level will not type check/resolve, because (correctly) thosecdeffunctions are not exposed to the.pyifile. The solution I used here was just to put these blocks of code at the top level into functions and call them, which is probably not a bad practice in terms of global namespace hygiene anyway.The biggest shortcoming I see with this approach is that the line numbers in the
.pyifiles don't map back to the original line numbers in the.pyxor.pxifiles. So when mypy displays a type error, you have to weave through and find the original source to edit it. (This is sort of what the#linedirective in the C preprocessor is for -- I don't think.pyifiles have an equivalent). I think we can live with that, but it's a bit of a pain point. All that said, my agent had no trouble realizing it had to go back and make changes to the.pyxfile, not directly in the.pyifile.Cc: @jon-edward (for viz)