[ports] Improve handling of decorator nodes in XML parser

[sea-bass]

This makes decorators actually work with the new auto-registration, since otherwise the code yells about these decorators not being part of the node registry. Small regression for the greater good!

It's also generally less hard-coded, though not perfect.

I tested this on a Repeat decorator, including passing in num_success="-1" in the XML.

[JenniferBuehler]

Great idea to fix this up! 👏 And we definitely need to address it. However just for some background. I don't think it was the new design with auto registration that broke the decorators - they were intended to be covered differently. PARENT_NODES_WITH_PORTS_TAGS is actually designed to contain decorators too - in previous uses, we had other nodes in the list, including decorators - but they were also PortsMixin derived (e.g. had number of retries as port). Built-in py_trees decorators (Repeat, Retry, …) aren't PortsMixin classes, so they aren't auto-registered, so get_class_from_registry("Repeat", …) would fail, and that was an issue that was there already.

I agree that we'll have to find a way to support also non-PortMixin derived classes more wholistic, at least for composites and decorators. Right now, the handling is awkward as you saw 🥴

  if tag == "sequence":
      node = py_trees.composites.Sequence(name=node_name, memory=memory, children=children)
  elif tag == "selector" or tag == "fallback":
      node = py_trees.composites.Selector(name=node_name, memory=memory, children=children)
  elif tag == "parallel":
      node = py_trees.composites.Parallel(name=node_name, policy=..., children=children)

Urgh, there has to be a better way, and this PR is a good start!

What I think we'll can improve is

• this PR removes the path of a parent node that is also a PortsMixin. That is useful for parent tags which have ports (e.g. a repeat node that has the number of repeats as input port). If we ever add one (and that's the plan!), this will fall through as "leaf" and just drop its children. We should cater in the fact that we'll later have decorators that are also PortsMixin derived. That was in the code before this PR, but not actively used, so it probably didn't appear meaningful.
• We should add tests for this 😄 For example for <Repeat num_success="-1">

I think this is a great step in the right direction, and we can go ahead with it. I left a few comments for improvements. Once they are addressed, let's go with it. I would then do the "more wholistic" approach in a separate follow-up.

Thanks again for this improvement, this is the right direction for changes that need to be added before this is more mature! 🙏

[JenniferBuehler]

I'm not sure I read this right, but this compares a class to a module, so it never excludes anything? The intent was to exclude the abstract base Decorator.
Like this, <Decorator> becomes a "valid" tag?

[sea-bass]

no good catch, this is supposed to be obj is not py_trees.decorators.Decorator which invalidates the tag you mentioned!

[JenniferBuehler]

How is this related, did this sneak in?
I guess this would mean a bad policy value now crashes it?

[sea-bass]

no, but I should have commented. A few lines above the code says:

policy = elem.attrib.get("policy", "success_on_one")

so basically policy was already defined and defaulted to a different fallback than the one I just removed. And I think success_on_one is the more reasonable default.

[JenniferBuehler]

What I meant was when an invalid value like "my_bad_policy" is posted, before it was then falling back to mapping.get(policy, SuccessOnAll). Now mapping[policy] would raise a KeyError.

I verified by adding below unit test to test_ports_xml_parser.py. It's fine to raise on an invalid value but perhaps the KeyError isn't ideal. Try catch maybe?

    def test_parallel_invalid_policy_falls_back(self) -> None:
        """An unknown <Parallel> policy value falls back to SuccessOnAll."""
        xml = """<root main_tree_to_execute="MainTree">
          <BehaviorTree ID="MainTree">
            <Parallel policy="my_bad_policy">
              <Producer name="prod" output="{out}" />
            </Parallel>
          </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())
            self.assertIsInstance(root.policy, py_trees.common.ParallelPolicy.SuccessOnAll)
        finally:
            os.unlink(path)

[sea-bass]

ah I see, I'll restore it but keep the same sensible default

[sea-bass]

Thanks @JenniferBuehler!

I added a test and addressed your other comment by rearranging the logic so that the node_registry always comes first and other special cases come later.

The reasoning is that as we mature this functionality, we can basically pre-populate the default node registry with all our built-in PortsMixin enabled composites/decorators/etc. instead of having to handle special cases.

(EDIT / getting in the weeds: We know based on isinstance / issubclass checks the rules for children: leaf nodes must have 0, decorators must have 1, composites can have >0 or >=0 depending on whether zero is allowed).

If you agree with this path, we can make a follow-up issue based on this comment.

[JenniferBuehler]

This is coming together nicely! I just have one minor open comment, and one that may have been misunderstood or overlooked in my last review. I'll make it more specific. Take this (add example to test_port_helpers.py for testing):

class RepeatWithPorts(PortsMixin, py_trees.decorators.Repeat):
    """A `py_trees.decorators.Repeat` decorator that also exposes ports.

    This is a parent node that is a decorator (it decorates a single child)
    that is *also* a ``PortsMixin`` and so is auto-registered and resolvable by tag.
    It publishes the number of successes it is configured to require via an output port.
    """

    OUTPUT_PORT = "count"

    @classmethod
    def input_ports(cls) -> dict:
        return {}

    @classmethod
    def output_ports(cls) -> dict:
        return {cls.OUTPUT_PORT: PortInformation(data_type=int, required=False)}

A port-enabled parent like this was supported before. Now, it would fail (add to test_ports_xml_parser.py):

    def test_ports_enabled_decorator_node(self) -> None:
        """A PortsMixin-derived decorator (parent node) should parse from XML.

        ``RepeatWithPorts`` is a ``Repeat`` decorator that is also a ``PortsMixin``.
        The parser should build its child and output the number of successes.

        This currently fails because the parser funnels any registered ``PortsMixin`` tag to the leaf path, which never builds the child.
        """
        xml = """<root main_tree_to_execute="MainTree">
          <BehaviorTree ID="MainTree">
            <RepeatWithPorts num_success="-1" count="{success_count}">
              <Producer name="prod" output="{out}" />
            </RepeatWithPorts>
          </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, -1)
            self.assertIsInstance(repeat.num_success, int)
            self.assertIsInstance(repeat.decorated, Producer)
        finally:
            os.unlink(path)

Perhaps it's easier to see what I mean by code snippets?

I think it was already not great before, it was already an item on the agenda as it is (these hardcoded parent tags, I mean).

Would it help if I push the suggestion for a fix to the branch, instead of trying to phrase my suggestion (it's easier to say in code)? Or you already know what I mean and want to re-arrange it so that we can support PortsMixin-parents?

Sorry to be a bum about this but PortsMixin-derived parent nodes are actually an important part in the usage of this library that I know exists today :)

[JenniferBuehler]

What I meant was when an invalid value like "my_bad_policy" is posted, before it was then falling back to mapping.get(policy, SuccessOnAll). Now mapping[policy] would raise a KeyError.

I verified by adding below unit test to test_ports_xml_parser.py. It's fine to raise on an invalid value but perhaps the KeyError isn't ideal. Try catch maybe?

    def test_parallel_invalid_policy_falls_back(self) -> None:
        """An unknown <Parallel> policy value falls back to SuccessOnAll."""
        xml = """<root main_tree_to_execute="MainTree">
          <BehaviorTree ID="MainTree">
            <Parallel policy="my_bad_policy">
              <Producer name="prod" output="{out}" />
            </Parallel>
          </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())
            self.assertIsInstance(root.policy, py_trees.common.ParallelPolicy.SuccessOnAll)
        finally:
            os.unlink(path)

[JenniferBuehler]

The reasoning is that as we mature this functionality, we can basically pre-populate the default node registry with all our built-in PortsMixin enabled composites/decorators/etc. instead of having to handle special cases.

(EDIT / getting in the weeds: We know based on isinstance / issubclass checks the rules for children: leaf nodes must have 0, decorators must have 1, composites can have >0 or >=0 depending on whether zero is allowed).

If you agree with this path, we can make a follow-up issue based on this comment.

Yes I agree, let's make this a lot less friciton-less for new nodes! The improvement can be a future PR. Only thing I meant in my review is that it shouldn't break existing PortsMixin-derived parents, even if the solution isn't yet pretty (the prettification can be for after this PR)

[sea-bass]

Yes! I see. If you'd like to add that test case and a possible fix here, that would be super helpful.

Perhaps my idea to check whether this registered node inherits from Decorator / Composite and therefore needs children is the way to go?

[sea-bass]

Actually I have inspiration... give me a minute...

[sea-bass]

Actually I have inspiration... give me a minute...

ok @JenniferBuehler I got something working with tests. Give it a look?

[JenniferBuehler]

Great, I ran the tests again and it all checks out now!!

Great work 👏 This is a real improvement.