Skip to content

Conversation

@strahlw
Copy link
Collaborator

@strahlw strahlw commented Oct 21, 2024

Fixes/Addresses/Summary/Motivation:

For the web interface, if a job experiences a sub-optimal termination status in PRIMO, we want information about the status to be propagated to the end user in some way. This PR adds a simple OptimizationException to accomplish this. Specifically, the added method passes any optimization run that returns an optimal completion status (i.e., solution status is optimal and the termination condition is "convergenceCriteriaSatisfied"). Any other result from an optimization triggers an OptimizationException, which passes a generic "optimization did not terminate optimally" and the associated results object. All errors that happen before the successful creation of the results object (e.g., feasibility and unboundedness with Highs) will raise an error that will be excepted by some other exception type. If the exception type is an OptimizationException, then all the information regarding the results are in the results object. If the exception type is different (e.g., RunTimeError for Highs detecting infeasibility of the problem before executing), then those exceptions can be handled differently. This PR proposes the simplest way of communicating optimization run errors from PRIMO to the web API, where they can be appropriately handled and information conveyed to the user.

While implementing the handling of optimization errors, it became very cumbersome to use the existing infrastructure with using Highs independent from pyomo (the results objects are different types, structures, etc.). This motivated the switch to using the appsi_highs solver interface with pyomo.

Changes proposed in this PR:

  • Introduced method to handle optimization exceptions that will provide a generic optimization termination message and communicate the results object
  • Changed from using Highs() in the utils/solver.py file to the pyomo interface with highs ("appsi_highs") to facilitate consistency in the types and fields of the results objects for easier handling
  • As far as I am aware, there is no motivation to not use a persistent solver interface as we will not be making "on the fly changes" that require informing the solver. Regardless, we could always create a new instance of the solver factory, pass in the modified model and solve as usual. We just need to ensure that the changes that we make are passed on to the solver.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my
contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.md file
    at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has
    rights to intellectual property that includes these contributions, I represent that I have
    received permission to make contributions and grant the required license on behalf of that
    employer.

@codecov
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
primo/opt_model/model_options.py 89.21% <100.00%> (+0.10%) ⬆️
primo/opt_model/opt_model.py 88.00% <100.00%> (ø)
primo/opt_model/tests/test_model_options.py 100.00% <100.00%> (ø)
primo/utils/__init__.py 81.81% <100.00%> (+1.81%) ⬆️
primo/utils/opt_utils.py 84.93% <100.00%> (+2.67%) ⬆️
primo/utils/solvers.py 97.05% <100.00%> (+4.03%) ⬆️
primo/utils/tests/test_opt_utils.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes


LOGGER.info(
"Added a constraint to ensure that at least 50 percent of the budget is used when the total budget is sufficient to plug all wells."
"Added a constraint to ensure that at least 50 percent of the \
Copy link
Collaborator

@apd-ypuranik apd-ypuranik Oct 29, 2024

Choose a reason for hiding this comment

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

Minor but string concatenation when using parenthesis does not require '\'. You can simply close the string and open it again on the next line.


Returns
-------
1 : Optimal termination
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we wrap the statuses in an enum class or atleast define them as global parameters somewhere? That way, we are not left with magic numbers (1 or 2) in other places in the code

# feasible solution - couldn't find a suitable test
if (
results.solver.status == SolverStatus.ok
and not results.solver.termination_condition == TerminationCondition.other
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure this is safe? Pareto implemented an explicit feasibility checker since solvers like CBC, Highs in general cannot be trusted. https://github.com/project-pareto/project-pareto/blob/b034951418c28d78679ce7a8759337fd7066b329/pareto/utilities/results.py#L2923

Copy link
Collaborator

@ruonanli61 ruonanli61 left a comment

Choose a reason for hiding this comment

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

I don't have additional comments in addition to Yash's.

@strahlw strahlw marked this pull request as draft November 18, 2024 14:54
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.

3 participants