FEAT: re-enable multi shot trees, ActionNode work function decorator, implement tests to utilize multi shot trees #43

joshc-slac · 2024-09-11T02:27:37Z

Description

Moved our wrapped work function to be a decorator. This freed us from bound member confusion such that the self referenced could be the ActionWorker self.
This also re-enabled multi shot functionality which had been added in PR ENH: making doing work better #34 but then broken (for good reasons) in PR ENH/REF: Enable logging through multiprocessing, refactor node work signatures #41, hence the "new" 😄
Used new decorator to decorate "work functions", this work function which are specifically to be consumed by ActionNode and spawned as a Worker process by ActionNode worker now have type specified as beams.typing_helper.ActionNodeWorkLoop, this is the type return by the decorator
The schema can be stated as follows:
- ActionNode requires a function to be called within a while loop that lives for the lifecycle of the program. ActionWorker handles the spawning of the process that contains the while loop, it does so by extending the Worker base class. This spawned process must have a function signature of beams.typing_helper.ActionNodeWorkLoop such that all needed variables can be passed to the spawned process
- Within the ActionWorker which spawns the ActionNodeWorkLoop the function which is called at the specified loop frequency must have the signature of type ActionNodeWorkFunction which ensures a BT status is generated each loop cycle.
A representative test with parity to the real world IM2L0 machine was added to verify multishot capability.

Motivation and Context

Sequence automation aims to actively manage the state of our configured devices such that should they transition to an undesired state they can automatically transition back along defined transition vectors. Up until this point all the work packaged in ActionNodes only ran once, this will keep it living with the life cycle of the program such that when the tree is traversed again and a CheckAndDo node notices a state is out of whack it can fire off the action to deal with it again!

How Has This Been Tested?

With unit tests

Alternatives:

There is a world where Worker when spawning the process only passes the Value that signals program termination. (This could also be an Event). This would allow the wrapper to have remained a bound member function; however using a decorator seems more pythonic in both the way that it signals to users, properly signatures the function, and percolates up errors.

Code works interactively
Code follows the style guide
Code contains descriptive docstrings, including context and API
New/changed functions and methods are covered in the test suite where possible
Test suite passes locally
Test suite passes on GitHub Actions
Ran docs/pre-release-notes.sh and created a pre-release documentation page
Pre-release docs include context, functional descriptions, and contributors as appropriate
[ ]

tangkong

I want to think about this a bit more, I think there's an opportunity here to really clean this up nicely. But functionally this does what we wanted it to do

tangkong · 2024-09-11T16:37:20Z

beams/behavior_tree/ActionNode.py

@@ -12,72 +12,80 @@
 logger = logging.getLogger(__name__)


