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 61.36364% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.72%. Comparing base (81ec316) to head (ae7d2d9).

Files with missing lines	Patch %	Lines
...ontrib/solver/solvers/gurobi/gurobi_direct_base.py	57.14%	9 Missing ⚠️
pyomo/contrib/solver/common/solution_loader.py	60.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3703      +/-   ##
==========================================
- Coverage   89.96%   88.72%   -1.24%     
==========================================
  Files         903      903              
  Lines      106604   106646      +42     
==========================================
- Hits        95902    94620    -1282     
- Misses      10702    12026    +1324

Flag	Coverage Δ
builders	`29.21% <36.36%> (+<0.01%)`	⬆️
default	`86.27% <61.36%> (?)`
expensive	`35.66% <36.36%> (?)`
linux	`61.81% <36.36%> (-27.65%)`	⬇️
linux_other	`61.81% <36.36%> (-25.61%)`	⬇️
oldsolvers	`28.14% <36.36%> (+<0.01%)`	⬆️
osx	`?`
win	`?`
win_other	`?`

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

jsiirola · 2026-04-27T23:16:47Z

+    def __init__(self) -> None:
+        pass
+
+    def get_solution_ids(self) -> List[Any]:


This is redundant (covered by the base class)

jsiirola · 2026-04-27T23:18:17Z

+    def get_number_of_solutions(self) -> int:
+        return 0
+
+    def load_solution(self):


This is redundant (the base class will call get_vars(), which will raise the exception)

jsiirola · 2026-04-27T23:18:27Z

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


This is redundant (the base class will call get_vars(), which will raise the exception)

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:32:31Z

 from pyomo.common.errors import PyomoException
 from pyomo.common.shutdown import python_is_shutting_down
 from pyomo.common.timing import HierarchicalTimer
+from pyomo.common.errors import InfeasibleConstraintException


Why was this added? It appears to be unused.

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 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

		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!

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!

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