Skip to content

refactor(sos): add Model.apply/undo_sos_reformulation methods#690

Open
FBumann wants to merge 5 commits into
masterfrom
refactor/sos-reformulation-methods
Open

refactor(sos): add Model.apply/undo_sos_reformulation methods#690
FBumann wants to merge 5 commits into
masterfrom
refactor/sos-reformulation-methods

Conversation

@FBumann
Copy link
Copy Markdown
Collaborator

@FBumann FBumann commented May 18, 2026

Summary

On top of the following this PR includes changes from #691

Stateful pair of methods on Model that own the SOS reformulation lifecycle. Model.solve(reformulate_sos=...) delegates to them; same external behavior.

  • Model.apply_sos_reformulation() — stashes the token on _sos_reformulation_state. Raises RuntimeError if already applied.
  • Model.undo_sos_reformulation() — restores the original SOS form. No-op if nothing is applied.
  • Solver._build() — raises ValueError when an SOS-bearing model meets a solver without SolverFeature.SOS_CONSTRAINTS, so the low-level Solver.from_model(...).solve() path is safe without going through Model.solve().

Persistence:

  • Model.copy() / copy.copy / copy.deepcopy deep-copy the token; the copy is independently undoable.
  • to_netcdf() raises if reformulation is active — undo first.

Why the lifecycle lives on Model, not on Solver

I considered three alternatives and rejected all of them:

On Solver.from_model() with undo on solver.close(). Tempting because Solver already manages env lifecycle. Rejected: reformulation is a model transformation, and the Solver only queries a feature flag — putting the lifecycle there makes Solver responsible for work it doesn't perform.

Build-time reformulation inside _build_*, no model mutation. Cleanest in theory. Rejected because the mutation is a feature: users can print the reformulated MILP, export it to LP/MPS, or write additional constraints referencing the aux binaries. Hiding it inside the build step strips all of that.

Standalone context manager. Rejected: it only handles the transient (auto-undo) case. The method pair supports transient, permanent, and decide-later workflows; a 3-line context manager over the methods can be added later if wanted.

What's left on Solver: one feature-flag check in _build(). Layering:

Layer Role
sos_reformulation.py implementation primitives
Model owns transformation state — apply / undo / inspect
Solver declares feature support; raises on incompatible state
Model.solve() bracketing for the one-shot ergonomic

Context

Same thread as #688. While reviewing the two-step Solver.from_model().solve() API from #682, I noticed Model.solve() was doing orchestration the Solver path couldn't reproduce. This PR addresses the SOS slice; validation / sanitization lifting can follow separately.

Test plan

  • pytest test/test_sos_reformulation.py — 69 tests, 9 new (apply/undo primitives, copy across all three protocols, netcdf guard, Solver-path raise).
  • pytest test/test_sos_constraints.py test/test_optimization.py test/test_io.py — 1544 pass.
  • ruff check, ruff format, mypy linopy/{model,solvers,io}.py clean.

🤖 Generated with Claude Code

Introduce a stateful pair of methods on Model that own the SOS
reformulation lifecycle:

- apply_sos_reformulation() stashes the reformulation token on the
  model (new _sos_reformulation_state attribute). Raises if already
  applied.
- undo_sos_reformulation() reads the stashed token and restores the
  original SOS form. No-op if nothing is applied.

Model.solve(reformulate_sos=...) now delegates to these methods rather
than threading the token through local state. The Solver path (which
was previously raising via Model.solve's pre-flight check) now gets a
clean ValueError directly from Solver._build() when an SOS-bearing
model is handed to a solver without native SOS support — making the
low-level API safe to use independently of Model.solve.

Persistence:
- copy() (and copy.copy / copy.deepcopy) carry the reformulation token
  with a deepcopy, so the copy is independently undoable.
- to_netcdf() raises if a reformulation is active; users must undo
  first to serialize a stable model state.

Context: motivated by the same investigation as #688 —
while reviewing the new Solver.from_model() API surface introduced by
#682, the SOS reformulation lifecycle stood out as load-bearing
orchestration that the Solver path couldn't reproduce.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@FBumann
Copy link
Copy Markdown
Collaborator Author

FBumann commented May 18, 2026

@FabianHofmann This is probably the heavy lifting of #688

@FBumann FBumann added this to the v0.8.0 milestone May 18, 2026
FBumann added a commit that referenced this pull request May 18, 2026
Remove the sanitize_zeros / sanitize_infinities kwargs from
Solver.from_model(). The Solver builder now never mutates the model.
Sanitization is exposed where it has always lived —
model.constraints.sanitize_zeros() / .sanitize_infinities() — and
Model.solve() calls them inline as part of its orchestration.

Rationale: model-state transformations should be Model-level primitives
(matches the SOS reformulation pattern from #690). The Solver's job is
to translate the model and run; it should not silently change the
caller's model on the way in. Users who go through the lower-level
Solver path apply sanitize explicitly when they want it.

Replaces TestSanitizeKwargs with TestSolverDoesNotMutateModel, pinning
the mutation-free invariant: building a Solver against a model with a
near-zero coefficient leaves model.constraints["c"].coeffs unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@FBumann FBumann marked this pull request as ready for review May 18, 2026 15:53
@FBumann
Copy link
Copy Markdown
Collaborator Author

FBumann commented May 18, 2026

@FabianHofmann Look at the reasoning in #691, where the guideline "Mutating the Model is done deliberately before using a Solver" also affects sanitization etc

…ulation-methods

# Conflicts:
#	linopy/model.py
#	linopy/solvers.py
@FabianHofmann
Copy link
Copy Markdown
Collaborator

yes, makes sense too. should we merge into #691 since they both go in the same direction? would make it easier for reviewing I'd say

@FBumann
Copy link
Copy Markdown
Collaborator Author

FBumann commented May 18, 2026

@FabianHofmann #691 is stacked on top anyway. So we can just clos this and point the other to master. But i thought this deserves its own deliberate PR with description and reasoning etc.
But feel free to do the review in #691.
Id like to merge them sequentially into master though.

@FabianHofmann
Copy link
Copy Markdown
Collaborator

@FabianHofmann #691 is stacked on top anyway. So we can just clos this and point the other to master. But i thought this deserves its own deliberate PR with description and reasoning etc. But feel free to do the review in #691. Id like to merge them sequentially into master though.

was confused with the references. that makes sense. I am reviewing #691 quickly and if you don't mind we merge #691 to here and I have another look at the combined diff. would that be fine?

@FBumann
Copy link
Copy Markdown
Collaborator Author

FBumann commented May 18, 2026

I would rather merge them both into master sequentially, so we keep the PR's in master. But Its not really important to be honest. Do as you please

…olver path (#691)

* refactor(solver): lift feature checks + sanitize/wiring to Solver path

Make Solver.from_name(...).solve() a real first-class entry point that
doesn't lose Model.solve()'s safety nets:

- Lift solver-feature gates into Solver._build() via a new
  _validate_model() hook: quadratic models against LP-only solvers and
  semi-continuous variables against solvers that don't support them.
  Removed the duplicate checks from Model.solve().
- Add sanitize_zeros / sanitize_infinities kwargs to Solver.from_model()
  (default True). The kwargs are processed in _build() before dispatch,
  so both file and direct io_apis honor them. Model.solve() forwards the
  kwargs through instead of pre-mutating the constraints itself.
- Extend Model.assign_result(result, solver=None) so the Solver-path
  canonical pattern works: solver = Solver.from_name(...); result =
  solver.solve(); model.assign_result(result, solver=solver). When the
  solver kwarg is provided, model.solver gets wired the same way
  Model.solve() wires it, so compute_infeasibilities() and friends keep
  working through the low-level API.

The empty-objective check stays on Model.solve() — to_gurobipy() /
to_highspy() and similar build-only converters legitimately work
against objectiveless models (gurobi/highs default to a zero
objective), so the check belongs at the actual submit point.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* move empty-objective check to Solver.solve() for entry-point parity

The empty-objective UX guardrail was previously only on Model.solve(),
leaving the lower-level Solver.from_name(...).solve() path with a
silent gap. Move it to Solver.solve() — the actual submit primitive
that both entry points go through — so the same check fires regardless
of which API the user reaches for.

Build-time translate-only paths (to_gurobipy(), to_highspy(),
to_file()) are unaffected since they don't call solve(). The cost of
catching the error after build instead of before is bounded and only
hits a programming-error case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test: parametrize empty-objective check across both entry points

Consolidate the Model.solve() and Solver.from_name(...).solve() tests
into one parametrized case — same check, two callers, one assertion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test: collapse parametrize to a single test with two raises blocks

Same property tested twice — no need for separate test IDs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* preserve empty-objective check for remote-solve path in Model.solve()

The remote-solve branch in Model.solve() short-circuits to a
RemoteHandler before reaching Solver.solve(), so the check now in
Solver.solve() doesn't cover it. Restore the early raise in
Model.solve() so behavior is unchanged for all Model.solve() callers
(mock, remote, local) while Solver.solve() still covers direct-Solver
callers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* move remote-path empty-objective check inside the remote branch

The early-position check was a workaround: the remote branch
short-circuits before Solver.solve() (where the canonical check now
lives), so empty-objective with remote=... wouldn't raise. Moving it
into the remote branch itself makes the intent local to where it's
needed, with a comment pointing at #683 where this duplication
disappears once OETC becomes a Solver subclass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* keep sanitize on Model; Solver.from_model() stays mutation-free

Remove the sanitize_zeros / sanitize_infinities kwargs from
Solver.from_model(). The Solver builder now never mutates the model.
Sanitization is exposed where it has always lived —
model.constraints.sanitize_zeros() / .sanitize_infinities() — and
Model.solve() calls them inline as part of its orchestration.

Rationale: model-state transformations should be Model-level primitives
(matches the SOS reformulation pattern from #690). The Solver's job is
to translate the model and run; it should not silently change the
caller's model on the way in. Users who go through the lower-level
Solver path apply sanitize explicitly when they want it.

Replaces TestSanitizeKwargs with TestSolverDoesNotMutateModel, pinning
the mutation-free invariant: building a Solver against a model with a
near-zero coefficient leaves model.constraints["c"].coeffs unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* address review: SOS hint, lp_only_solver fixture, assign_result doc

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Fabian <fab.hof@gmx.de>
@FabianHofmann
Copy link
Copy Markdown
Collaborator

Code review

Found 1 issue:

  1. Model.solve(reformulate_sos=True|"auto") leaks _sos_reformulation_state if Solver.from_name() or solver.solve() raises. The undo lives in a finally around assign_result() (lines 1830–1832), but from_name and solve are wrapped by a separate try/finally whose only job is file cleanup (lines 1788–1825). On any build/solve failure the second try is never reached, the model stays in reformulated form, and the next Model.solve() call hits RuntimeError: SOS reformulation has already been applied from apply_sos_reformulation (lines 1243–1245). Suggest a single try/finally wrapping both blocks so the auto-undo contract documented on apply_sos_reformulation actually holds for the transient case.

linopy/linopy/model.py

Lines 1786 to 1833 in 68d75cd

self.constraints.sanitize_infinities()
try:
self.solver = None # closes any previous solver
if io_api == "direct":
if set_names is None:
set_names = self.set_names_in_solver_io
build_kwargs: dict[str, Any] = {
"explicit_coordinate_names": explicit_coordinate_names,
"set_names": set_names,
"log_fn": to_path(log_fn),
}
if env is not None:
build_kwargs["env"] = env
else:
build_kwargs = {
"explicit_coordinate_names": explicit_coordinate_names,
"slice_size": slice_size,
"progress": progress,
"problem_fn": to_path(problem_fn),
}
self.solver = solver = solvers.Solver.from_name(
solver_name,
model=self,
io_api=io_api,
options=solver_options,
**build_kwargs,
)
if io_api != "direct":
problem_fn = solver._problem_fn
result = solver.solve(
solution_fn=to_path(solution_fn),
log_fn=to_path(log_fn),
warmstart_fn=to_path(warmstart_fn),
basis_fn=to_path(basis_fn),
env=env,
)
finally:
for fn in (problem_fn, solution_fn):
if fn is not None and (os.path.exists(fn) and not keep_files):
os.remove(fn)
try:
return self.assign_result(result)
finally:
if applied_sos_reformulation_here:
self.undo_sos_reformulation()

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

- undo_sos_reformulation() now raises if no state is applied (fail-fast)
- to_netcdf error no longer suggests poking the private state slot
- Solver._build runs _validate_model before _check_sos_unmasked so SOS on an
  LP-only solver surfaces the reformulate-first hint
- reformulate_sos_constraints docstring points at the stateful API
Comment thread linopy/io.py
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I thought someone might need it to use sos reformulation with remote/oetc. Doesnt this go through netcdf?

`Model.solve(reformulate_sos=...)` left `_sos_reformulation_state` set if
`Solver.from_name`, `solver.solve`, or the file-cleanup `finally`
raised, since the undo lived in a second `try` around `assign_result`
that those failures never reached. The next solve then hit
`RuntimeError: SOS reformulation has already been applied`.

Wrap sanitize, build/solve, file cleanup, and assign_result in a single
outer try/finally so the undo always runs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

2 participants