[decorators] finally-style decorators and idioms

[stonier]

Inspired from discussion in #425 (comment).

[amalnanavati]

Thanks for starting this so promptly!

I like the idea of a finally decorator to enforce that ticking the child happens in terminate rather than update. Although this in isolation isn't the finally functionality; as your comment mentions, it should be used within a Parallel to achieve that.

What do you think of renaming this decorator to something like TickOnTerminatation, and then having a finally idiom that takes in two children -- a main behavior and the cleanup behavior -- and puts them together in a parallel, with the TickOnTermination decorator on top of the cleanup behavior?

[amalnanavati]

Is a tick_once necessarily enough to process the child behavior's sequence? The child behavior might be making an asynchronous call (e.g., to a ROS service, as I contributed in this py_trees_ros PR). I think this should instead tick the child behavior to completion.

(Which brings up a question of how often to tick the child behavior -- perhaps that should be a parameter passed to __init__?)

[stonier]

Not at all, you can see in my comments to the class a single tick won't suffice for all cases (i.e. here.

I think what we need then is a renamed decorator and two idioms. One for a single-tick finally with the decorator and one for a more complex multi-tick finally as highlighted in that comment.

[amalnanavati]

Ah I missed that part of the comment.

Adding onto what you wrote, and in light of our discussion on cloning, the multi-tick idiom would need three children passed in: the main behavior and two copies of the finally behavior, one to run on success and one to run on failure.

[amalnanavati]

I'm still trying to think of how we can eliminate the need to clone behaviors. What if our idiom is as follows:

Sequence
  |  FailureIsSuccess
  |    |  StatusToBlackboard
  |    |    |  Work
  |  Finally
  |  BlackboardToStatus

This way, if the work fails, the failure is passed up, and if finally fails, the failure is passed up. The only way this tree succeeds if both the Work and Finally succeeds.

[stonier]

(Which brings up a question of how often to tick the child behavior -- perhaps that should be a parameter passed to init?)

With respect to this, I think the most intuitive default should be to tick as often as it needs to go to completion, i.e. to SUCCESS || FAILURE. If for some reason you explicitly need to control the # of ticks, you can use a Counter and a Parallel in the subtree to control that.

[amalnanavati]

Ah, I wasn't thinking about # of ticks, I was thinking about ticking rate. I agree with you that we should tick as necessary to complete the tree.

[stonier]

I'm still trying to think of how we can eliminate the need to clone behaviors. What if our idiom is as follows:

Sequence
  |  FailureIsSuccess
  |    |  StatusToBlackboard
  |    |    |  Work
  |  Finally
  |  BlackboardToStatus

After using them quite heavily, I found I am not a fan of the FailureIsSuccess style decorators and almost never use them now. They make it awfully hard to inspect a tree - human brains don't tend to fare so well with flip-flops, negations and even worse, double negations. A larger-tree is far more acceptable than a hard-to-read tree, especially if you make use of viz tooling that collapses parts of the tree.

Also, I think we should indeed pass in two handler behaviours (or subtrees). That makes it wonderfully general - you can pass in different handlers for failure/success or a clone of them.

[stonier]

Updated the PR with an OnTerminate decorator and eventually idiom.

Favouring the word eventually here, because:

1. I'm using eventually as a keyword for similar behaviour elsewhere (py_trees.behaviours.StatusQueue),
2. it avoids wierd naming workarounds to get around finally being a pythonic keyword AND
3. if I implement the multi-tick version of finally (next step), then an eventually idiom which can handle failure and success differently makes perfect sense.

[amalnanavati]

This all sounds good :) Fwiw, the multi-tick idiom you proposed is fundamentally a try-except-else clause, which as you mentioned is quite general and can encompass try-finally logic.

[stonier]

Alright, about done I think.

• decorators.OnTerminate() and test
• idiom.eventually, test and py-trees-demo-eventually
• idiom.eventually_swiss, test and py-trees-demo-eventually-swiss

I did think about switching the idiom names to try_finally and try_finally_else but I think this works ok - it still avoids some keyword conflicts in naming variables to the idiom, it also I think, avoids having to context switch between tree-thinking and python-thinking (I like on_completion, on_success, on_failure here) and lastly, not really keen to do another once-over on it all :P

I have paid homage to python's try-finally and try-finally-else in the idiom descriptions though.

I'll leave this PR open for a bit, feel free to comment on it further. Thanks for being a useful sounding board!

[amalnanavati]

Shouldn't this have memory? Once on_success_sequence fails, you no longer want to run it again; you only want to run on_failure_sequence till completion.

[amalnanavati]

I think the issue with memory=False doesn't arise in this test because the on_failure and on_success behaviors resolve in a single tick.

