[pydrake] Add function to make arbitrary ref cycles

[rpoyner-tri]

This patch adds internal::make_arbitrary_ref_cycle(), which will prove useful for awkward bindings like those involving multiple return values.

While in the neighborhood, it also adds a few previously missing tests.

---

This change is 

[rpoyner-tri]

speculative: +@SeanCurtis-TRI for feature review. If this is all too baffling, I'll try to find someone else. Let me know.

Context is dev branch #22022 .

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

---

a discussion (no related file):
This patch is infrastructure for re-spelling the AddMultibodyPlantSceneGraph bindings.

[SeanCurtis-TRI]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/test/ref_cycle_test_util_py.cc line 79 at r1 (raw file):
BTW Test code often plays a pedagogical role. The "location hint" you've provided here would produce an error message along the lines of:

object type at argument 0 for binding at 'from arbitrary_ok' is not tracked by garbage collection.

The "at-from" back-to-back combo looks a bit silly. Is this the kind of "location hint" pattern you would want to foster?

---

bindings/pydrake/common/ref_cycle_pybind.cc line 83 at r1 (raw file):

          "Could not activate ref_cycle: object type at index {} for "
          "function '{}' is not tracked by garbage collection. Was the object "
          "defined with `pybind11::class_<...>(... pybind11::dynamic_attr())`?",

BTW I'm curious about this cut. I know it was originally introduced due to a review comment of my own in the past. Obviously, it no longer has value? Is the guidance not helpful or not accurate?

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

    // neither should we complain. A None variable value could happen for any
    // number of legitimate reasons, and does not mean that the ref_cycle call
    // policy is defective.

Two thoughts on this:

