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

Catch catastrophic solver failure when building MIS #3403

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

bknueven
Copy link
Contributor

@bknueven bknueven commented Nov 6, 2024

Fixes IDAES/idaes-pse#1520

Summary/Motivation:

The minimal intractable system calculation had a try/except around a solve we expect to fail, but when we check for failure we failed to first check the results object against None.

Changes proposed in this PR:

  • Insert results is not None conditional to short-circuit, thus not calling pyo.check_optimal_termination(results) if results is None.

Legal Acknowledgement

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

  1. I agree my contributions are submitted under the BSD license.
  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.

@@ -287,7 +287,7 @@ def _constraint_generator():
except:
results = None

if pyo.check_optimal_termination(results):
if (results is not None) and pyo.check_optimal_termination(results):
Copy link
Member

Choose a reason for hiding this comment

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

Question: would it better to not swallow all exceptions from the solve() above and instead use load_solutions=False in the call? Then results should never be None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. Would that still not raise an exception if the solver returns a non-zero exit code? I think that's why I've implemented these as blanket try / except blocks.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I would just be worried about the bare except: -- that could be suppressing any number of errors and there would be no way to know what was going on.

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

I am OK merging this as it resolves a serious error blocking MIS users. However, this highlights fragility in the MIS implementation due to the use of a bare (catch-all) "except:" clause.

It would be good to revisit this and see if the bare "except:" can be removed.

@blnicho blnicho merged commit 952e67b into Pyomo:main Nov 7, 2024
30 checks passed
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.66%. Comparing base (c463632) to head (57f5e06).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3403      +/-   ##
==========================================
+ Coverage   88.63%   88.66%   +0.03%     
==========================================
  Files         879      879              
  Lines      100363   100363              
==========================================
+ Hits        88955    88986      +31     
+ Misses      11408    11377      -31     
Flag Coverage Δ
linux 86.12% <100.00%> (-0.01%) ⬇️
osx 76.11% <0.00%> (ø)
other 86.61% <100.00%> (-0.01%) ⬇️
win 84.46% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Solver issue with compute_infeasibility_explanation() in Diagnostic Toolbox
5 participants