sizmailov / pybind11-stubgen

Generate stubs for python modules
Other
218 stars 44 forks source link

On using `pybind11_stubgen`-defined classes in generated stubs #200

Open ringohoffman opened 7 months ago

ringohoffman commented 7 months ago

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 commented 7 months ago

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.

wojdyr commented 6 months ago

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.