1. In this documentation could you give an example of a valid case where a passed handle is None? (I imagine this is a runtime thing where a C++ API can return null and, therefore, we don't need to create a cycle between the corresponding None and the other thing). But a concrete example or two will mean that future readers don't have to start from zero to reach the conclusion that you've made here.
2. Is it possible that a None could be a problem? This kind of depends on whatever underlying properties belong to the examples of (1) above. But I can't help wonder if there's value in some call sites that can know a priori that None should never happen and we can declare it as such. In those cases, we would want None to bark.

(And, yes, I realize all of this reasoning and functionality predates this PR.)

[rpoyner-tri]

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Two thoughts on this:

1. In this documentation could you give an example of a valid case where a passed handle is None? (I imagine this is a runtime thing where a C++ API can return null and, therefore, we don't need to create a cycle between the corresponding None and the other thing). But a concrete example or two will mean that future readers don't have to start from zero to reach the conclusion that you've made here.
2. Is it possible that a None could be a problem? This kind of depends on whatever underlying properties belong to the examples of (1) above. But I can't help wonder if there's value in some call sites that can know a priori that None should never happen and we can declare it as such. In those cases, we would want None to bark.

(And, yes, I realize all of this reasoning and functionality predates this PR.)

1. Recall this is instrumenting Python, and could apply to either return values (from C++ which could be null, as you point out), or arguments (from Python, where None is always an option). In any case, we don't have enough context here to say much that is interesting.
2. None could always be a problem. This piece of code is not the place to make decisions about data structures at some entirely separate level of abstraction.

FWIW, I'm in good company: https://github.com/pybind/pybind11/blob/v2.13/include/pybind11/pybind11.h#L2275-L2277 . pybind11 keep_alive makes exactly the same judgment about None, for the same reasons (though the documentation there is somewhat thin).

---

bindings/pydrake/common/ref_cycle_pybind.cc line 83 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I'm curious about this cut. I know it was originally introduced due to a review comment of my own in the past. Obviously, it no longer has value? Is the guidance not helpful or not accurate?

I don't understand what you are saying.

[SeanCurtis-TRI]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

1. Recall this is instrumenting Python, and could apply to either return values (from C++ which could be null, as you point out), or arguments (from Python, where None is always an option). In any case, we don't have enough context here to say much that is interesting.
2. None could always be a problem. This piece of code is not the place to make decisions about data structures at some entirely separate level of abstraction.

FWIW, I'm in good company: https://github.com/pybind/pybind11/blob/v2.13/include/pybind11/pybind11.h#L2275-L2277 . pybind11 keep_alive makes exactly the same judgment about None, for the same reasons (though the documentation there is somewhat thin).

I know it's instrumenting python. I wasn't suggesting this code should make that determination.

I was thinking the person instrumenting might know that None is disallowed as a value for either parameter based on what was being bound and it might be nice if the binding infrastructure could allow such a declaration. This code would merely use the declaration to determine what act should be taken in the face of None.

Whether that has value or not is something else. (I.e., it's impossible for None to leak in, so the declaration would be redundant, or some such reason).

---

bindings/pydrake/common/ref_cycle_pybind.cc line 83 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

I don't understand what you are saying.

The phrase:

"Was the object defined with `pybind11::class_<...>(... pybind11::dynamic_attr())?"

appears to have been deleted by this PR. That's the "cut" to which I allude. Or did I just miss where it landed?

[rpoyner-tri]

Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I know it's instrumenting python. I wasn't suggesting this code should make that determination.

I was thinking the person instrumenting might know that None is disallowed as a value for either parameter based on what was being bound and it might be nice if the binding infrastructure could allow such a declaration. This code would merely use the declaration to determine what act should be taken in the face of None.

Whether that has value or not is something else. (I.e., it's impossible for None to leak in, so the declaration would be redundant, or some such reason).

With all that other purview, why wouldn't our wise user put the assertion somewhere else?

---

bindings/pydrake/common/ref_cycle_pybind.cc line 83 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The phrase:

"Was the object defined with `pybind11::class_<...>(... pybind11::dynamic_attr())?"

appears to have been deleted by this PR. That's the "cut" to which I allude. Or did I just miss where it landed?

Ah. good catch. I'll restore it.

[SeanCurtis-TRI]

Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

With all that other purview, why wouldn't our wise user put the assertion somewhere else?

I can't answer that. I don't know where else it would go. I don't have sufficient insight into the mechanisms. But if declaring such properties in the binding itself isn't the place for it, I can't imagine where else it would be.

But I'd like to take a step back. I feel this has gotten away from me, so I'd like to bring this back to the beginning and make it clear what my comment is responding to.

All of this comes down to the documentation. It communicated to me that a None isn't necessarily a problem (indicated by the phrase "any number of legitimate reasons"). The existence of circumstances where None is fine is not an argument that says None is always fine. So, I went on a speculative adventure. I couldn't help but wonder if it was possible to hit this code with a None and have that actually constitute a problem.

The code implicitly assumes that's impossible. If that's the view we want to take, I'd rather better align the documentation with the code. Strengthen the assertion from "there are legitimate circumstances" into something along the lies of, "None is never a problem because...." Or, the pragmatic, "We always treat None as 'fine' because we can't conceive of circumstances in which it isn't."

[rpoyner-tri]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I can't answer that. I don't know where else it would go. I don't have sufficient insight into the mechanisms. But if declaring such properties in the binding itself isn't the place for it, I can't imagine where else it would be.

But I'd like to take a step back. I feel this has gotten away from me, so I'd like to bring this back to the beginning and make it clear what my comment is responding to.

All of this comes down to the documentation. It communicated to me that a None isn't necessarily a problem (indicated by the phrase "any number of legitimate reasons"). The existence of circumstances where None is fine is not an argument that says None is always fine. So, I went on a speculative adventure. I couldn't help but wonder if it was possible to hit this code with a None and have that actually constitute a problem.

The code implicitly assumes that's impossible. If that's the view we want to take, I'd rather better align the documentation with the code. Strengthen the assertion from "there are legitimate circumstances" into something along the lies of, "None is never a problem because...." Or, the pragmatic, "We always treat None as 'fine' because we can't conceive of circumstances in which it isn't."

So, what wording of documentation would you find acceptable?

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

So, what wording of documentation would you find acceptable?

I tried to imply that I can't dictate that language. I don't know why we get to assume that None is never a problem. But whatever that reason is, that's what I'd document.

[rpoyner-tri]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I tried to imply that I can't dictate that language. I don't know why we get to assume that None is never a problem. But whatever that reason is, that's what I'd document.

We don't assume that None is never a problem. On the contrary, we check for it explicitly and take an action: do nothing.

Are there other actions we could take? Would they have value?

Obviously, if one of our peers is None, we can't construct a ref-cycle. So that's off the table.

We could emit some sort of warning. What would we say? To whom would we be saying it?

We could throw. How do we know from this line of code that receiving a None is grounds for terminating/interrupting the process?

[rpoyner-tri]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

We don't assume that None is never a problem. On the contrary, we check for it explicitly and take an action: do nothing.

Are there other actions we could take? Would they have value?

Obviously, if one of our peers is None, we can't construct a ref-cycle. So that's off the table.

We could emit some sort of warning. What would we say? To whom would we be saying it?

We could throw. How do we know from this line of code that receiving a None is grounds for terminating/interrupting the process?

So, I did the experiment of changing the action to throw. The most common case that experiment catches is the optional lcm argument to DrakeVisualizer::AddToBuilder. If it's passed non-None, we absolutely have to "keep it alive" because the underlying C++ code will not. If it's passed None, then the underlying C++ code makes its own lcm object and owns it. In the latter case, there is nothing for us to do, and we don't have enough information to have an opinion to share.

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):
I feel like we're not having the same conversation. You're asking me how I would change the code. In my early question, I did ask about the feasibility/meanginfulness of changing the code. But you persuaded me that such a change wouldn't be meaningful. I accept that.

I tried to return to what triggered my original concern: the disharmony between code and documentation.

For me, "None is never a problem" is equal to "when it is None, we do nothing". This is simply what the code does. In my mind, with that act, the code clearly implies that "doing nothing is always the right thing to do for None". I.e., even without knowing where this function gets called, the code asserts that doing nothing is the right thing for None. I suspect we both agree on what the code says/does/implies.

The issue I have is reconciling my reading of the documentation with my reading of the code. This comes down to the interpretation of this phrase:

A None variable value could happen for any number of legitimate reasons.

As I desperately try to understand why my concern appears so inscrutable to you, I'm inclined to attribute it to attachment ambiguity. What does "could" attach to in the documentation?

As I read it, it attaches to "legitimate reasons". I.e., "it could be None for any number of legitimate reasons...or it could be None for bad reasons". This is what was in my mind from the beginning. But I realize that may not be in your mind. It might simply be a reading that never occurred to you.

In an alternative interpretation it would attach to the appearance of None, indicating that of all the possible parameter values passed to this function, some could be None. However, if they are None it is always for legitimate reasons. (Note: this interpretation only occurred to me as the product of careful analysis in trying to figure out why this conversation might have gone awry.)

If that latter interpretation is consistent with yours, I would attempt to disambiguate the rhetorical language as:

None variable values happen for any number of legitimate reasons; appearance of None doesn't imply a defective use of the ref_cycle policy.

(Note: this implies no basis for drawing the conclusion, it merely attempts to disambiguate its articulation.)

[rpoyner-tri]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I feel like we're not having the same conversation. You're asking me how I would change the code. In my early question, I did ask about the feasibility/meanginfulness of changing the code. But you persuaded me that such a change wouldn't be meaningful. I accept that.

I tried to return to what triggered my original concern: the disharmony between code and documentation.

For me, "None is never a problem" is equal to "when it is None, we do nothing". This is simply what the code does. In my mind, with that act, the code clearly implies that "doing nothing is always the right thing to do for None". I.e., even without knowing where this function gets called, the code asserts that doing nothing is the right thing for None. I suspect we both agree on what the code says/does/implies.

The issue I have is reconciling my reading of the documentation with my reading of the code. This comes down to the interpretation of this phrase:

A None variable value could happen for any number of legitimate reasons.

As I desperately try to understand why my concern appears so inscrutable to you, I'm inclined to attribute it to attachment ambiguity. What does "could" attach to in the documentation?

As I read it, it attaches to "legitimate reasons". I.e., "it could be None for any number of legitimate reasons...or it could be None for bad reasons". This is what was in my mind from the beginning. But I realize that may not be in your mind. It might simply be a reading that never occurred to you.

In an alternative interpretation it would attach to the appearance of None, indicating that of all the possible parameter values passed to this function, some could be None. However, if they are None it is always for legitimate reasons. (Note: this interpretation only occurred to me as the product of careful analysis in trying to figure out why this conversation might have gone awry.)

If that latter interpretation is consistent with yours, I would attempt to disambiguate the rhetorical language as:

None variable values happen for any number of legitimate reasons; appearance of None doesn't imply a defective use of the ref_cycle policy.

(Note: this implies no basis for drawing the conclusion, it merely attempts to disambiguate its articulation.)

So, I wrote that documentation as a result of our last review of this code; clearly I would have been better off having written nothing.

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

So, I wrote that documentation as a result of our last review of this code; clearly I would have been better off having written nothing.

One thing we've learned about long reviewable threads is that at some point trying to sync up via email reaches a point of diminishing returns. Possibly this thread has reached that point. I suggest getting on a phone call to resolve the problem.

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers

---

bindings/pydrake/common/ref_cycle_pybind.cc line 67 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

One thing we've learned about long reviewable threads is that at some point trying to sync up via email reaches a point of diminishing returns. Possibly this thread has reached that point. I suggest getting on a phone call to resolve the problem.

Per f2f, I think we've converged.

[rpoyner-tri]

+@sammy-tri for platform review, by schedule.

Dismissed @SeanCurtis-TRI from a discussion.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)

[sammy-tri]

Reviewed 1 of 4 files at r1, 2 of 2 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion