[FBumann]

A second solver, the reorganisation it argued for, and the licence handling it
turned out to need.

1. The sink

A second solver reading the same ModelTables as HiGHS: dense column vectors
in one addMVar, the matrix in CSR blocks chunked by nonzeros. No
float→text→parse round trip.

lps.solve('model.yaml', sources, solver_name='gurobi')

scipy is in the [gurobi] extra, and is not a transitive convenience.
gurobipy's matrix API takes a matrix object where HiGHS takes the three CSR
arrays, so scipy.sparse.csr_matrix over the arrays the labels already laid
out is the only way to hand over a block of rows without one Python call per
row.

The condition map is linopy's, minus three entries it gets wrong. linopy
calls SOLUTION_LIMIT optimal and WORK_LIMIT / MEM_LIMIT
internal_solver_error; all three contradict Gurobi's documented status codes,
and the last also contradicts linopy's own HiGHS map, which sends
kMemoryLimit to resource_interrupt. Each is declared in
_LINOPY_DIVERGENCES with its reason, and test_solve_status.py pins both
directions — everything else still matches linopy, and every declared
divergence is still one, so if linopy fixes it the test fails and the exception
goes away.

2. Two families

The second solver made the flat sinks/ bag read wrong: its members are two
different shapes, and the naming still said solver_direct — a mechanism, when
what varies is which solver. How many solvers there are will change; what a
solver has to answer will not. So the taxonomy is the tree:

relational/sinks/
├── tables.py          the contract both families read
├── solvers/           chosen by name    — highs.py, gurobi.py, SOLVERS
└── writers/           chosen by suffix  — lp_file.py, WRITERS + PLANNED_WRITERS

A solver is four things that must agree — module named for the solver,
solve_<name>, a build_<name> seam for bench/, and the SOLVERS key — and
tests/test_architecture.py reads all four off the path, plus "no sink reaches
a sibling", which is what keeps gurobipy off the import path of a HiGHS
caller. Adding a solver is a module and a line: no method on the executor, no
branch in api.py, no name on the Python surface. api.write asks the writer
family instead of branching on suffix, and planned lives there too, so .mps
answers for itself.

Renamed rather than aliased, per CONTRIBUTING's breaking changes are free:
ex.write_lp(path) → ex.write(path), solve_direct → solve_highs, and the
RelationalBuildError deprecation alias is gone.

3. Licence and environment

Checked against linopy, which has iterated on this:

• solve_gurobi disposes model then environment in a finally — their order,
  and their comment says it is required for the license to be released even
  while user code still holds a solver_model reference.
• build_gurobi hands the model over with a weakref.finalize disposing the
  environment when the model is dropped. gurobipy has no Model.getEnv(), so a
  caller given only the model could not release it at all; linopy's equivalent
  case leaves it to gurobipy to free whenever.
• Solver options go to Env(params=...), not setParam. WLSAccessID /
  ComputeServer / TokenServer can only be set before an environment starts
  — applying options to the model locked out every Compute-Server and WLS user.

What needs a solver handle rather than a finalizer — one environment across
many solves, warm starts, in-place re-solve — is #424.

Evidence

tests/test_gurobi_sink.py, skipped without gurobipy: HiGHS is the oracle for
objective, primal and duals (including under maximize, where a sign
convention could differ and nothing else would notice), all 11 ported models
reach their published optima on gurobi, plus ragged block boundaries, the
MIP-has-no-duals path, options landing on the environment, the build seam, and
that nothing keeps a built model alive. Two architecture tests pin the
families; the closed-solver-set test lives in test_api.py so it runs without
the extra.

gurobipy's wheel carries a size-limited licence, so lpspec[gurobi] is in the
dev group and CI exercises the sink with nobody's licence; verified green with
gurobipy hidden as well.

Groundwork for #106, and Track 3's uneven capability table is now concrete
rather than hypothetical.

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds an optional Gurobi solver sink, shared dense model projections, and solver_name dispatch through the public API. It adds status, result, dependency, and integration tests. Documentation now describes installation, solver selection, sink differences, and updated capability status.

Changes

Solver expansion

