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

WIP: Support ownership transfer between C++ and Python with shared_ptr<T> and unique_ptr<T> for pure C++ instances and single-inheritance instances #1146

Conversation

EricCousineau-TRI
Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI commented Oct 14, 2017

UPDATE (2020-12-28): Decreed as stale.
For a more up-to-date and working implementation, see https://github.com/RobotLocomotion/pybind11/blob/drake/README_DRAKE.md (permalink)


This addresses #1132 and #1145 (and accommodates #1138 / incorporates #1139).

All unittests pass on my system (Python 2.7, release + debug).

Todo

  • Minimize changes to internals: detail::type_info and detail::instance
  • Implement test cases to examine ownership behavior with and without pybind11::wrapper<Base>
  • Squash commits + rebase on master, make PR to master.

@EricCousineau-TRI EricCousineau-TRI changed the title WIP: Support ownership transfer with shared_ptr<T> and unique_ptr<T> WIP: Support ownership transfer between C++ and Python with shared_ptr<T> and unique_ptr<T> for pure C++ instances and single-inheritance instances Oct 14, 2017
@wjakob
Copy link
Member

wjakob commented Oct 14, 2017

I don't think it's going to be feasible for us to even consider a commit of this scale to support moving std::unique_ptr (which should be discussed too -- I'm not really sure that this feature makes sense to me).

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Oct 16, 2017

I understand on the commit scale. I can certainly break this into smaller components to take this out of a WIP stage. (I can also collapse the commits - those were me just prototyping small tests as I was learning some of the internals of pybind11.)

For moving unique_ptr<>, can I ask what part does not make sense? (Well, aside from the bloat of the implementation, and the oddity of signaling a move via a list is a tad kludgy...)
This permits the equivalent of #971, but a with a few more checks, and supporting single inheritance.

This also includes preventing Python-subclass aliasing that would arise from passing shared_ptr<>s from Python to C++, which at present would fail silently. (This is my main focus at present, but I would still like to enable casting unique_ptr<>-held instances to permit full ownership transfer.) The solution shares an implementation with the unique_ptr<> approach.
Additionally, it addresses #1138 more robustly, and provides some limited mixing of compatible holder types (unique_ptr<T> -> shared_ptr<T>).

@EricCousineau-TRI
Copy link
Collaborator Author

I've squashed the commits in this branch. All of the granular experimental history is in feature/py_move_shared-wip on my repo.

The workflow for derived-type aliasing, specifically with shared_ptr<>, is now present in a more simplified unittest (hacky with ConstructorStats workarounds, but works):
https://github.com/pybind/pybind11/pull/1146/files#diff-5dd500b0d477a5b264d862e0157b4847R31

Still on my TODO is the unique_ptr<> modifications captured in pybind11-style unittest; however, I'll defer that pending more discussion on if you think it doesn't belong. (I'd like to have it, but I understand that at present it's a tad messy.)

@wjakob
Copy link
Member

wjakob commented Oct 16, 2017

I don't think squashing everything into a single commit addresses the problem, which is really the sheer size of the diff. I'll try to take a look in the next 1-2 weeks regarding the functional aspects (sorry for the delay, I have an unrelated deadline ATM).

@EricCousineau-TRI
Copy link
Collaborator Author

Sorry, I didn't mean to imply that squashing was the only problem :)
I will see if I can provide a few more concise unittests to help bubble up the features that I'm focused on; if possible, I will see if I can stage these features, possibly layering the commits in this PR.

Thanks!

@jagerman
Copy link
Member

One thing that I'm missing is a concise overview of what this feature is all about. Can I suggest that you write up a summary of a few paragraphs aimed at someone who has some pybind experience, but not a core developer that describes the motivation, feature, and simple implementation. (The reason I ask for a experienced-but-not-core-developer target is that this isn't just for evaluation of the PR: it would also form the basis of the docs/* content that would eventually need to be added before the PR could be merged).

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Oct 18, 2017

Most certainly!
I will type something up similar to what I had in #1132, but include the case for shared_ptr<T>.
For development, I will also try to identify the separate features, and use that as a basis for a PR-train if this is deemed appropriate.

Thank you for your patience and for considering this!

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Oct 24, 2017

I've added a quick draft of the main features of what this PR could introduce at a user-facing level. If you have time, please let me know if you have any comments!
https://github.com/EricCousineau-TRI/pybind11/blob/feature/py_move_shared-wip/docs/advanced/classes.rst#virtual-inheritance-and-lifetime

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Oct 25, 2017

I have updated the documentation, and included details on the holder stuff for ownership transfer as well:
https://github.com/EricCousineau-TRI/pybind11/blob/feature/py_move_shared-wip/docs/advanced/smart_ptrs.rst

EDIT: If you would like to see the overview in a different form, please let me know!

@EricCousineau-TRI
Copy link
Collaborator Author

@ppt-adsk mentioned that they have ran into a similar issue, at least for inheritance with shared_ptrs.

That being said, can I ask if there is any way that I could carve up the overview docs or this PR to make it easier to review?
I can re-vamp the PR to focus solely on shared_ptr<>, and defer mixing holder types to the convergence of #1161.

@EricCousineau-TRI EricCousineau-TRI force-pushed the feature/py_move_shared branch 3 times, most recently from d7162d8 to 2df39b2 Compare December 20, 2017 23:04
…` and `unique_ptr<T>` for pure C++ instances and single-inheritance instances
@EricCousineau-TRI
Copy link
Collaborator Author

I'm going to decree this PR as stale. I've updated the overview to indicate where to look for more up-to-date discussions.

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.

3 participants