-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Spike test extent to which Python can use the existing System framework #7185
Comments
sorry -- i dodged both on my quick spike (was just prototyping LinearSystem at the time). |
It seems like there may be a way to make |
My $0.02 is that requiring users to ensure that a python object has exactly one reference is too much of a burden. References can be created in all sorts of interesting and non-obvious ways, like inside a closure or a generator, and I've never encountered any python code that relied on an object having exactly one reference. Reference counting is also something that's not generally taught except when actually implementing python extensions in C, so it's something that I'd venture most python users won't understand or be able to easily work around. Just as an example, if I run the following inside an IPython shell: In [1]: import sys
In [2]: x = [1, 2, 3]
In [3]: sys.getrefcount(x)
Out[3]: 2
In [4]: x
Out[4]: [1, 2, 3]
In [5]: sys.getrefcount(x)
Out[5]: 8 IPython helpfully stores the result of each execution in the |
That makes sense, did not think about that use case. Thanks! Per the discussion on Slack with @jwnimmer-tri and @sammy-tri, the following options available are:
The main caveat to avoid is to try and avoid forcing users to worry about Python references (such as the strict reference counting option in (2) that Robin mentioned) and also C++ object lifetime (such as permitting the object lifetime to be detached via (1)). For (1), the proxy could possibly place something that is For (1) and/or (2), the Drake codebase could be refactored in such a way that we have a However, in all situations, people generating wrappers would need to ensure that any pure-references (such as returning a pointer to a C++ object that is not owned by the bound class) are returned as reference-only. There is still the possibility that this object will go out of scope in C++ before Python reference dry up, which would cause a (really annoying) segfault, especially in interactive sessions. This could be mitigated by using some variant of For (3), this may pose an issue if a Python object is destructed (seemingly disconnected to Python) when it is actually referenced by a C++ object. For (4), it is possible to change API interfaces to use |
weighing in as eager python users - we would like to use drake to find collision-free trajectories in dynamic environments where we are adding, removing, or moving bodies in the Since creating a new For example, some of our current pydrake code:
we would like to be able to do something like:
Then we would like to do things like how do the aforementioned design choices affect this workflow? Are there better ways to do this? |
@dmsj The design choices above do not really affect I've added #7251. |
Posted a (large and messy) WIP PR to For now, I'll start generating basic bindings for existing I will briefly investigate the usage of For a non- |
Given that #7590 has landed, I believe this issue is now satisfied per the scope of spike testing. |
This is meant to spike test the extent to which Python can use the existing Systems framework.
From Robin's initial prototype with
pybind11
and the follow-up implementations Drake now has, it seems likepybind11
should be able to handle most of the basic API.I've only hit two main sticking points thus far:
std::unique_ptr<>
may provide some barriers to easily taking ownership of Python-created objects. Discussion frompybind11
here, an example repro here (comparingunique_ptr
andshared_ptr
destruction).std::unique_ptr<>
(specifically, when trying to have Python-definedConverter<>
functions passed back toBaseConverter
- for testing Python-defined classes).Motivation for both of these:
Value<T>
instances forAbstractValue
sstd::unique_ptr
- Ensure that we can properly construct diagrams, create model values, do cloning, etc.@RussTedrake - can I ask if you had run into any of these issues when you were prototyping these things?
The text was updated successfully, but these errors were encountered: