Patrick's pull request:

https://github.com/aibasel/downward/pull/155

Following the discussion in the Fast Downward meeting today, I have merged the pull request as-is.

We had many unresolved pull request comments to improve the code further, but with Patrick having left the group, nobody was working on addressing them, and we didn't want to leave the issue in limbo indefinitely. We have a related open issue, "Make translator stricter and improve error output" (issue220), and I've left a comment there regarding these open suggestions for further improvement, so that we at least have a pointer.

The new code slows down the translator somewhat, but in our tests we saw nothing that looked concerning enough to prevent us from merging.

Thanks to everyone involved!

I added my comments to Patrick's pull request now. They originate from me trying out different ways to specify incorrect PDDL and getting crashes or error messages that were not helpful. While there are quite some, it is important to remember that the behavior of the current code base is the same or worse, so there would be no harm to merge the pull request as is and tackle these comments at a later date.

I still have some comments from a second code review, but I did not find time to put them into the pull request yet. I will do so until Tuesday (tomorrow) evening.

I understand.
Thank you so much for the efforts your putting into Fast Downward!
Just don't feel bad if in the end you don't tackle that tiny detail.

There was a lot of offline activity and discussion about this. I'm adding Salomé, Clemens and Simon to the issue, who were all involved in this.

The short story is that Patrick (with some collaborators, I think?) completely rewrote the PDDL error checking. There are now many more diagnostic messages and we no longer use assertions to check against user errors.

Since Patrick is no longer reading this, I'll take over the topic from him. I've updated the title and summary. The pull request is linked from the summary.

Salomé, Clemens, Florian and I all participated in the code review.

Clemens did a thorough test of this and identified many examples of malformed PDDL that are now detected and reported more nicely, and also examples that are still not dealt with nicely. We decided to merge this without dealing with the remaining cases in the interest of avoiding further delay.

@Clemens: did you mention your findings in an issue? If not, can you do this?

We also checked the performance impact of this. Simon (and I think others) ran some experiments. The parsing stage of the translator gets slowed down quite a bit, but we decided to accept this, especially since parsing is not generally a bottleneck (with the exception of some performance issues for large type hierarchies, which are not affected by this change).

As a next step, I'll look at the code review comments and will see what (if anything) needs to be done before we can merge this.

Honestly, if the team is not up with dealing with issues, you can drop it.

At the time of the writing, () was leading to an error - so it was not legal de facto.

If the decision can be changed, I would prefer letting it be legal, and evaluate to true.

Sorry if I missed something, but did we resolve the questions whether () should be legal or not? Like I said, I would not consider Daniel Kovacs's document as definitive, but I think we didn't check what the more official PDDL resources say (except for the original PDDL spec, but that's not the only one to consider).

I can copy a message by Patrick from Discord here. He continued to work on this, and now it is waiting for a review again. I'm not sure where the code lives, though.

-----------

Update the parser due to our last discussion (do not construct f-strings for errors only when needed and replace function calls which potententially perform multiple checks depending on their arguments by inlining the relevant checks).

- The translator output is still the same for all instances except for citycar and trucks (as expected).
- The memory footprint changes insignificantly (up to a few MB)
- The parsing time (translator_time_parsing) slowdown also decreased from 61% in the previous version to 51% (357s vs 237s over all instances). In most instances, the parsing time is insignificant (<1s). Thus, the slowdown could be ignored. I checked the three domains which make up most parsing time: airport (21s), openstacks-strips(113s), trucks-strips(114s)
The slowdown appears uniformly across all instances, i.e. no exponential scaling at harder instances. Thus, I’d say instances which we could parse in our old version can still be parsed in the new version. 

I conclude, if the runtime cost was good enough in the previous version, then it should still be good enough in the new version. Obviously, it is annoying if someone has a pregrounded task which takes now a significant amount of time longer. In these cases, we can suggest to use the old parsing_functions.py file or wait for the C++ translator.

Hi Victor, yes, the same as for issue1030 applies.
I'd love to show you something concrete so that you can see what is coming. :-)

Florian, do you know more? I know Patrick has been working on this, but like me his is currently on holiday.

Is the subject still being dealt with?

Happy New Year! Previous bump was probably lost among celebrations =D

Bump, if I may, now that the 22.12 release is out of the way.

The PR was split into https://github.com/aibasel/downward/pull/59 and https://github.com/aibasel/downward/pull/60

Ok, thanks a lot for that list, let us know what we do with my PR, please.

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.

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.

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.

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.

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"?

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.

Fixed issue description.

Initial, invalid PR: https://github.com/aibasel/downward/pull/43