Skip to content
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

On using pybind11_stubgen-defined classes in generated stubs #200

Open
ringohoffman opened this issue Nov 25, 2023 · 2 comments
Open

On using pybind11_stubgen-defined classes in generated stubs #200

ringohoffman opened this issue Nov 25, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@ringohoffman
Copy link
Contributor

ringohoffman commented Nov 25, 2023

Is there a specific philosophy that this repo has about using pybind11_stubgen-defined classes in the generated stubs?

At the moment, I see that this is done mainly for --numpy-array-wrap-with-annotated with pybind11_stubgen.typing_ext.FixedSize and pybind11_stubgen.typing_ext.DynamicSize("m", "n"), although I am also seeing it in #199.

My main concern with not making this choice explicit is that a lot of projects incorporating pybind11_stubgen into their builds might only include pybind11_stubgen as a build dependency and not as a required dependency of the whole project. This would mean users installing these projects would not have pybind11_stubgen installed, and type checkers would fail to resolve these expressions, in part or in full.

There are a few non-mutually exclusive options that I can think of:

  1. Do not use pybind11_stubgen-defined classes at all in generated stubs. I think this is the safest default approach.
  2. Inject definitions for these pybind11_stubgen-defined classes into the generated stubs. Maybe as Protocols? Would this mean re-defining a FixedSize protocol in every file it is used in?
  3. Allow importing from pybind11_stubgen, with the user explicitly acknowledging that they must mark pybind11_stubgen as a required dependency of their project in order to fully utilize the generated stubs.
@sizmailov
Copy link
Owner

I see pybind11_stubgen.typing_ext as a temporary solution for producing correct stubs. The classes/functions do not belong there.

FixedSize / DynamicSize are artifacts of a lack of convention for dimension annotation in python.
InvalidExpr is an artifact of recovering the signature from docstrings.

Probably there is a better place for those classes, e.g. typing/typing_extensions.

WRT options:

  1. I agree, there should be as little use of those classes as possible.
  2. I don't like the violation of the one-definition rule. Even if it's opt-in.
  3. Yes, it's problematic.

Please note that pybind11_stubgen is not a runtime but a dev dependency. Static checks would fail, but runtime will not.

I think it would be nice to have the option to choose where those classes should be imported from with the defaults to pybind11_stubgen.typing_ext. To avoid extra dependencies, one could have a copy of typing_ext.pyi in the distribution of your package:

pybind11-stubgen --typing-ext="my_package.typing_ext" my_package

As a (non-exclusive) alternative, we can provide a lightweight stubs-only package with those definitions only.

The better solution would be to not have typing_ext in the first place. But this is a long route for finding a compromise between pybind11, stubgen, mypy/typing.

@sizmailov sizmailov added the bug Something isn't working label Nov 27, 2023
@wojdyr
Copy link

wojdyr commented Dec 22, 2023

I'm trying out type checking tools and I noticed that pytype doesn't like the types from pybind11_stubgen.typing_ext:

      def size(self) -> typing.Annotated[list[int], pybind11_stubgen.typing_ext.FixedSize(3)]:
                                                   ^
  ParseError: 'Attribute' object has no attribute 'id'

Just to let you know. I can't tell if the problem is in pytype or something is missing in typing_ext.FixedSize.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants