Open amalnanavati opened 10 months ago
There's lots of nuances to this once you get beyond simple objects.
On the subject of Copies
A copy or deepcopy should logically give you a copy of the object in it's current state, so:
class Foo(behaviour.Behaviour):
def __init__(self, initial_value):
self.counter = initial_value
def update():
self.counter += 1
foo = Foo(5)
foo.copy() # a copy here should give you an object with self.counter = 5
foo.tick()
foo.tick()
foo.copy() # a copy here should give you an object with self.counter = 7
Q: Do you want constructed copies or runtime copies? Does it matter (i.e. if you want constructed copies, calling it before you tick it is sufficient)?
What about init() logic?
Suppose you want copies, but you have a behaviour like this:
class Foo(behaviour.Behahviour):
def __init__(self, channel_name):
connect_to_some_middleware_channel(channel_name, self.callback)
def callback(self):
println("May he reach out and bless you with a tickle from his noodly appendages")
foo = Foo("noodles")
bar = foo.copy() # a non-functional copy - no channel has been setup
The copy skips the business logic happening in __init__
- probably not what you want, potentially very surprising and great fuel for bugs. Creating threads and the like suffer from similar problems.
You might do things a little differently:
class Foo(behaviour.Behahviour):
def __init__(self, channel_name):
self.channel_name = channel_name
def setup(self):
connect_to_some_middleware_channel(self.channel_name, self.callback)
def callback(self):
println("May he reach out and bless you with a tickle from his noodly appendages")
foo = Foo("noodles")
bar = foo.copy() # a non-functional copy - no channel has been setup
bar.setup() # aha, now you are cooking, but this is a 'magic' step
bar.tick()
foobar = bar.copy()
foobar.setup() # ok here, but what if bar.tick() does things that break a future call on setup()?
You're depending on a user to keep their init() very clean and a call to setup() here. That might be ok for a specific behaviour in actual user code, but copy()
is not something I'd be comfortable about generalising and building into the py_trees framework as it's leaving users with potentially non-functional copies that require them to follow certain 'magic' steps to get it functional. Even after that it may still be non-functional (e.g. if the channel construction was in init().
What about Blackboards?
This is another problem - the blackboard can often be an indirect form of state for a behaviour. Do you copy that too? It can be a right thing to copy it in some situations and an entirely wrong thing to do so in other situations.
On the subject of Clones
The distinction between copy and clone is usually about whether or not you need to program some logic above and beyond a brute force copy. That could help you implement logic usually hidden in init(), but it requires a behaviour to do some extra programming.
So what to do.
The concept of copying is terribly problematic. I'd avoid this at all cost.
I could imagine implementing a clone()
method in behaviour.Behaviour that by default, throws an exception. Child classes could override it and put their cloning logic inside. Most of the behaviours in py_trees and py_trees_ros could probably implement this method. It's worth noting, that you might probably never be able to rely on every behaviour implementing this method though, but nonetheless, it could still be useful.
A helper function to help you create objects is a perfectly fine thing. I very often use the idiom pattern to create identical subtrees that need to be inserted in multiple locations across a complex tree.
Another point that might be pertinent - try to avoid instantiating duplicate ROS objects across the tree. This gets very heavy. One way to avoid this is to split your behaviours - put ROS machinery in one and programming logic in the other. Use the blackboard to connect them. e.g.
You can flip that around too and have ROS behaviours up front that write data to blackboard variables. Then pythonic behaviours in the tree make use of that data. This can save you having to install N listeners for behaviours that are cloned across the tree and may or may not be fired. See tutorial two. You could have that battery emergency subtree in multiple locations across the tree, but because Battery2BB is up front, there's only one listener.
Thank you for the detailed response. I agree with what you highlighted about the challenges of implementing a copy
function since it would require users to follow ‘magic’ steps to enable their behavior to work with it.
I like the idea of a clone
function that subclasses of Behavior can/should implement, and think it would be a good approach for my situation. However, I will detail a representative example of my situation below in case you have another suggestion for addressing it.
In terms of standalone ROS nodes, I have MoveIt2’s MoveGroup running. I want a tree that performs two consecutive motions -- the first ignoring a particular collision object in MoveIt's planning scene (done via service calls to MoveIt), and the second respecting that collision object. Hence, my “main tree” is:
SequenceWithMemory[
ToggleCollisionObjectOff,
MoveToA,
ToggleCollisionObjectOn,
MoveToB,
]
However, say there is a failure in MoveToA
. I still want to perform cleanup by toggling the collision object back on, before passing the failure up the tree (i.e., like a Python finally
statement). So my final tree looks like:
SelectorWithMemory[
SequenceWithMemory[
ToggleCollisionObjectOff,
MoveToA,
ToggleCollisionObjectOn,
MoveToB,
]
SuccessIsFailure[ToggleCollisionObjectOn]
]
Hence, I need to use the ToggleCollisionObjectOn
behavior multiple times in the tree.
I currently use an idiom to create ToggleCollisionObject
behaviors, but it takes many lines of code to create the behavior twice. The idiom needs to take in the behavior's name
, the ROS node
to associate the service client with, the collision_object_id
, whether to allow
or disallow collisions, and the logger
for that behavior. Calling this idiom multiple times in the tree is 5+ lines of code each time (using Black formatter and pylint). If, on the other hand, I could call a clone
function (perhaps taking in a new name
as an argument), then I can set the parameters of the behavior just once (with 5+ lines of code) and then call a single line to duplicate it.
The core issue here is that I’ve been making idioms generic enough to be used across multiple trees; hence they take in lots of parameters. However, when I want to reuse a behavior within a single tree, (nearly) all the parameters stay the same, so it seems overkill to call the whole idiom again.
As I was writing the above comment, I realized that Python partial functions can address what I need. So there is at least one way around this without a behavior clone
functionality.
With that said, I’d still love any suggestions you have on the tree structure, particularly on:
finally
style of cleanup behavior;ToggleCollisionObject
behavior has its own ROS service client, and each MoveTo
call has its own ROS action client. The only way I can see around that is putting those service/action clients on the Blackboard, but that seems against the spirit of the Blackboard (and deviates from how py_trees_ros
does it).Thanks again!
finally
You can get this behaviour with something like:
|| Parallel
- Moving Sequence
+ ...
+ Finally
with some implementation in Finally's terminate()
method:
class Finally(behaviour.Behaviour)
def update():
return RUNNING
def terminate():
// code that toggles the collision object
When the sequence completes the parallel will invalidate Finally, triggering it's terminate()
method and the parallel will reflect the sequence's result.
That doesn't let you reuse behaviours though. One idea that we could implement in py_trees
is to use a decorator.
|| Parallel
- Moving Sequence
+ ...
^ Finally Decorator
+ Toggle Collision Object On
The decorator would always return running, and if invalidated, it would call the child's update()
method from it's own terminate()
method. Then invalidate the child.
One nice advantage here - it's very easy to read from the tree what's happening.
The action clients aren't easy to move around like I suggested earlier - I should have mentioned that. The principle of shifting ROS mechanisms to the front and back of the tree mainly applies to publishers, subscribers and sometimes services.
In this case, the toggle collision object service doesn't actually need to be called right there and then in the middle of your tree tick. You could reset/write a boolean to the blackboard with (Un)SetBlackboardVariable
in the tree and at the end have a behaviour that reads from the blackboard and calls the service.
Got it, this all makes sense. Handling the finally
logic in a terminate function makes sense.
Thanks for starting the PR! I have some thoughts, which I'll put in the PR directly.
I'd like to be able to use the same behavior in multiple parts of a tree (e.g., calling a ROS service that toggles some robot functionality on/off multiple times in the same tree). #281 disallowed this, because it may be problematic if behaviors maintain state.
I don't mind re-creating another version of the same behavior, but I want to do so in a way that maximizes code re-use. I see two options:
In terms of the second option, I was wondering:
copy.deepcopy
work on a behavior? I imagine not, because things like theparent
attribute would also be copied over, which is what gives rise to the error in #281 .