Skip to content

refactor(solver): validation, sanitize kwargs, and result wiring on Solver path#691

Merged
FabianHofmann merged 9 commits into
refactor/sos-reformulation-methodsfrom
refactor/solver-from-model-options
May 18, 2026
Merged

refactor(solver): validation, sanitize kwargs, and result wiring on Solver path#691
FabianHofmann merged 9 commits into
refactor/sos-reformulation-methodsfrom
refactor/solver-from-model-options

Conversation

@FBumann
Copy link
Copy Markdown
Collaborator

@FBumann FBumann commented May 18, 2026

Stacked on #690. Closes the gap between Model.solve() and Solver.from_name(...).solve().

Core principle

Solver.from_model() does not mutate the model. Model-state transformations are Model-level primitives, called explicitly:

model.constraints.sanitize_zeros()        # drop zero-coefficient terms
model.constraints.sanitize_infinities()   # drop trivial <= +inf / >= -inf rows
model.apply_sos_reformulation()           # rewrite SOS as binary + linear (#690)
model.undo_sos_reformulation()            # restore the original SOS form (#690)

Model.solve() calls these internally as orchestration. The low-level Solver path expects the caller to apply them — no silent mutation on build.

Canonical low-level pattern:

model.constraints.sanitize_zeros()   # optional preprocessing
solver = Solver.from_name("highs", model, io_api="direct")
result = solver.solve()
model.assign_result(result, solver=solver)

What lives where after this PR

Port? = should it move to Solver. Status = ✅ done · ⬜ todo · — n/a.

Item Port? Status Where (on Solver or why not)
Quadratic / semi-continuous feature gates yes Solver._validate_model() (build-time, called from _build())
SOS feature gate yes ✅ (#690) Solver._validate_model() (build-time)
Empty-objective check yes Solver.solve() (run-time); scoped dup in Model.solve()'s remote branch, removed by #683
Result wiring (model.solver + primal/dual/status) yes Model.assign_result(result, solver=...) (post-solve)
Build-time kwargs: io_api, explicit_coordinate_names, set_names, problem_fn, slice_size, progress, **solver_options yes Solver.from_model(...)
Run-time kwargs: basis_fn, warmstart_fn yes Solver.solve(...)
env, log_fn yes accepted on both from_model() and solve() today; PR3 consolidates to from_model() only (13 subclasses)
sanitize_zeros / sanitize_infinities no Model-level: model.constraints.sanitize_*(). Mutates — caller-owned.
SOS apply/undo lifecycle no Model-level: model.apply_sos_reformulation() / undo_sos_reformulation() (#690). Mutates — caller-owned.
reformulate_sos (True/False/"auto") no Model.solve()-side convenience wrapping model.apply_sos_reformulation(). Solver-path users call apply/undo directly.
OETC remote (remote=OetcHandler(...)) yes ⬜ (#683) refactor into a Solver subclass
Generic remote=RemoteHandler(...) ? could follow #683
mock_solve=True ? could become a Mock Solver subclass
"Any solver installed" raise ? Solver.__post_init__ covers "this-installed"; "any-installed" is module-level
solver_name auto-pick, default solution_fn, keep_files cleanup, info logs, reset_solution() no orchestration: belongs above the Solver layer

Docs follow-up. Keeping sanitize off the Solver kwargs makes it less discoverable for users coming from Model.solve(). The user guide / example notebooks should surface the model-level primitives — separate doc PR #685, not in scope here.

FBumann and others added 6 commits May 18, 2026 16:57
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>
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>
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>
Same property tested twice — no need for separate test IDs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
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>
@FBumann FBumann added this to the v0.8.0 milestone 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>
…r-from-model-options

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

@FabianHofmann FabianHofmann left a comment

Choose a reason for hiding this comment

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

this is extremely sensible! I did not think about model mutations in the solver refactor pr. thanks for addressing this. let me know when this is ready to review

Comment thread linopy/solvers.py
Comment on lines +601 to +603
Passing ``solver=`` to :meth:`Model.assign_result` wires
``model.solver`` so post-solve helpers like
:meth:`Model.compute_infeasibilities` keep working.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

very good catch!

Comment thread test/test_solvers.py Outdated
Comment on lines +472 to +474
@pytest.mark.skipif(
"highs" not in solvers.available_solvers, reason="HiGHS not installed"
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we definitely need a helper function that makes these skips snappy

Comment thread linopy/solvers.py
@FBumann
Copy link
Copy Markdown
Collaborator Author

FBumann commented May 18, 2026

this is extremely sensible! I did not think about model mutations in the solver refactor pr. thanks for addressing this. let me know when this is ready to review

It is ready, but i think its less about correctness, but more about the conceptual split between Model and Solver, and the ergonomics for users using the new Solver class

@FabianHofmann FabianHofmann merged commit 68d75cd into refactor/sos-reformulation-methods May 18, 2026
2 checks passed
@FabianHofmann FabianHofmann deleted the refactor/solver-from-model-options branch May 18, 2026 18:53
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