+def wrapped_action_work(func):
+    def work_wrapper(
+        work_self,


We claimed to have removed unclear self arguments but here it is again? I think this should explicitly pass in the do_work.value and avoid accessing members of an object that might have non-process safe members, for the sake of being clear to our future selves exactly what values we can manipulate

You're not wrong here, I debated this a little bit when implementing. I thought the most clarity came from this notion of work_self being understood to be an instance of the actual Worker (ActionWorker) that is running this thing made things the most clear but your ownership argument is very compelling.

To be clear this would be considered the "super-super" user level..... upon more reflection the "extensibility" I was aiming at with this decision may be misplaced. I can cut this change into the PR

tangkong · 2024-09-11T16:38:43Z

beams/behavior_tree/ActionNode.py

@@ -12,72 +12,80 @@
 logger = logging.getLogger(__name__)


+def wrapped_action_work(func):


In the future we'll probably have to come up with a more descriptive name for this decorator, since we'll probably have different ones for different types of actions. This one is a multi-shot decorator, but do we want ones with different timeout options in the future?

Thats a great perspective. This could also be the base wrapper or the wrapper could take a time out argument nice way to advertise to user space

tangkong · 2024-09-11T17:22:18Z

beams/behavior_tree/ActionNode.py

@@ -12,72 +12,80 @@
 logger = logging.getLogger(__name__)


+def wrapped_action_work(func):


I think my main ~~gripe~~ discomfort with this whole construction is how it masks the arguments that the (decorated) work function requires. We pass some of them into ActionWorker, then another in Worker, ultimately requiring a bunch of examination. This is something that I think has existed for a while and is a bit exacerbated by this decorator

A musing:

Where we can we might have the decorator take arguments, to reduce the amount of passing around to Worker subclasses. This also makes clear which args come from which scope, though it requires us to decorate the function in said scope

@wrapped_action_work(volatile_status, completion_condition) -> Callable[[work_gate, log_queue, log_configurer], None]

Maybe this also lets us keep the expected work function signature consistent inside the worker?

Ahhh I actually liked this abstraction. The work function is just the work function. All the "super" user who is not just editing config files has to concern themself with is writing the function that (thanks to #41) just has to logically return SUCCESS or FAILURE

But agree that we should much more thoroughly document how and why this works, likely with a small passage in ActionWorker that can be grabbed for more general documentation purposes

ZLLentz

I think this is good step forward, I don't have specific objections about the core ideas of the PR or the implementation.

Currently the PR text itself reads confusingly:

Title: multi shot trees
description: decorators (and motivation for decorators)
motivation: multi shot trees

I'd like to see this rearranged for documentation purposes so that we can document exactly what part of this PR enabled multi-shot trees (I assume it's clearing the work_gate + maybe one other thing?)

ZLLentz · 2024-10-07T22:10:33Z

beams/sequencer/helpers/Worker.py

@@ -42,12 +44,12 @@ def start_work(self):
        logger.debug("Starting work")

    def stop_work(self):
-        logger.info("Calling stop work on")
+        logger.debug("Calling stop work on")


Small pre-existing condition I'd like to note: these and similar log messages lack any sort of helpful context
"Calling stop work on {name}" for example

tangkong · 2024-10-07T22:27:13Z

Test failures stem from the use of StrEnum, which was only added in py3.11.

tangkong

I think this looks good. If our offline conversations are any indication, this will be an often-refactored part of our codebase, haha. But I do think this is an improvement.

tangkong · 2024-10-08T16:12:32Z

beams/tests/test_check_and_do.py


 logger = logging.getLogger(__name__)


 def test_check_and_do():
    percentage_complete = Value("i", 0)

+    @wrapped_action_work(loop_period_sec=0.001)
    def thisjob(comp_condition) -> None:


This type hint is now incorrect, this should return a Status

tangkong · 2024-10-08T17:00:11Z

I think aside from tests passing, I'd just want some documentation in the docstring of ActionNode or ActionItem about what we expect the work func signature to be

beams/typing_helper.py

tangkong

This looks mostly good to me. Some stray questions

tangkong · 2024-10-18T22:49:56Z

beams/tests/test_worker.py

@@ -32,7 +32,7 @@ def work_func(self):
    def test_inline_instantation(self):
        val = Value("i", 10)

-        def work_func():
+        def work_func(myself):


this myself crept back in here, and it remains un-used. The work function signature actually gets passed a Value now right?

yes, myself should be better named to represent Value. This parameter is now needed after we decided to remove the self and explicitly pass just the Value. We could even make a wrapper to edit the work function signature again...

BEAMS/beams/sequencer/helpers/Worker.py

Lines 33 to 35 in 885ddb0

self.work_proc = proc_type(target=self.work_func,

name=self.proc_name,

args=(self.do_work, *self.add_args,))

But we can hold off on that and leave this breadcrumb for future git blamers

tangkong · 2024-10-18T22:51:30Z

beams/tree_config.py

            try:
                # Set to running
-                value = caget(self.termination_check.pv)
+                value = caget(self.pv)  # double caget, this


Is there a continuation to this comment?

There is adding, Zach addresses it in future PR so I am happy to just leave call out

tangkong · 2024-10-18T22:53:16Z

beams/typing_helper.py

+Evaluatable = Callable[..., bool]
+
+'''
+Defines signature for an ActionNode work loop function handle.


Giga nitpick: I think technically block comments are supposed to have each line prepended with # . In my head triple quotes == docstrings, so this was very slightly disorienting to me.

Good to know

I'm somewhat curious what the pep8 reasoning is on this one
Maybe something to do with making it very syntactically clear which text is a comment and which text is to some extent functional?

beams/tests/mock_iocs/IM2L0.py

ZLLentz

I'm dropping a before-I-leave approval stamp, I like this. It's worth addressing small things now if they aren't too scope-changing or aren't handled elsewhere.

check flips state

…his adds clarity. clean up of some Worker debug messages, fixed wrong type hint, made compatible with py 3.10 and 3.9

…k Value argument

tangkong

👍 LTGM

joshc-slac requested a review from tangkong September 11, 2024 02:27

joshc-slac linked an issue Sep 11, 2024 that may be closed by this pull request

Take advantage of multi shot trees #36

Closed

tangkong reviewed Sep 11, 2024

View reviewed changes

joshc-slac marked this pull request as ready for review October 2, 2024 18:38

joshc-slac force-pushed the joshc-slac/new-multi-shot branch from ac37dff to 739ea4b Compare October 4, 2024 22:26

joshc-slac requested a review from tangkong October 4, 2024 22:28

ZLLentz previously approved these changes Oct 7, 2024

View reviewed changes

ZLLentz reviewed Oct 7, 2024

View reviewed changes

tangkong previously approved these changes Oct 8, 2024

View reviewed changes

joshc-slac changed the title ~~FEAT: enable multi shot trees~~ FEAT: re-enable multi shot trees, ActionNode work function decorator, implement tests to utilize multi shot trees Oct 16, 2024

joshc-slac dismissed stale reviews from tangkong and ZLLentz via 74707c9 October 16, 2024 20:09

ZLLentz reviewed Oct 16, 2024

View reviewed changes

beams/typing_helper.py Outdated Show resolved Hide resolved

ZLLentz reviewed Oct 16, 2024

View reviewed changes

beams/typing_helper.py Outdated Show resolved Hide resolved

joshc-slac requested a review from tangkong October 18, 2024 22:13

tangkong reviewed Oct 18, 2024

View reviewed changes

ZLLentz previously approved these changes Oct 18, 2024

View reviewed changes

joshc-slac dismissed ZLLentz’s stale review via 7310454 October 18, 2024 23:25

joshc-slac added 10 commits October 18, 2024 19:51

feat: reenabled mutlishot work by moving work wrapper to a decorator

ef7b406

taking advantage of multi shot once more

6aa98a3

decorated and fixed tests

106c741

fix: not passing the entirety of work instance, just critical Value

edca510

enh: addressing Zach's valid concern about where loop period sleep goes

a0f374c

chore: add tests that exercise ability for tree to fix itself after a

012b3c6

check flips state

chore: aggregated custom types in new file typing_helper. hopefully t…

53a956a

…his adds clarity. clean up of some Worker debug messages, fixed wrong type hint, made compatible with py 3.10 and 3.9

good nits

8716fb5

pre-commit :p

a687b6e

fix(test_worker): work functions now need to accept the passed do_wor…

68a3d79

…k Value argument

joshc-slac force-pushed the joshc-slac/new-multi-shot branch from 7310454 to 573ab0b Compare October 19, 2024 02:54

good nits p2

dcefc01

joshc-slac force-pushed the joshc-slac/new-multi-shot branch from 573ab0b to dcefc01 Compare October 19, 2024 02:56

joshc-slac requested a review from tangkong October 21, 2024 19:41

tangkong approved these changes Oct 21, 2024

View reviewed changes

joshc-slac merged commit 53b006f into master Oct 23, 2024
14 checks passed

joshc-slac deleted the joshc-slac/new-multi-shot branch October 23, 2024 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: re-enable multi shot trees, ActionNode work function decorator, implement tests to utilize multi shot trees #43

FEAT: re-enable multi shot trees, ActionNode work function decorator, implement tests to utilize multi shot trees #43

joshc-slac commented Sep 11, 2024 •

edited

Loading

tangkong left a comment

tangkong Sep 11, 2024

joshc-slac Sep 11, 2024

tangkong Sep 11, 2024

joshc-slac Sep 11, 2024

tangkong Sep 11, 2024 •

edited

Loading

joshc-slac Sep 11, 2024

joshc-slac Sep 11, 2024

ZLLentz left a comment

ZLLentz Oct 7, 2024 •

edited

Loading

tangkong commented Oct 7, 2024

tangkong left a comment

tangkong Oct 8, 2024

tangkong commented Oct 8, 2024

tangkong left a comment

tangkong Oct 18, 2024

joshc-slac Oct 18, 2024

tangkong Oct 18, 2024

joshc-slac Oct 18, 2024

tangkong Oct 18, 2024

joshc-slac Oct 18, 2024

ZLLentz Oct 18, 2024

ZLLentz left a comment

tangkong left a comment

		@@ -12,72 +12,80 @@
		logger = logging.getLogger(__name__)


		def wrapped_action_work(func):

	self.work_proc = proc_type(target=self.work_func,
	name=self.proc_name,
	args=(self.do_work, *self.add_args,))

FEAT: re-enable multi shot trees, ActionNode work function decorator, implement tests to utilize multi shot trees #43

FEAT: re-enable multi shot trees, ActionNode work function decorator, implement tests to utilize multi shot trees #43

Conversation

joshc-slac commented Sep 11, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Alternatives:

tangkong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangkong Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz left a comment

Choose a reason for hiding this comment

ZLLentz Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

tangkong commented Oct 7, 2024

tangkong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tangkong commented Oct 8, 2024

tangkong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz left a comment

Choose a reason for hiding this comment

tangkong left a comment

Choose a reason for hiding this comment

joshc-slac commented Sep 11, 2024 •

edited

Loading

tangkong Sep 11, 2024 •

edited

Loading

ZLLentz Oct 7, 2024 •

edited

Loading