[decorators] finally-style decorators and idioms

[stonier]

Inspired from discussion in https://github.com/splintered-reality/py_trees/issues/425#issuecomment-1701545897.

[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.

• [x] decorators.OnTerminate() and test
• [x] idiom.eventually, test and py-trees-demo-eventually
• [x] 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]

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.