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

[DEV-master-smart_holder] Bake smart_holder functionality into class_ and type_caster_base #5213

Closed
wants to merge 155 commits into from

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Jun 29, 2024

Description

This PR was used to rebuild the smart_holder functionality on top of the current master branch, pulling code fragments from the smart_holder branch. The approach was to pull one or more tests at a time from the smart_holder branch, then make them work by pulling production code fragments from the smart_holder branch and bake them into class_ and the code around type_caster_base.

Next step: Close this PR and continue the work under #5257.

Milestone reached at commit c4c3d9a under this PR:

Testing via GHA passes:

  • All existing tests under the master branch (@ commit 6d4805c), with the only minor diffs as shown below.
  • All existing tests under the smart_holder branch (@ commit ec557ff), effectively exactly as-is (see a summary of the non-functional changes below; those changes are to skip tests if the smart_holder functionality is not available).
  • All tests pass (or are skipped) with and without the smart_holder functionality, depending on the active internals version, which is tied to PYBIND11_VERSION_MAJOR >= 3. See example test output below.

Google-global testing passes, with several minor Google-internal adjustments outside the pybind11 source tree. Internal testing ID: OCL:654225008:BASE:654373952:1721520078863:60337b49


$ git checkout master
$ git diff -R bakein *.cpp
diff --git b/tests/test_class.cpp a/tests/test_class.cpp
index 9001d86b..3f2f2370 100644
--- b/tests/test_class.cpp
+++ a/tests/test_class.cpp
@@ -211,11 +211,12 @@ TEST_SUBMODULE(class_, m) {
     m.def("mismatched_holder_1", []() {
         auto mod = py::module_::import("__main__");
         py::class_<MismatchBase1, std::shared_ptr<MismatchBase1>>(mod, "MismatchBase1");
-        py::class_<MismatchDerived1, MismatchBase1>(mod, "MismatchDerived1");
+        py::class_<MismatchDerived1, std::unique_ptr<MismatchDerived1>, MismatchBase1>(
+            mod, "MismatchDerived1");
     });
     m.def("mismatched_holder_2", []() {
         auto mod = py::module_::import("__main__");
-        py::class_<MismatchBase2>(mod, "MismatchBase2");
+        py::class_<MismatchBase2, std::unique_ptr<MismatchBase2>>(mod, "MismatchBase2");
         py::class_<MismatchDerived2, std::shared_ptr<MismatchDerived2>, MismatchBase2>(
             mod, "MismatchDerived2");
     });
@@ -609,8 +610,10 @@ CHECK_NOALIAS(8);
 CHECK_HOLDER(1, unique);
 CHECK_HOLDER(2, unique);
 CHECK_HOLDER(3, unique);
+#ifndef PYBIND11_ACTUALLY_USING_SMART_HOLDER_AS_DEFAULT
 CHECK_HOLDER(4, unique);
 CHECK_HOLDER(5, unique);
+#endif
 CHECK_HOLDER(6, shared);
 CHECK_HOLDER(7, shared);
 CHECK_HOLDER(8, shared);
$ git diff -R bakein *.py
<empty diff>

git checkout bakein
$ git difftool --extcmd=diff --no-prompt smart_holder tests/test_class_sh_*.cpp | grep '^[<>]' | sort | uniq -c
     19 >
     17 > #else
      1 < #endif
      5 > #endif
     18 > #endif // PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
     17 >         false;
      6 > #ifdef PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
     17 > #ifndef PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT
      1 < #if true // Trick to avoid clang-format changes, in support of PR #5213.
     17 >     m.attr("defined_PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT") =
     17 >         true;
$ git difftool --extcmd=diff --no-prompt smart_holder tests/test_class_sh_*.py | grep '^[<>]' | sort | uniq -c
     22 >
      1 > if not m0.defined_PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT:
     17 > if not m.defined_PYBIND11_HAVE_INTERNALS_WITH_SMART_HOLDER_SUPPORT:
      4 > import pytest
     18 >     pytest.skip("smart_holder not available.", allow_module_level=True)

pytest output when building with -DPYBIND11_INTERNALS_VERSION=5 (i.e. without smart_holder functionality):

============================= test session starts ==============================
platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.5.0
C++ Info: 13.2.0 C++20 __pybind11_internals_v5_gcc_libstdcpp_cxxabi1018__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False PYBIND11_NUMPY_1_ONLY=False
rootdir: /usr/local/google/home/rwgk/forked/pybind11/tests
configfile: pytest.ini
plugins: xdist-3.5.0
created: 48/48 workers
48 workers [884 items]

........................................................................ [  8%]
........................................................................ [ 16%]
........................................................................ [ 24%]
........................................................................ [ 32%]
........................................................................ [ 40%]
........................................................................ [ 48%]
........................................................................ [ 57%]
........................................................................ [ 65%]
......................................................................... [ 73%]
.............................................s.......................... [ 81%]
........................................................................ [ 89%]
........................................................................ [ 97%]
...................                                                      [100%]
=========================== short test summary info ============================
SKIPPED [1] test_class_sh_basic.py:11: smart_holder not available.
SKIPPED [1] test_class_sh_disowning.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_disowning_mi.py:9: smart_holder not available.
SKIPPED [1] test_class_sh_factory_constructors.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_inheritance.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_mi_thunks.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_module_local.py:9: smart_holder not available.
SKIPPED [1] test_class_sh_property.py:11: smart_holder not available.
SKIPPED [1] test_class_sh_property_non_owning.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_shared_ptr_copy_move.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_trampoline_basic.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_trampoline_self_life_support.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_trampoline_shared_from_this.py:12: smart_holder not available.
SKIPPED [1] test_class_sh_trampoline_shared_ptr_cpp_arg.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_trampoline_unique_ptr.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_unique_ptr_custom_deleter.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_unique_ptr_member.py:8: smart_holder not available.
SKIPPED [1] test_class_sh_virtual_py_cpp_mix.py:8: smart_holder not available.
SKIPPED [1] test_stl.py:149: no <experimental/optional>
======================= 883 passed, 19 skipped in 4.14s ========================

Suggested changelog entry:

@rwgk
Copy link
Collaborator Author

rwgk commented Jun 29, 2024

Current state testing with the Google-internal toolchain, using ASAN:

7 failing tests:

//third_party/pybind11/tests:test_copy_move
//third_party/pybind11/tests:test_opaque_types
//third_party/pybind11/tests:test_smart_ptr
//third_party/pybind11/tests:test_stl
//third_party/pybind11/tests:test_stl_binders
//third_party/pybind11/tests:test_tagbased_polymorphic
//third_party/pybind11/tests:test_vector_unique_ptr_member

All other tests run ASAN-clean. For completeness, this is the list of passing tests:

40 passing tests:

//third_party/pybind11/tests:test_async
//third_party/pybind11/tests:test_buffers
//third_party/pybind11/tests:test_builtin_casters
//third_party/pybind11/tests:test_call_policies
//third_party/pybind11/tests:test_callbacks
//third_party/pybind11/tests:test_chrono
//third_party/pybind11/tests:test_class
//third_party/pybind11/tests:test_const_name
//third_party/pybind11/tests:test_constants_and_functions
//third_party/pybind11/tests:test_custom_type_casters
//third_party/pybind11/tests:test_custom_type_setup
//third_party/pybind11/tests:test_docstring_options
//third_party/pybind11/tests:test_eigen_matrix
//third_party/pybind11/tests:test_eigen_tensor
//third_party/pybind11/tests:test_enum
//third_party/pybind11/tests:test_eval
//third_party/pybind11/tests:test_exceptions
//third_party/pybind11/tests:test_factory_constructors
//third_party/pybind11/tests:test_gil_scoped
//third_party/pybind11/tests:test_iostream
//third_party/pybind11/tests:test_kwargs_and_defaults
//third_party/pybind11/tests:test_local_bindings
//third_party/pybind11/tests:test_methods_and_attributes
//third_party/pybind11/tests:test_modules
//third_party/pybind11/tests:test_multiple_inheritance
//third_party/pybind11/tests:test_numpy_array
//third_party/pybind11/tests:test_numpy_dtypes
//third_party/pybind11/tests:test_numpy_vectorize
//third_party/pybind11/tests:test_operator_overloading
//third_party/pybind11/tests:test_pickling
//third_party/pybind11/tests:test_python_multiple_inheritance
//third_party/pybind11/tests:test_pytypes
//third_party/pybind11/tests:test_sequences_and_iterators
//third_party/pybind11/tests:test_thread
//third_party/pybind11/tests:test_type_caster_pyobject_ptr
//third_party/pybind11/tests:test_union
//third_party/pybind11/tests:test_unnamed_namespace_a
//third_party/pybind11/tests:test_unnamed_namespace_b
//third_party/pybind11/tests:test_virtual_functions
//third_party/pybind11/tests:test_wip

Ralf W. Grosse-Kunstleve added 6 commits June 29, 2024 16:13
…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).
@rwgk
Copy link
Collaborator Author