[amalnanavati]

Not sure if this is the best example of eventually_swiss because the on_failure and on_success each only take one tick. In fact, this example is basically equivalent to your eventually example above. Perhaps on_success and on_failure each be a sequence with a tick counter, to better illustrate the multi-tick nature of this idiom?

[amalnanavati]

Other than the above comments, looks good :)

[amalnanavati]

Thinking about this more, I realized that the multi-tick eventually_swiss doesn't cover preemptions (e.g., if a node higher up in the tree, or the code ticking the tree, calls stop(INVALID)).

• In eventually, on_completion will get ticked once if worker succeeds, fails, or is the entire idiom is preempted.
• However, in eventually_swiss we only allow users to pass in on_success and on_failure, but if the tree gets preempted then neither may run.

I think to address this, we need a decorator for OnTerminateMultiTick, which does the following:

• On update(), immediately returns FAILURE
• On terminate(INVALID), ticks on_completion until it reaches a non-RUNNING state.

Then, eventually_swiss should become:

SelectorWithoutMemory
  | OnTerminateMultiTick
  |    | on_preempt
  | SelectorWithMemory
  |    | SequenceWithMemory
  |    |    | worker
  |    |    | on_success
  |    | SequenceWithMemory
  |    |    | on_failure
  |    |    | Failure

I believe this formulation ensures the following:

1. on_preempt is called (and ticked to completion) only when stop(INVALID) is called on the root.
2. on_success is called only when worker succeeds.
3. on_failre is called only when worker fails.

(Note that I considered achieving this behavior with a Parallel at the root, as in eventually. However, the trouble is that Parallel calls INVALID on its children regardless of whether it reaches FAILURE, SUCCESS, or INVALID, which means that OnTerminateMultiTick as a child of Parallel won't be able to distinguish a preemption from a success/failure. On the other hand, Selectors only call stop(INVALID) on its children when stop(INVALID) is called on the selector itself, allowing us to distinguish those situations.)

Let me know what you think. I know this is getting a bit unwieldy, but the ability to perform cleanup on preemption seems pretty important, and it is currently only supported if the cleanup behavior can execute in a single tick.

[amalnanavati]

For what it's worth, I went ahead and implemented the above changes in my local repo. If you want to see the changes I made to your files, they are in this commit, and I'm happy to PR them into this branch if you're interested. If you want to see the scoped_behavior idiom I build on top of eventually_swiss, feel free to look at the PR.

The only downside of the above formulation is on preemption, on_preempt is run before the terminate function for worker, whereas it should be swapped imo. I have yet to figure out how to address that.

[amalnanavati]

Alright, I addressed the above issue in the PR on my repo. I also wrote comprehensive test cases, that should guarantee the following:

1. on_success is run if and only if workers succeed.
2. on_failure is run if and only if workers fail (that is a bug in your current implementation -- on_failure runs if on_success fails).
3. on_preempt is run if and only if a node above the root calls root.stop(INVALID).
4. If workers succeed, the return status is the success/failure status of on_success.
5. If workers fail, the return status is failure.
6. If a node above the root calls root.stop(INVALID), first terminate(INVALID) is called on workers, on_success/on_failure, and THEN on_preempt is ticked to completion.

Achieving the above functionality required generalizing OnTerminate into an OnPreempt decorator that takes a separate behavior other than its child to tick on preemption (and to make it the option to be multi-tick). This is a strict generalization -- OnTerminate can be created by passing behaviours.Running as the child to OnPreempt. The only downside I see of OnPreempt is that the preemption behavior is not part of the tree structure/iterator, but I would argue that that is fine since it is termination bheavior and not standard ticking behavior.

The idiom eventually_swiss is as follows.

OnPreempt(on_preempt)
  |-SequenceWithMemory
  |   |-SelectorWithMemory
  |   |    |-SequenceWithMemory
  |   |    |    |-workers[1]
  |   |    |    |-...
  |   |    |    |-workers[n]
  |   |    |-SequenceWithMemory
  |   |    |    |-on_failure
  |   |    |    |-Failure
  |   |-on_success

And as I said before, I'm happy to PR the changes into this branch and/or repo -- let me know if you're interested in that! You're also welcome to re-use my test cases -- I believe the only area where my test cases may have overfit to my implementation is when it comes to the new_status values that the terminate functions are called with.

[amalnanavati]

@stonier, any updates on this? I have multiple changes that improve upon this PR (e.g., a multi-tick generalization of OnTerminate, a re-factoring of eventually_swiss that handles preemptions). We've been using it for a year in production code, and I'd love to PR the changes into this repo. But it would be easiest/cleanest if this PR gets merged in, so my PR can directly build upon it.