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

Add case return_value_policy::_clif_automatic in type_caster_base.h #30087

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

rwgk
Copy link
Contributor

@rwgk rwgk commented Jan 16, 2024

Description

This is required for PyCLIF-pybind11, to support mixing manually-written pybind11 extension modules and PyCLIF-pybind11-generated extension modules. Fixes about 140 failures when testing PyCLIF-pybind11 Google-globally.

Suggested changelog entry:

Copy link

@rainwoodman rainwoodman left a comment

Choose a reason for hiding this comment

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

Looks good to me. A super nitpick on the document string and the naming of the option. It seems to me from the description that legacy_clif is a more accurate description, as the word 'automatic' can be removed without affecting the definition:

This policy should only be used by PyCLIF to automatically select a
return value policy. Legacy PyCLIF automatically decides object lifetime
management based on their properties

@rwgk
Copy link
Contributor Author

rwgk commented Jan 17, 2024

Looks good to me. A super nitpick on the document string and the naming of the option. It seems to me from the description that legacy_clif is a more accurate description, as the word 'automatic' can be removed without affecting the definition:

This policy should only be used by PyCLIF to automatically select a return value policy. Legacy PyCLIF automatically decides object lifetime management based on their properties

That's a good point, but realities aren't favorable:

PyCLIF-C-API will be around for the foreseeable future, therefore I stopped calling it "legacy" anywhere.

The PyCLIF lifetime conventions are here to stay: it would be a huge project to change them. There is just too much built around/on top of the current behavior.

Maybe the "clif_automatic" behavior can be evolved a little bit, but the concept is extremely unlikely to go away.

To expand on PyCLIF-C-API / PyCLIF-pybind11 co-existence: 99.999% of all Google-global tests pass with PyCLIF-pybind11, but the effort to get the rest is estimated to be around 1 SWE year still. Therefore we will only switch the default to PyCLIF-pybind11 (to get the benefits: features, OSS ecosystem compatibility), but keep a small fraction on PyCLIF-C-API.

@rwgk rwgk merged commit 24a3b1c into google:main Jan 17, 2024
147 checks passed
@rwgk rwgk deleted the type_caster_base_clif_automatic_pywrapcc branch January 17, 2024 18:04
rwgk pushed a commit to google/clif that referenced this pull request Jan 22, 2024
For easy direct access:

* google/pybind11clif#30034

* google/pybind11clif#30087

* google/pybind11clif#30088

Note regarding the change in std_containers_copy_move_test.py:

When the test was added (cl/570839777), the undocumented expectation was that it is hyper-sensitive, but that it will be adjusted or made more permissive (similar to e.g. google3/third_party/clif/testing/python/return_value_policy_test.py;l=25;rcl=534200687) as needed.

#MIGRATION_3P_PYBIND11__DEFAULT

```
  - 24a3b1c3729401ca661efacfbbd13a83117e1bae Add `case return_value_policy::_clif_automatic` in type_c... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - 015834b13a99d233c15406c36a3a44d27ebfc846 Change `array_caster` in pybind11/stl.h to support value ... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
  - ce00785ef877f0bb1dba10115f00366111583b3c Add `release_gil_before_calling_cpp_dtor` annotation for ... by Ralf W. Grosse-Kunstleve <rwgk@google.com>
```

PiperOrigin-RevId: 599883612
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