Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverting is_valid_domain in popf_plan_solver back to its original fu… #234

Closed
wants to merge 1 commit into from

Conversation

jjzapf
Copy link
Collaborator

@jjzapf jjzapf commented Jul 28, 2022

The check_domain function in popf_plan_solver.cpp was recently changed and renamed to is_valid_domain. The DomainExpertNode calls this function in the on_configure callback. Previously, a failure would occur if the returned string was not empty. Now a failure will occur if the string does not contain "Solution Found". If the string is supposed to be empty, how can it contain "Solution Found"? Has something changed in the implementation such that the string should no longer be empty? For my examples, the string still appears to be empty.

Making the suggested change in this MR should revert the code back to it's previous logic. Without this change my previously working examples no longer work. ... I'm not really sure how this part of the code was intended to work, so my fix my not be the best solution. BTW, I am running Foxy, not Humble.

…nctionality.

Signed-off-by: Josh Zapf <joshua.j.zapf.civ@us.navy.mil>
@fmrico
Copy link
Contributor

fmrico commented Jul 28, 2022

Hi @jjzapf (CC @devis12)

I have not paid much attention to it when reviewing these PRs, so we will try to fix this in this PR definitively:

The behavior of POPF (branch: ros2) in ROS2 Humble (Ubuntu 22.04) is:

When a solution is found:

Constructing lookup tables: [10%] [20%] [30%] [40%] [50%] [60%] [70%] [80%] [90%] [100%]
Post filtering unreachable actions:  [10%] [20%] [30%] [40%] [50%] [60%] [70%] [80%] [90%] [100%]
All the ground actions in this problem are compression-safe
b (7.000 | 5.001)b (6.000 | 5.001)b (5.000 | 5.001)b (3.000 | 5.001)b (2.000 | 15.004)b (1.000 | 20.007);;;; Solution Found
; Time 1.23
0.000: (move robot1 assembly_zone body_car_zone)  [2.000]
0.000: (move robot2 assembly_zone sterwheel_zone)  [2.000]
0.000: (move robot3 assembly_zone wheels_zone)  [2.000]
...

When checking a domain with an empty problem:

Constructing lookup tables:
Post filtering unreachable actions: 
Optimising makespan, lower bound is 0.000
;;;; Solution Found
; Time 0.00

When there is not a valid plan:

Constructing lookup tables: [10%] [20%] [30%] [40%] [50%] [60%] [70%] [80%] [90%] [100%]
A problem has been encountered, and the problem has been deemed unsolvable
--------------------------------------------------------------------------
The goal fact:
(car_assembled car1)

...cannot be found either in the initial state, as an add effect of an
 action, or as a timed initial literal.  As such, the problem has been deemed
unsolvable.

When there is a syntax problem in the domain:

Warnings encountered when parsing Domain/Problem File
-----------------------------------------------------

The supplied domain/problem file appear to violate part of the PDDL
language specification.  Specifically:

Errors: 0, warnings: 1
/tmp/check_domain.pddl: line: 56: Warning: Undeclared symbol: robot_availabale

The planner will continue, but you may wish to fix your files accordingly
Type Errors Encountered in Domain File
--------------------------------------

Due to type errors in the supplied domain file, the planner
has to terminate.  The log of type checking is as follows:

Type-checking move
...action passes type checking.
Type-checking transport
Predicate robot_availabale not found
Effects fail type-checking.
[ros2run]: Process exited with failure 1

or

Critical Errors Encountered in Domain/Problem File
--------------------------------------------------

Due to critical errors in the supplied domain/problem file, the planner
has to terminate.  The errors encountered are as follows:

Errors: 2, warnings: 0
/tmp/check_domain.pddl: line: 63: Error: Syntax error in durative-action declaration.
/tmp/check_domain.pddl: line: 63: Error: Unreadable structure

Checking the string "Solution Found" seems to be correct, IMHO. I have also checked it in 20.04+Foxy with the same result.

Recently, I made some changes in popf that could affect it (I am not sure). @jjzapf @devis12, may you check that you have pulled all the changes?

Best

@jjzapf
Copy link
Collaborator Author

jjzapf commented Aug 3, 2022

Hi @fmrico, I haven't had time to dig into this too much. However, I did verify that I have all the latest commits. Unfortunately, I still have to use my temporary fix to get things working.

@sarcasticnature
Copy link
Contributor

sarcasticnature commented Aug 24, 2022

I have the same issue, which is seemingly due to mkdir not creating directories if the node has a namespace (e.g. /foo/mynode needs to create the parent directory foo before the directory for node mynode can be created). Because the directory isn't there, the files aren't there, therefore the check string is empty and if (!check.empty()) will always evaluate to false i.e. no error.

I have has suspicions of the bug for a bit but have not had time to fix 😬. I have other changes that I would like merged into master already, so I will fix the issue (if it is what I expect) as part of it and make a new PR.

edit:
ah, also I believe popf will not produce any output using the problem file generated by the is_valid_domain function (despite given a valid domain)... will try to work around it

@fmrico
Copy link
Contributor

fmrico commented Sep 17, 2022

Please, check if #241 fixed your problems. Have into account that master points to Humble.

@jjzapf
Copy link
Collaborator Author

jjzapf commented Sep 20, 2022

Yes! This seems to have fixed the issue. I am good closing this issue if it works for @sarcasticnature as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants