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

Option for arg/return type hints and correct typing for std::filesystem::path #5450

Merged
merged 35 commits into from
Dec 8, 2024

Conversation

timohl
Copy link
Contributor

@timohl timohl commented Nov 24, 2024

Description

This PR adds the option for defining arg_name and return_name in a custom type_caster, which allows having different python type hints for arguments and return value. To check if arg_name or return_name is available or name should be used as fallback is implemented using the template classes as_arg_type and as_return_type.

Being entirely optional, this should not impact any existing type_caster.
This type hint specialization system is applied to the type_caster for std::filesystem::path.
Here, Union[os.PathLike, str, bytes] is the argument type hint and Path is the return type hint.
This is more accurate than the previous usage of os.PathLike for both argument and return type hint.

Also, most classes in pybind11::typing now support this feature for nested types, e.g., py::typing::List<std::filesystem::path> becomes List[Union[os.PathLike, str, bytes]] in arguments and List[Path] in return types.

I have added unit tests to check for the new signatures of stl/filesystem in typing containers and also to verify that name is used as fallback for other types (tested for std::vector<std::filesystem::path>).

Suggested changelog entry:

Added option for different arg/return type hints to type_caster.
Updated stl/filesystem to use correct arg/return type hints.
Updated pybind11::typing to use correct arg/return type hints for nested types.

Possible TODOs

  • Update Custom type casters documentation
  • Update type hints for Eigen (currently uses np.ndarray as arg type hint but also takes lists and tuples (should probably be numpy.typing.ArrayLike + maybe typing.Annotated for shape/dtype annotation (going into follow-up PR)

Tests for py::typing classes:

  • Test for std::vector<T> (fallback)
  • Test for Tuple<T...>
  • Test for Tuple<T, ellipsis>
  • Test for Dict<K, V>
  • Test for List<T>
  • Test for Set<T>
  • Test for Iterable<T>
  • Test for Iterator<T>
  • Test for Callable<Return(Args...)>
  • Test for Callable<Return(ellipsis)>
  • Test for Union<T...>
  • Test for Optional<T>
  • Test for TypeGuard<T>
  • Test for TypeIs<T>

I would love to get feedback on this!
Especially on the "Possible TODO" regarding Eigen. Should I implement that here in this PR, or should I open a separate PR later?

Currently, I am working on adding typing stubs to Open3D (isl-org/Open3D#6917) using pybind11-stubgen.
Applying mypy/pyright on existing example code showed that a lot of type checks failed, which could be fixed by this PR.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Update Custom type casters documentation

Could you please try to include that in this PR? — For awareness, please see this comment. Not sure if that changes anything for you. If not, completely fine.

Update type hints for Eigen

Best in a follow-on PR.

include/pybind11/cast.h Outdated Show resolved Hide resolved
include/pybind11/typing.h Show resolved Hide resolved
@timohl
Copy link
Contributor Author

timohl commented Nov 24, 2024

Update Custom type casters documentation

Could you please try to include that in this PR? — For awareness, please see this comment. Not sure if that changes anything for you. If not, completely fine.

Sure! I can also try to adopt some of the documentation updates from #3862 as you suggested in #5416 since I am already at it.

@timohl
Copy link
Contributor Author

timohl commented Nov 25, 2024

While thinking about a test function for TypeGuard and TypeIs, I realized, that it probably should behave differently.
So, I changed it in 9ad445d to always use the return type of the nested type when available.
Both are used only in return type hints and narrow down the type of the first argument.
Since return type hints are usually narrower than argument types, I think return type is more appropriate here.
(If any typing expert disagrees, please let me know)

Outstanding task: "Update Custom type casters documentation"
I will probably do that tomorrow.

@timohl timohl requested a review from henryiii as a code owner November 26, 2024 15:21
@timohl
Copy link
Contributor Author

timohl commented Nov 26, 2024

I have updated the documentation.
It now uses an example of a 2D double point, to convey the idea of having different argument and return type hints.
Here, Sequence[float] is the argument type to signal that list and tuple are allowed (or any type implementing the Sequence protocol), and tuple[float, float] is the return type.
I have added tests for the code snippets as well.

This should also fix #5416 since the example changed (and hopefully I made no mistake).

One CI check failed, but that is a dependency timeout (maybe you can rerun it?).

@timohl timohl requested a review from rwgk November 26, 2024 18:12
@rwgk
Copy link
Collaborator

rwgk commented Nov 26, 2024

One CI check failed, but that is a dependency timeout (maybe you can rerun it?).

Done and it passed (very quickly).

I can look at the code changes only later (might be a day or two).

tests/test_docs_advanced_cast_custom.cpp Outdated Show resolved Hide resolved
tests/test_docs_advanced_cast_custom.cpp Outdated Show resolved Hide resolved
@timohl
Copy link
Contributor Author

timohl commented Dec 1, 2024

I have changed the example to only use the pybind11 API.
Additionally, I have changed the test for py::typing classes to only use pybind11 API for the caster.

The documentation text was also changed a little bit:

  • Added recommendation to use pybind11 API to warning about Python C API
  • Added note about return_value_policy and convert arguments referring to their respective sections in the documentation

@timohl timohl requested a review from rwgk December 1, 2024 15:29
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, thanks a lot!

@cryos could you help with a second set of eyes? Is there anything that stands out to you when glancing through?

docs/advanced/cast/custom.rst Show resolved Hide resolved
docs/advanced/cast/custom.rst Outdated Show resolved Hide resolved
@timohl
Copy link
Contributor Author

timohl commented Dec 5, 2024

Made a small correction in the docs/advanced/cast/overview section in 6ea4704.
I have searched the docs and this should be the only mention of os.PathLike, pathlib.Path or std::filesystem::path.

@rwgk
Copy link
Collaborator

rwgk commented Dec 8, 2024

Since there was no feedback from anyone else, I looked through the entire PR again. I found only one tiny oversight (aa21ab5).

I also merged master.

Waiting for CI to finish. When I see it passing I'll merge.

@rwgk rwgk merged commit 1d09fc8 into pybind:master Dec 8, 2024
81 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Dec 8, 2024
@HolyBlackCat
Copy link

static constexpr auto return_name = const_name("Path"); seems to be wrong, Path isn't a valid type? I've reported #5477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants