feat(persistent): in-place solver updates (Solver framework + HiGHS/Gurobi/Xpress/Mosek)#718
feat(persistent): in-place solver updates (Solver framework + HiGHS/Gurobi/Xpress/Mosek)#718FabianHofmann wants to merge 34 commits into
Conversation
Tracks per-Constraint coefficient mutation via a single boolean slot, flipped in coeffs/vars/lhs setters. Pure-constant rhs writes now short-circuit and leave coeffs/vars buffers untouched (by identity), so rhs-only updates don't trigger expensive coefficient recompare on the persistent-solver fast path.
Pure-Python snapshot primitives for the persistent-solver Phase 1. Deep-copies value-side fields (var_lb/ub, con_rhs/sign, obj_linear), holds vlabels/clabels by reference, stores canonical CSR (indptr, indices) per constraint container. No Solver import.
Pure-function diff for the persistent-solver Phase 1. Detects structural, coord, sparsity, quadratic-objective, value-only var/con, and objective-linear/sense changes. Supports same_model fast path via _coef_dirty and cross-model full re-scan. Includes a focused test suite covering capture, mutation paths, deep-copy invariant, and the same_model toggle.
- supports_persistent_update class flag (default False) - snapshot/_rebuilds/_in_place_updates/_last_rebuild_reason fields - snapshot capture at end of direct _build, _clear_coef_dirty helper - apply_update stub raising UnsupportedUpdate - solve(model, assign) dispatcher with diff-or-rebuild path - update(model, apply=True) primitive returning ModelDiff - threading.Lock around diff+apply+resnapshot - __getstate__/__setstate__ drop native handle and snapshot
…date support Skip diff computation entirely when supports_persistent_update is False on apply, per plan: 'dispatcher checks flag before calling — if False, skips diffing entirely and goes to rebuild.'
Replace xarray-based snapshot and CSR pattern compare with per-row canonicalised numpy buffers; new ContainerVarUpdate / ContainerRowUpdate payloads. Gurobi/HiGHS apply_update rewritten around batched setAttr / changeColsBounds / changeColsCost / changeColsIntegrality; coefficient writes touch only changed cells. Cross-model diff now ~matches same-model cost for bound/rhs/coef-value sweeps.
compute_diff/Solver.solve/Solver.update grow an ignore_dims kwarg. None (default) keeps the current no-coord-check behaviour; any iterable opts into per-container coord-equality on every dim not in the set, supporting rolling-horizon workflows where e.g. the snapshot dim is expected to drift.
…_rebuild - Solver.from_name now accepts model=None; the first solve(m, ...) builds. - compute_diff folded into ModelDiff.from_snapshot classmethod; new ModelDiff.from_models diffs two linopy models directly. - Solver.solve grows disallow_rebuild=True, which raises RebuildRequiredError instead of falling back to a rebuild.
…m_models - Add `track_updates` flag (default False) to Solver; skip ModelSnapshot capture when disabled. Raise UpdatesDisabledError on solve(model)/update() if a built solver was constructed without tracking. - Rewrite ModelDiff.from_models to build directly from two models without capturing snapshots; share helpers with from_snapshot. - Update persistent tests to opt into track_updates=True; add coverage for the disabled path.
Cross-instance resolves now diff via from_models against the previously built model, with no snapshot. Same-instance mutation still raises UpdatesDisabledError. Snapshot recapture is skipped in this mode. Add cross-instance solve/update tests for the no-snapshot path.
Collapse _diff_objective QUAD_OBJ branches; cache n_coef_updates; short-circuit _canonicalize_rows when rows already sorted; tighten buffer extraction. Introduce VarKind enum used across snapshot/diff and HiGHS/Gurobi apply_update; reuse linopy.constants sign tokens. Move _clear_coef_dirty into ModelSnapshot.capture.
Source con buffers from Constraint.to_matrix_with_rhs, replacing the dense (n_rows, max_n_term) arrays with CSR (indptr, indices, data). Sign dtype adopts 'U1' across the persistent layer and apply_update in HiGHS/Gurobi consumes CSR-slice payloads instead of -1 masks. Deletes _canonicalize_rows and the _INT64_MAX sentinel.
Replace per-container ContainerVarUpdate/ContainerRowUpdate dicts with flat arrays (var_bounds_*, var_type_*, con_coef_* COO, con_rhs_*, con_sign_*) plus VarSlice/ConSlice per-container offsets for diagnostics. Add con_rhs_as_bounds() for ranged-row solvers. Backend apply_update bodies collapse to flat-array calls; remove duplicated label->position resolution.
Implement in-place model updates for Xpress (chgbounds/chgrhs/chgmcoef/ chgrowtype/chgobj/chgobjsense/chgcoltype) and Mosek (chgvarbound/ chgconbound/putaijlist/putclist/putvartypelist/putobjsense). Mosek rejects constraint sign change to trigger rebuild. Consolidate gurobi/highs apply_update tests into a single parametrized file that also covers xpress and mosek.
for more information, see https://pre-commit.ci
|
@FBumann here we go, if you want to take a first look. docs to come |
Ill have a look ltoday |
* hold solver lock through _run_direct so two threads calling solve(model) on the same Solver no longer race on the native handle (HiGHS returned 0.0 from the second concurrent solve). * narrow Optional ndarrays in persistent.diff.push_var / push_con and in HiGHS/Gurobi/Xpress/Mosek apply_update objective paths. * widen Constraint.rhs setter to ExpressionLike | VariableLike | ConstantLike to match the as_expression call in the body. * widen Constraints.__getitem__(str) return type to Constraint (the dominant case) so tests can set .rhs/.coeffs/.sign without ignores. * add docs for in-place solver updates.
|
@FabianHofmann Sorry, I wont be able to review this today. |
take your time, there is no hurry. I'll do some integration tests anyway |
|
Are there any conflicts with #717? Are we sure we want to merge and publish this before we go v1? Just making sure... |
|
Benched PR #718 ModelDiff vs hand-rolled
Diff compute + Manually calling Suggestion: expose an opt-in on the persistent API so users with presolve-heavy LPs can drop the warm basis without bypassing the encapsulation. Either:
Zero structural rebuilds observed across all 7 re-solves in both ModelDiff routes ( Caveats:
|
|
I don't understand the intricacies of model snapshots yet, so can't comment on the weakref proposal. mark's comment is close to what i had in mind with a context manager for making internal modifications. i'd hope the user does not have to specify whether she is passing the same model. i like the .update proposal, especially for cases where multiple things are updated. I'd keep the getters and allow the user to make in place updates but only when using the context manager which could set a dirty mark. |
|
@coroa @FBumann @FabianHofmann The context-manager framing reconciles the A-vs-B split cleanly — the context manager is the consent signal, so we keep the fast path by default without trusting undocumented discipline. Sketch: with c.editing() as arr: # thaws .values, no dirty flag yet
arr.coeffs.values[mask] = ...
# __exit__: arr.flags.writeable = False; c._coef_dirty = True
Open edges to close (from the writeable-flag audit upthread):
If that shape works I can take the CM + freeze plumbing as a follow-up so #718 lands on the current API and the freeze arrives non-breaking. |
|
@MaykThewessen Im not sure if the complexity and instability of manipulating |
* feat: Variable.update() / Constraint.update() as canonical mutation API Introduces typed ``.update()`` methods on Variable and Constraint as the single, validated, multi-attribute mutation entry point. - ``Variable.update(lower=, upper=)``: validates non-constant inputs are rejected, new dims are rejected, and the resulting ``lower <= upper`` invariant holds across all coords. Returns ``self`` for chaining. - ``Constraint.update(rhs=, sign=)``: constant RHS only. The legacy ``c.rhs = variable`` rearrange-to-lhs path stays on the setter (different semantic, deserves its own explicit method). The existing ``.lower`` / ``.upper`` / ``.sign`` setters become thin shims that forward to ``.update()``, so single-attribute writes (``z.lower = 2``) stay ergonomic and the canonical validation runs in one place. The ``.rhs`` setter forwards constants through ``.update()`` and keeps the expression-rhs rearrange behaviour. This is the on-top experiment for the design discussion on #718. ``.lhs`` / ``.coeffs`` / ``.vars`` setters are untouched for now. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(update): Constraint.update accepts Variable/Expression rhs Mirrors the existing ``c.rhs = expr`` setter and ``add_constraints`` which both accept mixed-side input and rearrange the residual onto lhs. ``c.update(rhs=x + 5)`` now subtracts ``x`` from lhs and stores ``5`` on rhs. ``.rhs`` setter collapses to a one-line shim. Variable bound rejection of Variable/Expression is kept (bounds are numeric, not symbolic); docstring clarified to spell out that pandas / xarray / numpy arrays are first-class (time-varying bounds). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(update): extend Constraint.update to lhs/coeffs/vars; shim all setters Adds lhs / coeffs / vars to the canonical mutation API. All .lhs / .coeffs / .vars setters now forward to .update() — every Constraint mutation goes through one method with one validation path, one place that flips _coef_dirty. Composition rules: - lhs= replaces the whole expression first; subsequent rhs= rearrangement (Variable/Expression in rhs) sees the new lhs. - lhs= and coeffs= / vars= are mutually exclusive (whole replacement vs partial array update). - sign= is applied last so it composes cleanly. Internal Constraint.sanitize_zeros migrated to update(vars=, coeffs=) — no more internal setter calls in linopy/. 389 tests pass across mutation + persistent-solver suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(update): rename Constraint.update kwarg vars= -> variables= Avoids shadowing Python's vars() builtin. The .vars attribute on Constraint stays (it parallels the .data.vars internal name); only the kwarg gets the unambiguous spelling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(update): accept positional ConstraintLike in Constraint.update Mirrors add_constraints' dispatch: c.update(x + 5 <= 3) is now shorthand for c.update(lhs=x, sign='<=', rhs=-2), extracted from the AnonymousConstraint / ConstraintBase the comparison produces. Mutually exclusive with the per-attribute kwargs; clear error when mixed. Also reverts the internal sanitize_zeros migration. The setters are pure shims forwarding to update(), so the migration didn't change behaviour or cost — just spelling. The original setter syntax reads more naturally there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(update): note kwarg form is the targeted, cheap path The positional ConstraintLike form (c.update(x + 5 <= 3)) always rewrites lhs / sign / rhs and flips _coef_dirty. For hot loops that only touch one part, kwarg form (c.update(rhs=...)) skips the unchanged attributes and is materially cheaper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(persistent): default ModelDiff.from_snapshot(same_model=False) Closes the A1 residual from the #718 review. The flag-trust path (`skip_coef_compare = same_model and not coef_dirty`) is correct through Constraint.update() (set in one place, shims forward), but `c.coeffs.values[...] = ...` still bypasses _coef_dirty. With same_model=True as the default, that bypass silently produces wrong diffs. Flip the default to False. Cross-model paths (the only production caller, Solver._update_locked, passes explicitly) are unaffected. Same-model warm-update paths now value-diff the CSR data — small perf hit (50-200ms at Mayk-scale per Mayk's bench), correct by default. Solver-aware callers who own the mutation contract can opt back into the optimization with `same_model=True`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: teach .update() in tutorials; mark setters as syntactic sugar - examples/manipulating-models.ipynb: rewrite mutation cells to use Variable.update / Constraint.update; setter form is mentioned in notes as syntactic sugar for the same call. - examples/creating-constraints.ipynb: reframe the CSRConstraint vs Constraint API table around .update() as the mutation API; setters are sugar. - Setter docstrings now say 'syntactic sugar for Constraint/Variable .update; do not add logic here so the contract stays single-sourced' — a directive to future contributors as much as to readers. No deprecation, no breaking change. .update() is the documented canonical mutation API; the seven setters continue to exist as one-line shims. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * deprecate(update): warn on mutation setters; promote .update() in docs Adds DeprecationWarning to all seven mutation setters (Variable.lower, Variable.upper, Constraint.coeffs, Constraint.vars, Constraint.sign, Constraint.rhs, Constraint.lhs). Each setter still forwards to .update() so existing code keeps working; the warning points at the canonical API. Internal sanitize_zeros migrated off setters (the last linopy/ caller). api.rst gains Modification sections listing .update() for both Variable and Constraint; tutorial notes rewritten to teach .update() and flag setters as deprecated. Release note added. dual.setter / solution.setter untouched — result assignment, not mutation, different deprecation track. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(update): edge-case coverage; document rhs-rearrangement invariant Constraint.update tests: lhs-only, coeffs-only (vars preserved), compound lhs+sign, mutually-exclusive lhs+coeffs and lhs+variables. Variable.update tests: upper-only, valid array bound. Migrate test_constraint_coef_dirty.py from the now-deprecated setters to .update(), exercising the canonical path; add positional-form and no-op cases. Net effect: same dirty-flag invariants, 7 fewer warnings per pytest run. Docs: Constraint.update rhs= gains a worked example showing the two forms (constant vs variable/expression). add_constraints rhs gets a matching note pointing at the linopy invariant so the rearrangement rule is documented at the creation site too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review(update): address inline feedback on #727 - Constraint._assign_lhs_expr → _assign_lhs (drop redundant suffix; the method already takes a LinearExpression, so the type was in the signature, not the name). - Add Constraint._assign_data(**fields) helper. Wraps the four ``self._data = assign_multiindex_safe(self.data, **kw)`` callsites inside update() (rhs / coeffs / vars / sign). Untouched: the same pattern in dual.setter, sanitize_missings, sanitize_infinities — those aren't update() and stay out of scope here. - Add types.CONSTANT_TYPES tuple, derived from ConstantLike via get_args so the two cannot drift. Variable.update bound validation flipped from negative (isinstance against a hand-rolled non_constant tuple) to positive (isinstance against CONSTANT_TYPES); drops a redundant in-function ``from linopy import expressions`` (the module-level import already covered it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * deprecate(update): FutureWarning on DataArray as variables=; extract _validate_update Constraint - sanitize_zeros now writes _data via _assign_data directly (no longer round-trips through update(variables=DataArray), which would self-trigger the new deprecation warning). - Constraint.update(variables=...) emits FutureWarning when passed a raw DataArray of integer labels; Variable is the supported input. The path stays accepted for back-compat and will be removed alongside the v1 cleanup. Variable - Extract Variable._validate_update(*, lower, upper) — validates, broadcasts, and runs the cross-bound (lb<=ub) check, returning a dict ready for assignment. update() body shrinks to ~3 lines. Coord validation (parity with add_variables) deferred to #726 land. Tests - test_constraint_coef_dirty's variables= test now passes a Variable instead of a DataArray (matches the supported input). - test_constraint_vars_setter_with_array wrapped in pytest.warns(FutureWarning) — locks in the deprecation contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(constraints): _assign_data → _update_data; manage _coef_dirty inside The helper now writes fields AND flips _coef_dirty when the written set includes coeffs or vars. Callsites in update() (coeffs / vars branches) and sanitize_zeros drop their explicit `self._coef_dirty = True` lines — the rule lives in one place, can't be forgotten by future field additions. rhs / sign writes still don't dirty (correctly). _assign_lhs is untouched: it uses a different write mechanic (drop_vars + assign) and manages its own flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
|
Benchmarked it. Isolated the mutation step (no solve) — typed constraint setter (
The 2 M row / 12 M nnz line matches our production LP (1.14 M cols × 1.97 M rows). Worst case — RHS + coeffs both updated — is ~52 ms typed vs ~1.7 ms raw, so ~51 ms/chunk saved by the raw path. Against a ~12 s solve that's ~0.4%; across a 52-chunk full-year run, ~2.7 s total. Decomposed the typed cost to confirm it's the setter, not a benchmark artifact: assigning a prebuilt So I agree with @FBumann: the raw- Bench script: |
great @MaykThewessen! very helpful. let's follow the easier route then and add non-supported value setting logics in the documentation |
Restores symmetry with Constraint.vars (property) and data.vars (underlying xarray Dataset key) — the original rename traded one small asymmetry (read vs. mutate kwarg) for a worse one (Python property name vs. Dataset key name). The `vars()` builtin shadowing inside the kwarg position is benign: we never call `vars()` here, and dropping the rename also lets the top-level `linopy.variables` module be used directly inside the function body instead of importing `Variable as _Variable` to dodge the kwarg shadow. Closes #730. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverts cf845e1. Under the A/B framing for the .vars naming question, update(variables=...) is Category A — it accepts a linopy.Variable. The previous "restore symmetry with .vars / data.vars" argument conflated two layers: - Public Python API speaks about linopy variables → variables= - Internal xarray Dataset key stays as "vars" (xarray collision on Dataset.variables blocks renaming the key) The asymmetry between property/kwarg name and Dataset key name is principled (API layer vs. storage layer), not arbitrary — same pattern ORMs / serializers use. Keeping variables= here lines up with the broader .vars → .variables direction now being considered for properties. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Took a first cut at A6 (incremental snapshot advance) as a follow-up branch off this PR's head, so it's ready to rebase + open once #718 lands. Branch: What
Bench (M1, median of 7, isolated snapshot-refresh step)
Speedup scales as Scope notes
Tests
Happy to open this as a real PR against |
|
@FabianHofmann happy to fold the A6 commit ( Two ways:
Either works. Let me know which you prefer. |
coroa
left a comment
coroa
left a comment
There was a problem hiding this comment.
Ok, first set of comments. I still need to go over the solvers implementation.
Everything looks sane in general, i'd still try to make a couple of improvements:
persistent/diff.pycan be cleaned up a bit. Nothing major, mostly moving stuff around. Maybe a preference to not use a COO constraint coeff diff.persistent/snapshot.pyandconstraints.py: I'd like to make ModelSnapshots (or more explicitly their con and maybe var buffers) share the explicit underlying numpy objects with the Constraint objects in the case of CSRConstraints to save memory. coef_dirty is then not necessary anymore (for the CSR ones), because you can directly test whether it is still the same object. Arguments are memory and cpu efficiency.
| def _cat(parts: list[np.ndarray], dtype: type) -> np.ndarray: | ||
| if not parts: | ||
| return np.empty(0, dtype=dtype) | ||
| return np.concatenate(parts).astype(dtype, copy=False) | ||
|
|
||
|
|
||
| def _cat_obj(parts: list[np.ndarray]) -> np.ndarray: | ||
| if not parts: | ||
| return _EMPTY_KIND | ||
| return np.concatenate(parts) | ||
|
|
||
|
|
||
| def _cat_str(parts: list[np.ndarray]) -> np.ndarray: | ||
| if not parts: | ||
| return _EMPTY_U1 | ||
| return np.concatenate(parts) |
There was a problem hiding this comment.
The _cat variants are all the same duplicated code.
ModelDiff.from_snapshot/from_models return RebuildReason on rebuild (NONE dropped); diff walkers moved onto _DiffBuilder with context in __init__, single _cat helper. Snapshot buffers share constraint arrays (identity fast path); CSRConstraint.sanitize_zeros copy-on-write. Use isinstance(val, ConstantLike) in Variable._validate_update.
|
@coroa thanks, addressed in 11b02e6:
Two pushbacks:
Solvers part of your review still pending from your side — no rush. |
|
@coroa thanks, I addressed most of the points. The refactoring was really needed (already pointed out by @FBumann). COO still looks like the right choice. Note AI written
Two pushbacks:
|
# Conflicts: # doc/release_notes.rst
could move into a property on the model diff maybe instead of changing the coefficient. rewriting the constraint would be faster. unsure whether this would lead to having to change the constraint mapping. |
Variable.fix()/unfix() set both bounds atomically via update() instead of sequential deprecated setters (tripped new lower<=upper cross-validation). Fail fast on quadratic lhs in Constraint.update; type-narrowing fixes for mypy.
ModelDiff stores per-container _CoefDelta (changed rows referencing the CSR buffers); con_coef_rows/cols/vals materialize on first access via cached property. Expansion is now vectorized; backends guard on n_coef_updates. Follows coroa's suggestion on PR 718.
…(A2+A6) _DiffBuilder records target buffers/coords; ModelDiff.snapshot replaces the O(nnz) re-capture after in-place updates. ModelSnapshot.capture no longer mutates the model: the _coef_dirty clear moves to the solver, coupled to snapshot adoption (build + successful apply, never on apply=False).
Base Solver orchestrates the diff sections and validates up front (sign support; Mosek semi-continuous now fails before any native mutation); backends implement _apply_* hooks. Binary [0,1] re-clamp lifted to base with Gurobi no-op (VType 'B' implies bounds natively). self.sense now set uniformly; HiGHS vtype map cached; Xpress/Mosek list-conversion helpers.
…y (A5) update(model, apply=False) computes the diff without the solver lock (immutable snapshot buffers, same_model=False since _coef_dirty cannot be trusted concurrently). solve keeps the coarse lock: apply->run must be atomic and native handles are not thread-safe. Tests pin the non-blocking preview and the preview/apply asymmetry for raw .values mutations.
4 participants
Changes proposed in this Pull Request
Adds an opt-in persistent-update framework so a built solver can be re-solved against a mutated
Modelwithout a full rebuild.Core
linopy.persistent:ModelSnapshot,ModelDiff,StructuralKey,VarKind,RebuildReason.ModelDiffstores changes in flat-native arrays (bounds / var-types / RHS / signs / COO coefs / linear objective / sense) plus per-containerVarSlice/ConSliceviews.ModelDiff.from_snapshot(snap, model)andModelDiff.from_models(m1, m2)— snapshot-based and snapshot-free diffs._coef_dirtyflag on constraints with RHS-setter short-circuit so RHS-only edits skip the coefficient re-walk.Solver orchestration
Solvergainstrack_updates, lazy-build (firstsolve(model, …)builds),apply_update,update,disallow_rebuild, and structured rebuild reasons. Backends without persistent-update support short-circuit to rebuild.apply_update:changeColsBounds/changeColsIntegrality/changeRowBounds/changeCoeff/changeColsCost/changeObjectiveSense. Sign change → rebuild.setAttr(LB/UB/VType/RHS/Sense/Obj),chgCoeff,ModelSense. In-place sign change.chgbounds/chgcoltype/chgrhs/chgrowtype/chgmcoef/chgobj/chgobjsense. In-place sign change.chgvarbound/chgconbound/putvartypelist/putaijlist/putclist/putobjsense. Sign change → rebuild.Tests
test_persistent_snapshot_diff.pycovering allModelDiffsemantics.test_persistent_apply_update.pyrunning 9 cases × 4 backends (skipped per backend when license/installation is unavailable).test_persistent_solver_extras.py.Follow-ups (tracked)
Findings from a symmetric rolling-horizon benchmark on a PyPSA-Eur network (426k vars / 1.28M cons, 168h window, 24h step, HiGHS):
Basis policy afterTo be done by the user, add to docsapply_update: the in-place path reuses the stale basis unconditionally. With HiGHS a valid basis skips presolve, so a 24h-shifted window hot-starts dual simplex on the full unpresolved LP — 126–219s vs 9.4s afterclearSolver()(13–23x regression). Expose areset_solver-style option onSolver.solveand/or auto-clear the basis when the diff touches a large fraction of rows.Solver.solve(model=...)path:Model.solverunssanitize_zeros/sanitize_infinitiesbefore building (drops ~11% of rows on the benchmark network); the bareSolver.solvepath never sanitizes, so persistent users submit a larger problem. Can't be bolted on unconditionally — sanitize-induced mask changes between solves can themselves trigger rebuilds (STRUCTURAL_LABELS/SPARSITY).Checklist
doc.doc/release_notes.rstof the upcoming release is included.