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

UP-BFGP interface v0.2.0, few-shot engine and BFGP++ generalized planner v0.1.0 #514

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

jsego
Copy link

@jsego jsego commented Nov 7, 2023

This code is required for the Final Deliverable of the "Integration of a Generalized Planner in the Unified Planning Framework" project.

The PR includes a new engine to do few-shot planning, where the input is a set of problems in a given domain. Also registers a new planner named BFGP whose interface can be found in the up-bfgp repository.

Copy link
Member

@mikand mikand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @jsego! (And sorry for the latency)

This is super-interesting, I left some comments and suggestions, let me know what you think.

def is_fewshot_planner() -> bool:
return True

def solve(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename this method, otherwise it would conflict with the OneshotPlanner Operation Mode, making it impossible to have an engine supporting both FewshotPlanner and OneshotPlanner

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a OneshotPlanner a specialization of a FewshotPlanner, so if some software could do both, then it is a FewshotPlanner, i.e. a OneshotPlanner is a FewshotPlanner that always works with one instance.

unified_planning/engines/mixins/fewshot_planner.py Outdated Show resolved Hide resolved
This method takes a list of `AbstractProblem`s and returns a `PlanGenerationResult`,
which contains information about the solution to the problems given by the generalized planner.

:param problems: is the list of `AbstractProblem`s to solve.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any assumption on the structure of these problems? What happens if they come from radically different domains?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have such assumptions we need to make them explicit in the OM

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It assumes the domain is shared for all instances.
You can see an example in the UP-BFGP interface.

) -> List["up.engines.results.PlanGenerationResult"]:
"""
This method takes a list of `AbstractProblem`s and returns a `PlanGenerationResult`,
which contains information about the solution to the problems given by the generalized planner.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain how this is different from calling a OneshotPlanner n-times

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main purpose of a FewshotPlanner is to produce domain-specific knowledge that i) inform about the structure and complexity of the solution for any instance in that domain, and ii) scales up to larger and more complex planning instances. Thus, it must be more scalable than calling a classical planner n times on new and larger planning instances, if the domain knowledge is computable in reasonable time and memory.

return self._solve(problems, heuristic, timeout, output_stream)

@abstractmethod
def _solve(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this method neds to be renamed to avoid clash with OneshotPlanner

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Commented above)

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.

2 participants