Skip to content

stubgen & pybind11: can we make them play nice(r)? #16306

Open
@rwgk

Description

@rwgk

Bug Report
We're experimenting under pybind/pybind11#4888 with a pybind11 behavior change, to generate

Annotated[Any, CppTypePybind11("cpp_namespace::UserType")]

instead of a bare cpp_namespace::UserType.

It seems like mypy does handle outer-level Annotated, but not nested ones, e.g.

Iterator[str, Annotated[Any, CppTypePybind11("cpp_namespace::UserType")]]

The results from stubgen (1.5.1, Google-internal toolchain) are like this:

def values(self, *args, **kwargs) -> Any: ...

Desired is:

def values(self) -> Iterator[str, Annotated[Any, CppTypePybind11("cpp_namespace::UserType")]]: ...

Does this ring any bells?

If you need more details, please let me know. (Note though that I don't know a lot about typing and mypy. This fell into my lap as Google-internal pybind11 owner & pybind11 maintainer on github.)

A more general question: What are your thought about changing the pybind11 behavior in this way? Is there a better way?

(A clear and concise description of what the bug is.)

To Reproduce

# Ideally, a small sample program that demonstrates the problem.
# Or even better, a reproducible playground link https://mypy-play.net/ (use the "Gist" button)

Expected Behavior

Actual Behavior

Your Environment

  • Mypy version used: 1.5.1, Google-internal toolchain
  • Mypy command-line flags:
  • Mypy configuration options from mypy.ini (and other config files):
  • Python version used: 3.10

Activity

added
bugmypy got something wrong
on Oct 21, 2023
changed the title [-]stubgen & `Annotated[Any, CppTypePybind11("cpp_namespace::UserType")]`[/-] [+]stubgen & pybind11: can we make them play nice(r)?[/+] on Oct 27, 2023
sizmailov

sizmailov commented on Nov 11, 2023

@sizmailov
Contributor

@JelleZijlstra @chadrik

Could you please take a look at the issue?

chadrik

chadrik commented on Nov 11, 2023

@chadrik
Contributor

Happy to help.

I saw that my new changes to stubgen were released in mypy 1.7, and those changes included some improvements to parsing recursive types.

@rwgk If possible, it would be great if you could test mypy 1.7 as there is a chance this problem is fixed. If it's difficult to get updated tooling installed inside Google, that's ok, my suggestion below should be fine on its own:

The fastest way to resolve this will be for you to modify the mypy pybind11 demo source code to demonstrate your problem and make a PR with the changes. I'll then be able to much more easily improve the stubgen code to resolve the issue.

rwgk

rwgk commented on Nov 13, 2023

@rwgk
Author

@rwgk If possible, it would be great if you could test mypy 1.7 as there is a chance this problem is fixed.

Trying. (It's not easy; I have to run global testing and fix anything that breaks. Might take a while.)

The fastest way to resolve this will be for you to modify the mypy pybind11 demo source code to demonstrate your problem and make a PR with the changes.

I could add to that file, but asking first: Are you set up to test in combination with an open pybind11 PR (pybind/pybind11#4888)?

Or alternatively, maybe better: Could you point me to the commands I'd need to run to test myself, in my fork of mypy?

chadrik

chadrik commented on Nov 13, 2023

@chadrik
Contributor

I could add to that file, but asking first: Are you set up to test in combination with an open pybind11 PR (pybind/pybind11#4888)?

No. But maybe there's an even simpler way to cooperate on this. Can you provide here the exact docstring that would be inspected from a function with this new capability? I can write a simpler unit test just focused on the string parsing.

chadrik

chadrik commented on Nov 13, 2023

@chadrik
Contributor

I should add that if the annotation in the docstring parser could be tripped up by use of parentheses in the types: Iterator[str, Annotated[Any, CppTypePybind11("cpp_namespace::UserType")]]. Is this something that you expect mypy or other type analyzers to understand? In order for this to work we would also need to generate an import statement for CppTypePybind11, I assume.

rwgk

rwgk commented on Nov 13, 2023

@rwgk
Author

I tried to get a docstring for bindings in the wild, but no luck in the time I can give it right now. I'll try again asap.

I don't know a lot about typing. My main idea is that we change pybind11 so that mypy is not tripped up by a raw C++ name (e.g. cpp_namespace::UserType). I don't know (at all) what exactly is best. I'll go with your advice.

Would this be better?

Iterator[str, Annotated[Any, "cpp_namespace::UserType"]]
bluenote10

bluenote10 commented on Nov 13, 2023

@bluenote10
Contributor

It seems like mypy does handle outer-level Annotated, but not nested ones, e.g.

Why is it nested to begin with? Iterator only takes one generic argument, so Iterator[str, Annotated[Any, "cpp_namespace::UserType"]] doesn't seem to make sense as a type. Annotated[Iterator[str], "cpp_namespace::UserType"] would.

chadrik

chadrik commented on Nov 13, 2023

@chadrik
Contributor

Annotated[Any, "cpp_namespace::UserType"] is preferable to Annotated[Any, CppTypePybind11("cpp_namespace::UserType")] unless there is an actual importable python type called CppTypePybind11, in which case you should include the module name and the new version of stubgen in mypy 1.7 should create the import statement. e.g. Annotated[Any, pybind11.CppTypePybind11("cpp_namespace::UserType")]

chadrik

chadrik commented on Nov 13, 2023

@chadrik
Contributor

With regard to nesting, Annotated should wrap the type that represents cpp_namespace::UserType. @bluenote10 is correct that Iterator only takes one argument.

So, if the Any represents the UserType, then this would be correct:

Iterator[Annotated[Any, "cpp_namespace::UserType"]]

Or if the iterator of strings represents the UserType, this would be correct:

Annotated[Iterator[str], "cpp_namespace::UserType"]]

Or if the intent was that it's an iterator of a tuple of (str, Any), where the Any is the UserType, then this would be correct:

Iterator[tuple[str, Annotated[Any, "cpp_namespace::UserType"]]]
rwgk

rwgk commented on Nov 13, 2023

@rwgk
Author

Or if the intent was that it's an iterator of a tuple of (str, Any), where the Any is the UserType, then this would be correct:

Iterator[tuple[str, Annotated[Any, "cpp_namespace::UserType"]]]

Thanks! That's exactly what I needed to know.

I'll work on constructing pybind11 unit tests (in my pybind11 PR), then share the docstrings here.

sizmailov

sizmailov commented on Nov 14, 2023

@sizmailov
Contributor

I should add that if the annotation in the docstring parser could be tripped up by use of parentheses in the types

It would be a bug in the parser, then. It's not uncommon to have arbitrary expressions in Annotated[...]

Note that pybind11 already uses Annotated for fixed-sized arrays (pybind/pybind11#4679):
std::array<int, 2> is rendered as Annotated[List[int], FixedSize(2)]. (not sure if stubgen 1.7 is ok with that)

Annotated[Any, "cpp_namespace::UserType"] is preferable to Annotated[Any, CppTypePybind11("cpp_namespace::UserType")] unless there is an actual importable python type called CppTypePybind11,

Bare-string annotations are less clear and always would be troublesome for a third-party tool that works with annotations. I agree, that having a valid importable class is even better.

The following annotation for a raw C++ type should be perfect.

Annotated[Any, pybind11.CppType("cpp_namespace::UserType")]

17 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @chadrik@rwgk@sizmailov@bluenote10@henryiii

        Issue actions

          stubgen & pybind11: can we make them play nice(r)? · Issue #16306 · python/mypy