Issue1030

Title Better error message when facing empty conditions
Priority feature Status in-progress
Superseder Nosy List florian, jendrik, malte, victor.paleologue
Assigned To victor.paleologue Keywords
Optional summary
When an empty condition statement "()" is provided in a goal, the parsing fails but the error message is not clear about what happens.

Created on 2021-07-20.10:19:18 by victor.paleologue, last changed by victor.paleologue.

Summary
When an empty condition statement "()" is provided in a goal, the parsing fails but the error message is not clear about what happens.
Messages
msg10435 (view) Author: victor.paleologue Date: 2021-09-02.11:07:47
Ok, thanks a lot for that list, let us know what we do with my PR, please.
msg10434 (view) Author: malte Date: 2021-09-01.18:28:30
Unfortunately there isn't a single definitive source. Some things are specified in official TRs and papers, but they are not complete and most of them describe deltas to previous PDDL versions rather than being self-contained.

In 2008 I compiled this list: https://ipc08.icaps-conference.org/deterministic/PddlResources.html

The McDermott et al. paper I mentioned is the first link there, but you only have to go as far as the second paper (from IPC-2000) to find inconsistencies between the specs. :-( IPC-2000 requires "and", "or" etc. to have at least one argument, so I see no obvious way of specifying an empty goal according to this document. (It also doesn't allow "()".)

Perhaps the organizers of IPC-2000 thought that nobody need an empty goal because it's not useful in the context of the competition. Empty preconditions are possible by omitting the ":preconditions (...)" part altogether from the action description. Not sure if we implemented this correctly in Fast Downward, to be honest.

There are other things that are weird in this document, like specifying multiple ":goal" sections, which I don't think anyone supports and where I'm not sure what the semantics should be. And it seems that the BNF for the domain file is missing a closing parenthesis.

So unfortunately the different "official" documents contradict each other on some points occasionally. It's a bit of a mess. Someone should clean it up, but it would be a bit of a thankless task.
msg10427 (view) Author: victor.paleologue Date: 2021-09-01.10:12:30
Interesting! Could you point me to the official PDDL documents? I know some of them, but I do not know which are the official ones.

I updated my PR with Florian's recommendation. Improving the error message is already an improvement, and does not require FD to change anything in whether to support those empty conditions, in the end.
msg10422 (view) Author: malte Date: 2021-08-30.20:06:25
Daniel Kovacs's BNF is not an official document, and it is controversial in the planning community.

Using "()" for an empty (trivially true) condition is not permitted in the original PDDL 1.2 spec by McDermott et al. If it's part of a later official PDDL doc, we should consider supporting it.

Note that the empty conjunction "(and)" *is* an uncontroversial way of specifying a trivially true condition that can be used, so "()" is not strictly necessary.

If we don't correctly support "(and)" everywhere (e.g. not in the goal), then we should rectify this.

Regarding "()", we should only support is if we find a better source that it should be allowed. If we don't support it, we should of course give a decent error message.

@Victor, regarding your msg10374: yes, we use "resolved" for rejected issues.
msg10375 (view) Author: florian Date: 2021-07-20.16:16:10
I tried running the translator with an empty goal and it lead to this crash. From the specification, it looks like empty preconditions and effects are fine but the goal has to be non-empty. So empty conditions should be ignored (not crash) for the former and exit with an error specific to the goal for the latter.

For preconditions and effects, I saw code that creates an empty conjunction instead of calling the parse function in case the list is empty, so that seems to have the correct behavior.

That leaves the case for the goal. I suggest to check and report this error in parse_task_pddl at the place that currently has this assertion:
  assert goal[0] == ":goal" and len(goal) == 2
The missing bit there is to also check that goal[1] is non-empty, so quick fix would be to change the assertion to 
  assert goal[0] == ":goal" and len(goal) == 2 and goal[1]
or alternatively add a better error message in that place.
msg10374 (view) Author: victor.paleologue Date: 2021-07-20.12:58:01
The point was that it was not supported later on by fast downward, so I wanted it to fail earlier, with a better error message.

But looking at the language specification again (http://pddl4j.imag.fr/repository/wiki/BNF-PDDL-3.1.pdf), it appears that empty statements "()" are valid as precondition and as effect of action.

So if somewhere down fast downward something goes wrong with them, the solution is not to improve error messages, but to fix the issue. Since I cannot remember the initial issue, I think we can reject it. If there really is an issue, it will pop again.

What status stands for "rejected"? "solved"?
msg10372 (view) Author: florian Date: 2021-07-20.12:15:25
Victor recreated the pull request at https://github.com/aibasel/downward/pull/60

I'm fine with merging this but I wonder if the PDDL standard is clear on whether this is valid PDDL or not. I know that there are some grey areas in the standard but am not sure about this one. If this is valid PDDL (e.g., if empty conditions are defined as true), the parser should not fail here.
msg10370 (view) Author: victor.paleologue Date: 2021-07-20.11:20:03
Fixed issue description.
msg10368 (view) Author: victor.paleologue Date: 2021-07-20.10:19:18
Initial, invalid PR: https://github.com/aibasel/downward/pull/43
History
Date User Action Args
2021-09-02 11:07:47victor.paleologuesetmessages: + msg10435
summary: When an empty condition statement "()" is provided in either an action precondition or a goal, the parsing fails but the error message is not clear about what happens. -> When an empty condition statement "()" is provided in a goal, the parsing fails but the error message is not clear about what happens.
2021-09-01 18:28:30maltesetmessages: + msg10434
2021-09-01 10:12:31victor.paleologuesetmessages: + msg10427
2021-08-30 20:06:25maltesetnosy: + malte, jendrik
messages: + msg10422
2021-07-20 16:16:10floriansetmessages: + msg10375
2021-07-20 12:58:01victor.paleologuesetmessages: + msg10374
2021-07-20 12:15:26floriansetnosy: + florian
messages: + msg10372
2021-07-20 11:20:03victor.paleologuesettitle: Support empty condition in actions and goals -> Better error message when facing empty conditions
messages: + msg10370
summary: When an empty condition statement "()" is provided in either an action precondition or a goal, the parsing fails. -> When an empty condition statement "()" is provided in either an action precondition or a goal, the parsing fails but the error message is not clear about what happens.
2021-07-20 10:19:18victor.paleologuecreate