Added ignore_requirements and warn_only_once

[fjulian]

This PR introduces two new features and a fix:

Feature 1: When parsing a domain file, there may be requirements that the parser does not know about, but which do not impact/prevent normal domain parsing. Until now, an exception was raised for unknown requirements, and parsing was aborted. Here, I propose an additional optional argument to the parsing function, allowing to specify requirements that may be in the domain file and that can safely be ignored by the parser. The default behavior does not change, so any existing code using the parser will still work.

Feature 2: The parser prints a warning when encountering elements in the domain description which it does not know about, once every time it encounters such an element. However, in most cases, warning the user once is enough. I propose here a mechanism that optionally warns the user only once. The default behavior does not change, so any existing code using the parser will still work.

Fix: The function split_predicate could until now not handle empty fields, such as :precondition (). Now it can.

[Maumagnaguagno]

I already did the custom requirement part here, but did not commit as I still had to make tests. My idea is to add a requirements parameter to parse_domain with a default argument, so it can be modified by the caller to include more/less requirements as desired, instead of adding a secondary list. EDIT: see commit e7806cb

The second part is subjective, I prefer repeated warnings to make it clear the mistake happened multiple times. Otherwise you fix one mistake and the warnings look the same.

I will look into the empty fields problem, it may affect other elements, and add tests for each. EDIT: see commit b0c745b

Just hold on for a while.

[Maumagnaguagno]

Done! I think the requirements parameter makes it more flexible. Now we can even disallow requirements supported by the parser, but not supported by a planner or analysis tool that uses the parsed data.

I forgot to port the empty fields fix from HyperTensioN's PDDL parser to this one. Thanks for reporting it. I am using a conditional around the block without the early return to match the original fix from Maumagnaguagno/HyperTensioN@fead630

I am going to let this PR open for a few days if you want to discuss the warnings issue you are facing, otherwise you can close it.

[fjulian]

Thanks for the quick response.

Regarding the warnings issue: I agree with you, it's subjective. With the implementation I propose, the user can choose which of the two behaviors they prefer. The default is still to print the warning every time, which is what you prefer. Therefore, I don't see a disadvantage in adding this and making the parser's behavior more customizable. In my case, I'm working with domain fails with dozens of tags that are not supported by the parser, leading the command line to be spammed with these warnings. Therefore, I prefer to be warned only once.

[fjulian]

Let me know if you want to merge this. I would clean it up then and make it compatible with the changes you did to master in the mean time.

[Maumagnaguagno]

The disadvantage is one more parameter to document and expect people to understand and correctly implement when inheriting from PDDL Parser to support their own extensions. Imagine what would happen if somebody thought that warn only once must be true because they previously worked with multi-pass compilers that could warn about the same thing repeatedly. Or an implementation mistake, for example, you only modified parse_domain_extended, leaving parse_problem_extended and parse_action_extended with the old behavior. Considering that this modification focuses only on extended methods, isn't this an extension/inheritance problem?

From my perspective your warning issue should be solved by inheritance, where you can correctly filter/process the extended elements using the parse_*_extended methods while warning about typos and other elements that remain unsupported. The real question is this: Is there an issue with the current design that makes it impossible/hard to solve your issue through inheritance?

[fjulian]

Alright, your call.

[Maumagnaguagno]

I was really interested in knowing if the current design works nicely with inheritance and your issue appears to be a good test candidate.

[fjulian]

I think it does work nicely with inheritance, and you're right, this issue can be fixed like this, since only the function parse_domain_extended needs to be overloaded.

I guess it's just up to a library's maintainers to decide whether they want to offer a feature to all users, or if they require every user to implement it themself.

If you're curious, here is how I overloaded your parser:

from pddl_parser.PDDL import PDDL_Parser

class CustomPDDLParser(PDDL_Parser):
    def __init__(self, warn_only_once=True):
        self.warn_only_once = warn_only_once
        self.already_warned_about = set()

        self.ignored_requirements = [":hierarchy", ":method-preconditions"]

    def parse_domain(self, domain_filename, requirements=None):
        requirements = PDDL_Parser.SUPPORTED_REQUIREMENTS + self.ignored_requirements
        super().parse_domain(domain_filename, requirements)

    def parse_domain_extended(self, t, group):
        if self.warn_only_once and t in self.already_warned_about:
            return
        super().parse_domain_extended(t, group)
        self.already_warned_about.add(t)

[Maumagnaguagno]

I see. I was actually trying to help you skip the whole multiple warnings issue. Something like the following:

from pddl_parser.PDDL import PDDL_Parser

class CustomPDDLParser(PDDL_Parser):

    CUSTOM_REQUIREMENTS = PDDL_Parser.SUPPORTED_REQUIREMENTS + [":hierarchy", ":method-preconditions"]

    def parse_domain(self, domain_filename):
        super().parse_domain(domain_filename, self.CUSTOM_REQUIREMENTS)

    def parse_domain_extended(self, t, group):
        if t not in [':custom1', ':custom2']:
            super().parse_domain_extended(t, group)

[fjulian]

Thank you. My intent was only to share my solution and save future users some effort.