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

Check domain validity always fails #232

Open
devis12 opened this issue Jul 19, 2022 · 10 comments
Open

Check domain validity always fails #232

devis12 opened this issue Jul 19, 2022 · 10 comments

Comments

@devis12
Copy link

devis12 commented Jul 19, 2022

Good morning, I have a question related to the new implementation to check the domain validity doing at startup by the domain expert.
As before, you're running popf from the ros2 popf pkg with the loaded domain(s) and checking whether or not it gives an error. However, by running it on an empty problem file, it does not print anything if the domain is valid, nor it gives you any error (which it would if the domain is not valid). Nonetheless, you're checking if the ouput contains "Solution Found", which it never will, avoiding plansys2 to boot.
return result.find("Solution Found") != result.npos;

I'm running on ROS2 Foxy and Mint 20.3 (should be Ubuntu Focal equivalent), but I don't think that's the issue, because I hadn't any before the pull that I performed this morning. Thank you for your support

@devis12 devis12 changed the title Check domain validity Check domain validity always fails Jul 19, 2022
@devis12
Copy link
Author

devis12 commented Jul 20, 2022

I would suggest to just check if there is any "error"/"Error" substring within the empty string that should be returnedas output by the planner.

@fmrico
Copy link
Contributor

fmrico commented Jul 20, 2022

Hi @devis12

I think it could be a good fix. May you open a PR with your proposal?

Thanks!!

@roveri-marco
Copy link
Collaborator

@fmrico I believe that the best solution is to check for "Solution found" as per your last fix to your modified version of popf or the output file is empty (as it was happening and it is expected in the standard version of popf).
cc: @jjzapf @sarcasticnature

@jjzapf
Copy link
Collaborator

jjzapf commented Sep 21, 2022

I closed my PR (#234) related to this issue because I thought it was solved. However, it appears that I accidentally tested the wrong branch. I will leave my PR closed as I do not think it was the right solution. I think we should continue the discussion here to determine the best path forward.

I concur with description of the issue provided by @devis12. In particular, I would note that this line creates a "mostly" empty problem file. I say mostly, because the file does contain the single line "(define (problem void) (:domain plansys2))"

https://github.com/PlanSys2/ros2_planning_system/blob/master/plansys2_popf_plan_solver/src/plansys2_popf_plan_solver/popf_plan_solver.cpp#L144

In the following line, we then run POPF using the domain specified at startup and the problem file defined above. When I run this command manually, it does not produce any output. However, @fmrico stated in #234 that he sees the following.

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

@fmrico, are you positive that you are seeing this output using the problem file specified above? I cannot seem to replicate this.

@sarcasticnature
Copy link
Contributor

@jjzapf you must be using either the humble (or I believe ros2) branches of popf itself for the changes to work, as some bugfix changes the behavior.

@sarcasticnature
Copy link
Contributor

@roveri-marco
Copy link
Collaborator

Let me comment here, although it is not the right place! I changed my idea w.r.t. my previous comment!

@fmrico @sarcasticnature @jjzapf I do not like the changes performed to POPF! This may change the behavior of the planner.
Moreover, we have a pddl parse, thus I do not understand why using POPF to validate the domain! In some parts we use the parser, in some other cases we use POPF.. By the way, what happens if one change the planner to use TFD? The system with these fixes become non standard, unless also TFD has been modified accordingly!

@jjzapf
Copy link
Collaborator

jjzapf commented Sep 22, 2022

@sarcasticnature, thanks for the heads up regarding which branch to use. You were right. Switching to the ros2 branch fixed the issues I was seeing.

@roveri-marco, I agree with your comments about using the parser rather than POPF to validate the domain. That makes a lot of sense, especially as we open up PlanSys2 to more planners. Unfortunately, I do not know enough about the details of POPF to comment on whether the changes modify the behavior of the planner.

@fmrico
Copy link
Contributor

fmrico commented Sep 22, 2022

Hi all,

Yes, we should use the parser. If anyone is interested in pushing a PR for this, it would be perfect. Otherwise, I could work on it the following week.

Best

@roveri-marco
Copy link
Collaborator

@fmrico At a certain point I can work on this...

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

No branches or pull requests

5 participants