Autonomous processes events #629

hstairs · 2024-09-18T09:01:33Z

In this branch we add support for PDDL+ specification. The branch includes extensions to the problem representation (so allowing to specify processes and events), pddl writer (so allowing to use domain independent off-the-shelf PDDL+ solvers), pddl reader (so allowing get PDDL+ specifications into up data structures). There is some design choice that would need discussion though.

mikand

@hstairs Thanks fo this, it looks good but there are few things I would do before merging:

Add some tests in the examples showing the new feature
Add EVENTS to the kind and handle it
Do not inherit the Event class from InstantaneousAction, in many places we check with isinstance(x, InstantaneousAction)
I am ok with limiting ourselves with linear derivatives for now, but I would design the interface of processes to be generic, otherwise it is very hard to change it in the future. Also, the parser and the code should raise meaningful exceptions when a non-linear case is encountered, now they operate wrongly.

mikand · 2024-10-04T12:59:59Z

unified_planning/io/pddl_reader.py

@@ -526,6 +552,8 @@ def _parse_exp(
                        solved.append(self._em.FluentExp(problem.fluent(exp.value)))
                    elif problem.has_object(exp.value):  # object
                        solved.append(self._em.ObjectExp(problem.object(exp.value)))
+                    elif exp.value == "#t":
+                        solved.append(self._em.Int(1))


Don't we need to model this explicitly?
I see this works for the linear case, but how can we catch an expression like (* #t #t) and raise an exception?

I think we shall handle this in a more principled way.

In PDDL+ we can express first time derivatives only, meaning that effects are always going to be (* #t exp) where exp is to be #t-free (citing from the PDDL+ paper). Note that exp is not required to be linear or am I enforcing that?

Having said that there is definitely a cleaner way to parse that!

mikand · 2024-10-04T13:06:00Z

unified_planning/model/action.py

+
+
+class Process(Action):
+    """This is the `Process` class, which implements the abstract `Process` class."""


? It seems to me it implements the abstract Action class 😄

I thinkit is not too bad to have processes and events to be Actions, even though they are semantically different. In hindsight we might have called the abstract action class somethig else.
@alvalentini maybe we can consider a refactoring of this name, eventually...

One could call this transition, and I have hinted at something like that to Alessandro...but it is a major change no?

mikand · 2024-10-04T13:09:56Z

unified_planning/model/action.py

+        Adds the given `derivative effect` to the `process's effects`.
+
+        :param fluent: The `fluent` objective of the time derivative definition.
+        :param value: The given `fluent` is incremented by the given `value`.


This comment is wrong, and I think that how we model derivatives is a central issue that shall be very well documented.

mikand · 2024-10-04T13:11:23Z

unified_planning/model/action.py

+
+    @property
+    def preconditions(self) -> List["up.model.fnode.FNode"]:
+        """Returns the `list` of the `Action` `preconditions`."""


All the comments in this class are copy-pasted, we need to replace Action with Process

mikand · 2024-10-04T13:13:10Z

unified_planning/model/action.py

+        )
+
+
+class Event(InstantaneousAction):


This is kind of dangerous: beware that isinstance(x, InstantaneousAction) would return True if x is an Event. I get that we save some code duplication, but here I would inherit from Action only... Then, maybe we can create a mixin to avoid the code duplication

For example, https://github.com/aiplan4eu/unified-planning/blob/master/unified_planning/model/mixins/actions_set.py#L60 is probably broken by this choice

mikand · 2024-10-04T13:17:25Z

unified_planning/model/problem.py

@@ -331,6 +331,10 @@ def _get_static_and_unused_fluents(
                    unused_fluents.clear()
                    for f in se.fluents:
                        static_fluents.discard(f.fluent())
+            elif isinstance(a, up.model.action.Process):


What about events here? Again, they are probably absorbed by the isinstance(a, up.model.action.InstantaneousAction), so we need to fix this

mikand · 2024-10-04T13:17:47Z

unified_planning/model/problem.py

@@ -682,6 +686,9 @@ def _kind_factory(self) -> "_KindFactory":
        for goal in chain(*self._timed_goals.values(), self._goals):
            factory.update_problem_kind_expression(goal)
        factory.update_problem_kind_initial_state(self)
+        self.processes
+        if len(list(self.processes)) > 0:
+            factory.kind.set_time("PROCESSES")


Add also some flags for events!

not sure we need to differentiate processes and events. I think a flag (likely not "processes") for both is fine

One further comment: if we only have events we need to find a way to understand the semantics. I am not sure this case is well documented in the syntax of PDDL+. For instance, ENHSP considers temporal only a problem where there is at least one process. I don't see any use for events only (as they are understood in PDDL+) as events make sense if triggered by something which is changed as an effect of some passage of time.

mikand · 2024-10-04T13:18:36Z

unified_planning/model/problem.py

@@ -1003,6 +1010,8 @@ def update_problem_kind_action(
            if len(action.simulated_effects) > 0:
                self.kind.set_simulated_entities("SIMULATED_EFFECTS")
            self.kind.set_time("CONTINUOUS_TIME")
+        elif isinstance(action, up.model.action.Process):
+            pass


I think we should add PROCESSES to the kind

yep, connected with the previous

mikand · 2024-10-04T13:19:36Z

unified_planning/model/problem_kind.py

+process_temporal_kind = ProblemKind(version=LATEST_PROBLEM_KIND_VERSION)
+process_temporal_kind.set_problem_class("ACTION_BASED")
+process_temporal_kind.set_time("PROCESSES")
+process_temporal_kind.set_time("CONTINUOUS_TIME")


no FLAT_TYPING here?

Isn't FLAT_TYPING a restriction on the kind of types we can define (like no hierarchy)? If so, there is no restriction here, or I didn't understand this :)

codecov · 2024-10-06T17:27:38Z

Codecov Report

Attention: Patch coverage is 88.65979% with 44 lines in your changes missing coverage. Please review.

Project coverage is 85.38%. Comparing base (6ee4e08) to head (6e3db94).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
unified_planning/model/action.py	82.42%	29 Missing ⚠️
unified_planning/io/pddl_writer.py	81.81%	8 Missing ⚠️
unified_planning/io/pddl_reader.py	90.90%	4 Missing ⚠️
unified_planning/model/mixins/actions_set.py	70.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
+ Coverage   85.36%   85.38%   +0.01%     
==========================================
  Files         202      204       +2     
  Lines       27319    27653     +334     
==========================================
+ Hits        23322    23611     +289     
- Misses       3997     4042      +45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…have done the most of the work to get the modeler formulate the problem and solve it through PDDL+ planner with the PDDLWriter.

…aneous transitions (including events). Polished comments, add the identification of the problem kind and finally added an example. Need to double check things

…hich is called derivative. This way we can have the semantics a bit more explicit in the code

…ion of up-enhsp for testing purposes

…eader

…I testing workflow

hstairs · 2024-11-12T08:01:49Z

I guess I addressed the requested changes and added a few more tests together with a running example. I think there can be other possible refactoring in the structure of action/event/processes/sensing/durativeactions but I run out of time and did only some of them.

Ezpeon · 2024-11-29T11:07:47Z

Hello, I started looking a bit into this and I think I could help a bit with the refactoring that was previously mentioned in the other comments.
I started another PR #646 where I will try to better organize the features (still work in progress, it does not currently pass mypy checks and other changes are needed, but pytest tests should still work).
I will let you know when it is in a state where it would make sense to look more into it to decide if it is worth it to adopt the changes I made.

hstairs requested a review from alvalentini September 18, 2024 09:01

hstairs self-assigned this Sep 19, 2024

hstairs linked an issue Sep 19, 2024 that may be closed by this pull request

PDDL+ #621

Open

mikand requested changes Oct 4, 2024

View reviewed changes

Enrico Scala and others added 21 commits October 16, 2024 10:11

Introduced Actions and Events as subclasses of Action. Then I should …

4e973fd

…have done the most of the work to get the modeler formulate the problem and solve it through PDDL+ planner with the PDDLWriter.

Small changes and run black

8975ee2

Towards PDDL+ parsing

a816869

Cleaning and finalising

0331778

Reformatting

0c38023

Last fix

11be820

Refactored action so as to have a mixin for the definition of instant…

4d2a797

…aneous transitions (including events). Polished comments, add the identification of the problem kind and finally added an example. Need to double check things

Added tests for processes and events and added a new type of effect w…

c91c644

…hich is called derivative. This way we can have the semantics a bit more explicit in the code

Done a pass with black

42c08b3

Fix

f7d12ec

Add some example

20ed48f

New test and example plus changed test in yaml file to use right vers…

af993d5

…ion of up-enhsp for testing purposes

made enhsp runnable for some test with processes

34bb53a

new enhsp version for testing

b13691c

Adding some more meanigful output in raising exception error in pddlr…

f208db5

…eader

Likely I found the issue on why up-enhsp wasn't working well in the C…

5e7ca46

…I testing workflow

black passed..

4587381

Small fix

d82f18b

Added a test for processes and events parsing

602895d

Black passed...

7f1c124

Fixed a bug

aabe009

hstairs force-pushed the autonomous_processes_events branch from ea544c8 to aabe009 Compare October 16, 2024 08:12

Enrico Scala added 3 commits October 16, 2024 19:44

New tests and some refactoring

cfba2c0

Black

801c439

Another small test for processes

b7f61f0

Enrico Scala and others added 3 commits November 4, 2024 12:48

Add new test and fixed a number of upcoming issues

0d6b4de

bug fixing

b4fc3b1

Some refactoring

6e3db94

Ezpeon mentioned this pull request Nov 29, 2024

Refactoring autonomous processes events #646

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autonomous processes events #629

Autonomous processes events #629

hstairs commented Sep 18, 2024

mikand left a comment

mikand Oct 4, 2024

hstairs Oct 4, 2024

hstairs Oct 5, 2024

mikand Oct 4, 2024

hstairs Oct 4, 2024

mikand Oct 4, 2024

hstairs Oct 5, 2024

mikand Oct 4, 2024

hstairs Oct 5, 2024

mikand Oct 4, 2024

mikand Oct 4, 2024

hstairs Oct 5, 2024

mikand Oct 4, 2024

hstairs Oct 5, 2024

mikand Oct 4, 2024

hstairs Oct 5, 2024

hstairs Oct 6, 2024

mikand Oct 4, 2024

hstairs Oct 5, 2024

mikand Oct 4, 2024

hstairs Oct 5, 2024

codecov bot commented Oct 6, 2024 •

edited

Loading

hstairs commented Nov 12, 2024

Ezpeon commented Nov 29, 2024



		class Process(Action):
		"""This is the `Process` class, which implements the abstract `Process` class."""

Autonomous processes events #629

Are you sure you want to change the base?

Autonomous processes events #629

Conversation

hstairs commented Sep 18, 2024

mikand 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

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

codecov bot commented Oct 6, 2024 • edited Loading

Codecov Report

hstairs commented Nov 12, 2024

Ezpeon commented Nov 29, 2024

codecov bot commented Oct 6, 2024 •

edited

Loading