Skip to content

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