Skip to content

558 reporting intermediate solutions#561

Open
IgnaceBleukx wants to merge 44 commits intomasterfrom
558-reporting-intermediate-solutions
Open

558 reporting intermediate solutions#561
IgnaceBleukx wants to merge 44 commits intomasterfrom
558-reporting-intermediate-solutions

Conversation

@IgnaceBleukx
Copy link
Copy Markdown
Collaborator

Following the discussion in #558.

It adds quite some code in all solver interfaces, but I tried to do it somewhat consistent.
Some solvers are missing still:

For Exact,it is somewhat hacky as we need to go through "runOnce" instead of the nice "toOptimal" function. So there will be some more ping-pong through the interface.

@IgnaceBleukx IgnaceBleukx linked an issue Dec 20, 2024 that may be closed by this pull request
Copy link
Copy Markdown
Collaborator

@ThomSerg ThomSerg left a comment

Choose a reason for hiding this comment

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

Seems a nice quality-of-life improvement. But yeah, the one for exact seems hack-y, especially with the "warning" part. Is there a way to restore the solver afterwards?

@ThomSerg
Copy link
Copy Markdown
Collaborator

ThomSerg commented Jan 2, 2025

Besides that it is never mentioned + silently enforced that "display" is only for optimization, it seems good to me.

@Wout4
Copy link
Copy Markdown
Collaborator

Wout4 commented Jan 6, 2025

we could add the gcs callback also since that bug is fixed or not?

@IgnaceBleukx
Copy link
Copy Markdown
Collaborator Author

IgnaceBleukx commented Jan 9, 2025

Made quite some changes to this PR after the last review, so maybe another round of revieuwing from @ThomSerg and/or @Wout4 would be appropriate before merging.

I'm wondering if we should really fill the solver.objective_ argument directly from the underlying solver object.
Alternatively, we can store the original objective expression and compute the objective value based on that. There were quite some tricky things to get right in this PR...

Copy link
Copy Markdown
Collaborator

@ThomSerg ThomSerg left a comment

Choose a reason for hiding this comment

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

Went through the entire thing again

self.chc_solver.limit_time(str(time_limit) + "s")

if display is not None:
raise NotImplementedError("Choco does not support solution callbacks, (TODO?)")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is on purpose to leave this "TODO" here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choco in Java does have callbacking, so just leaving it here for if and when they implement callbacking directly from Python as well

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We could also implement it as in Exact, but there will be a lot more overhead in Choco (needs to pass trough C++ and Java...)



if display is not None:
raise NotImplementedError("TODO: implement MiniZinc callbacks -- requires new async methhod (similar to solveAll())")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Meant to leave this TODO?

Copy link
Copy Markdown
Collaborator Author

@IgnaceBleukx IgnaceBleukx Jan 13, 2025

Choose a reason for hiding this comment

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

MiniZinc does support it, but it requires quite a bit of engineering so better for another PR I think

@IgnaceBleukx
Copy link
Copy Markdown
Collaborator Author

Should be fine now @ThomSerg

@tias tias marked this pull request as draft April 7, 2025 12:56
@IgnaceBleukx
Copy link
Copy Markdown
Collaborator Author

This PR has been stale for quite some time now, and not sure how to continue on it.
Do we need a more structured approach to support these callbacks? Do we want it at all?
I think it's a nice quality of life improvement, but it also adds a lot of extra code to our solve functions which hinders future maintainability...
Therefore, I'm also fine with abandoning this for now and coming back to it later -- interested to hear others opinions : )

@tias
Copy link
Copy Markdown
Collaborator

tias commented Feb 2, 2026

I propose we do not add it for all solvers, but instead solver interfaces can choose to implement the display= function.

So we would add it to the template and implement it for OrTools. But we would not attempt to make it work for all solvers.

@IgnaceBleukx IgnaceBleukx marked this pull request as ready for review April 3, 2026 13:43
@IgnaceBleukx
Copy link
Copy Markdown
Collaborator Author

Merged master into this one and removed the hacky callbacking for solvers who don't support it natively.

I'm slightly stuck on a couple of things:

  • Docs: how to describe what the callback is? For now I've copied over the description we use in solveAll
  • Semantics: solvers have different semantics for when the callback is called... Gurobi and OR-Tools call it for each (improving?) solution, Hexaly just calls the callback periodically during solve
  • Backwards compatibility: most solvers already had a solution_callback argument that you could give (ortools, cpo and hexaly). Do we want to keep this possibility? For now I've kept it but the argument has been moved to **kwargs. Alternatively, we can simply re-use the solution_callback argument and check if it is already a callback? E.g., an OrtSolutionPrinter for ortools. Otherwise we wrap it in such a callback class ourself?

If we get the callbacking somewhat clean, we can massively cleanup the solveall code for the solvers that support it too I think. But definately for another PR

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.

Reporting intermediate solutions

4 participants