Support decorators and composites with ports

Author: sea-bass

py_trees/parsers/behaviour_tree_xml.py

@@ -574,6 +574,7 @@ def build_port_remappings(
 
 def instantiate_ports_node(
     elem: ET.Element,
+    cls: type[PortsMixin],
     node_registry: dict,
     remapping_table: dict[str, str],
     subtree_namespace: str,
@@ -591,6 +592,7 @@ def instantiate_ports_node(
 
     Args:
         elem: The XML element to parse.
+        cls (type[py_trees.behavious.Behaviour]): A behaviour class type.
         node_registry (dict): Mapping from class names (str) to callables (constructors or partials)
             that return ``PortsMixin``-derived instances.
         remapping_table (dict): Mapping from keys (str) to absolute keys (str).
@@ -602,15 +604,6 @@ def instantiate_ports_node(
     Returns:
         PortsMixin: the fully initialised node.
     """
-    portsmixin_name = elem.tag
-    # Get the class definition from node_registry - needed to check the ports.
-    try:
-        cls = get_class_from_registry(portsmixin_name, node_registry)
-    except KeyError as e:
-        raise NotImplementedError(
-            f"Class name '{portsmixin_name}' not found in node_registry. Supporting other types is still TODO."
-        ) from e
-
     port_remappings = build_port_remappings(
         elem=elem,
         class_=cls,
@@ -621,22 +614,19 @@ def instantiate_ports_node(
 
     instance_name = generate_node_name(
         explicit_name=elem.attrib.get("name", None),
-        general_name=portsmixin_name,
+        general_name=elem.tag,
         prefix=parent_names_str,
     )
     logger.debug(f"PortsMixin node '{instance_name}' in namespace {subtree_namespace} remappings: {port_remappings}")
 
-    if portsmixin_name not in node_registry:
-        raise ValueError(f"PortsMixin class '{portsmixin_name}' not found in node_registry table")
-
     constructor_kwargs = constructor_kwargs or {}  # create constructor_kwargs
     for attrib_key, attrib_value in elem.attrib.items():
         if attrib_key == "name":
             continue
         if attrib_key not in cls.input_ports() and attrib_key not in cls.output_ports():
             if is_key(attrib_value):
                 raise ValueError(
-                    f"'{portsmixin_name}'(name='{instance_name}'): Port remappings are "
+                    f"'{elem.tag}'(name='{instance_name}'): Port remappings are "
                     f"not supported for non-port attributes ('{attrib_key}'='{attrib_value}'). "
                 )
             # Consistency check: if constructor_kwargs already has this key, and it's conflicting,
@@ -654,7 +644,7 @@ def instantiate_ports_node(
             )
             constructor_kwargs[attrib_key] = attrib_value
 
-    ctor_callable = node_registry[portsmixin_name]
+    ctor_callable = node_registry[elem.tag]
     # Try to convert the constructor arguments to the correct type.
     ignore_keys = {"child", "children", "behaviour_class_name"}
     constructor_kwargs, success = apply_type_hints(ctor_callable, constructor_kwargs, logger=logger, ignore=ignore_keys)
@@ -665,14 +655,14 @@ def instantiate_ports_node(
         )
     try:
         # Pass the behaviour_class_name (the tag/registry name) to the constructor
-        node: PortsMixin = node_registry[portsmixin_name](
+        node: PortsMixin = ctor_callable(
             name=instance_name,
-            behaviour_class_name=portsmixin_name,
+            behaviour_class_name=elem.tag,
             **constructor_kwargs,
         )
     except Exception as e:  # Catch everything that may go wrong in the constructor
         raise XMLParserError(
-            f"Failed to instantiate '{portsmixin_name}' with args {constructor_kwargs} "
+            f"Failed to instantiate '{elem.tag}' with args {constructor_kwargs} "
             f"(check: does the instantiated class have kwargs in __init__?): {e}"
         ) from e
 
@@ -718,24 +708,73 @@ def build_tree_from_xml(
     logger.debug(f"Processing tag: '{elem.tag}' with attributes {elem.attrib}. Remapping table: {remapping_table}")
     if elem.tag in node_registry:
         # Prioritize nodes that are in the registry, which could even be user overrides for built-ins.
-        # This is any leaf PortsMixin node (any PortsMixin + Behaviour combination).
-        logger.debug(f"Creating PortsMixin leaf node for tag {elem.tag}.")
+        # This is any PortsMixin node (any PortsMixin + Behaviour combination).
+        try:
+            cls = get_class_from_registry(elem.tag, node_registry)
+        except KeyError as e:
+            raise NotImplementedError(
+                f"Class name '{tag}' not found in node_registry. Supporting other types is still TODO."
+            ) from e
+
+        if not (issubclass(cls, PortsMixin) and issubclass(cls, py_trees.behaviour.Behaviour)):
+            raise TypeError(
+                f"XML tag '{elem.tag}' did not instantiate a PortsMixin + "
+                f"py_trees.behaviour.Behaviour; got {cls.__name__}"
+            )
+
+        children = []
+        constructor_kwargs: dict | None = None
+        is_decorator = issubclass(cls, py_trees.decorators.Decorator)
+        is_composite = issubclass(cls, py_trees.composites.Composite)
+        if is_decorator and len(elem) != 1:
+            raise XMLParserError(f"Decorator node '{tag}' must have exactly 1 child, found {len(elem)}.")
+        if is_decorator or is_composite:
+            for child in elem:
+                # Use no general name fallback for the parent_names_str to pass to the children.
+                # Otherwise, the generated node names will get too long and not very readable.
+                # It also means that we lose the guarantee of unique names, but that can be avoided by
+                # assigning an explicit name to the parent tags.
+                concise_parent_names_str = generate_node_name(
+                    explicit_name=elem.attrib.get("name", None),
+                    general_name="",
+                    prefix=parent_names_str,
+                    no_uuid=True,
+                )
+                child_node = build_tree_from_xml(
+                    child,
+                    remapping_table,
+                    node_registry,
+                    bt_index,
+                    logger=logger,
+                    subtree_namespace=subtree_namespace,
+                    parent_names_str=concise_parent_names_str,
+                )
+                if not isinstance(child_node, py_trees.behaviour.Behaviour):
+                    raise TypeError(f"Child node of type {type(child_node).__name__} is not a valid py_trees Behavior.")
+                children.append(child_node)
+
+        if is_decorator:
+            constructor_kwargs = {"child": children[0]}  # already checked its length
+        elif is_composite:
+            constructor_kwargs = {"children": children}
+
+        logger.debug(f"Creating PortsMixin node for tag {elem.tag}.")
         node = instantiate_ports_node(
             elem=elem,
+            cls=cls,
             node_registry=node_registry,
             remapping_table=remapping_table,
             subtree_namespace=subtree_namespace,
             logger=logger,
+            constructor_kwargs=constructor_kwargs,
             parent_names_str=parent_names_str,
         )
-        if not (isinstance(node, PortsMixin) and isinstance(node, py_trees.behaviour.Behaviour)):
-            raise TypeError(
-                f"XML tag '{elem.tag}' did not instantiate a PortsMixin + "
-                f"py_trees.behaviour.Behaviour; got {type(node).__name__}"
-            )
         return node
+
     elif tag in PARENT_NODES_TAGS:
-        # Composite/Decorator nodes which can have children:
+        # Composite/Decorator nodes which can have children.
+        # TODO: Everything in this section should be eventually replaced with actual
+        # built-in behaviours with ports that are part of the node registry.
         node_name = generate_node_name(
             explicit_name=elem.attrib.get("name", None),
             general_name=elem.tag,

tests/test_ports_xml_parser.py

@@ -19,7 +19,7 @@
 
 import py_trees
 from py_trees.parsers.behaviour_tree_xml import is_key, parse_behaviour_tree_xml
-from py_trees.ports import BehaviourWithPorts, PortInformation, get_ports_registry
+from py_trees.ports import BehaviourWithPorts, PortInformation, PortsMixin, get_ports_registry
 from py_trees.ports_utils import (
     find_node_by_class,
     find_node_by_name,
@@ -664,6 +664,91 @@ def test_decorator_node(self) -> None:
         finally:
             os.unlink(path)
 
+    def test_decorator_ports_node(self) -> None:
+        """Verify that a decorator with ports is instantiated correctly."""
+
+        class RepeatWithPorts(PortsMixin, py_trees.decorators.Repeat):
+            """A `py_trees.decorators.Repeat` decorator that also exposes ports."""
+
+            @classmethod
+            def input_ports(cls) -> dict:
+                return {}
+
+            @classmethod
+            def output_ports(cls) -> dict:
+                return {"count": PortInformation(data_type=int, required=False)}
+
+        xml = """<root main_tree_to_execute="MainTree">
+          <BehaviorTree ID="MainTree">
+            <Sequence>
+              <RepeatWithPorts num_success="5">
+                <Producer name="prod" output="{out}" />
+              </RepeatWithPorts>
+            </Sequence>
+          </BehaviorTree>
+        </root>"""
+
+        with tempfile.NamedTemporaryFile(delete=False, mode="w", suffix=".xml") as tf:
+            tf.write(xml)
+            path = tf.name
+
+        try:
+            root = parse_behaviour_tree_xml(path, logger=StdoutLogger())
+            repeat = find_node_by_class(root, RepeatWithPorts)
+            self.assertIsNotNone(repeat)
+            self.assertEqual(repeat.num_success, 5)
+            self.assertIsInstance(repeat.num_success, int)
+            self.assertIsInstance(repeat.decorated, Producer)
+
+            # num_success=-1 repeats indefinitely, so the decorator stays RUNNING.
+            btree = py_trees.trees.BehaviourTree(root)
+            btree.tick()
+            self.assertEqual(repeat.status, py_trees.common.Status.RUNNING)
+        finally:
+            os.unlink(path)
+
+    def test_composite_ports_node(self) -> None:
+        """Verify that a decorator with ports is instantiated correctly."""
+
+        class SequenceWithPorts(PortsMixin, py_trees.composites.Sequence):
+            """A `py_trees.composites.Sequence` composite that also exposes ports."""
+
+            @classmethod
+            def input_ports(cls) -> dict:
+                return {}
+
+            @classmethod
+            def output_ports(cls) -> dict:
+                return {}
+
+        xml = """<root main_tree_to_execute="MainTree">
+          <BehaviorTree ID="MainTree">
+            <SequenceWithPorts memory="true">
+                <Producer name="prod1" output="{out1}" />
+                <Producer name="prod2" output="{out2}" />
+            </SequenceWithPorts>
+          </BehaviorTree>
+        </root>"""
+
+        with tempfile.NamedTemporaryFile(delete=False, mode="w", suffix=".xml") as tf:
+            tf.write(xml)
+            path = tf.name
+
+        try:
+            root = parse_behaviour_tree_xml(path, logger=StdoutLogger())
+            sequence = find_node_by_class(root, SequenceWithPorts)
+            self.assertIsNotNone(sequence)
+            self.assertIsInstance(sequence.children, list)
+            self.assertEqual(len(sequence.children), 2)
+            self.assertIsInstance(sequence.children[0], Producer)
+            self.assertIsInstance(sequence.children[1], Producer)
+
+            btree = py_trees.trees.BehaviourTree(root)
+            btree.tick()
+            self.assertEqual(sequence.status, py_trees.common.Status.SUCCESS)
+        finally:
+            os.unlink(path)
+
     def test_direct_portsmixin_leaf_accepted(self) -> None:
         """
         A leaf class that is ``PortsMixin + Behaviour`` (not via ``BehaviourWithPorts``)