Fixes and improvements in the parsing of typed lists #82

marcofavorito · 2023-06-07T08:26:11Z

Proposed changes

This PR brings several fixes and improvements in the parsing of typed lists (names/variables).

Fixes

n/a

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply.

I have read the CONTRIBUTING doc
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works

Further comments

n/a

The goal was just '(and )', restore the original reachability goal.

This commit adds the parsing utility class "TypesIndex". TypesIndex is an index for PDDL types and PDDL names/variables. It is used to index PDDL names and variables by their types. OrderedDict is used to preserve the order of the types and the names, e.g. for predicate variables. Other types of validations are performed to ensure that the types index is consistent. In this commit, it is used only to parse typed lists of names (not variables).

…ompatible) This commit changes the way typed_list_name rule is parsed. This does not change the parsing behaviour from the user perspective, but it is a non-functional change to improve the performances of the Lark parser. With the previous version of the rule 'typed_list_name: NAME+ TYPE_SEP primitive_type (typed_list_name)', the Lark parser internals would perform a recursive call whenever a typed_list_name is matched. This might cause an arbitrarily long stack of calls whenever the parser got an input like: ``` element1 - t1 element2 - t1 element3 t1 ... ``` The above list of tokens is parsed as: ``` element1 - t1 <typed_list_name> element2 - t1 <typed_list_name> element3 - t1 <typed_list_name> ... ``` For large inputs, this will easily hit the recursion depth limit. With the new approach, the entire list of tokens is parsed at once. This adds some complexity in the parsing function for the typed_list_name rule, but we have more control in how the parsing is performed; in particular, we can parse the tokens *iteratively* rather than *recursively*. Finally, the NAME* pattern is appended at the end of the typed_list_name rule. This is because, according to the syntax specification, the last sublist of names might be non-typed. The implementation exploits the newly added TypesIndex class which handles corner cases and syntax errors (e.g. duplicated entries in the list).

The order is taken from pag. 168 of the PDDL textbook, where colon ':' is ignored in determining the order.

codecov-commenter · 2023-06-07T09:59:23Z

Codecov Report

Merging #82 (edcc295) into main (b25b3a4) will increase coverage by 0.62%.
The diff coverage is 94.66%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
+ Coverage   89.67%   90.30%   +0.62%     
==========================================
  Files          22       24       +2     
  Lines        1182     1423     +241     
  Branches      193      247      +54     
==========================================
+ Hits         1060     1285     +225     
- Misses         92      102      +10     
- Partials       30       36       +6

Flag	Coverage Δ
unittests	`90.30% <94.66%> (+0.62%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pddl/logic/base.py	`86.66% <75.00%> (-1.01%)`	⬇️
pddl/core.py	`92.12% <87.71%> (-0.14%)`	⬇️
pddl/requirements.py	`90.00% <90.00%> (ø)`
pddl/action.py	`93.02% <93.02%> (ø)`
pddl/formatter.py	`95.94% <93.75%> (-2.42%)`	⬇️
pddl/_validation.py	`92.30% <94.59%> (+2.56%)`	⬆️
pddl/parser/typed_list_parser.py	`94.87% <94.87%> (ø)`
pddl/custom_types.py	`100.00% <100.00%> (ø)`
pddl/helpers/base.py	`97.40% <100.00%> (+3.75%)`	⬆️
pddl/logic/predicates.py	`94.56% <100.00%> (+0.44%)`	⬆️
... and 4 more

... and 1 file with indirect coverage changes

The value of type dict passed to the Domain constructor in the README is set to None, denoting the fact that the type has no parent type.

marcofavorito · 2023-06-07T13:42:14Z

pddl/parser/domain.py


-    def typed_list_variable(self, args) -> Dict[str, Set[str]]:
+    def typed_list_variable(self, args) -> OrderedDictType[name, Set[name]]:


OrderedDict is essential, predicates variables and action parameters require an ordered (possibly typed) list of variables, and the ordering in the PDDL input file must be remembered.

The same cannot be said for forall/exists expressions, but having them ordered does not harm.

marcofavorito · 2023-06-07T13:42:54Z

pddl/parser/symbols.py

@@ -21,34 +21,36 @@
 class Symbols(Enum):


just sorted alphanumerically as in the PDDL textbook:

i.e. terms with the same name should have the same type tags.

francescofuggitti · 2023-06-09T19:53:25Z

.github/workflows/docs.yml

@@ -13,7 +13,7 @@ jobs:
    strategy:
      matrix:
        os: [ubuntu-latest]
-        python-version: [3.7]
+        python-version: ["3.8"]

    timeout-minutes: 30



Shall we change:

- uses: actions/checkout@master - uses: actions/setup-python@master

to main as well?

Yes sure, thank you for spotting it

francescofuggitti · 2023-06-09T19:53:38Z

.github/workflows/test.yml

@@ -13,7 +13,7 @@ jobs:
    strategy:
      matrix:
        os: [ubuntu-latest]
-        python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"]
+        python-version: ["3.8", "3.9", "3.10", "3.11"]

    timeout-minutes: 30



ssardina · 2023-06-13T06:42:15Z

hi @francescofuggitti @marcofavorito , I was about to submit a PR to fix the constant section the domain, as they currently do not come with their types.

However, I just found this massive PR that seems to deal with related things and will change many things, so don't want to cause conflicts and so I am planning to wait until this is merge. Is it almost done?

I think the problem is in pddl.formatter:

    body += _sort_and_print_collection("(:constants ", domain.constants, ")\n")

That can be improved I think to do the same as objects in problems... I think.

(BTW, great work on this parser, it's very clean!)

marcofavorito · 2023-06-13T07:23:06Z

hi @francescofuggitti @marcofavorito , I was about to submit a PR to fix the constant section the domain, as they currently do not come with their types.

However, I just found this massive PR that seems to deal with related things and will change many things, so don't want to cause conflicts and so I am planning to wait until this is merge. Is it almost done?

I think the problem is in pddl.formatter:
    body += _sort_and_print_collection("(:constants ", domain.constants, ")\n")
That can be improved I think to do the same as objects in problems... I think.

(BTW, great work on this parser, it's very clean!)

Hi @ssardina , thank you for your appreciation... being useful to the community is the lymph for our work and motivation!

Given your message, we will finish the PR very very soon (~hours). We will keep you posted here :)

- types are now printed correctly in the :constants section - in the :objects section, the objects are grouped by type (as in constants) - minor fixes in how :init and :goal are printed (they are always printed even if empty). The code changes added here are to be considered temporary, since a refactoring of the formatting module is required. Nevertheless, the printing of the typing information of constants will remain.

marcofavorito · 2023-06-13T08:49:12Z

tests/test_formatter.py

+        """\
+    (define (domain my_domain)
+        (:requirements :typing)
+        (:types type_2 type_3 - type_1 type_1)


Note, earlier we did not consider types with no parents, and we printed type_1 in the first position. However, this is not correct wrt PDDL syntax; typed names with no type should be at the end of the list, o/w there is no way the parsing rules can spot them:

marcofavorito · 2023-06-13T08:49:54Z

tests/test_formatter.py

+    (define (domain my_domain)
+        (:requirements :typing)
+        (:types type_2 type_3 - type_1 type_1)
+        (:constants a b c - type_1 d e f - type_2 g h i - type_3 j k l)


the same applies here: first group by type, print groups by type name in alphanumerical order, and then print constants sorted within the group; however, constants with no type must be at the end.

marcofavorito · 2023-06-13T08:50:36Z

tests/test_formatter.py

+    (define (problem problem-1)
+        (:domain my_domain)
+        (:requirements :typing)
+        (:objects a b c - type_1 d e f - type_2 g h i - type_3 j k l)


the same approach used for constants applies here.

marcofavorito · 2023-06-13T08:51:20Z

tests/test_formatter.py

+        (:init )
+        (:goal (and))


we should decide what to do with empty init and goal sections. The PDDL grammar specifies these sections are mandatory. We will tackle this in a next PR, taking into account formatter refactoring #72

marcofavorito · 2023-06-13T08:55:18Z

@ssardina this commit edcc295 should solve your request. The formatting issue was planned for another PR, but it is even better that we've added it here.

@francescofuggitti, @haz, any concerns? Would you like to give a final review? I have other changes planned (integrate consistency checks, started here), but I think this PR is already too big; so I will continue on another branch/PR.

ssardina · 2023-06-13T09:38:47Z

@ssardina this commit edcc295 should solve your request. The formatting issue was planned for another PR, but it is even better that we've added it here.

Thanks, yes it seems to work well, with types listed for constants!

Thanks!

ssardina · 2023-06-13T09:44:16Z

Now @marcofavorito , this is related to types and this PR, but maybe you want to handled it later on another thread. It doesn't like a type having two parent types, like in this Storage case:

Is this part of PDDL constraints or we are over-checking here:

This is the error given in the main branch:

However, in this PR I get an even more primitive error complaining that area is duplicated:

I believe a parent type can indeed be a child type in PDDL and is used in many domains, right? Like here area:

marcofavorito · 2023-06-13T10:25:05Z

Thank you for your reply @ssardina . In this issue #70, there has been a discussion regarding whether we should support them or not. The discussion converged to NOT supporting types with multiple parent types (I think it is summarized by this comment, that cites the PDDL textbook: #70 (comment))

I would be very glad to know your opinion on the topic though!

marcofavorito · 2023-06-13T10:34:41Z

Regarding the problem on area, thank you, it will be fixed very soon!

marcofavorito · 2023-06-13T10:39:43Z

However, in this PR I get an even more primitive error complaining that area is duplicated:

I believe a parent type can indeed be a child type in PDDL and is used in many domains, right? Like here area:

@ssardina I think the problem in this case is that area occurs twice as child type, this is not allowed:

The error message will be changed so to be more informative.

This should make the error more clear to the user: instead of just saying that the name is "already present" in the list, we clarify that the problem is that not only it occurs, but it is because it occurs (again) as inheriting name.

francescofuggitti

Looks awesome! Thank you :)

haz · 2023-06-13T14:42:52Z

marcofavorito added 2 commits June 6, 2023 22:17

fix: remove constants.py module and use pddl/parser/symbols.py

e3a249e

fix: add missing __invert__ method for True/FalseFormula classes

658dc62

marcofavorito changed the title ~~Fix typed list order~~ Fixes and improvements in the parsing of typed lists Jun 7, 2023

marcofavorito added 6 commits June 7, 2023 10:55

test: fix blocksworld_fond/p01.pddl goal

d7d0534

The goal was just '(and )', restore the original reachability goal.

chore: add 'check' function to generalize assert_ for any exception type

d5b509c

test: add problem parsing in test_formatter.py

cf25df5

test: fix expected error messages when duplicated names/types occur

ccb5097

marcofavorito force-pushed the fix-typed-list-order branch from a3b6101 to ccb5097 Compare June 7, 2023 09:22

marcofavorito requested review from francescofuggitti and haz June 7, 2023 09:32

marcofavorito added 3 commits June 7, 2023 11:50

fix: minor fix to object-supertypes-error

7a7836c

fix: use 'name' type to validate tokens of typed list

f1e3316

chore: sort Symbols in alphanumerical order

48ffa56

The order is taken from pag. 168 of the PDDL textbook, where colon ':' is ignored in determining the order.

fix: add keyword validation for typed list names

976f481

marcofavorito marked this pull request as draft June 7, 2023 10:28

marcofavorito added 2 commits June 7, 2023 14:45

fix: renaming variables to avoid shadowing from outer scope

889b3fc

feat: use TypesIndex to parse and validate typed_list_variables

73e4fac

marcofavorito force-pushed the fix-typed-list-order branch from 662eee9 to 73e4fac Compare June 7, 2023 12:47

marcofavorito added 4 commits June 7, 2023 14:53

test: split domain parser tests from problem parser tests

d2dded9

docs: fix types argument passed to Domain in README

67d3b1d

The value of type dict passed to the Domain constructor in the README is set to None, denoting the fact that the type has no parent type.

fix: problem optionally accepts requirements

124a60a

lint: fix vulture's whitelist

03abf7d

marcofavorito force-pushed the fix-typed-list-order branch from 37909e9 to 03abf7d Compare June 7, 2023 13:36

marcofavorito commented Jun 7, 2023

View reviewed changes

marcofavorito force-pushed the fix-typed-list-order branch 2 times, most recently from 1092fae to fada331 Compare June 7, 2023 14:40

marcofavorito force-pushed the fix-typed-list-order branch from 4b0ddd6 to ec8c7d9 Compare June 9, 2023 14:24

marcofavorito added 3 commits June 9, 2023 16:38

feat: add validation of no-duplicates in type-tags of a Term

2c67fde

feat: make Term non-instantiatable

b8e0a2b

feat: check terms consistency wrt type tag

3661f64

i.e. terms with the same name should have the same type tags.

marcofavorito force-pushed the fix-typed-list-order branch from ec8c7d9 to 3661f64 Compare June 9, 2023 14:39

marcofavorito added 2 commits June 9, 2023 16:40

feat: add term type checks in EqualTo class

924c88c

test: add more tests for problem parsing

f7996d1

francescofuggitti self-requested a review June 9, 2023 19:50

francescofuggitti reviewed Jun 9, 2023

View reviewed changes

ci: change GH action version from master to main

f5858b3

marcofavorito commented Jun 13, 2023

View reviewed changes

marcofavorito requested a review from francescofuggitti June 13, 2023 08:55

francescofuggitti approved these changes Jun 13, 2023

View reviewed changes

marcofavorito merged commit 79c0d24 into main Jun 13, 2023
9 checks passed

marcofavorito deleted the fix-typed-list-order branch June 13, 2023 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and improvements in the parsing of typed lists #82

Fixes and improvements in the parsing of typed lists #82

marcofavorito commented Jun 7, 2023

codecov-commenter commented Jun 7, 2023 •

edited

marcofavorito Jun 7, 2023 •

edited

marcofavorito Jun 7, 2023 •

edited

francescofuggitti Jun 9, 2023

marcofavorito Jun 9, 2023

francescofuggitti Jun 9, 2023

ssardina commented Jun 13, 2023

marcofavorito commented Jun 13, 2023

marcofavorito Jun 13, 2023

marcofavorito Jun 13, 2023 •

edited

marcofavorito Jun 13, 2023

marcofavorito Jun 13, 2023 •

edited

marcofavorito commented Jun 13, 2023

ssardina commented Jun 13, 2023

ssardina commented Jun 13, 2023 •

edited

marcofavorito commented Jun 13, 2023

marcofavorito commented Jun 13, 2023

marcofavorito commented Jun 13, 2023 •

edited

francescofuggitti left a comment

haz commented Jun 13, 2023


		def typed_list_variable(self, args) -> Dict[str, Set[str]]:
		def typed_list_variable(self, args) -> OrderedDictType[name, Set[name]]:

Fixes and improvements in the parsing of typed lists #82

Fixes and improvements in the parsing of typed lists #82

Conversation

marcofavorito commented Jun 7, 2023

Proposed changes

Fixes

Types of changes

Checklist

Further comments

codecov-commenter commented Jun 7, 2023 • edited

Codecov Report

marcofavorito Jun 7, 2023 • edited

Choose a reason for hiding this comment

marcofavorito Jun 7, 2023 • edited

Choose a reason for hiding this comment

francescofuggitti Jun 9, 2023

Choose a reason for hiding this comment

marcofavorito Jun 9, 2023

Choose a reason for hiding this comment

francescofuggitti Jun 9, 2023

Choose a reason for hiding this comment

ssardina commented Jun 13, 2023

marcofavorito commented Jun 13, 2023

marcofavorito Jun 13, 2023

Choose a reason for hiding this comment

marcofavorito Jun 13, 2023 • edited

Choose a reason for hiding this comment

marcofavorito Jun 13, 2023

Choose a reason for hiding this comment

marcofavorito Jun 13, 2023 • edited

Choose a reason for hiding this comment

marcofavorito commented Jun 13, 2023

ssardina commented Jun 13, 2023

ssardina commented Jun 13, 2023 • edited

marcofavorito commented Jun 13, 2023

marcofavorito commented Jun 13, 2023

marcofavorito commented Jun 13, 2023 • edited

francescofuggitti left a comment

Choose a reason for hiding this comment

haz commented Jun 13, 2023

codecov-commenter commented Jun 7, 2023 •

edited

marcofavorito Jun 7, 2023 •

edited

marcofavorito Jun 7, 2023 •

edited

marcofavorito Jun 13, 2023 •

edited

marcofavorito Jun 13, 2023 •

edited

ssardina commented Jun 13, 2023 •

edited

marcofavorito commented Jun 13, 2023 •

edited