Layer / File(s)	Summary
Shared solver projection pyproject.toml, src/lpspec/relational/sinks/tables.py, src/lpspec/relational/sinks/highs.py	Adds the Gurobi dependency extra and moves dense column projection into ModelTables, which both sinks can use.
Gurobi sink implementation src/lpspec/relational/sinks/gurobi.py, tests/test_gurobi_sink.py, tests/test_solve_status.py	Adds Gurobi model construction, CSR loading, option forwarding, status mapping, primal and dual extraction, dependency diagnostics, and comprehensive tests.
Solver registry and API dispatch src/lpspec/api.py, src/lpspec/relational/engines/polars/executor.py, src/lpspec/relational/sinks/__init__.py, docs/api.md, docs/ARCHITECTURE.md	Adds solver_name, registers highs and gurobi, and dispatches solves through the selected sink.
Setup and capability documentation CONTRIBUTING.md, README.md, docs/index.md, docs/SPEC.md, docs/ROADMAP.md, src/lpspec/relational/sinks/README.md	Documents the Gurobi extra, licensing and availability boundaries, sink-specific capabilities, and updated roadmap entries.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant API as lpspec.api.solve
  participant Executor as PolarsExecutor.solve
  participant Registry as solver
  participant Gurobi as solve_gurobi
  participant Tables as ModelTables
  API->>Executor: Forward solver_name and solver_options
  Executor->>Registry: Look up selected solver
  Registry-->>Executor: Return solve_gurobi
  Executor->>Gurobi: Pass ModelTables and solver options
  Gurobi->>Tables: Read dense solver projections
  Gurobi-->>Executor: Return status, objective, primal, and dual results

Loading

Possibly related PRs

• FBumann/lpspec#87: Updates overlapping architecture, roadmap, specification, README, and API documentation areas.
• FBumann/lpspec#388: Reclassifies quadratic support around solver sink capabilities in related roadmap and architecture documentation.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 94.12% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the Gurobi sink as the primary change and remains related to the solver-family updates.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/gurobi-sink

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[read-the-docs-community]

Documentation build overview

📚 lpspec | 🛠️ Build #33856314 | 📁 Comparing 6a374dd against latest (8197084)

  🔍 Preview build  

5 files changed · ± 5 modified

± Modified

• index.html
• ARCHITECTURE/index.html
• ROADMAP/index.html
• SPEC/index.html
• api/index.html

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (5)

src/lpspec/relational/sinks/gurobi.py (2)

228-243: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Validate the concatenated dual length against row_count.

_labelled assigns row labels from np.arange(len(values)), so the dual frame is correct only if the blocks tile 0..row_count-1 exactly. _duals cannot see row_count, so a tiling gap in row_chunks_by_nonzeros would shift every dual label after the gap and stay silent. The HiGHS sink does not carry this exposure, because it passes model.row_count to its own _labelled (src/lpspec/relational/sinks/highs.py Line 173).

Pass model.row_count into _duals and assert the concatenated length.

♻️ Proposed change

-def _duals(blocks: list[Any]) -> pl.DataFrame | None:
+def _duals(blocks: list[Any], row_count: int) -> pl.DataFrame | None:
@@
     try:
         values = [block.Pi for block in blocks]
     except (AttributeError, gurobipy.GurobiError):
         return None
-    return _labelled('row', np.concatenate(values) if values else np.empty(0, dtype=np.float64))
+    duals = np.concatenate(values) if values else np.empty(0, dtype=np.float64)
+    if len(duals) != row_count:
+        msg = f'gurobi returned {len(duals)} duals for {row_count} rows'
+        raise AssertionError(msg)
+    return _labelled('row', duals)

Update the call site at Line 129 to _duals(blocks, model.row_count).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lpspec/relational/sinks/gurobi.py` around lines 228 - 243, Update
`_duals` to accept `row_count`, and update its call site to pass
`model.row_count`. After concatenating the block dual values, validate that
their length equals `row_count` before calling `_labelled`; preserve the
existing `None` behavior when duals are unavailable.

---

122-129: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Dispose the Gurobi model and environment after solving.

_load() creates an explicit gurobipy.Env and gurobipy.Model(env=...), but solve_gurobi() returns after optimize() without deterministic cleanup. With remote/token licenses, the license remains held until GC collects the model; the size-limited wheel license does not show this behavior. The dependency constraint is gurobipy>=11, where the Python context-manager protocol is supported. Keep build_gurobi() unchanged because the caller owns the returned model.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lpspec/relational/sinks/gurobi.py` around lines 122 - 129, Update
solve_gurobi() to deterministically dispose of the model and its explicitly
created environment after optimize(), including the early return for unreadable
status, using gurobipy>=11 context-manager support; leave build_gurobi()
unchanged because its caller owns the returned model.

src/lpspec/relational/sinks/tables.py (1)

68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Annotate the NumPy arrays instead of using Any.

numpy is a base runtime dependency, so numpy.typing can be imported under TYPE_CHECKING and the four vectors can carry real types. The current Any return hides the element type from every caller, including the Gurobi sink, which passes integral to np.where.

♻️ Proposed typing change

 if TYPE_CHECKING:
+    import numpy as np
+    import numpy.typing as npt

-    def dense_columns(self, infinity: float) -> tuple[Any, Any, Any, Any]:
+    def dense_columns(
+        self, infinity: float
+    ) -> tuple[npt.NDArray[np.float64], npt.NDArray[np.float64], npt.NDArray[np.float64], npt.NDArray[np.bool_]]:

-def _scattered(count: int, at: Any, values: Any, absent: Any) -> Any:
+def _scattered(count: int, at: npt.NDArray[Any], values: npt.NDArray[_T], absent: object) -> npt.NDArray[_T]:

Note: the guideline "Keep pyrefly strict checking at zero errors; fix types rather than widening them" applies here. As per coding guidelines.

Also applies to: 109-109

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lpspec/relational/sinks/tables.py` at line 68, Update the dense_columns
return annotation and its related typing imports to use numpy.typing array types
for all four vectors instead of Any, importing the NumPy typing module under
TYPE_CHECKING as needed. Preserve the existing runtime behavior while exposing
the vectors’ element types to callers such as the Gurobi sink and keeping strict
type checking error-free.

Source: Coding guidelines

tests/test_solve_status.py (1)

118-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Let this helper serve the HiGHS test too.

_linopy_condition_map repeats the AST walk that test_the_highs_mapping_matches_linopy performs inline at Lines 63-85. The only difference is the node attribute read from each key and value pair: the HiGHS map uses ast.Attribute (.attr), and the Gurobi map uses ast.Constant (.value). Both tests state that they are brittle on purpose, so the two copies also rot in two places.

Parametrize the helper by the node type it expects, then call it from both tests.

♻️ Proposed shape

-def _linopy_condition_map(solver: str) -> dict[int, str]:
+def _linopy_condition_map(solver: str, keyed_by: type[ast.expr], attribute: str) -> dict[Any, str]:
@@
     return {
-        key.value: value.value
+        getattr(key, attribute): getattr(value, attribute)
         for key, value in zip(literals[0].keys, literals[0].values, strict=True)
-        if isinstance(key, ast.Constant) and isinstance(value, ast.Constant)
+        if isinstance(key, keyed_by) and isinstance(value, keyed_by)
     }

test_the_highs_mapping_matches_linopy then reads _linopy_condition_map('Highs', ast.Attribute, 'attr'), and the Gurobi test reads _linopy_condition_map('Gurobi', ast.Constant, 'value').

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_solve_status.py` around lines 118 - 144, Generalize
_linopy_condition_map to accept the expected AST key/value node type and
attribute name, then move its existing literal extraction and mapping logic into
that parameterized helper. Replace the inline AST walk in
test_the_highs_mapping_matches_linopy with _linopy_condition_map('Highs',
ast.Attribute, 'attr'), and call it for Gurobi with
_linopy_condition_map('Gurobi', ast.Constant, 'value'), preserving each test’s
existing assertions.

tests/test_gurobi_sink.py (1)

199-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This test does not need gurobipy, and the module-level skip hides it.

Line 27 skips the whole module when gurobipy is absent. test_the_set_of_solver_sinks_is_closed asserts registry behavior only: an unknown solver_name raises, and SOLVERS holds exactly two names. Neither assertion loads Gurobi. On a contributor machine or a CI job without the [gurobi] extra, the closed-set contract therefore goes unchecked.

Move this test to the registry test module, so it runs in every configuration.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_gurobi_sink.py` around lines 199 - 205, Move
test_the_set_of_solver_sinks_is_closed from tests/test_gurobi_sink.py into the
registry test module, preserving its unknown-solver exception assertion and
exact sinks.SOLVERS membership check. Ensure it is no longer covered by the
module-level gurobipy skip and runs without the Gurobi extra.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/api.md`:
- Line 21: Update the `lps.solve` example on the Gurobi line to assign its
return value to the result variable, matching the preceding HiGHS invocation so
subsequent `result.*` expressions use the Gurobi result.

In `@src/lpspec/relational/sinks/__init__.py`:
- Around line 33-59: Update the solver registry near SOLVERS by storing the
entries in a private mutable dictionary, then expose SOLVERS as a read-only
MappingProxyType view of that registry. Ensure solver(name) continues resolving
through SOLVERS so the exported mapping cannot be modified while preserving the
existing solver entries and behavior.

