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

[work in progress] Big main #6

Closed
wants to merge 90 commits into from

Conversation

guicho271828
Copy link
Contributor

@guicho271828 guicho271828 commented Nov 19, 2021

this is a work-in-progress PR which I made to make it easy to follow the update.
The idea is to have a central init file which can generate any domain with any hyperparameter, also controlling for the output pathname naming convention.

todo:

  • global init file and API for each generator
  • support for individual domain
  • agricola
  • assembly
  • barman
  • blocksworld
  • briefcaseworld
  • cavediving
  • childsnack
  • citycar
  • crewplanning
  • data-network
  • depots
  • driverlog
  • elevators
  • ferry
  • floortile
  • freecell
  • fridge
  • goldminer
  • grid
  • gripper
  • grippers
  • hanoi
  • hiking
  • logistics
  • maintenance
  • matchcellar
  • miconic
  • miconic-fulladl
  • miconic-simpleadl
  • movie
  • mprime
  • mystery
  • nomystery
  • npuzzle
  • nurikabe
  • openstacks
  • parking
  • pathways
  • pegsol
  • rovers
  • satellite
  • scanalyzer
  • schedule
  • snake
  • sokoban
  • spanner
  • spider
  • storage
  • termes
  • tetris
  • tidybot
  • tms
  • tpp
  • transport
  • trucks
  • tsp
  • turnandopen
  • tyreworld
  • visitall
  • woodworking
  • zenotravel

@haz
Copy link
Contributor

haz commented Nov 19, 2021

I'd like to see this added to planutils when it's set, and then an easy transition to the new remote planning service about to be released.

@guicho271828 guicho271828 force-pushed the big-main branch 2 times, most recently from cd9a0b3 to 59849c9 Compare November 19, 2021 16:43
@guicho271828
Copy link
Contributor Author

planutil is just for conveniently calling planners, correct?

@haz
Copy link
Contributor

haz commented Nov 19, 2021

Nope. View it as a sort of "apt" for planning-like software. So, yes, easy install of planners, but also libraries, utilities, etc.

@jendrikseipp
Copy link
Collaborator

Hi Masataro, before you start hacking away: this PR plans to address many issues at the same time (parameter specifications, task filtering, task transformation, batch task creation, uniform output filenames). I think some of these deserve their own PR.

Also, task filtering and task transformation feel like candidates for external postprocessing tools to me. I think it's best to have them as standalone tools.

For batch task creation, I think we can eventually move the code from https://github.com/rleap-project/batch-pddl-generator into this repo. Your __main__,py file has a "-n" parameter, but how would you choose the n instances?

That leaves mainly the parameter specification issue. Which use case do you envision here? If the goal is to get good error messages for an invalid parameter combination, I think the best approach is to change the generator code and add argument checking and error messages there. If the goal is to present the valid parameter ranges to users, I think the same applies: add or improve the existing help messages in the generator code. This would definitely be helpful for users.

@guicho271828 guicho271828 force-pushed the big-main branch 2 times, most recently from ecabb62 to e3f5817 Compare November 19, 2021 18:44
@guicho271828
Copy link
Contributor Author

task filtering and task transformation

This is not an immediate task and I do not plan to do it for now. I just made a placeholder.

https://github.com/rleap-project/batch-pddl-generator

This is a HUGE reinvention of wheel and I do not think worth pursuing.
I would simply use GNU parallel. For example,

parallel pddl-generator blocksworld-3ops --seed {1} -blocks {1}::: {0..4} ::: {0..3}

@guicho271828
Copy link
Contributor Author

ah, didn't catch the part that you use the SMAC hyperparamter tuner.
The part I am making will be the interface to be called from SMAC then.

@guicho271828
Copy link
Contributor Author

That leaves mainly the parameter specification issue. Which use case do you envision here? If the goal is to get good error messages for an invalid parameter combination, I think the best approach is to change the generator code and add argument checking and error messages there. If the goal is to present the valid parameter ranges to users, I think the same applies: add or improve the existing help messages in the generator code. This would definitely be helpful for users.

I don't pursue these non-trivial goals. I simply make this set of generators usable and streamlined. Nothing new, simply refine and improve. Currently I have to manually look into each generator, learn the usage from README, and eventually write code that does the job I want. I did it for 5 domains for my recent paper. The result of this PR will be a generator with a common API that is well documented in a centralized manner.

@jendrikseipp
Copy link
Collaborator

Alright. In any case, I suggest to start with a minimal solution and to only tackle one domain in the first PR. Ideally, a domain where not all parameter combinations are allowed. Then we can see whether a centralized design is helpful or whether it's better to improve the generators themselves.

@guicho271828 guicho271828 force-pushed the big-main branch 8 times, most recently from 5a17cea to 8d7985b Compare November 24, 2021 21:52
@jendrikseipp
Copy link
Collaborator

I see you're hard at work here Masataro. Let me reiterate though: I won't be able to review the changes for ~60 domains. If you want to eventually have your work merged into this repo, you need to tackle a single domain in its own PR.

@guicho271828
Copy link
Contributor Author

it is just about 3 hours of burst work ... It is nothing.
btw, I want to show you how the resulting experience would be like.
Please take a look at this asciinema. https://asciinema.org/a/0dS4eHPc3D8y4DGybouogrn9j

@guicho271828
Copy link
Contributor Author

If you would take time to review changes (thanks!), I would recommend checking the commits tagged as [api] and [implementation].
The rest of the commits (tagged as [domain] ) are largely just copy-and-paste from pddl-generators/__template , using copy-template.sh script. There is not much to review, and I have not spent significant time in them. They are separated just for the sake of clean commit messages.

So nothing in any specific domain really matters in these commits, I treat them largely as test cases for assessing the utility of the tooling. I left the details as the room for improvement for future users.

By the way, I have not introduced a single line of change in the existing code base --- I have only moved the original files (it was necessary for being compatible with python tooling) and added several new files, no existing files are modified significantly.

@jendrikseipp
Copy link
Collaborator

Please tackle a single domain in the first PR. Ideally, a domain where not all parameter combinations are allowed. Then we can see whether a centralized design is helpful or whether it's better to improve the generators themselves. Also, I'd like to keep the generator dirs in the top-level.

@guicho271828
Copy link
Contributor Author

guicho271828 commented Dec 5, 2021

I am afraid you still don't get the aim and the scope of this PR.
Currently this repository is just a collection of generators put in a single place without a common API and that is what I fix in this PR. I make this repository more usable and that is the only purpose of this PR.
Besides, this repo is not pip loadable and does not fit in the typical python workflow.

I understand a parameter validation feature is useful, but that is clearly the next step and not this step.

As I said earlier, I can separate the PR into just a API proposal, removing any domain-related commits.
The priority of reviewing should be on the API itself.

@guicho271828
Copy link
Contributor Author

By the way, I have already started using the PR branch in my project and it is quite useful already.

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.

3 participants