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

Use multiple solvers in qalpha #140

Merged
merged 19 commits into from
Aug 25, 2023
Merged

Use multiple solvers in qalpha #140

merged 19 commits into from
Aug 25, 2023

Conversation

edenfrenkel
Copy link
Collaborator

@edenfrenkel edenfrenkel commented Jul 28, 2023

This defines a BasicSolver trait in solver/src/basics.rs, which allows making very basic check-sat queries which return a sat result together with a trace, an unsat result together with an unsat-core, or unknown. By abstracting this basic functionality it is possible to implement this trait for a single SolverConf, as well as for a vector of SolverConf's in two different ways, once as FallbackSolvers tried sequentially and second as ParallelSolvers tried in parallel. This is utilized in qalpha, which now uses ParallelSolvers for its inductiveness and safety checks by default, and is configurable to use FallbackSolvers instead via --fallback. The simulation queries in qalpha still use a single SolverConf, now abstracted behind the BasicSolver trait as well.

Solver cancellation has also been moved out of the inference code and made more generic, using the new BasicSolverCanceler trait and SolverCancelers struct, which aggregates multiple BasicSolverCanceler's and itself implements BasicSolverCanceler. This is used recursively to create hierarchical tree-like cancellation, where the cancellation of a node cancels all of its descendants. The idea is that functions should only cancel queries which they (perhaps recursively) launched and not assume anything about the behavior of their callers. When the result is bubbled up to those callers, they should cancel their queries according to their own desired behavior. This allows more general applicablity which is independent of the specific goals of any single use-case, (in this case qalpha, which cancels all solvers on the first sat result.)

Another change is that now transition queries parallelize over disjunctive transitions, with proper cancellation if a sat response is encountered.

@edenfrenkel edenfrenkel requested a review from tchajed July 28, 2023 16:29
Copy link
Collaborator

@tchajed tchajed left a comment

Choose a reason for hiding this comment

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

I left a bunch of comments, but it's mostly just code-style advice.

This PR would be a great candidate for benchmarking, so you might want to add some benchmarking on top of #142. If you already have some data on the performance impact of this change from your own benchmarking script please include that data in the PR description.

inference/src/basics.rs Outdated Show resolved Hide resolved
inference/src/basics.rs Outdated Show resolved Hide resolved
inference/src/basics.rs Outdated Show resolved Hide resolved
inference/src/basics.rs Outdated Show resolved Hide resolved
inference/src/basics.rs Outdated Show resolved Hide resolved
solver/src/backends.rs Show resolved Hide resolved
solver/src/conf.rs Outdated Show resolved Hide resolved
solver/src/conf.rs Outdated Show resolved Hide resolved
inference/src/basics.rs Outdated Show resolved Hide resolved
inference/src/fixpoint.rs Outdated Show resolved Hide resolved
@edenfrenkel
Copy link
Collaborator Author

@tchajed When trying to factor out try_conf I realized I should probably just implement multi-solvers more generally using a solver trait, like you said. This turned out to be more tricky than expected, unless I reduced solver behavior to something very basic, which is now implemented in the BasicSolver trait in solver/src/basics.rs. This trait allows making very simple check_sat queries which return sat+trace, unsat+unsat-core, or unknown. I implemented this trait for a single SolverConf, as well as for a vector of SolverConf's twice, first as fallback solvers and second as parallel solvers. Solver cancellation has also been moved out of the inference code and made more generic.

Let me know what you think. I'll rebase and update the pull request description soon.

Copy link
Collaborator

@tchajed tchajed left a comment

Choose a reason for hiding this comment

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

I like the trait solution, that nicely abstracts over what we need in this case!

Can you update the PR description? Other than that (and the usual rebase onto main) this is ready to merge.

Signed-off-by: edenfrenkel <[email protected]>
Signed-off-by: edenfrenkel <[email protected]>
Signed-off-by: edenfrenkel <[email protected]>
Signed-off-by: edenfrenkel <[email protected]>
This also defines two implementations of the basic solver trait, one for a single solver configuration and another for multiple solvers used sequentially in a fallback fashion. Using this, the multi-solver bahavior previously implemented in inference has been factored out and made more generic.

Signed-off-by: edenfrenkel <[email protected]>
The option to use fallback solvers as before is maintained via the flag `--fallback`.

Signed-off-by: edenfrenkel <[email protected]>
Using the hierarchical cancellation enabled by `SolverCancelers`, functions should only cancel queries which they (perhaps recursively) launched. When the result is bubbled up to higher callers, they should cancel their queries according to their own desired behavior. This allows more general applicablity which is independent of the specific goals of any single use-case, (in this case qalpha.)

Signed-off-by: edenfrenkel <[email protected]>
@edenfrenkel
Copy link
Collaborator Author

@tchajed updated and rebased

@tchajed tchajed added this pull request to the merge queue Aug 25, 2023
Merged via the queue into main with commit e59d72a Aug 25, 2023
2 checks passed
@tchajed tchajed deleted the qalpha-multiple-solvers branch August 25, 2023 18:11
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