In `@src/lpspec/relational/sinks/README.md`:
- Around line 49-52: Update the sink authoring guide’s solver return contract to
document the required four-tuple: status, objective, primal values, and dual
values. Keep the writer sink’s None behavior unchanged, and align the solver
wording with the SOLVERS registry and PolarsExecutor.solve() unpacking contract.

---

Nitpick comments:
In `@src/lpspec/relational/sinks/gurobi.py`:
- Around line 228-243: Update `_duals` to accept `row_count`, and update its
call site to pass `model.row_count`. After concatenating the block dual values,
validate that their length equals `row_count` before calling `_labelled`;
preserve the existing `None` behavior when duals are unavailable.
- Around line 122-129: Update solve_gurobi() to deterministically dispose of the
model and its explicitly created environment after optimize(), including the
early return for unreadable status, using gurobipy>=11 context-manager support;
leave build_gurobi() unchanged because its caller owns the returned model.

In `@src/lpspec/relational/sinks/tables.py`:
- Line 68: Update the dense_columns return annotation and its related typing
imports to use numpy.typing array types for all four vectors instead of Any,
importing the NumPy typing module under TYPE_CHECKING as needed. Preserve the
existing runtime behavior while exposing the vectors’ element types to callers
such as the Gurobi sink and keeping strict type checking error-free.

In `@tests/test_gurobi_sink.py`:
- Around line 199-205: Move test_the_set_of_solver_sinks_is_closed from
tests/test_gurobi_sink.py into the registry test module, preserving its
unknown-solver exception assertion and exact sinks.SOLVERS membership check.
Ensure it is no longer covered by the module-level gurobipy skip and runs
without the Gurobi extra.

In `@tests/test_solve_status.py`:
- Around line 118-144: Generalize _linopy_condition_map to accept the expected
AST key/value node type and attribute name, then move its existing literal
extraction and mapping logic into that parameterized helper. Replace the inline
AST walk in test_the_highs_mapping_matches_linopy with
_linopy_condition_map('Highs', ast.Attribute, 'attr'), and call it for Gurobi
with _linopy_condition_map('Gurobi', ast.Constant, 'value'), preserving each
test’s existing assertions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8f5753c9-f78d-4314-b0f8-cac28f1314c0

📥 Commits

Reviewing files that changed from the base of the PR and between 8197084 and 876814c.

📒 Files selected for processing (17)

• CONTRIBUTING.md
• README.md
• docs/ARCHITECTURE.md
• docs/ROADMAP.md
• docs/SPEC.md
• docs/api.md
• docs/index.md
• pyproject.toml
• src/lpspec/api.py
• src/lpspec/relational/engines/polars/executor.py
• src/lpspec/relational/sinks/README.md
• src/lpspec/relational/sinks/__init__.py
• src/lpspec/relational/sinks/gurobi.py
• src/lpspec/relational/sinks/highs.py
• src/lpspec/relational/sinks/tables.py
• tests/test_gurobi_sink.py
• tests/test_solve_status.py

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Bind the Gurobi result in the example.

Line 20 stores the HiGHS result. Line 21 discards the Gurobi result. The following result.* expressions therefore still refer to HiGHS.

-lps.solve('model.yaml', sources, solver_name='gurobi')  # the other solver sink
+result = lps.solve('model.yaml', sources, solver_name='gurobi')

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	lps.solve('model.yaml', sources, solver_name='gurobi') # the other solver sink
	result = lps.solve('model.yaml', sources, solver_name='gurobi')

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/api.md` at line 21, Update the `lps.solve` example on the Gurobi line to
assign its return value to the result variable, matching the preceding HiGHS
invocation so subsequent `result.*` expressions use the Gurobi result.

[FBumann]

Licence/environment handling is now closed for both paths without a solver handle, after checking how linopy does it:

• solve_gurobi disposes model then environment in a finally — linopy's order, and their comment says it is required for the license to be released even while user code still holds a solver_model reference.
• build_gurobi hands the model over with a weakref.finalize disposing the environment when the model is dropped. gurobipy has no Model.getEnv(), so a caller given only the model could not release it otherwise; linopy's equivalent (a model detached from its Solver) leaves it to gurobipy to free whenever.
• Solver options moved to Env(params=...): WLSAccessID / ComputeServer / TokenServer can only be set before an environment starts, so applying them to the model locked out every Compute-Server and WLS user.

What genuinely needs a handle — one environment shared across many solves (a token-server round trip per solve today) and warm starts / in-place re-solve — is filed as #424, with linopy's Solver as the reference shape.