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

Refactoring models invocation #614

Merged
merged 23 commits into from
Jun 6, 2023

Conversation

Halbaroth
Copy link
Collaborator

@Halbaroth Halbaroth commented Jun 5, 2023

This PR adds the support for the instruction get-model.
The command is only supported through the new frontend Dolmen. It means you cannot use the statement get-model without the option --frontend dolmen.

There is no support for the get-model statement in the native language of AE.

@Halbaroth Halbaroth force-pushed the refactoring-models-invocation branch from 13f95f9 to 1aeea7a Compare June 5, 2023 15:25
@Halbaroth Halbaroth marked this pull request as ready for review June 5, 2023 16:34
@Halbaroth
Copy link
Collaborator Author

It is ready

@Halbaroth
Copy link
Collaborator Author

AE still displays the status after printing the model, even if you use the native language.

Comment on lines +395 to +396
(* The command line option overrides the behavior of the get-model
commands. *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we discussed this multiple times and agreed that command line options should not override the behavior of in-band commands. If you use the option to get a model on each check_sat / check-sat, you should get that. If you also use get-model, you should get two models (or more, depending on the number of times get-model is called).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that comment should now be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't worry :) I didn't forget it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies for misunderstanding, i thought you were done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

You didn't forget but you still merged with the comment in there…

src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
src/lib/util/options.ml Outdated Show resolved Hide resolved
@bclement-ocp
Copy link
Collaborator

AE still displays the status after printing the model, even if you use the native language.

I assume you mean before printing the model? But that's good, thanks for checking!

@Halbaroth
Copy link
Collaborator Author

AE still displays the status after printing the model, even if you use the native language.

I assume you mean before printing the model? But that's good, thanks for checking!

Alas, I didn't. As I explained on Zulip, AE prints its models in a exception handler. After printing it, the exception is raised again and a second handler prints the status.

@Halbaroth
Copy link
Collaborator Author

I think we can fix the issue with the order between the status and the model in another PR. Modifying the way AE catches exceptions can be tricky :)

@bclement-ocp
Copy link
Collaborator

I don't think we need to modify the way we catch exceptions, we should get away with simply adding the model as payload to I_dont_know.

But I agree, this should be a separate PR.

src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
src/lib/reasoners/fun_sat.ml Outdated Show resolved Hide resolved
src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
src/bin/common/solving_loop.ml Outdated Show resolved Hide resolved
@Halbaroth
Copy link
Collaborator Author

I don't think we need to modify the way we catch exceptions, we should get away with simply adding the model as payload to I_dont_know.

Sounds good :) I will do it quickly :D

@@ -212,6 +212,9 @@ val set_input_format : input_format -> unit
*)
val set_interpretation : interpretation -> unit

(** [all_interpretations] accessible with {!val:get_all_interpretations}. *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Docstring was not updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was merged without updating the docstring.

Copy link
Collaborator

@bclement-ocp bclement-ocp left a comment

Choose a reason for hiding this comment

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

LGTM; please fix the comments and maybe change the name of sat_env if you are motivated, then we are good to merge.

@Halbaroth Halbaroth added this to the 2.5.0 milestone Jun 6, 2023
@Halbaroth Halbaroth merged commit d300a26 into OCamlPro:next Jun 6, 2023
@Halbaroth Halbaroth mentioned this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants