Add tests for trivial constraints in pyomo/contrib/solver by michaelbynum · Pull Request #3703 · Pyomo/pyomo

michaelbynum · 2025-08-14T00:08:48Z

Summary/Motivation:

This PR adds a test for trivial constraints (both feasible and infeasible) to the new solver tests. It also fixes some related bugs.

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:

I agree my contributions are submitted under the BSD license.
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 · 2025-08-14T05:41:41Z

Codecov Report

❌ Patch coverage is 82.69231% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.99%. Comparing base (c1ce36d) to head (fe00fdd).

Files with missing lines	Patch %	Lines
pyomo/contrib/solver/solvers/gams.py	77.96%	13 Missing ⚠️
pyomo/contrib/solver/common/solution_loader.py	66.66%	4 Missing ⚠️
pyomo/contrib/solver/common/results.py	94.44%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3703   +/-   ##
=======================================
  Coverage   89.98%   89.99%           
=======================================
  Files         904      904           
  Lines      106891   106934   +43     
=======================================
+ Hits        96191    96231   +40     
- Misses      10700    10703    +3

Flag	Coverage Δ
builders	`29.16% <14.42%> (+<0.01%)`	⬆️
default	`86.32% <82.69%> (?)`
expensive	`35.59% <14.42%> (?)`
linux	`87.46% <82.69%> (-2.03%)`	⬇️
linux_other	`87.46% <82.69%> (+<0.01%)`	⬆️
oldsolvers	`28.09% <14.42%> (+<0.01%)`	⬆️
osx	`82.82% <37.50%> (-0.01%)`	⬇️
win	`85.89% <82.69%> (-0.01%)`	⬇️
win_other	`85.89% <82.69%> (-0.01%)`	⬇️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

eminyouskn · 2025-08-14T13:25:36Z

+        return self.var_map[self.v2id]
+
+
+class GurobiDirectQuadratic(GurobiDirectBase):


Shouldn't this class be named GurobiConsistentQuadratic? In GurobiObserver this is the used name.

I'm confused. Can you point me to to the place in GurobiObserver you are referring to?

I was referring to the field opt of the class _GurobiObserver.

This class does not use the observer. This one is not persistent. Maybe I am missing something?

Sorry, maybe I wasn’t clear enough. From what I see, in _GurobiObserver you are requesting opt as a GurobiPersistantQuadratic, but this class doesn’t appear to be defined. However, GurobiPersistant inherits from GurobiDirectQuadratic and is what gets passed to _GurobiObserver. So either I’m missing something, or there’s a naming issue.

Good catch! Thank you. The observer is expecting GurobiPersistent not GurobiPersistentQuadratic. I will fix that.

eminyouskn · 2025-08-14T14:04:33Z

+        self._solver_model.setObjective(gurobi_expr + repn_constant, sense=sense)
+
+
+class _GurobiObserver(Observer):


Since this class is just calling the "same" functions on the opt field. Why not just inherits Observer in GurobiPersistantSolver?

It has to be done this way in order for "manual mode" of the persistent solvers to work. If someone calls opt.add_constraint directly on the solver interface, we need the observer to be updated so things stay synchronized.

You’re probably right, and I might be missing something. However, I don’t see why adding Observer as a superclass of GurobiPersistent wouldn't mean the observer is always updated, since it’s essentially the same as the persistent object.

The important thing is that the _change_detector methods get called. This makes my head hurt a little. Let me think on it.

So if we did something like

class GurobiPersistent(GurobiDirectQuadratic, Observer): def add_constraints(self, cons): self._change_detector.add_constraints(cons)

Then there would not be a way to call _add_constraints. It would just be an infinte loop. If GurobiPersistent.add_constraints gets called, it would call the _change_detector, and the _change_detector would again call add_constraints on the observer, which is GurobiPersistent.add_constraints.

That is convoluted, but did it make any sense?

With the current design, this will always happen: the solver calls the detector, which calls the observer, which in turn calls the solver (Opt) again. Or am I missing something?

If GurobiPersistent.add_constraints gets called, then that will call ModelChangeDetector.add_constraints, which will then call _GurobiObserver.add_constraints, which then calls GurobiPersistent._add_constraints and not GurobiPersistent.add_constraints. Very subtle but important difference there. However, this conversation is making me realize how convoluted this is. Maybe someone has a better idea?

Okay, understood! One possible fix would be to give all Observer functions a common prefix (e.g., on_*) and make GurobiPersistantSolver a subclass of Observer. We could then reimplement the functions accordingly.

eminyouskn · 2025-08-17T13:14:49Z

+        raise NoSolutionError()
+
+    def load_vars(
+        self, vars_to_load: Sequence[VarData] | None = None, solution_id=None


Shouldn't we use Optional here?

jsiirola

Some minor nits (we can debate those), and one significant one (we should really propagate information about the infeasible constraint out through the NoSolutionError).

jsiirola · 2026-04-27T14:21:08Z

+        except InfeasibleConstraintException:
+            res = self._get_infeasible_results(config=config)


This exception should have information about the infeasible constraint in its message. That should be preserved and added to the NoSolutionError / NoOptimalSolutionError / NoFeasibleSolutionError exceptions

I implemented this. Let me know what you think.

jsiirola · 2026-04-27T23:23:12Z

+    ) -> Mapping[VarData, float]:
+        raise NoSolutionError()
+
+    def load_import_suffixes(self):


Is this is redundant? If the model has dual / rc Suffixes, then the base class implementation will call get_duals() / get_reduced_costs(), which will raise the exception. If those suffixes aren't defined, then nothing will happen ... but that might be consistent with the behavior implied by the base class implementation?

jsiirola · 2026-04-28T20:54:24Z

            }
        return GurobiDirectBase._tc_map

