-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Use annotated for array #4679
Use annotated for array #4679
Conversation
Hi @cielavenir did you already see the AppVeyor failure? It looks like a couple more tests need updating. I'm holding off triggering the rest of the GitHub Actions until you get a chance to fix. I believe without a fix there will be so many failures that we're not learning much from the full results. |
@rwgk thank you, fixed |
oh why valarray is array_caster not list_caster? hopefully fixed though |
(I triggered a rerun to get rid of a notorious flake.) |
I ran this by my team mates (includes pytype developers) they didn't like the plain so I looked here: https://docs.python.org/3/library/typing.html#typing.Annotated where I see e.g. based on that I suggested which they liked but pointed out that
I'm totally out of my depth ... how does that idea play with pybind11-stubgen? |
It works with future
(but still, let me check mypy output) |
@rwgk oh, mypy works (after another patch of pybind11-stubgen) but pyright shows this |
in Python3 typing documentation, ValueRange and MaxLen are not defined - how they write sample code in the way pyright does not understand? |
And... do you mean this stackoverflow post is not ok? |
As mentioned in this SO post, the It would be great if there were defaults in from typing import TYPE_CHECKING
if TYPE_CHECKING:
def FixedSize(value):
pass # Type hint annotation for containers of fixed length I note that both |
@felixvd pybind11-stubgen update: https://github.com/cielavenir/pybind11-stubgen/commits/addFixedSize |
Looks great to me! (purely from a readability pov; I know very little about typing) I want to run this through global testing (Google-internal toolchain) after you had a chance to fix up the tests. |
Nice, all green! I'll run this through global testing tonight. |
There is one failing test in the global testing, exactly what I was afraid would happen. It's in the non-public part of our codebase, which is good news in the sense that I can change that code together with the import of this PR. Bad news for me because I'll probably have to spend at least a couple hours to negotiate with the owning team and see it through. AFAIU there is a tool that harvests the pybind11 docstrings to create typestubs of some sorts. They have a command to update the typestubs automatically, the result is akin to a commit to be merged. Currently it looks like this: - def world_origin_world_to_camera(self) -> List[float[3]]: ...
+ def world_origin_world_to_camera(self) -> Any: ... It's not a surprise to see a change, but apparently the tool doesn't know at all how to handle As I hinted, I can work with the team owning the typestub generator, but in the bigger picture, the rest of the world might have similar problems. Just pointing out, I think this is a good change and getting the world to adjust is an OK cost. @cielavenir & @felixvd, could you please formally approve this PR if you agree? I'll merge this when I see both approvals. Internal reference: OCL:534690556:BASE:534836523:1684939647732:8df15d67 (to help me find it again later) |
@rwgk pull request author cannot approve his own changeset, but I'm fine if I don't need to perform further changes on pybind11.
interesting if other than pybind11-stubgen, though I will have to ask pybind11-stubgen staff to merge https://github.com/cielavenir/pybind11-stubgen/commits/addFixedSize anyway |
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.
As you said, this is a change that tools may need to adjust to. But since it follows PEP 593 and I believe is an improvement over the previous custom syntax which predates the PEP, I agree that the cost is worth it.
Thank you for the quick review and testing. Much appreciated.
It might be a good idea to loop in the team owning the typestub generation tool so they can comment before merging.
I reached out already; waiting to hear back. We're not deploying directly from master, I'll go ahead and merge here. Thanks all! |
@cielavenir & @felixvd Do you have experience working on mypy stubgen, or know someone who could make the required changes there?
We converged on this idea (full context but the only point that matters here is 4.):
|
@rwgk So you mean https://github.com/python/mypy/blob/master/mypy/stubgenc.py one right? I was not aware of that module (I was aware of mypy itself though). |
I think the answer is yes. What I know for sure is that we're using https://github.com/python/mypy/blob/master/mypy/stubgen.py This version exactly, which calls https://github.com/python/mypy/blob/b5914b51304800e82d435310e548547d2b55298c/mypy/stubgen.py#L110 |
|
Description
Addressed #3916 (comment) Fixes #3912
Suggested changelog entry:
(oh I copy-pasted 3916)
/cc @Skylion007
/cc @felixvd