[WIP] Bake smart_holder functionality into class_ and type_caster_base
#5213
+5,468
−15
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
…mart_holder (test fails)
…stance() specialization. Also bring in dynamic_raw_ptr_cast_if_possible.h. Does not build because try_initialization_using_shared_from_this() is still missing.
…holder_type_casters.h. With this test_wip builds and succeeds.
skip-checks: true
|
Current state testing with the Google-internal toolchain, using ASAN: 7 failing tests: All other tests run ASAN-clean. For completeness, this is the list of passing tests: 40 passing tests: |
…ch (does not build).
…with is_same<Holder<Class>, smart_holder> (still does not build).
…() from smart_holder_type_casters.h (with this test_wip builds and runs successfully).
…l (4) BAKEIN_BREAK from test_factory_constructors.py (test_factory_constructors builds and runs successfully).
|
Tracking progress: test_factory_constructors builds and runs successfully WITH ALL BAKEIN_BREAK removed. This is still the same: |
…ly_holder_caster<unique_ptr>. No functional changes.
…_ptr_to_python() there, add smart_holder_from_unique_ptr()
…ipped tests pass locally with C++17).
…(replaces BAKEIN_BREAK).
…ill needs debugging and more testing: ``` ========================================================= short test summary info ========================================================== SKIPPED [2] test_class_sh_property.py:19: BAKEIN_BREAK: Failed: DID NOT RAISE <class 'ValueError'> SKIPPED [1] test_class_sh_property.py:87: BAKEIN_BREAK: m_uqcp_readwrite does not build SKIPPED [1] test_class_sh_property.py:140: BAKEIN_BREAK: m_uqmp_readwrite does not build SKIPPED [1] test_class_sh_property.py:140: BAKEIN_BREAK: m_uqcp_readwrite does not build SKIPPED [1] test_class_sh_basic.py:156: unconditional skip SKIPPED [1] test_class_sh_property.py:87: BAKEIN_BREAK: m_uqmp_readwrite does not build SKIPPED [1] test_stl.py:149: no <experimental/optional> SKIPPED [1] test_smart_ptr.py:301: BAKEIN_EXPECTED: Failed: DID NOT RAISE <class 'RuntimeError'> FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_rw-expected1-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_rw-expected1-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_ro-expected0-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_ro-expected4-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_ro-expected4-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_rw-expected5-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_ro-expected0-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_rw-expected5-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr). ================================================ 8 failed, 1056 passed, 9 skipped in 3.96s ================================================= ```
…nch:
smart_holder branch:
```
static std::shared_ptr<type> shared_ptr_from_python(handle responsible_parent)
```
Renamed in this commit:
```
static std::shared_ptr<type> shared_ptr_with_responsible_parent(handle responsible_parent)
```
Use in `property_cpp_function<>` specializations.
Fixes all 8 test failures introduced with the previous commit (1bcf572):
```
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_ro-expected0-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_rw-expected1-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_rw-expected1-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_ro-expected4-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_ro-expected4-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_rw-expected5-True] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_value_ro-expected0-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
FAILED test_class_sh_property_non_owning.py::test_core_fld_common[core_fld_raw_ptr_rw-expected5-False] - RuntimeError: Non-owning holder (loaded_as_shared_ptr).
```
…just (simplify) the `property_cpp_function<...unique_ptr...>::write` implementation (all tests pass). This PR (pybind#5213) at this commit also passes ASAN, MSAN, UBSAN testing with the Google-internal toolchain.
…ng it in pybind11/stl_bind.h
Introduce two helper types: * copyable_holder_caster_shared_ptr_with_smart_holder_support_enabled * move_only_holder_caster_unique_ptr_with_smart_holder_support_enabled
Use cases in the wild: * `test_readonly_char6_member()`: https://github.com/pytorch/pytorch/blob/4410c44ae6fd8eb36f2358ac76f7d988ca7537c5/torch/csrc/cuda/Module.cpp#L961 * `test_readonly_const_char_ptr_member()`: https://github.com/facebookresearch/nle/blob/862a439a84f52abca94d1f744d57061da12c5831/include/permonst.h#L43
…in `property_cpp_function<>` specializations.
…d use systematically in all `property_cpp_function<>` specializations.
This fixes a PyTorch build error (using the Google-internal toolchain):
```
In file included from third_party/py/torch/torch/csrc/distributed/c10d/init.cpp:15:
In file included from third_party/py/torch/torch/csrc/distributed/c10d/PyProcessGroup.hpp:4:
In file included from third_party/py/torch/torch/csrc/jit/python/pybind_utils.h:7:
In file included from third_party/pybind11/include/pybind11/complex.h:14:
In file included from third_party/pybind11/include/pybind11/pybind11.h:14:
In file included from third_party/pybind11/include/pybind11/detail/class.h:14:
In file included from third_party/pybind11/include/pybind11/detail/../attr.h:16:
third_party/pybind11/include/pybind11/detail/../cast.h:856:19: error: static assertion failed due to requirement 'std::is_base_of<pybind11::detail::type_caster_base<c10::intrusive_ptr<c10d::Store, c10::detail::intrusive_target_default_null_type<c10d::Store>>>, pybind11::detail::type_caster<c10::intrusive_ptr<c10d::Store, c10::detail::intrusive_target_default_null_type<c10d::Store>>, void>>::value': Holder classes are only supported for custom types
856 | static_assert(std::is_base_of<base, type_caster<type>>::value,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
third_party/pybind11/include/pybind11/detail/../cast.h:982:48: note: in instantiation of template class 'pybind11::detail::copyable_holder_caster<c10::intrusive_ptr<c10d::Store>, std::shared_ptr<c10::intrusive_ptr<c10d::Store>>>' requested here
982 | class type_caster<std::shared_ptr<T>> : public copyable_holder_caster<T, std::shared_ptr<T>> {};
| ^
third_party/crosstool/v18/stable/src/libcxx/include/__type_traits/is_base_of.h:22:91: note: in instantiation of template class 'pybind11::detail::type_caster<std::shared_ptr<c10::intrusive_ptr<c10d::Store>>>' requested here
22 | struct _LIBCPP_TEMPLATE_VIS is_base_of : public integral_constant<bool, __is_base_of(_Bp, _Dp)> {};
| ^
third_party/pybind11/include/pybind11/detail/../cast.h:1382:30: note: in instantiation of template class 'std::is_base_of<pybind11::detail::type_caster_generic, pybind11::detail::type_caster<std::shared_ptr<c10::intrusive_ptr<c10d::Store>>>>' requested here
1382 | detail::enable_if_t<std::is_base_of<type_caster_generic, make_caster<Return>>::value, void>> {
| ^
third_party/pybind11/include/pybind11/pybind11.h:287:19: note: during template argument deduction for class template partial specialization 'return_value_policy_override<Return, detail::enable_if_t<std::is_base_of<type_caster_generic, make_caster<Return>>::value, void>>' [with Return = std::shared_ptr<c10::intrusive_ptr<c10d::Store>>]
287 | = return_value_policy_override<Return>::policy(call.func.policy);
| ^
third_party/pybind11/include/pybind11/pybind11.h:287:19: note: in instantiation of template class 'pybind11::detail::return_value_policy_override<std::shared_ptr<c10::intrusive_ptr<c10d::Store>>>' requested here
third_party/pybind11/include/pybind11/pybind11.h:269:55: note: while substituting into a lambda expression here
269 | rec->impl = [](function_call &call) -> handle {
| ^
third_party/pybind11/include/pybind11/pybind11.h:147:9: note: in instantiation of function template specialization 'pybind11::cpp_function::initialize<(lambda at third_party/pybind11/include/pybind11/pybind11.h:1714:17), std::shared_ptr<c10::intrusive_ptr<c10d::Store>>, pybind11::handle, pybind11::is_method>' requested here
147 | initialize(
| ^
third_party/pybind11/include/pybind11/pybind11.h:1713:20: note: in instantiation of function template specialization 'pybind11::cpp_function::cpp_function<(lambda at third_party/pybind11/include/pybind11/pybind11.h:1714:17), pybind11::is_method, void>' requested here
1713 | return cpp_function(
| ^
third_party/pybind11/include/pybind11/pybind11.h:1964:54: note: in instantiation of function template specialization 'pybind11::property_cpp_function<c10d::DistributedBackendOptions, c10::intrusive_ptr<c10d::Store>>::read<c10::intrusive_ptr<c10d::Store> c10d::DistributedBackendOptions::*, 0>' requested here
1964 | property_cpp_function<type, D>::read(pm, *this),
| ^
third_party/py/torch/torch/csrc/distributed/c10d/init.cpp:945:8: note: in instantiation of function template specialization 'pybind11::class_<c10d::DistributedBackendOptions>::def_readwrite<c10d::DistributedBackendOptions, c10::intrusive_ptr<c10d::Store>>' requested here
945 | .def_readwrite("store", &::c10d::DistributedBackendOptions::store)
| ^
1 error generated.
```
…erence` There are no strong reasons for accepting or rejecting `return_value_policy::reference`. Accepting to accommodate existing use cases in the wild.
…e_ownership` There are no strong reasons for accepting or rejecting `return_value_policy::take_ownership`. Accepting to accommodate existing use cases in the wild.
|
Milestone reached at commit ed27c37:
NOTE: smart_holder is still the default holder. — This is mainly intended to be a stress test. The Google-global testing (internal ID: OCL:652909258:BASE:653492046:1721283711718:c0b9e029) identified only 3 use cases that break with smart_holder as the holder. These cases were fixed by explicitly requesting
|
This is in support of PR pybind#5213: * trampoline_self_life_support.h depends on value_and_holder.h * type_caster_base.h depends on trampoline_self_life_support.h
…ter`: the correct `load_value()` return type is `void` (as defined in `type_caster_generic`) For easy future reference, this is the long-standing inconsistency: * https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/detail/type_caster_base.h#L634 * https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/cast.h#L797 Noticed in passing while working on PR pybind#5213.
…from smart_holder branch almost as-is. This ensures IWYU correctness in client code. The only change in trampoline_self_life_support.h is to replace `#include "detail/type_caster_base.h"` with "#include detail/value_and_holder.h".
…d on the smart_holder branch (all the way back in 2021).
rwgk
added a commit
that referenced
this pull request
Jul 19, 2024
* Factor out detail/value_and_holder.h (from detail/type_caster_base.h) This is in support of PR #5213: * trampoline_self_life_support.h depends on value_and_holder.h * type_caster_base.h depends on trampoline_self_life_support.h * Fix a minor and inconsequential inconsistency in `copyable_holder_caster`: the correct `load_value()` return type is `void` (as defined in `type_caster_generic`) For easy future reference, this is the long-standing inconsistency: * https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/detail/type_caster_base.h#L634 * https://github.com/pybind/pybind11/blob/dbf848aff7c37ef8798bc9459a86193e28b1032f/include/pybind11/cast.h#L797 Noticed in passing while working on PR #5213. * Add `DANGER ZONE` comment in detail/init.h, similar to a comment added on the smart_holder branch (all the way back in 2021).
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
STILL WORK IN PROGRESS
Latest milestone reached: #5213 (comment)
Aiming for this next milestone: Make
std::unique_ptrthe default holder again.Suggested changelog entry: