aig-upf / tarski

Tarski - An AI Planning Modeling Framework
Apache License 2.0
59 stars 19 forks source link

Generated PDDL is missing the object type #113

Closed haz closed 3 years ago

haz commented 3 years ago

Either actions/fluents shouldn't have - object types included, or the object type should be included on export. From an untyped domain, we read/write and wind up with something that fails.

http://editor.planning.domains/#read_session=A0K4xBtDuZ

Adding object to line 9 of the domain file makes it solve.

haz commented 3 years ago

@gfrances If you let me know what the fix should (roughly) be, and the branch to work off of, I can try to put something in.

CC: @beckydvn

miquelramirez commented 3 years ago

Hi @haz,

feel free to work on devel.

haz commented 3 years ago

Ack. And desired behaviour? No types on actions/predicates or object type declared?

miquelramirez commented 3 years ago

I will try to find tonight some time to check this, but IIRC tarski's FirstOrderLanguage defines a sort Object by default on construction.

The desired behaviour of the PDDL parser should be that if you come across a constant symbol that you cannot resolve into one of the domain-specific types, then you register it as a constant of the Object type. Also, it needs to be ensured that if the type object is defined in the domain, this is taken as a reference to the tarski Object sort.

I will confirm the first bit regarding initialization of languages, but that should be it.

When it comes to generation (or mapping onto an off-the-shelf-solver) you need to figure out how to map the types onto the target type hierarchy (or how to define it automatically). This is straightforward when the target is say, SMT-LIB, and trivial for optimization suites like Google or-tools (where the domains of Object sub-sorts are mapped into sets of natural numbers).

haz commented 3 years ago

So then I think the missing piece is having object listed as a type in the generated PDDL.

We aren't doing anything fancy -- loading a solvable dom/prob pair without types, writing it back to file, and observing that it can no longer be solved. It's because all the parameters now get the - object addition, without the :types section including object.

miquelramirez commented 3 years ago

Okay, so it is not the parsing/reading issue, but a specific writer issue (slaps forehead). We do add the object type on construction (see src/tarski/fol.py line 39).

Looking through the class FstripsWriter in src/tarski/io/fstrips.py I see that the method get_type() (line 243-254) deliberately skips the built-in types from the (:types ...) section. I think it isn't correct to mention types at all in the generated domain. It should be the case that in :requirements we would have to specify the keyword :typing (see remarks here.

The keyword :typing is not being specified due to the function is_typed_problem() in src/tarski/fstrips/representation.py. As you will see, it only generates the :typing requirement if there is a sort in the language which is not a built-in. But the code generating the list of variables in the schema is not aware of this (see src/tarski/io/fstrips.py, function print_variable_list()):

def print_variable_list(parameters):
    return " ".join(f"{print_variable_name(p.symbol)} - {p.sort.name}" for p in parameters)

this should be changed, probably adding an optional argument (e.g. with_type_information=True) and then having a conditional ignoring sort names when generating the string, or having them (as is the default right now).

For predicates there is a similar issue, this time in build_signature_string():

def build_signature_string(domain):
    if not domain:
        return ""

    return " ".join(f"{print_variable_name(f'x{i}')} - {tarski_to_pddl_type(t)}" for i, t in enumerate(domain, 1))

where the type information is being specified regardless of the :typing settings.

How does look the code for generating the domain in the example? do you guys really want to generate untyped PDDL?

haz commented 3 years ago

Hrmz...those methods seem to be fairly static, so where would the optional parameter be set? I agree that the more natural solution would be to avoid types entirely, but then those outputs should have their conditioning based on is_typed_problem().

The code to cause this is pretty simple afaik -- just load a non-typed domain and then write it again. It stems from a need to generate multiple problem instances given one. We sample reachable states, sample subsets of fluents to be a goal, and then write those as new problem instances. So everything remains the same except the goal. But with the typing bug introduced, our generated problems are no longer "solvable".

We'll very soon be generating untyped PDDL as well for other reasons, but so far I think they are all object/parameter-free (all predicates are 0-arity).

miquelramirez commented 3 years ago

Hi @haz,

where would the optional parameter be set?

Instances of FstripsWriter are the ones with the information, and clearly, the methods that invoke the functions mentioned above should be the ones passing along the "typing" status of the domain and instance.

BTW, now that I think of it, the method involved in writing down the object definition in the instance file needs to be aware of the typed/untyped status too.

I think they are all object/parameter-free (all predicates are 0-arity).

👍

gfrances commented 3 years ago

Hi guys,

would always printing object as a type solve the issue? Seems like that would be OK with the "standard", and while it is aesthetically a bit less pleasing perhaps that having untyped parameters, declarations, etc. when possible, maybe it's not that terrible?

miquelramirez commented 3 years ago

Depends on the planner @haz is using... If we add the : typing keyword and probably add object too in the :types section to help planners that ignore the requirements tags, I think no planner could have grounds to behave in an unseemly way.

haz commented 3 years ago

That'd work. We're less interested in the planner used, as the encodings made.

gfrances commented 3 years ago

I'm implementing this, but finding a rather silly issue with the PDDL 3.1 BNF grammar. According to the grammar, the (:types ...) definition needs to be made up of typed list elements. If I interpret it correctly, there would be no room for a simple (:types object) declaration, since all typenames need to be followed by a dash plus its parent type (which of course object doesn't have)? The grammar is not too helpful, since the rule I'd be interested in is marked as "deprecated", whatever that means in a non-maintained grammar such as this.

What's your take on this?

haz commented 3 years ago

Ya, well there's the standard and then there's the "common standard" in what planners support ;). You could technically have untyped languages compiled with every object of type objekt and then just use (:types objekt - object) to be complicit with the BNF, but I'm pretty sure there are loads of domains that "get it wrong" (TM).

haz commented 3 years ago

...thus, my general take is just go with (:types object), as long as the common planners don't complain. It's far easier than modifying the rest of the spots to not spit out - object.

gfrances commented 3 years ago

Could you guys check whether commit fa46b66 fixes your issue?

haz commented 3 years ago

@gfrances That seems to do the trick, thanks!

Can you let us know when it makes it into dev, and we'll update everything on our end? (i.e., pull out the hot-fixes)

gfrances commented 3 years ago

This should be fixed on devel now. Thanks guys!