Elide to-python conversion of setter return values #4621
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 10 commits
April 10, 2023 21:02
…solve GCC build failures.
Example of failure resolved by this change:
g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
```
cd /build/tests && /usr/bin/c++ -DPYBIND11_TEST_EIGEN -Dpybind11_tests_EXPORTS -I/mounted_pybind11/include -isystem /usr/include/python3.8 -isystem /build/_deps/eigen-src -g -std=c++17 -fPIC -fvisibility=hidden -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -MD -MT tests/CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o -MF CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o.d -o CMakeFiles/pybind11_tests.dir/test_buffers.cpp.o -c /mounted_pybind11/tests/test_buffers.cpp
In file included from /mounted_pybind11/include/pybind11/stl.h:12,
from /mounted_pybind11/tests/test_buffers.cpp:10:
/mounted_pybind11/include/pybind11/pybind11.h: In instantiation of ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def_property(const char*, const Getter&, const Setter&, const Extra& ...) [with Getter = pybind11::cpp_function; Setter = std::nullptr_t; Extra = {pybind11::return_value_policy}; type_ = pybind11::buffer_info; options = {}]’:
/mounted_pybind11/include/pybind11/pybind11.h:1716:58: required from ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def_property_readonly(const char*, const pybind11::cpp_function&, const Extra& ...) [with Extra = {pybind11::return_value_policy}; type_ = pybind11::buffer_info; options = {}]’
/mounted_pybind11/include/pybind11/pybind11.h:1684:9: required from ‘pybind11::class_<type_, options>& pybind11::class_<type_, options>::def_readonly(const char*, const D C::*, const Extra& ...) [with C = pybind11::buffer_info; D = long int; Extra = {}; type_ = pybind11::buffer_info; options = {}]’
/mounted_pybind11/tests/test_buffers.cpp:209:61: required from here
/mounted_pybind11/include/pybind11/pybind11.h:1740:25: error: call of overloaded ‘cpp_function(std::nullptr_t&, pybind11::is_setter)’ is ambiguous
1740 | name, fget, cpp_function(method_adaptor<type>(fset), is_setter()), extra...);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/mounted_pybind11/include/pybind11/pybind11.h:101:5: note: candidate: ‘pybind11::cpp_function::cpp_function(Func&&, const Extra& ...) [with Func = std::nullptr_t&; Extra = {pybind11::is_setter}; <template-parameter-1-3> = void]’
101 | cpp_function(Func &&f, const Extra &...extra) {
| ^~~~~~~~~~~~
In file included from /mounted_pybind11/include/pybind11/stl.h:12,
from /mounted_pybind11/tests/test_buffers.cpp:10:
/mounted_pybind11/include/pybind11/pybind11.h:87:5: note: candidate: ‘pybind11::cpp_function::cpp_function(std::nullptr_t, const Extra& ...) [with Extra = {pybind11::is_setter}; std::nullptr_t = std::nullptr_t]’
87 | cpp_function(std::nullptr_t, const Extra &...) {}
| ^~~~~~~~~~~~
```
rwgk
pushed a commit
to rwgk/pybind11clif
that referenced
this pull request
Apr 12, 2023
3 tasks
added 2 commits
April 26, 2023 17:21
…rementing the reference count for None, but no. Discovered via many failing tests in the wild (10s of thousands). It is very tricky to construct a meaningful unit test for this bug specifically. It's unlikely to come back, because 10s of thousands of tests will fail again.
rwgk
pushed a commit
to google/pybind11clif
that referenced
this pull request
Apr 28, 2023
* Snapshot of pybind/pybind11#4621 applied to pywrapcc * Bug fix: obvious in hindsight. I thought the original version was incrementing the reference count for None, but no. Discovered via many failing tests in the wild (10s of thousands). It is very tricky to construct a meaningful unit test for this bug specifically. It's unlikely to come back, because 10s of thousands of tests will fail again.
|
@Skylion007 @lalaland This is a really small tweak removing an obvious stumbling block. It would be great if someone could review. @henryiii Do you know why we're getting this failure? docs/readthedocs.org:pybind11 — Read the Docs build failed! I saw this the first time about a week ago (smart_holder and/or pywrapcc, not sure). |
EthanSteinberg
approved these changes
May 8, 2023
Thanks Ethan, the new test ensures already that the conversion is not triggered, see this line in test_methods_and_attributes.cpp: This test will fail at runtime without the production code change. In this case an exception is thrown from |
|
Oh, oops, missed that part of the test. Looks good to me then. |
|
Thanks a lot Ethan! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Setter return values are inaccessible, at least for all practical purposes (see e.g. https://stackoverflow.com/questions/16615866/python-setter-to-also-return-a-value). pybind11 laboriously converts them from C++ to Python, only to be discarded. This PR elides the to-python conversion of setter return values.
The new test is a reduced example, reflective of a large number of use cases in the wild (Google codebase) in which the unnecessary to-python conversion fails. Eliding the to-python conversion fixes such failures.
For manually written bindings, eliding the to-python conversion is a minor optimization that could also be achieved by wrapping setters in lambda functions, but this small change removes the need completely to 1. think about it and 2. write the lambda function.
For automatically generated bindings (PyCLIF-pybind11) this small PR is a major simplification.
This PR was ported to the pywrapcc repo/pseudo-branch under google/pybind11clif#30027 (merged there already).
Suggested changelog entry: