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

Make stl.h list|set|map_caster more user friendly. #30045

Merged
merged 16 commits into from
Jul 17, 2023

Conversation

rwgk
Copy link
Contributor

@rwgk rwgk commented Jun 6, 2023

Description

pybind/pybind11#4686 applied to pywrapcc.

Snapshot of the pybind/pybind11 PR description as of 2023-07-17 14:27 PDT:

With this PR, it is no longer necessary to explicitly convert Python iterables to tuple(), set(), or map() in several common situations, for example:

-    fn(tuple({"x": 8, "y": 11}.values()))
+    fn({"x": 8, "y": 11}.values())

-    fn(set({5: None, 12: None}.keys()))
+    fn({5: None, 12: None}.keys())

-    fn(dict(PyMappingWithItems().items()))
+    fn(PyMappingWithItems())

See pybind/pybind11#4686 (comment) below for a more complete demonstration of the behavior differences. (Note that the diff in that comment is reversed, what is + here is - there and vice versa.)

This PR strikes a careful compromise between being user friendly and being safe. This compromise was informed by 10s of thousands of use cases in the wild (Google codebase, which includes a very large number of open source third-party packages). Concretely, in connection with PyCLIF-pybind11 integration work, the behaviors of PyCLIF (http://github.com/google/clif) and pybind11 needed to be converged. Originally PyCLIF was extremely far on the permissive side of the spectrum, by accepting any Python iterable as input for a C++ vector/set/map argument, as long as the elements were convertible. The obvious (in hindsight) problem was that any empty Python iterable could be passed to any of these C++ types, e.g. {} was accpeted for C++ vector/set arguments, or [] for C++ map arguments. To fix this, the code base was cleaned up, and gating functions were added to enforce the desired behavior:

The exact same gating functions are adopted here. Without this behavior change in pybind11, it would be necessary to insert a very large number of explicit tuple(), set(), map() conversions in the Google codebase, to complete the PyCLIF-pybind11 integration work. While it is very easy to tell new users to pass e.g. tuple(iterable) instead of iterable, it is not easy to convince hundreds of happy existing PyCLIF users to accept retroactively inserting the explicit conversions. The obvious question that will come back: Why do we have to clutter our code like this? There is no good reason. The exact implementation of the gating functions may seem a little arbitrary at first sight, but is in fact informed by what amounts to a large-scale field experiment, with the originally very permissive behavior of PyCLIF.

Suggested changelog entry:

@rwgk rwgk changed the title WIP: Change list_caster (stl.h) to also accept generator objects. Make stl.h list|set|map_caster more user friendly. Jul 15, 2023
@rwgk rwgk requested a review from wangxf123456 July 17, 2023 21:32
@rwgk rwgk marked this pull request as ready for review July 17, 2023 21:32
is that this conversion is not reducing. Implicit conversions of this kind
are also fairly commonly used, therefore enforcing explicit conversions
would have an unfavorable cost : benefit ratio; more sloppily speaking,
such an enforcement would be more annyoing than helpful.
Copy link
Contributor

Choose a reason for hiding this comment

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

annyoing -> annoying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks!
(I'm surprised the pre-commit spell check didn't pick this up.)

@rwgk rwgk merged commit 8724346 into google:main Jul 17, 2023
@rwgk rwgk deleted the list_caster_pass_generator_pywrapcc branch July 17, 2023 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants