Adopt one universal implementation for writing parameter lists to PDDL throughout codebase

[aefrank]

Tl;dr

PDDL has a lot of cases where a list of (sometimes typed) parameters are represented in the form (?var1 - type1 ?var2 - type2 ...). Throughout the codebase, several classes implement different routines to generate this same string format from a collection of Variable objects.

For consistency and ease of maintenance/modification, pddl should choose a single universal implementation for formatting typed parameter lists in PDDL syntax and modify all relevant classes to invoke this shared code instead of defining their own class-specific methods. The function _typed_parameters from pddl/helpers/base.py is a perfect candidate for this.

(issue requested by @haz here)

Context

In the discussion on the recently merged #71, I remarked that although the classes ForallCondition and Action produce identical parameter strings in their __str__ methods, they use different code to do so. ForallCondition.__str__ (inherited from QuantifiedCondition¹) constructs its parameter list via a list comprehension that uses a locally defined function build_tags(tags) to generate each variable's (possibly empty) type annotation individually. In contrast, the :parameters portion of Action.__str__² imports _typed_parameters from pddl/helpers/base.py to handle the entire Optional[Collection[Variable]] -> str process.

I'm of the opinion that the latter approach is preferable for three main reasons:

1. using a shared implementation means consistent PDDL across classes and updates,
2. wrapping the parameter list translation in a separate function limits the chance of unintended interactions with other steps of __str__, and
3. _typed_parameters seems to have been specifically designed to handle generating valid PDDL parameter lists, including handling the absence of types and empty parameter lists.

Accordingly, since #71, Forall.__str__'s parameter string is produced via _typed_parameters as well.

Suggested fix

For consistency and ease of maintenance/modification, pddl should designate pddl/helpers/base.py::_typed_parameters as the single universal implementation for formatting typed parameter lists in PDDL syntax and modify all relevant classes to invoke this shared code instead of defining their own class-specific methods. Specific action items include the following:

• QuantifiedCondition should be modified to use the same _typed_parameters-based implementation as Action and Forall.
• If other methods of generating PDDL for typed parameter lists exist elsewhere in the codebase, they should be replaced as necessary with _typed_parameters.
• Future updates that add new PDDL objects with parameter lists (e.g. exists effects) should opt to use _typed_parameters in their __str__ methods to maintain this consistency (and should also reference fix (1) from Forall fix #71 when writing their parsing rules, see "§Universal parameter list parsing function" below).

Additional comments

PDDL-agnostic Python objects

It's worth considering whether it should be the job of __str__ methods to output the canonical PDDL representation. One reason for hesitation is that there is not always one "correct" representation bereft of context. For example, Variable.__str__³ (from pddl/logic/terms.py) prints ?varname regardless of whether or not its type has been defined. This is desired behavior when the variable occurs as an argument of a ground predicate, e.g. preconditions: (at ?loc), but not when it is being declared, e.g. as an action parameter parameters: (?loc - location).

It could be argued that all consideration of "PDDL syntax" should be left to the parser/formatter, and that the Python objects should be as agnostic as possible to how they will be represented outside of Python. In this case, the point of this issue remains, but it should be the formatter that uses _typed_parameters instead of each class's __str__ method.

Universal parameter list parsing function

A similar thing is happening indomain.py where parameter lists are occur in several different contexts that all interpret the tokens as a collection of Variable objects, but there is not a unified implementation. This actually was the cause of bug (1) in #71 , where pddl/parser/domain.py::DomainTransformer::c_effect was not casting the parameters of forall effects to Variable objects as they are for non-effect forall conditions via gd_quantifiers and actions via action_parameters.

In domain.lark, we can see that these all invoke the typed_list_variable⁴ rule for their parameter lists. It seems as though it could be useful to have casting parameters to Variable objects handled in typed_list_variable instead of in each individual context for the same reasons as above.

---

Footnotes

Footnotes

1. pddl/logic/base.py::QuantifiedCondition::__str__ (inherited by ForallCondition)

   ...
   def __str__(self) -> str:
       """Get the string representation."""
   
       def build_tags(tags):
           if len(tags) == 0:
               return ""
           return f" - {' '.join(tags)}"
   
       var_block = " ".join([f"{v}{build_tags(v.type_tags)}" for v in self.variables])
       return f"({self.SYMBOL} ({var_block}) {self.condition})"
   ...
   

   ↩

2. pddl/core.py::Action::__str__

   ...
   def __str__(self):
           """Get the string."""
           operator_str = "(:action {0}\n".format(self.name)
           operator_str += f"    :parameters ({_typed_parameters(self.parameters)})\n"    # <<< parameter string
           if self.precondition is not None:
               operator_str += f"    :precondition {str(self.precondition)}\n"
           if self.effect is not None:
               operator_str += f"    :effect {str(self.effect)}\n"
           operator_str += ")"
           return operator_str
   ...
   

   ↩

3. pddl/logic/terms.py::Variable::__str__

   def __str__(self) -> str:
           """Get the string representation."""
           return f"?{self._name}"
   

   ↩

4. pddl/parser/domain.py::DomainTransformer::typed_list_variable

   def typed_list_variable(self, args):
           """
           Process the 'typed_list_variable' rule.
   
           Return a dictionary with as keys the terms and as value a set of types for each name.
   
           :param args: the argument of this grammar rule
           :return: a typed list (variable)
           """
           return self._typed_list_x(args)
   

   ↩