Issue1030 #155

PatrickFerber · 2023-03-27T20:11:46Z

No description provided.

…oncepts

salome-eriksson · 2023-09-14T10:38:39Z

src/translate/pddl_parser/parsing_functions.py

+
+def check_named_block(alist, names):
+    return isinstance(alist, list) and alist and alist[0] in names
+


I'd rather not have two functions that do almost the same (check_named_block vs assert_named_block). I suggest only having assert_named_block. The check version is used in a few places where we also check for some other things (e.g. is the second element of alist a string), but we could just call first assert_named_block and then check the rest in a separate if block. This also allows us to give a more detailed error message.

Sounds good to me.

salome-eriksson · 2023-09-14T14:56:30Z

src/translate/pddl_parser/parsing_functions.py

+                                      "or '(either WORD*)")
+                for item in items:
+                    if only_variables and not item.startswith("?"):
+                        context.error("Expected item to be a variable", item)


Is it clear to the user what "item" is here? And it may also be helpful to mention that variables are word/strings that start with ?
The output the user gets is:

Expected item to be a variable
Got: x

I'd rather suggest something like

Expected a variable, but got 'x'. Did you forget the leading question mark?

Sounds good.

salome-eriksson · 2023-09-14T15:03:19Z

src/translate/pddl_parser/parsing_functions.py

+            error_msg += f"\nGot: {item}"
+        raise ParseError(error_msg)
+
+    def expected_word_error(self, name, *args, **kwargs):


Calling this function is always preceded by checking if name is of type string. I would instead suggest a function like assert_string or similar (like assert_named_block on line 86) that does the type check and creates the error message.

I agree that it would be nicer to have a function do the check and yielding the error (if necessary). This applies to all three cases expected_word_error, expected_list_error, and expected_name_block_error. I also checked the calls to these functions and they all have similar if-clauses preceding them, so it shouldn't be a problem to unify this in a function.

If I understood Malte's comment in the daily sprint meeting correctly, they should preferrably be named check... rather than assert... because assert should be reserved for code errors rather than user input errors.

Another thing I noticed is that if the PDDL has numbers where it expects a string the number will just be interpreted as a string. I don't know if this is the behaviour we want, but if not then we could maybe add a check for numbers in the new check_string function (or however we end up calling it).

salome-eriksson · 2023-09-14T15:08:12Z

src/translate/pddl_parser/parsing_functions.py

+    if not check_named_block(alist, names):
+        context.expected_named_block_error(alist, names)
+
+def construct_typed_object(context, name, _type):


It does not seem intuitive to only run checks on name but not on _type. The function is only used on line 136 in a loop where name changes but _type doesn't, so I understand that checking _type in this function would be wasteful. Maybe just do both checks outside the function? (Although I'm not happy with this idea either...)

I agree with your point that _type should also be tested. After thinking about it a little bit, I don't see an issue with just doing the checks outside the function. Sure, one could argue that it makes sense to test this whenever trying to construct an object. However, within the constructing function, it would make more sense to assert this in my opinion.

salome-eriksson · 2023-09-14T15:10:56Z

src/translate/pddl_parser/parsing_functions.py

+                           (_type and _type[0] == "either" and
+                            all(isinstance(_sub_type, str) for _sub_type in _type[1:]))):
+                        context.error("Type value is expected to be a single word "
+                                      "or '(either WORD*)")


This message was at first unclear to me. Maybe expand it to the following?

Type value is expected to be a single word, or a choice of words denoted by '(either word1 word2 ...)'

Now from my digging in the issue tracker yesterday it sounded like we don't support either. Here I see the opposite and from the code it seems it's supported. We might need to clean that up in the issue tracker then.

That being said, I prefer your formulation.

salome-eriksson · 2023-09-14T15:17:44Z

src/translate/pddl_parser/parsing_functions.py

+    def expected_word_error(self, name, *args, **kwargs):
+        self.error(f"{name} is expected to be a word.", *args, **kwargs)
+
+    def expected_list_error(self, name, *args, **kwargs):


I imagine the same comment as for expected_word_error applies (I did not check all function calls here though).

salome-eriksson · 2023-09-14T15:19:15Z

src/translate/pddl_parser/parsing_functions.py