rwgk commented Jun 30, 2024

Tracking progress:

test_factory_constructors builds and runs successfully WITH ALL BAKEIN_BREAK removed.

This is still the same:

#5213 (comment)

Ralf W. Grosse-Kunstleve added 11 commits July 19, 2024 00:55
… the `std::shared_ptr` specialization and remove `pytest.skip("BAKEIN_EXPECTED: ...)` in test_smart_ptr.py
…ALS_WITH_SMART_HOLDER_SUPPORT is not defined.
…S_WITH_SMART_HOLDER_SUPPORT is not defined.
…DULE_LOCAL_ID`"

`"_sh_baked_in"` is no longer needed because the smart_holder functionality is now tied to the `PYBIND11_INTERNALS_VERSION`.

This reverts commit 884305e.
…INTERNALS_WITH_SMART_HOLDER_SUPPORT` block.
rwgk pushed a commit that referenced this pull request Jul 20, 2024
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jul 22, 2024
…: 1. To avoid compiler warnings for unused code in the unnamed namespace. 2. To avoid clang-format changes.
rwgk pushed a commit that referenced this pull request Jul 22, 2024
… To avoid compiler warnings for unused code in the unnamed namespace. 2. To avoid clang-format changes. (#5258)
@rwgk rwgk changed the title [WIP] Bake smart_holder functionality into class_ and type_caster_base [DEV-master-smart_holder] Bake smart_holder functionality into class_ and type_caster_base Jul 22, 2024
@rwgk
Copy link
Collaborator Author

rwgk commented Jul 22, 2024

Closing, as explained in the PR description.

All checks have passed: 2 skipped and 155 successful checks

CI — https://github.com/pybind/pybind11/actions/runs/10038035526

CI-SH-DEF — https://github.com/pybind/pybind11/actions/runs/10038035539

@rwgk rwgk closed this Jul 22, 2024
henryiii pushed a commit that referenced this pull request Aug 13, 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant