ENH: rework tree serialization, accompanying tests #30

tangkong · 2024-08-07T23:59:37Z

Description

Rework tree config file serialization and in-memory representation to more directly match Behavior Tree Nodes
- Each dataclass is a BaseItem subclass
- Each BaseItem has a get_tree method that returns the py_trees.behaviour.Behaviour` specified by the dataclass
- This keeps logic isolated to each item
Reworks tests to match new serialization scheme
- Keeps test scope focused. If we are testing a serialization round-trip, we don't need to write to file. If we are pulling from a file, don't compare to in-memory representations.
One small bugfix, making sure status is set after completion condition is met (this was causing failures for me locally)

Motivation and Context

Closes #18

How Has This Been Tested?

Tests have been adjusted

Where Has This Been Documented?

This PR

Pre-merge checklist

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

…serialization

… always set status

joshc-slac · 2024-08-08T00:12:23Z

beams/serialization.py

+def get_all_subclasses(cls: type) -> Iterator[type]:
+    """Recursive implementation of type.__subclasses__"""
+    for sub_cls in cls.__subclasses__():
+        yield sub_cls


beginning to get comfortable with this pythonic yield idea used in recursion, still might need my own toy example to understand it well enough to explain simply

Note: it seems this should actually be hinted as returning a Generator oops

In this case the yield is just pausing calculation and "yielding" the value. ~~Iterators~~ Generators are a way to do make sure we only do calculations when we want the next value.

Using a generator is the same as using a fully defined list or other iterator, we just doesn't know a-priori how long that generator is

The type hinting documentation presents Iterator[type] and Iterable[type] as fully-allowable sub-ins for Generator[type, None, None] (I think because they behave the same way to the end user)

joshc-slac · 2024-08-08T00:14:21Z

beams/tests/artifacts/eggs.json

-    "Pv": "PERC:COMP",
-    "Mode": "INC",
-    "Value": 10
+  "root": {


wish i could heart react files this looks great!

joshc-slac · 2024-08-08T00:29:26Z

beams/tree_config.py

+@as_tagged_union
+@dataclass
+class ActionItem(BaseItem):
+    loop_freq: float = 1.0


crazy pedantic, I may have even suggested the original name.

Suggested change

loop_freq: float = 1.0

loop_sleep_sec: float = 1.0

Not a huge fan of this either (does not roll off tongue) but makes it very clear to others what it is

Critically this is the period, not the frequency 🤦

True too, but as other things may also take up clock cycles in the action I think indicating it as a sleep could be helpful. Semantics, whatever you pick is good.

beams/tree_config.py

joshc-slac · 2024-08-08T00:33:21Z

beams/tree_config.py

+    def get_condition_function(self) -> Callable[[], bool]:
+        op = getattr(operator, self.operator.value)
+
+        def cond_func():


Reminder to myself / us in a future PR to potentially nest this in a subclass much the same as we have done with ActionItem that hardcodes this type of Conditional (where we are only cagetting) such that we can have a slightly more generic ConditionNode baseclass

Yeah this should definitely be a PVComparisonConditionItem or something

joshc-slac · 2024-08-08T01:10:51Z

Linking issue #32, will build off of this awesome work

joshc-slac · 2024-08-08T01:27:45Z

beams/tree_config.py

+
+
+@dataclass
+class BehaviorTreeItem:


I am wondering if more things should subclass this and not base item so that they have the root object to tick? tell me if I am misunderstanding design

My understanding of py_trees is that all "trees" should be a py_trees.tree.BehaviourTree. The root node of that BehaviourTree gives the pre/post tick hooks and other logging niceties. https://py-trees.readthedocs.io/en/devel/trees.html

We can individually tick nodes / sub-trees (and we should during testing), but when ingesting a full behavior tree I think this is probably the object we should interact with

joshc-slac

Super potent PR, awesome stuff

ZLLentz

I'm on board, just one seemingly unused and undocumented thing that confused me

ZLLentz · 2024-08-08T22:29:28Z

beams/serialization.py

+
+
+def alternative_constructor(func: Func) -> Func:
+    """Alternative constructor for a given type."""


What is this function used for? It looks like it essentially registers another function (not stated in the docstrings here), but I don't see any place where this is called in beams. I do see one place where the empty constructors stash is used.

We don't use it, but we could if we wanted to create an alternative constructor. We don't make use of this but it came as part of the magic spell from wyfo:

@alternative_constructor def sized_line( start: float, stop: float, size: Annotated[float, schema(min=1)] ) -> "Line": return Line(start=start, stop=stop, step=(stop - start) / (size - 1))

https://wyfo.github.io/apischema/0.18/examples/subclass_tagged_union/

I believe it's used for some un-tagged union parsing, but that's a deeper dive I'd need some time to complete wyfo/apischema#56 (reply in thread)

tangkong added 4 commits August 7, 2024 16:42

MNT/REF: refactor tree dataclasses to support arbitrary children and …

8bef16b

…serialization

MNT: make ConditionNode run caget, run condition checks after loop to…

0a550d3

… always set status

TST: adjust tests to match new serialization schema

25611c7

DOC: pre-release notes

de5f99e

joshc-slac reviewed Aug 8, 2024

View reviewed changes

beams/tree_config.py Outdated Show resolved Hide resolved

tangkong mentioned this pull request Aug 8, 2024

TST: expand serialization / tree generation tests #31

Closed

joshc-slac reviewed Aug 8, 2024

View reviewed changes

tangkong requested a review from ZLLentz August 8, 2024 00:34

joshc-slac reviewed Aug 8, 2024

View reviewed changes

tangkong mentioned this pull request Aug 8, 2024

ENH: Determine if we want on success / on failure actions encoded into our action nodes OR if we want to be more BT principic #32

Closed

MNT: rename loop_freq -> loop_period_sec for clarity

cd8bb71

joshc-slac self-requested a review August 8, 2024 20:37

joshc-slac approved these changes Aug 8, 2024

View reviewed changes

ZLLentz reviewed Aug 8, 2024

View reviewed changes

ZLLentz approved these changes Aug 8, 2024

View reviewed changes

tangkong merged commit 02e53ff into pcdshub:master Aug 9, 2024
6 of 11 checks passed

tangkong mentioned this pull request Aug 9, 2024

JSON generated trees #8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: rework tree serialization, accompanying tests #30

ENH: rework tree serialization, accompanying tests #30

tangkong commented Aug 7, 2024 •

edited

Loading

joshc-slac Aug 8, 2024

tangkong Aug 8, 2024 •

edited

Loading

ZLLentz Aug 8, 2024 •

edited

Loading

joshc-slac Aug 8, 2024

joshc-slac Aug 8, 2024 •

edited

Loading

tangkong Aug 8, 2024

joshc-slac Aug 8, 2024

joshc-slac Aug 8, 2024

tangkong Aug 8, 2024

joshc-slac commented Aug 8, 2024

joshc-slac Aug 8, 2024

tangkong Aug 8, 2024

joshc-slac left a comment

ZLLentz left a comment

ZLLentz Aug 8, 2024

tangkong Aug 8, 2024

tangkong Aug 8, 2024



		def alternative_constructor(func: Func) -> Func:
		"""Alternative constructor for a given type."""



		@dataclass
		class BehaviorTreeItem:

ENH: rework tree serialization, accompanying tests #30

ENH: rework tree serialization, accompanying tests #30

Conversation

tangkong commented Aug 7, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Pre-merge checklist

Choose a reason for hiding this comment

tangkong Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

ZLLentz Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshc-slac Aug 8, 2024 • edited Loading

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

joshc-slac commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshc-slac left a comment

Choose a reason for hiding this comment

ZLLentz 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

tangkong commented Aug 7, 2024 •

edited

Loading

tangkong Aug 8, 2024 •

edited

Loading

ZLLentz Aug 8, 2024 •

edited

Loading

joshc-slac Aug 8, 2024 •

edited

Loading