+        effects = []
+        for eff in alist[1:]:
+            if not isinstance(eff, list):
+                context.error("All sub-effects of a conjunction have to be blocks.",


Why not use expected_list_error?

Yeah, seems a bit inconsistent. Though, this gives clearer instructions where to look for the faulty input. would the context-layer provide enough info about this as well? Then I don't see an issue with turning this into expected_list_error.

salome-eriksson · 2023-09-14T15:19:33Z

src/translate/pddl_parser/parsing_functions.py

+            context.error("'when' effect expects exactly two arguments.",
+                          syntax=SYNTAX_EFFECT_WHEN)
+        if not isinstance(alist[1], list):
+            context.error(


Why not use expected_list_error?

salome-eriksson · 2023-09-14T15:21:40Z

src/translate/pddl_parser/parsing_functions.py

+        elif exp.replace(".", "").isdigit() and exp.count(".") <= 1:
+            return pddl.NumericConstant(float(exp))
+        elif exp[0] == "-":
+            context.error("Expression cannot be a negative number",


I liked the old phrasing "Negative numbers are not supported" better, mainly because it does not use the word "expression" and thus is still understandable if you're not too familiar with PDDL terminology.

Sounds reasonable, I support changing it back.

For what it's worth, I think we don't support fractional numbers either. I think it's a mistake that we accept ".".

salome-eriksson · 2023-09-14T15:24:38Z

src/translate/pddl_parser/parsing_functions.py

+        yield use_metric
+
+        for _ in iterator:
+            assert False, "This line should be unreachable"


This can happen when we have malformed PDDL, i.e. when someone defines more things after metric, correct? Then we should give an error message.

As an aside, if someone does not specify a metric but other things, do they get swallowed and the PDDL accepted? I will test this.

I can neither confirm that this can happen nor do I understand the context to meaningfully comment on this.

I ran this and found three issues:

( ... (:goal ... ) (foo) (:metric minimize (total-cost)) (bar) )

Anything after (:goal ...) other than (:metric ...) is silently ignored. The assert actually never triggers because the loop above consumes all elements. But there shouldn't be a loop to ignore (foo) to begin with, right?

The other issue is this with this code:

( ... (:goal ... ) (:metric minimize (total-cost)) () )

where the check entry[0] == ":metric" fails.

maltehelmert · 2023-09-21T14:03:31Z

src/translate/translate.py

@@ -753,3 +754,6 @@ def handle_sigxcpu(signum, stackframe):
        traceback.print_exc(file=sys.stdout)
        print("=" * 79)
        sys.exit(TRANSLATE_OUT_OF_MEMORY)
+    except pddl_parser.ParseError as e:
+        print(e)
+        sys.exit(TRANSLATE_INPUT_ERROR)


This changes the meaning of TRANSLATE_INPUT_ERROR compared to what we say on the wiki ("Usage error: wrong command line options"). If nobody things we need two different error codes for command-line errors vs. input file errors, the code can stay is is, but then we should update the wiki documentation to cover both cases.

maltehelmert · 2023-09-21T14:05:35Z

src/translate/pddl_parser/parsing_functions.py

+
+def assert_named_block(context, alist, names):
+    if not check_named_block(alist, names):
+        context.expected_named_block_error(alist, names)


I would like to keep the word "assert" for actual assertions (which test program errors, not user errors). can we rename this? From line 88, a possible verb could be "expect". Others could be "verify", "validate", "check".

victorpaleologue · 2023-10-23T11:00:23Z

src/translate/pddl/tasks.py

@@ -68,15 +68,22 @@ def dump(self):
            for axiom in self.axioms:
                axiom.dump()

+
+REQUIREMENT_LABELS = [


Since Python 3.8, there's a notion of Final: https://peps.python.org/pep-0591/

victorpaleologue · 2023-10-23T11:01:07Z

src/translate/pddl_parser/lisp_parser.py


 # Basic functions for parsing PDDL (Lisp) files.
 def parse_nested_list(input_file):
    tokens = tokenize(input_file)
    next_token = next(tokens)
    if next_token != "(":
-        raise ParseError("Expected '(', got %s." % next_token)
+        raise ParseError("Expected '(', got '%s'." % next_token)


switch to f-string?

victorpaleologue · 2023-10-23T11:02:25Z

src/translate/pddl_parser/pddl_file.py

-    except lisp_parser.ParseError as e:
-        raise SystemExit("Error: Could not parse %s file: %s\nReason: %s." %
+    except parse_error.ParseError as e:
+        raise parse_error.ParseError("Error: Could not parse %s file: %s\nReason: %s" %


or, in a more idiomatic way:

except parse_error.ParseError as e: raise parse_error.ParseError(f"Error: Could not parse {type} file: {filename}") from e

salome-eriksson

These comments are from notes I made when testing different ways to specify wrong PDDLs. Unfortunately it was a while ago and I don't always remember the details.
I think basically all of my comments are not handled better by the current code, meaning we can also merge issue1030 as is and tackle these comments at a later time.

salome-eriksson · 2023-10-24T18:38:48Z

src/translate/pddl_parser/parsing_functions.py

+        if syntax:
+            error_msg += f"\nSyntax: {syntax}"
+        if item:
+            error_msg += f"\nGot: {item}"


If we explicitly pass something like an empty list as item, the if statement will not trigger. For example, if we expect a named block and got an empty one, it will just say that it expected a non-empty block with starting with a word from some given set (see function expected_named_block_error on line 71).

salome-eriksson · 2023-10-24T18:53:36Z

src/translate/pddl_parser/parsing_functions.py

+            return pddl.Requirements(alist)
+        except ValueError as e:
+            context.error(f"Error in requirements.\n"
+                          f"Reason: {e}")


This allows for listing the same requirement multiple times, e.g. :adl :adl

salome-eriksson · 2023-10-24T18:56:59Z

src/translate/pddl_parser/parsing_functions.py

+            error_msg += f"\nGot: {item}"
+        raise ParseError(error_msg)
+
+    def expected_word_error(self, name, *args, **kwargs):


Another thing I noticed is that if the PDDL has numbers where it expects a string the number will just be interpreted as a string. I don't know if this is the behaviour we want, but if not then we could maybe add a check for numbers in the new check_string function (or however we end up calling it).

salome-eriksson · 2023-10-24T19:14:08Z

src/translate/pddl_parser/parsing_functions.py

+        if not isinstance(name, str):
+            context.expected_word_error("Predicate name", name)
+    with context.layer(f"Parsing arguments of predicate '{name}'"):
+        arguments = parse_typed_list(context, alist[1:], only_variables=True)
    return pddl.Predicate(name, arguments)


Something like (available) (?x - object) (instead of (available ?x - object)) will get accepted. The first part because the first while-loop of parsed_type_list (line112) does not trigger since alist[1:] is empty, and the second because the items defined on line 125 are empty and thus the loop on line 133 does not trigger.

salome-eriksson · 2023-10-24T19:17:31Z

src/translate/pddl_parser/parsing_functions.py

+def parse_predicates(context, alist):
+    with context.layer("Parsing predicates"):
+        the_predicates = []
+        for no, entry in enumerate(alist):


Maybe start counting at 1? "Parsing 0. predicate" might be confusing.

salome-eriksson · 2023-10-24T20:07:43Z

src/translate/pddl_parser/parsing_functions.py

+                              f"Reason: two '{field}' specifications.")
+            if (seen_fields and
+                correct_order.index(seen_fields[-1]) > correct_order.index(field)):
+                msg = f"\nWarning: {field} specification not allowed here (cf. PDDL BNF)"


When this error triggers the output looks somewhat weird:
b'\nWarning: :requirements specification not allowed here (cf. PDDL BNF)\n'
(This happens in other places as well, mostly when errors from the PDDL classes trigger.)

salome-eriksson · 2023-10-24T20:11:50Z

src/translate/pddl_parser/parsing_functions.py

@@ -42,49 +153,111 @@ def set_supertypes(type_list):
        type_name_to_type[desc_name].supertype_names.append(anc_name)


Missing types can cause a KeyError crash. Unfortunately I don't remember anymore how I triggered it and I don't understand the function enough to pinpoint why it might happen (maybe it happens in graph.transitive_closure?)

salome-eriksson · 2023-10-24T20:13:26Z

src/translate/pddl_parser/parsing_functions.py

+                    len(goal) != 2 or not isinstance(goal[1], list) or
+                    not goal[1]):
+                context.error("Expected non-empty goal.", syntax=SYNTAX_GOAL)
+            yield parse_condition(context, goal[1], type_dict, predicate_dict)


Too many parenthesis can cause a crash, e.g. (:goal ((blocked philosopher-1)))
(Not sure if this can happen with conditions in general or just the goal.)

salome-eriksson · 2023-10-24T20:15:52Z

src/translate/pddl_parser/parsing_functions.py

+                assert entry[0] == ":action"
+                with context.layer(f"Parsing {len(the_actions) + 1}. action"):
+                    action = parse_action(context, entry, type_dict, predicate_dict)
+                    if action is not None:


Why is a none action allowed and gets added?

salome-eriksson · 2023-10-24T20:18:13Z

src/translate/pddl_parser/parsing_functions.py

+                        the_actions.append(action)
+    return the_axioms, the_actions
+
+def parse_init(context, alist):


It seems we do not check if predicates specified in init exist (in the goal we do).

maltehelmert · 2024-01-18T14:07:55Z

[issue1030] translator: Improve PDDL error reporting.

We have substantially revised the PDDL syntax checks in the translator. Many PDDL errors are now diagnosed much better and lead to a meaningful error message and exit code where previously they would trigger failed assertions or be silently accepted with unclear semantics.

PatrickFerber added 6 commits March 14, 2023 09:50

introduce usage of ParseError at more places

7e0c002

Introduce context, context layers, and syntax strings for different c…

cbeb3d8

…oncepts

add many new checks to parsing_functions.py

6f09b1e

fix incorrect indent and undid changed behaviour in metric parsing

c0d7aeb

fix style

ad978e1

remove unecessary check

cc72144

salome-eriksson reviewed Sep 14, 2023

View reviewed changes

maltehelmert reviewed Sep 21, 2023

View reviewed changes

This was referenced Oct 23, 2023

PDDL Parser: more explicit error when facing empty expressions #60

Closed

Translate: improve error message when files do not start with "(define " #59

Closed

victorpaleologue reviewed Oct 23, 2023

View reviewed changes

salome-eriksson reviewed Oct 24, 2023

View reviewed changes

Use f-strings.

d1ad865

maltehelmert merged commit 768091d into aibasel:main Jan 18, 2024
6 of 11 checks passed


		def check_named_block(alist, names):
		return isinstance(alist, list) and alist and alist[0] in names

		@@ -42,49 +153,111 @@ def set_supertypes(type_list):
		type_name_to_type[desc_name].supertype_names.append(anc_name)

Issue1030 #155

Issue1030 #155

Conversation

PatrickFerber commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salome-eriksson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maltehelmert commented Jan 18, 2024