add an outer_bound_only option to solve_one#802
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #802 +/- ##
==========================================
- Coverage 76.19% 76.18% -0.02%
==========================================
Files 169 169
Lines 22224 22268 +44
==========================================
+ Hits 16934 16965 +31
- Misses 5290 5303 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thread the new solve_one outer_bound_only option through solve_loop (SPOpt and PHBase) and switch the Lagrangian spokes onto it, so their solves skip loading a solution they never look at. Whether a solution is needed is a property of the spoke rather than of each call, so it is an `outer_bound_only` attribute on _LagrangianMixin rather than an argument on lagrangian(). Three cases opt out: - ReducedCostsSpoke reads x and the rc suffix off the solved subproblems (class attribute). - subgradient_while_waiting makes Compute_Xbar read the x values left behind by the previous solve (set in LagrangianOuterBound.main). - lagranger, until the first nonants arrive from the hub: it computes xbar from its own x until it has the hub's. Also in solve_one's outer_bound_only branch: mark solution_available False (no solution was loaded, so the Vars hold pre-solve values, and the staleness check and PRIOR_SOLUTION warmstart must not treat them as a solution), re-raise a solver exception instead of letting it surface as an AttributeError on the missing results object, and fix the assert, which asserted a tuple and so could never fire. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two bugs, both caught by CI on the previous commit. Gate the outer_bound_only bound extraction behind the existing not_good_enough_results check instead of running ahead of it. Without prox, a Lagrangian subproblem is routinely unbounded, and the results object then carries no usable bound: gurobi leaves Lower_bound None (so Ebound died on prob * None) while other solvers return garbage, which was published as a valid outer bound -- test_cvar_lagrangian_outer_bound saw 1.9e9 against a true optimum of -220700. A bad solve now takes the path it took before this option existed: gripe, no solution, and outer_bound left at its previous value, which is still a valid outer bound since any Lagrangian dual value bounds the original problem. The remaining None check can only fire on an otherwise usable solve. Pass solve_loop's arguments by keyword in cgbase. It called super() positionally, so inserting outer_bound_only ahead of warmstart in the signature slid warmstart=True into the outer_bound_only slot, tripping the assert. cgbase was the only positional caller. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an outer_bound_only option to subproblem solves so callers (notably Lagrangian outer-bound spokes) can compute bounds without paying the cost of loading solutions back into Pyomo models.
Changes:
- Add
outer_bound_onlyparameter toSPOpt.solve_one/SPOpt.solve_loopandPHBase.solve_loop, including documentation and forwarding. - Implement a bound-only solve path in
SPOpt.solve_onethat setssolution_available=Falseand populatesouter_boundwithout loading variable values. - Update Lagrangian-based spokes/bounders to default to bound-only solves, with opt-ins to load solutions when required (e.g., ReducedCostsSpoke, subgradient-while-waiting behaviors).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mpisppy/spopt.py | Adds outer_bound_only to single/loop subproblem solve APIs and implements bound-only result handling. |
| mpisppy/phbase.py | Threads outer_bound_only through PHBase’s solve loop API to the underlying solver loop. |
| mpisppy/cylinders/reduced_costs_spoke.py | Ensures reduced-cost spoke continues to load solutions by setting outer_bound_only=False and simplifying overridden methods. |
| mpisppy/cylinders/lagrangian_bounder.py | Defaults Lagrangian spokes to bound-only solves and flips to solution-loading when needed (e.g., subgradient while waiting). |
| mpisppy/cylinders/lagranger_bounder.py | Dynamically toggles outer_bound_only based on whether nonants have arrived (to support xbar computation needs). |
| mpisppy/cgbase.py | Refactors the super().solve_loop(...) call to use keyword arguments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
521
to
+529
| super().solve_loop( | ||
| solver_options, | ||
| dtiming, | ||
| gripe, | ||
| disable_pyomo_signal_handling, | ||
| tee, | ||
| verbose, | ||
| need_solution, | ||
| warmstart, | ||
| solver_options=solver_options, | ||
| dtiming=dtiming, | ||
| gripe=gripe, | ||
| disable_pyomo_signal_handling=disable_pyomo_signal_handling, | ||
| tee=tee, | ||
| verbose=verbose, | ||
| need_solution=need_solution, | ||
| warmstart=warmstart, |
not_good_enough_results asks whether there is a usable *solution*, which is the wrong question for a bound-only solve: a subproblem stopped by a time limit or a gap can carry no incumbent at all and still report the outer bound the caller is there for. Gating the bound-only path behind it threw those bounds away. So test outer_bound_only first, with its own gate. no_outer_bound_results is the bound-shaped analog of not_good_enough_results: same disqualifying terminations (infeasible, unbounded, error), minus the two clauses about solution presence and solution status. When it does disqualify a solve there is no meaningful bound to report -- solvers variously return None, a sentinel, or garbage -- so outer_bound keeps its previous value, which is still a valid outer bound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The helper rules on the routine, condition-detectable outcomes; the try block catches what it cannot foresee (an empty results.Problem, say) and re-raises with the scenario and status attached. Raising is right there: no_outer_bound_results has already accounted for the bound-less solves, so anything still throwing is a surprise rather than a routine outcome. Note this try block is not what catches an unbounded subproblem. Pyomo assigns None (gurobi) or the last iterate's objective (cplex) to Problem[0].Lower_bound rather than raising, so nothing propagates out of the attribute access; the termination condition is the only signal that survives both plugins, and the None check backstops the rest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Raise a ValueError rather than assert on outer_bound_only with need_solution. python -O strips asserts, and this guard earns its keep precisely there: it is what caught cgbase passing warmstart into the outer_bound_only slot, whose silent form is bound-only solves in code that builds columns from the solutions. Verified it now fires under both python and python -O. This matches the convention already spelled out in reduced_costs_spoke.py. Accept and forward outer_bound_only in CGBase.solve_loop, which had drifted from the base signature -- that drift is what made the positional call break in the first place. Column generation needs the solutions, so the option is not useful there; it is accepted to keep the override honest, and a caller who asks for it raises out of solve_one because need_solution defaults True. xhat_eval is deliberately left alone: its signature is a different shape and it defaults need_solution=False, so the guard would not catch misuse there. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xpress reports a MIP that ran out of time with no incumbent as status=aborted, TerminationCondition=error (mip_no_sol_found in xpress_direct.py), and then hands over xprob_attrs.bestbound anyway -- a perfectly good outer bound. Screening on error threw it away and the Lagrangian spoke died in Ebound with no bound to report, which is the case outer_bound_only exists to serve. Screen as little as possible here: a bound from a solve that went badly is the whole point of asking for a bound only. Infeasible and unbounded stay screened, because there the danger is not a missing value but a misleading one -- cplex fills in the last iterate's objective for an unbounded LP, a plausible finite number that is not a bound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Only solve_one ever wrote outer_bound, so until it did there was nothing to read, and Ebound died with "'ScalarBlock' object has no attribute 'outer_bound'" whenever a first solve reported no bound. Give every subproblem the bound it actually holds before its first solve: -inf minimizing, +inf maximizing. That is what is known at that point, it keeps Ebound arithmetic, and being the worst bound there is it never wins a comparison at the hub. This predates outer_bound_only -- the old not_good_enough_results path left the attribute unset the same way. cgbase reads outer_bound as a reduced cost and tested hasattr to decide whether one existed, which is now always true, so it tests the value instead: it sums these into sum_redcosts, where an infinity would quietly poison the total rather than fail loudly as None did. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bknueven
reviewed
Jul 15, 2026
Comment on lines
+593
to
+596
| for k,s in self.local_scenarios.items(): | ||
| s._mpisppy_data.outer_bound = -np.inf if self.is_minimizing else np.inf | ||
| s._mpisppy_data.inner_bound = np.inf if self.is_minimizing else -np.inf | ||
|
|
Collaborator
There was a problem hiding this comment.
Should we set these to None instead?
Collaborator
Author
There was a problem hiding this comment.
I agree, but it will take a little work to propagate the meaning of None, which will be done.
Collaborator
Author
There was a problem hiding this comment.
Agreed — done in 67db658. _set_initial_bounds now seeds None ("not computed"), not a vacuous ±inf. Ebound returns None if any scenario is still missing its outer bound (the check is Allreduce'd so all ranks agree before the sum), and the consumers treat None as "no bound to offer": the Lagrangian/Lagranger spokes skip send_bound, PHBase.Iter0 and Subgradient keep the prior best bound, post_solve_bound prints "not available", and the cross-scenario cut extension folds None to the weakest bound for its max/min aggregation (the one place a number is genuinely required). Added solver-free coverage in test_outer_bound_only.py for the Ebound-is-None path.
Comment on lines
202
to
206
| disable_pyomo_signal_handling=False, | ||
| update_objective=True, | ||
| need_solution=True, | ||
| outer_bound_only=False, | ||
| warmstart=sputils.WarmstartStatus.FALSE, |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 8044274: `outer_bound_only` is now keyword-only (moved after `warmstart`, behind a `*`) in `solve_one` and all three `solve_loop` overrides, and `warmstart` is back in its original positional slot, so a positional argument can no longer land on `outer_bound_only`. A signature test in `test_outer_bound_only.py` pins this down.
Comment on lines
+234
to
+238
| outer_bound_only (boolean, optional): | ||
| If True, populates outer_bound *only* and raises an exception | ||
| if the bound is not available. No solution is loaded, so | ||
| need_solution must be False. | ||
| Default False |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 8044274: the docstrings now say the previous outer bound is kept (a valid, if vacuous, bound) when a bound-only solve reports no new bound, matching the code — it does not raise merely because no bound was obtained.
| print ("status=", results.solver.status) | ||
| print ("TerminationCondition=", | ||
| results.solver.termination_condition) | ||
| raise e |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 8044274: changed to a bare `raise` so the original traceback is preserved.
Comment on lines
+344
to
+349
| if outer_bound_only: | ||
| # No solution is loaded, so the Vars still hold whatever they | ||
| # held before this solve; say so, or the staleness check and a | ||
| # PRIOR_SOLUTION warmstart would read them as a solution. | ||
| s._mpisppy_data.solution_available = False | ||
|
|
Collaborator
Author
There was a problem hiding this comment.
Added in 8044274: `mpisppy/tests/test_outer_bound_only.py` covers the bound-only path (bound populated, `solution_available` forced False), the `need_solution`/`outer_bound_only` guard, the normal solve, and the keyword-only contract. Follow-up 67db658 adds solver-free coverage of the not-computed (None) bound path through `Ebound`. Wired into run_coverage.bash and test_pr_and_main.yml.
…aceback, tests - Make outer_bound_only keyword-only and restore warmstart to its original positional slot in solve_one and the three solve_loop overrides, so existing positional calls can't land on it. - Correct the docstrings: a bound-only solve that reports no bound keeps the previous (possibly vacuous) outer bound rather than raising. - Use a bare raise when an unexpected bound-extraction error surfaces so the original traceback is preserved. - Add mpisppy/tests/test_outer_bound_only.py covering the bound-only path, the need_solution guard, the normal solve, and the keyword-only contract; wire it into run_coverage.bash and test_pr_and_main.yml. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses bknueven's review: a subproblem that has not been solved (or whose bound-only solve reported no bound) has not *computed* a bound, so it should hold None rather than a vacuous +/-inf dressed up as a real number. - _set_initial_bounds: seed outer_bound/inner_bound with None. - Ebound: return None if any scenario (collectively, across ranks) is missing its outer bound, since the expectation cannot then be formed; the missing check is Allreduce'd so every rank agrees before the sum reduction. - Teach the consumers to treat None as 'no bound to offer': the Lagrangian and Lagranger spokes skip send_bound; PHBase.Iter0 and Subgradient keep the prior best bound; post_solve_bound prints 'not available'; the cross-scenario cut extension folds None to the weakest bound for its max/min aggregation. - Extend test_outer_bound_only with solver-free coverage of the None path: initial bounds are None, Ebound is None when any bound is missing and the weighted sum when all are present, and is a finite number after a successful bound-only solve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.