+    def _get_infeasible_results(self, config):


Should this be promoted to a standard method (in results / util) that is specialized here?

michaelbynum · 2026-05-03T18:44:05Z

I still have one comment left to address.
There were several failing tests that I had to fix. I think there is still one more. These were real failures. There was a spot in the highs interface where I was using the wrong attribute to determine if there was a feasible solution or not. It works in most cases, but not all. I think I have that fixed. There was also an edge case in the gurobi persistent interface where a trivially infeasible constraint did not get caught if it started as a trivially feasible constraint and changed because of a mutable parameter or fixed variable value.

michaelbynum · 2026-05-03T18:53:34Z

Tests are not running. I'm going to mark this as ready and see if that helps.

michaelbynum · 2026-05-04T01:48:28Z

-                    skip_trivial_constraints
-                    and (lb is None or lb <= offset)
+                    # skip_trivial_constraints
+                    # and (lb is None or lb <= offset)
+                    (lb is None or lb <= offset)


Not skipping trivially feasible constraints does not seem to work. When I create a trivially feasible constraint and do not skip them, I get exceptions (https://github.com/Pyomo/pyomo/actions/runs/25291155719/job/74143137773). I checked codecov, and this bit of code is not covered on the main branch. I think we should just always skip trivially feasible constraints in the gams writer. Thoughts? If everyone agrees, I can deprecate the option.

michaelbynum · 2026-05-04T12:18:05Z

The 13 uncovered lines in gams.py are not related to this PR. I just indented that code inside of a try/except block.

michaelbynum added 3 commits August 13, 2025 18:00

adding tests for trivial constraints and fixing bugs

d16bee5

Merge branch 'solver_api' into trivial_constraints

2001d15

run black

d25e721

eminyouskn reviewed Aug 14, 2025

View reviewed changes

michaelbynum added 2 commits August 16, 2025 09:34

Merge branch 'solver_api' into trivial_constraints

02f383d

Merge branch 'solver_api' into trivial_constraints

3d302a1

eminyouskn reviewed Aug 17, 2025

View reviewed changes

michaelbynum added 2 commits August 18, 2025 09:15

Merge branch 'solver_api' into trivial_constraints

ecd602d

merge solver_api into trivial_constraints

92e77ba

jsiirola added this to Pyomo 6.9.5 Release Oct 7, 2025

github-project-automation Bot moved this to Todo in Pyomo 6.9.5 Release Oct 7, 2025

blnicho removed this from Pyomo 6.9.5 Release Oct 17, 2025

pyomo-autotest added the AT: STALE label Oct 26, 2025

blnicho added this to Pyomo 6.10 Nov 11, 2025

github-project-automation Bot moved this to Todo in Pyomo 6.10 Nov 11, 2025

merge main

045f537

pyomo-autotest removed the AT: STALE label Dec 12, 2025

michaelbynum added 2 commits December 18, 2025 09:33

Merge branch 'solver_api' into trivial_constraints

253bdd9

Merge branch 'solver_api' into trivial_constraints

c3ad9b5

blnicho removed this from Pyomo 6.10 Feb 4, 2026

michaelbynum added 4 commits February 13, 2026 10:24

Merge branch 'solver_api' into trivial_constraints

ef6dcf6

Merge branch 'solver_api' into trivial_constraints

fb38368

merge solver_api into trivial_constraints

0365ae5

Merge branch 'solver_api' into trivial_constraints

7293c29

michaelbynum mentioned this pull request Mar 24, 2026

Writers and solver interfaces should (could?) ignore trivially satisfied constraints #2485

Open

michaelbynum added 3 commits March 26, 2026 15:45

Merge branch 'solver_api' into trivial_constraints

75fee85

Merge branch 'solver_api' into trivial_constraints

29a72f7

Merge branch 'solver_api' into trivial_constraints

c216e19

michaelbynum mentioned this pull request Apr 17, 2026

Rework SolutionLoader #3701

Merged

merge main

b754a29

michaelbynum changed the title ~~[Depends on #3701] Add tests for trivial constraints in pyomo/contrib/solver~~ Add tests for trivial constraints in pyomo/contrib/solver Apr 27, 2026

run black

ae7d2d9

jsiirola requested changes Apr 28, 2026

View reviewed changes

blnicho marked this pull request as draft April 30, 2026 13:01

michaelbynum added 5 commits May 1, 2026 10:01

Merge remote-tracking branch 'origin/main' into trivial_constraints

87a0f75

remove redundant code and propagate error messages

d0c0515

fix highs interface

bf6f6fd

better handling of trivially feasible and infeasible constraints

a1ed8e0

run black

29b9bf3

michaelbynum marked this pull request as ready for review May 3, 2026 18:53

michaelbynum added 3 commits May 3, 2026 15:20

better handling of trivially infeasible constraints

eb7abb6

run black

c987a9f

better handling of trivially infeasible constraints

fe00fdd

michaelbynum commented May 4, 2026

View reviewed changes

blnicho requested a review from jsiirola May 4, 2026 19:49

		return self.var_map[self.v2id]


		class GurobiDirectQuadratic(GurobiDirectBase):

		self._solver_model.setObjective(gurobi_expr + repn_constant, sense=sense)


		class _GurobiObserver(Observer):

		except InfeasibleConstraintException:
		res = self._get_infeasible_results(config=config)

Conversation

michaelbynum commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary/Motivation:

Legal Acknowledgement

Uh oh!

codecov Bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsiirola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelbynum commented May 3, 2026

Uh oh!

michaelbynum commented May 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelbynum commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michaelbynum commented Aug 14, 2025 •

edited

Loading

codecov Bot commented Aug 14, 2025 •

edited

Loading