-
Notifications
You must be signed in to change notification settings - Fork 62
Initial support for a 'Boolector-native' timeout function via 'term' call-backs #62
Conversation
I've seen that this branch is failing on Travis. I checked it locally with the
Does |
About the smtcomp19 branch: It is currently broken due to CaDiCal's recent API change. I already fixed that on master (5230a62). I'm currently working on cleaning up the smtcomp19 branch to merge it back to master. In the meantime it's not recommended to use smtcomp19 with the newest CaDiCaL version. I'll have a proper look at this PR asap. Thanks for the work! |
Here is what I think:
What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I have a few change requests. In general, this seems like a nice solution to have a timeout per check-sat call, but as it is implemented right now it only sets the timeout for the check-sat call of the SAT solver instead of the check-sat call of Boolector.
If this timeout should be only valid for the check-sat call and nothing else, it's probably better to initialize the terminate handler each time at the beginning of btor_check_sat
and reset it at the end of btor_check_sat
.
Rather than fragment the comment over multiple smaller comments, I think here's the high-level gist (I just want to make sure we're on the same page before I make changes):
Seem like the right idea to you? |
On the other points:
|
Yes |
@andrewvaughanj Can you maybe rebase on master if you have time? The commits from the smtcomp19 branch are now merged to master. |
7331581
to
581e8e6
Compare
581e8e6
to
cbdb608
Compare
cbdb608
to
6e5efd7
Compare
6e5efd7
to
9731819
Compare
…call-backs Signed-off-by: Andrew V. Jones <andrew.jones@vector.com>
9731819
to
06684d4
Compare
Temporarily closing this as I am (actually!) about to start looking at it, so don't want the pipelines to continually run on in-progress changes. |
… applying to the SAT solver calls
… reading the current values via 'btor_opt_get'
Re-opening this for initial consideration. Changes:
What's not done:
I'm opening-up this PR now, as these last few are "nice to have", and I don't want to invest time in those if you're unhappy with the rest! As discussed, this will get squashed once we're all happy. |
One thing I'm not certain of is, for the "unsupported" solvers, the ordering of the output is a bit odd:
Notice: |
So I was interested in #84 and seeing how timeouts may/may not work in that context. Consider something like:
If I use Z3 on this, then I get:
however, on Boolector with my timeout changes I get:
because things "get terminated" and Boolector doesn't carry on. I then changed However, I then hit a bigger issue in the SMT2 parser. We have this flow I haven't tried this (yet), but I believe that API users would be immune to this, because they're not going to keep checking the timeout inside of their application, which means the Boolector flow would continue:
However, #84 might hit me here, because Boolector might not run the second My bigger issue here is that this might cause some "end-user confusion" if people try to use Thoughts? |
A clean solution would have global and local limits. In CaDiCaL I have 'solve' local conflict (and decision) limits for the API. The stand-alone solver has command line option conflict and decision limits, which is fine as long the CNF triggers one check, but I started to work on an INCCNF version (for cube-and-conquer), where now there could be global and local limits. The same is true for time limits. So you might want to have local and global versions. Then one could adopt the policy that local limits terminate only certain calls, particularly 'solve' calls, but not for instance 'parsing' or 'assert' calls. After a local limit is hit (in 'solve' in general), the solver remains in a sane state, the aborted command is in essence not changing the state of the solver (except for learned clauses, updates to counters and heuristics), except that for instance assumptions or those local limits are forgotten. For global limits we could keep the current Boolector semantics, i.e., after a global limit is hit the solver in essence becomes unusable (except maybe getting out some deduced facts). Does this make sense? ['solve' = 'check-sat'] |
@arminbiere thanks for the comments -- I think I need to leave some of these decisions up to @aniemetz/@mpreiner. However, I think the issue I highlighted only affects the SMT2 front-end, because that also checks the timeout, in a way that maybe doesn't make sense if you're using timeouts for termination. I think to work out the best way forward is to decide what |
Curious if there are any updates on this? I'm in favor of a solution that results in matching Z3's behavior here. (I'm the guy behind issue #84, and I'm also the primary author of the Haybale symbolic execution engine, which I've been very happy to build using Boolector - thanks for everyone's work on Boolector! To that end I'm also the maintainer of these safe Rust bindings for Boolector, where I recently implemented a timeout feature before discovering the issue in #84. I'm happy to contribute to discussions if anyone is interested in my use cases, and also willing to help with implementation for this timeout stuff if that would be helpful, although I have very little familiarity with the internals of Boolector.) |
I think |
@mpreiner okay, so, as a first step, I'll take a look at getting this working as part of parsing a SMT2 file in incremental mode and making sure multiple sat-checks are possible (and timeout). If you have any comments about other parts of the branch so far, let me know. |
"SAT solver %s does not support termination functions; timeout " | ||
"option may not have effect", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"SAT solver %s does not support termination functions; timeout " | |
"option may not have effect", | |
"SAT solver %s does not support termination functions; Timeout " | |
"option may not have any effect.", |
btor_opt_check_solver_supports_timeout (uint32_t timeout, uint32_t sat_engine) | ||
{ | ||
if (timeout | ||
&& (sat_engine == BTOR_SAT_ENGINE_CMS | ||
|| sat_engine == BTOR_SAT_ENGINE_MINISAT | ||
|| sat_engine == BTOR_SAT_ENGINE_PICOSAT)) | ||
{ | ||
BTOR_WARN (true, | ||
"SAT solver %s does not support termination functions; timeout " | ||
"option may not have effect", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check could be done in btor_sat_init. It should be enough to check if the timeout option is enabled and if smgr->api.setterm
is available.
|
||
/* | ||
* we might not have called btor_set_opt yet with the "new" value, so we | ||
* pass it in, but read the value for the other option. | ||
*/ | ||
btor_opt_check_solver_supports_timeout ( | ||
btor_opt_get (btor, BTOR_OPT_TIMEOUT), val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto (btor_sat_init).
/* | ||
* we might not have called btor_set_opt yet with the "new" value, so we | ||
* pass it in, but read the value for the other option. | ||
*/ | ||
btor_opt_check_solver_supports_timeout ( | ||
val, btor_opt_get (btor, BTOR_OPT_SAT_ENGINE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
Not required. |
This is an attempt to support a 'Boolector-native' timeout, utilising Boolector's 'term' call-backs for the individual solvers, without the user needing to write their own terminate functions.
Please note that this is a PR into the
smtcomp19
branch.How to use it (where
<VALUE>
is your timeout in millis):pass
-to <VALUE>
to the main Boolector executableadd
(set-option :timeout <VALUE>)
to your .smt2 filecall
btor.Set_opt(boolector.BTOR_OPT_TIMEOUT, <VALUE>)
via the Python APISome points to discuss:
This only works for Lingeling and CaDiCaL, but will "silently do nothing" for other SAT solvers (e.g., MiniSAT, CMS or PicoSAT). What do we want to here? Error out the user?
Currently, the timeout "takes precedence" over the term function (i.e., if you call
Set_term
after setting a timeout then timeout is still going to the one that gets checked). See: btorsat.c. Is this appropriate? If not, what's our preference? This is the most important point to resolve in my opinion.CMS also supports a millisecond-based timeout approach via its API (so not a call-back), I think it would be straightforward to call CMS's API with
BTOR_OPT_TIMEOUT
before invoking the SAT solver. See: set_max_time. However, this might not be in a "released" version of CMS yet (while the API existed before CMS 5.8.6, it didn't actually work and was fixed in May). Do we also want to support this? If so, do we want to changesetup-cms.sh
to usemaster
rather than a release tar.gz on GitHub.com?This code currently uses
clock()
on all platforms (which I think is fine). However, there are other parts of Boolector that usegetrusage
(on platforms that support it), which might be more accurate. See: btorutil.c. Do we want to choose betweenclock
andgetrusage
depending on the CMake checks?This change was made by
clang-format-diff.py
and not manually by me. I can manually undo this if we care?Do we want tests for this? Do we want API usage examples? Do we want documentation?
Signed-off-by: Andrew V. Jones andrew.jones@vector.com