feat!: a solve result tells you whether it has one - #148
Merged
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (27)
📝 WalkthroughWalkthroughThe solver now returns ChangesResult status and API vocabulary migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FarkasAPI
participant DuckdbExecutor
participant Result
Caller->>FarkasAPI: call fk.solve(...)
FarkasAPI->>DuckdbExecutor: build and solve
DuckdbExecutor->>Result: create Result with status and objective
Result-->>Caller: expose status and readable result data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
FBumann
force-pushed
the
fix/solution-without-a-solution
branch
from
July 26, 2026 20:18
3a36dd7 to
6fa0fcb
Compare
FBumann
force-pushed
the
fix/solution-without-a-solution
branch
2 times, most recently
from
July 27, 2026 06:22
33a1daf to
d020d43
Compare
`solve()` on an infeasible model returned a full table of zeros. Nothing
marked it meaningless — `status` was the only signal, and the documented
usage does not check it. A scenario sweep writing `to_parquet` per case
silently produced an all-zeros scenario for every infeasible one.
Rather than invent a guard, this adopts linopy's status model, which had
already solved it: reading primals from a non-optimal model raises, and the
objective is `nan`, not `0.0`.
**Two axes, linopy's spellings.** `termination_condition` is what the solver
said (`optimal`, `infeasible`, `time_limit`…); `status` is what it means
(`ok`, `warning`, `error`, `aborted`, `unknown`). `is_ok` is the question a
caller actually has — and note it is *not* "was it optimal": a run stopped at
a time limit with an incumbent is `ok`, because there are values worth
reading. My own first attempt hardcoded `frozenset({'Optimal'})` and would
have got that wrong.
**`nan`, not a raise, for the objective.** linopy's choice and the better one:
a sentinel propagates through a sweep, where a raise stops a batch that only
wanted to record which cases were infeasible. Reading *primals* still raises,
because there is no sentinel for a whole table.
**`Solution` becomes `Result`.** It matches linopy's envelope, but the local
argument is stronger: the object is returned when the solve produced nothing,
so "solution" was a lie in precisely the case this commit is about. Call
sites follow — `as result`, not `as sol`.
**linopy is the oracle for the vocabulary, not a dependency.** The engine may
not import linopy (hard rule 2), so the tables are copied — and
`tests/test_solve_status.py` imports linopy and asserts they still match, the
same arrangement the differential tests use for the math. The HiGHS map is
read out of linopy's source, since linopy builds it inside a method; if that
moves, the test says so rather than passing vacuously.
ARCHITECTURE.md gains the convention this sets: where a concept is already
linopy's, use linopy's name — copy it, never import it, and let a test prove
the copy. Plus what the rule does not cover, so it is not read as licence to
mirror everything.
One payoff beyond the bug: `test_resolution_parity` compared the lanes'
statuses through `.lower()`, because their vocabularies differed. Both now
speak linopy's, so the assertion is exact.
BREAKING CHANGE: `Solution` is now `Result`; `status` returns the coarse axis
(`ok`) rather than the solver's wording (`Optimal`) — `termination_condition`
carries that, and `is_ok` is what most call sites meant. `objective` is `nan`
and `primal`/`to_*` raise `NoSolutionError` when the solve produced nothing.
Closes #115.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
force-pushed
the
fix/solution-without-a-solution
branch
from
July 27, 2026 06:28
d020d43 to
5a5a2c5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
66-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComplete the documentation migration to the guarded
ResultAPI.
CLAUDE.md#L66-L70: usefk/resultand checkresult.is_okbeforeprimal.README.md#L76-L81: add theis_okguard before reading objective/primal.SPEC.md#L360-L383: guardprimal/to_*reads and useresultin thefk.buildexample.These readers raise
NoSolutionErrorwhen no readable solution exists.🤖 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 `@CLAUDE.md` around lines 66 - 70, Complete the guarded Result API migration: in CLAUDE.md lines 66-70, use fk/result and check result.is_ok before calling primal; in README.md lines 76-81, add the is_ok guard before reading objective or primal; and in SPEC.md lines 360-383, guard primal and to_* reads and use result in the fk.build example.
🤖 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 `@src/farkas/relational/status.py`:
- Around line 45-78: Track primal-solution availability separately from the
coarse termination status: extend SolveStatus and its construction in status.py
to carry HiGHS’s Info.primal_solution_status, and make the readability check
depend on that validity signal rather than treating time_limit as sufficient. In
highs.py, register the sol table only when the solution is valid, preventing
col_value persistence without an incumbent. Add coverage for time-limit solves
both with and without an incumbent in the affected status and HiGHS paths.
In `@tests/test_resolution_parity.py`:
- Around line 152-155: Update the assertion in the resolution-parity test to
explicitly preserve the documented status divergence: eager solving should
return optimal while the relational solve returns infeasible. Replace the
equality assertion using eager_status and relational_status with an assertion
that verifies these two expected termination conditions.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 66-70: Complete the guarded Result API migration: in CLAUDE.md
lines 66-70, use fk/result and check result.is_ok before calling primal; in
README.md lines 76-81, add the is_ok guard before reading objective or primal;
and in SPEC.md lines 360-383, guard primal and to_* reads and use result in the
fk.build example.
🪄 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: 7a454ff0-fe66-4db8-8f45-d76b6c0fa6d6
⛔ Files ignored due to path filters (1)
examples/walkthrough.outis excluded by!**/*.out
📒 Files selected for processing (31)
ARCHITECTURE.mdCHANGELOG.mdCLAUDE.mdREADME.mdSPEC.mdexamples/walkthrough.pysrc/farkas/__init__.pysrc/farkas/api.pysrc/farkas/errors.pysrc/farkas/linopy/__init__.pysrc/farkas/lowering.pysrc/farkas/relational/__init__.pysrc/farkas/relational/executor.pysrc/farkas/relational/sinks/README.mdsrc/farkas/relational/sinks/highs.pysrc/farkas/relational/status.pytests/differential.pytests/test_api.pytests/test_dimensions.pytests/test_doc_examples.pytests/test_group_sum.pytests/test_language_boundary.pytests/test_lowering.pytests/test_milp.pytests/test_piecewise.pytests/test_relational.pytests/test_resolution_parity.pytests/test_roll.pytests/test_solve_status.pytests/test_walkthrough.pytests/test_yaml_loading.py
Comment on lines
+45
to
+78
| #: Which termination conditions roll up to which coarse status. | ||
| STATUS_TO_TERMINATION_CONDITIONS: dict[str, frozenset[str]] = { | ||
| 'ok': frozenset({'optimal', 'time_limit', 'iteration_limit', 'terminated_by_limit', 'suboptimal', 'imprecise'}), | ||
| 'warning': frozenset({'infeasible', 'infeasible_or_unbounded', 'unbounded', 'other'}), | ||
| 'error': frozenset({'internal_solver_error', 'error'}), | ||
| 'aborted': frozenset({'user_interrupt', 'resource_interrupt', 'licensing_problems'}), | ||
| 'unknown': frozenset({'unknown'}), | ||
| } | ||
|
|
||
|
|
||
| def status_of(termination_condition: str) -> str: | ||
| """The coarse status a termination condition rolls up to.""" | ||
| for status, conditions in STATUS_TO_TERMINATION_CONDITIONS.items(): | ||
| if termination_condition in conditions: | ||
| return status | ||
| return 'unknown' | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class SolveStatus: | ||
| """The outcome of a solve, on both axes plus the solver's own wording.""" | ||
|
|
||
| termination_condition: str | ||
| #: Exactly what the solver called it, for a message a user can search for. | ||
| solver_wording: str = '' | ||
|
|
||
| @property | ||
| def status(self) -> str: | ||
| return status_of(self.termination_condition) | ||
|
|
||
| @property | ||
| def is_ok(self) -> bool: | ||
| """Whether the solve left values worth reading. Not "was it optimal".""" | ||
| return self.status == 'ok' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/farkas/relational/sinks/highs.py --items all --type function --match solve_direct
fd -a '^(pyproject\.toml|uv\.lock|requirements.*)$' . -x rg -n -C2 'highspy' {}
fd -a '^test_solve_status\.py$' tests -x sed -n '1,180p' {}Repository: FBumann/farkas
Length of output: 5175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== highs.py outline and relevant sections =="
ast-grep outline src/farkas/relational/sinks/highs.py
sed -n '1,170p' src/farkas/relational/sinks/highs.py
sed -n '170,360p' src/farkas/relational/sinks/highs.py
echo "== status.py outline and contents =="
sed -n '1,130p' src/farkas/relational/status.py
echo "== noSolution/primal references =="
rg -n "NoSolutionError|primal|to_parquet|is_ok|has_solution|valid|incumbent|time_limit" src tests -g '*.py'Repository: FBumann/farkas
Length of output: 28867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== status.py outline and contents =="
sed -n '1,130p' src/farkas/relational/status.py
echo "== NoSolutionError definition/usages =="
fd -a 'errors.py' . | xargs sed -n '1,160p'
rg -n "class NoSolutionError|def .*primal|to_parquet|NoSolutionError|raise NoSolutionError|NoSolutionError\(" src tests -g '*.py'Repository: FBumann/farkas
Length of output: 7614
🌐 Web query:
HiGHS HighsModelStatus kTimeLimit kObjectiveBound getSolution hasValues getSolutionStatus public API
💡 Result:
In the HiGHS solver, the status and results of an optimization are managed through specific enumerations and accessor methods within the public API [1][2][3]. HighsModelStatus This is an enumeration that defines the termination status of the model after a solver run (e.g., via the run method) [4][5]. Key values include: - kOptimal: The model was solved to optimality [4][5]. - kTimeLimit: The solver terminated because the specified time limit was reached [4][5]. - kObjectiveBound: The solver terminated because a defined objective bound was reached [4][5]. - Other states include kInfeasible, kUnbounded, kIterationLimit, kSolutionLimit, and kMemoryLimit [4][5]. Accessing Solutions and Status - getModelStatus: This method is called on the Highs instance to return the current HighsModelStatus [1][6][3]. - getInfo: This method returns the HighsInfo structure, which contains scalar information, including primal_solution_status and dual_solution_status [1][3][7]. These statuses indicate whether a solution is available [8]. - getSolution: This method retrieves the HighsSolution structure, containing the primal and dual values for columns and rows [1][2][8]. Checking for Values While there is no single boolean method named hasValues in the public C++ API, you can determine if a solution is present by checking the primal or dual solution status stored in the HighsInfo structure [3][8]. In practice, this is often done by checking if the returned status from the solver (or the solution status fields) indicates that a valid solution exists (e.g., verifying if the status is not kSolutionStatusNone) [4][8][7]. Example Usage const HighsInfo& info = highs.getInfo; const bool has_values = (info.primal_solution_status!= kSolutionStatusNone); // Logical check [8][7] const HighsSolution& solution = highs.getSolution; [8]
Citations:
- 1: https://ergo-code.github.io/HiGHS/stable/guide/basic/
- 2: https://github.com/ERGO-Code/HiGHS/blob/master/highs/interfaces/highs_c_api.h
- 3: https://ergo-code.github.io/HiGHS/stable/interfaces/cpp/library/
- 4: https://ergo-code.github.io/HiGHS/stable/structures/enums/
- 5: https://fossies.org/dox/scipy-1.17.1/HConst_8h.html
- 6: https://fossies.org/dox/scipy-1.17.1/classHighs.html
- 7: https://ergo-code.github.io/HiGHS/stable/structures/structs/HighsInfo/
- 8: https://github.com/ERGO-Code/HiGHS/blob/master/examples/call_highs_from_cpp.cpp
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
try:
import highspy
from highspy import Highs
print("highspy available", getattr(highspy, "__version__", "no explicit version"))
print("has Info.primal_solution_status?", hasattr(highspy.HighsInfo, "primal_solution_status"))
print("has Info.primal_solution_status value:", getattr(highspy.HighsInfo, "primal_solution_status", None))
print("Has HighsInfo:", hasattr(highspy, "HighsInfo"))
print("Has Highs.kSolutionStatusNone:", hasattr(highspy.Highs, "kSolutionStatusNone") or hasattr(highspy, "kSolutionStatusNone"))
except Exception as exc:
print(type(exc).__name__, exc)
PY
rg -n "primal_solution_status|primalSolutionStatus|kSolutionStatusNone|kSolutionStatus" src tests -g '*.py'Repository: FBumann/farkas
Length of output: 198
Track primal availability separately from termination status.
kTimeLimit/time_limit currently rolls up to ok, so HiGHS can mark a timed-out run as readable and persist col_value even when no incumbent exists. Use HiGHS’s public solution-validity signal, e.g. Info.primal_solution_status, to populate a separate availability field in SolveStatus and register the sol table only for valid solutions. Add coverage for time-limit cases both with and without an incumbent.
📍 Affects 2 files
src/farkas/relational/status.py#L45-L78(this comment)src/farkas/relational/sinks/highs.py#L111-L119
🤖 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/farkas/relational/status.py` around lines 45 - 78, Track primal-solution
availability separately from the coarse termination status: extend SolveStatus
and its construction in status.py to carry HiGHS’s Info.primal_solution_status,
and make the readability check depend on that validity signal rather than
treating time_limit as sufficient. In highs.py, register the sol table only when
the solution is valid, preventing col_value persistence without an incumbent.
Add coverage for time-limit solves both with and without an incumbent in the
affected status and HiGHS paths.
Comment on lines
+152
to
+155
| with fk.build(path, data, coords=coords) as ex: | ||
| relational_status = ex.solve().termination_condition | ||
|
|
||
| assert eager_status.lower() == relational_status.lower() | ||
| assert eager_status == relational_status |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the intentional status divergence assertion.
The preceding docstring says linopy drops the orphaned row and returns optimal, while the relational lane retains it and returns infeasible; this equality assertion therefore fails deterministically.
Proposed fix
- assert eager_status == relational_status
+ assert eager_status == 'optimal'
+ assert relational_status == 'infeasible'📝 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
| with fk.build(path, data, coords=coords) as ex: | |
| relational_status = ex.solve().termination_condition | |
| assert eager_status.lower() == relational_status.lower() | |
| assert eager_status == relational_status | |
| with fk.build(path, data, coords=coords) as ex: | |
| relational_status = ex.solve().termination_condition | |
| assert eager_status == 'optimal' | |
| assert relational_status == 'infeasible' |
🤖 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_resolution_parity.py` around lines 152 - 155, Update the assertion
in the resolution-parity test to explicitly preserve the documented status
divergence: eager solving should return optimal while the relational solve
returns infeasible. Replace the equality assertion using eager_status and
relational_status with an assertion that verifies these two expected termination
conditions.
This was referenced Jul 27, 2026
FBumann
added a commit
that referenced
this pull request
Jul 27, 2026
Two halves of one gap, found by review of #148. **`solver_options` was missing.** linopy takes a dict and applies it (`for k, v in self.solver_options.items(): h.setOptionValue(k, v)`); we forwarded nothing — every kwarg on `farkas.solve` stopped at `DuckdbExecutor.__init__` (memory_limit, chunk_rows, threads, workdir), none of which reach HiGHS. So no time limit, no MIP gap, no solver knob at all. Now forwarded verbatim, in linopy's shape, and kept separate from the build knobs because those govern construction and never reach the solver. **`is_ok` was too coarse a gate.** It is linopy's rollup of the termination condition, and `ok` includes `time_limit`, `iteration_limit`, `terminated_by_limit`, `suboptimal` and `imprecise` — all of which mean "stopped early", where an incumbent may or may not exist. HiGHS answers that separately via `Info.primal_solution_status`, so `SolveStatus` now carries it and readers gate on `is_readable = is_ok and has_primal`. This is a deliberate divergence from linopy, and worth stating: its `safe_get_solution` gates on `is_ok` alone, so a MIP stopped before finding any feasible point has its zero-filled `col_value` read as an answer. That is the bug #115 just fixed one level down; inheriting it for the sake of parity would be a poor trade. The *vocabulary* stays linopy's — `is_ok` still means exactly what linopy means by it. The two halves are one commit because neither is testable without the other: before `solver_options` there was no way to reach a non-`optimal` `ok` status at all. With it, the case is deterministic: time_limit=0 cond=time_limit is_ok=True has_primal=False obj=nan no options cond=optimal is_ok=True has_primal=True obj=46337878.0 and reading the primal of the first raises rather than returning 60 zeros. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
added a commit
that referenced
this pull request
Jul 27, 2026
…169) Two halves of one gap, found by review of #148. **`solver_options` was missing.** linopy takes a dict and applies it (`for k, v in self.solver_options.items(): h.setOptionValue(k, v)`); we forwarded nothing — every kwarg on `farkas.solve` stopped at `DuckdbExecutor.__init__` (memory_limit, chunk_rows, threads, workdir), none of which reach HiGHS. So no time limit, no MIP gap, no solver knob at all. Now forwarded verbatim, in linopy's shape, and kept separate from the build knobs because those govern construction and never reach the solver. **`is_ok` was too coarse a gate.** It is linopy's rollup of the termination condition, and `ok` includes `time_limit`, `iteration_limit`, `terminated_by_limit`, `suboptimal` and `imprecise` — all of which mean "stopped early", where an incumbent may or may not exist. HiGHS answers that separately via `Info.primal_solution_status`, so `SolveStatus` now carries it and readers gate on `is_readable = is_ok and has_primal`. This is a deliberate divergence from linopy, and worth stating: its `safe_get_solution` gates on `is_ok` alone, so a MIP stopped before finding any feasible point has its zero-filled `col_value` read as an answer. That is the bug #115 just fixed one level down; inheriting it for the sake of parity would be a poor trade. The *vocabulary* stays linopy's — `is_ok` still means exactly what linopy means by it. The two halves are one commit because neither is testable without the other: before `solver_options` there was no way to reach a non-`optimal` `ok` status at all. With it, the case is deterministic: time_limit=0 cond=time_limit is_ok=True has_primal=False obj=nan no options cond=optimal is_ok=True has_primal=True obj=46337878.0 and reading the primal of the first raises rather than returning 60 zeros. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
added a commit
that referenced
this pull request
Jul 27, 2026
#148 split the solve result into linopy's two fields — `status` is the solver's health ('ok'), `termination_condition` is what it concluded ('optimal'). It landed after the harness did, so the gate has been comparing `status` against 'Optimal' and would abort every run on main.
This was referenced Jul 27, 2026
FBumann
added a commit
that referenced
this pull request
Jul 27, 2026
#148 split the solve result into linopy's two fields — `status` is the solver's health ('ok'), `termination_condition` is what it concluded ('optimal'). It landed after the harness did, so the gate has been comparing `status` against 'Optimal' and would abort every run on main.
FBumann
added a commit
that referenced
this pull request
Jul 27, 2026
…eeds today (#182) * bench: attribute build time to the SQL that spends it docs/benchmarks.md says the ~2x is "dominated by fixed work — re-scanning the vars table per section, ~100M printf calls, a 2.6 GB CSV write". Those are all emit-side costs, and the committed phase timings say emit is not the problem. At the l rung, taking the best of each repeat: case/size build fk build lp ratio emit fk emit lp ratio dispatch/l 3.89s 0.40s 9.7x 2.78s 2.79s 1.0x transport/l 5.49s 0.90s 6.1x 3.76s 3.07s 1.2x Emit is at parity; the whole gap is build. The phases are not doing the same work — linopy allocates arrays cheaply and materialises coefficients at write time, so a phase-to-phase ratio is not apples to apples — but it localises our cost: build is 58% of our wall at dispatch/l, and emit is already as fast as the writer we are compared against. bench/profile_build.py tags every statement with the build step that issued it. On dispatch/l, _build_variable is 40% of all SQL time and one statement inside it — the chunked INSERT INTO var_p that assigns labels — is most of it. The cost is the ORDER BY in ROW_NUMBER() OVER (ORDER BY t_snapshot.ord, t_generator.ord) which sorts every chunk. Measured on the same data, building the same 10M-row frame four ways: 8.3s as written, 2.4s with the ORDER BY dropped, and 4.4s storing ordinals rather than coordinate values. Packing the two sort columns into one BIGINT saves 15%, so it is the sort itself and not the key width. Dropping the ORDER BY is not free: it changes which coordinate gets which solver index, which tests/test_walkthrough.py pins as a golden file because that mapping is part of the documented story. There is a variant that keeps the mapping exactly. When the mask does not depend on the leading dim — `where: p_max > 0` depends only on generator — the surviving trailing coordinates are the same for every leading value, so they can be ranked once over a tiny table and the label computed arithmetically, with no window function and no sort. Verified by building both frames and diffing them: 0 rows differ at 1M and at 10M, 2.2x and 3.2x faster. Two smaller items on the same path, measured at 10M rows: the vtype column is VARCHAR ('continuous' written ten million times) where UTINYINT is 2.5x faster (0.87s -> 0.35s), and the objective runs SUM(coeff) GROUP BY col over groups that each hold exactly one row when the objective has a single term, where skipping the aggregate is 2.7x faster (0.60s -> 0.22s). No engine change here — this commit adds the profiler and the finding. The docs' attribution is left alone until a fix lands to replace it with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0145rKirxsCRGodhkKozjaYW * bench: a regression suite on memray, and the measurement that says why `bench/run.py` publishes how farkas compares to linopy. It cannot answer "did this PR make it worse" well: peak RSS moves with machine load, and I mis-read a coin-flip OOM as a regression twice this week before bisecting proved otherwise. `bench/regressions/` is the other half — pytest-benchmem, which is pytest-benchmark timing plus a memray peak pass, with `isolate=True` so each pass gets a fresh process. That matters twice over: duckdb would otherwise be measured with a warm buffer pool, and isolation is what makes whole-process `rss` available beside the memray peak, so the two can be watched for divergence. Measured, `dispatch/m`: | arm | ru_maxrss | memray peak | |---|---|---| | farkas | 309 MB | 211 MB | | linopy | 604 MB | 2967 MB | memray counts polars' reserved arenas as allocated and does not count the interpreter or mapped libraries at all, so the bias points in opposite directions in the two lanes: the peak ratio is 0.51x by RSS and 0.07x by memray. Publishing that would turn a defensible 2x memory win into a fictitious 14x one, false to anyone who ran /usr/bin/time. Within one lane the bias sits on both sides of a diff and cancels. So: RSS for the comparison we publish, memray for the regressions we chase, and `--benchmark-memory-compare-fail` to make the second a gate. Its own dependency group, not `dev` — nothing in the test suite or the published ladder needs memray. `testpaths` is still `tests`, so `uv run pytest` does not collect it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(bench): the parity gate reads termination_condition, not status #148 split the solve result into linopy's two fields — `status` is the solver's health ('ok'), `termination_condition` is what it concluded ('optimal'). It landed after the harness did, so the gate has been comparing `status` against 'Optimal' and aborts every run on main before a single timing is taken. --------- Co-authored-by: Claude <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
1 participant
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.
Closes #115.
solve()on an infeasible model returned a full table of zeros.statuswas the only signal, and the documented usage doesn't check it — so a sweep writingto_parquetper scenario silently produced an all-zeros file for every infeasible case.Rather than invent a guard, this adopts linopy's status model, which had already solved it:
What the surface looks like now
is_okis not "was it optimal". A run stopped at a time limit with an incumbent isok, because there are values worth reading — that's the question a caller actually has. My first attempt hardcodedfrozenset({'Optimal'})and would have got this wrong; linopy's rollup already has it right.nanfor the objective, not a raise. linopy's choice and the better one: a sentinel propagates through a scenario sweep, where a raise stops a batch that only wanted to record which cases were infeasible. Reading primals still raises — there's no sentinel for a whole table.Solution→ResultMatches linopy's envelope (
Result= status + solution + report;Solution= raw arrays). But the local argument is the stronger one: this object is returned when the solve produced nothing, soSolutionwas a lie in exactly the case the PR is about.linopy is the oracle, not a dependency
Hard rule 2 forbids the engine importing linopy, so the tables are copied — and
tests/test_solve_status.pyimports linopy and asserts they still match, the same arrangement the differential tests use for the math. A copy nobody checks is a copy that rots.The HiGHS map is read out of linopy's source, because linopy builds it inside a method rather than exposing it. Brittle to a refactor on purpose: if it moves, the test says so instead of passing vacuously.
Two renames the class rename dragged in
Call sites bind
result, notsol. Renaming the class and leaving everydocumented example saying
as sol:would have been half a rename — those arethe lines users copy.
lybecomesfk. The documented alias was stillimport farkas as ly—a leftover from
linopy_yamlthat #127's rename did not reach.lynow standsfor nothing, so every example was teaching a name that does not exist.
This is scope I added without being asked; it is mechanical and it is in
the same lines as the
solrename, so doing it separately would mean touchingthem twice. Say the word and I will split it out.
The convention is now written down
ARCHITECTURE.mdgains a naming rule, because this PR is where it gets set:where a concept is already linopy's, use linopy's name — spelling, field
names, decomposition. Copy it, never import it (rule 2 forbids that), and let a
test assert the copy still matches. It also says what the rule does not cover:
where the design genuinely differs it stays ours, and we have no
Solutionofdense arrays to hold because the values live in duckdb.
Written into the doc rather than left implicit so the next shared concept —
SolverReportmetrics (#34), duals (#78) — does not get re-litigated.A payoff beyond the bug
test_resolution_paritycompared the two lanes' statuses through.lower(), because their vocabularies differed. Both now speak linopy's, so the assertion is exact:Breaking
Pre-1.0, no released consumers.
Solution→Result;statusreturns the coarse axis (ok) where it used to return the solver's wording (Optimal) —termination_conditioncarries that now, andis_okis what nearly every call site actually meant (10 test files updated accordingly).Not in scope
SolverReport(runtime, mip_gap, dual_bound, iteration counts) is worth adopting too, and highspy exposes all of it — but that's #34 (build/solve observability), not this. Flagging so the field names get copied from linopy when it lands.Verification
uv run pytest— 318 passed, 1 xfailed (5 new) ·ruff/pyreflyclean · walkthrough golden regenerated (status ok (optimal)).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
SolutiontoResult.NoSolutionErrorwhen result values are unavailable.Documentation
farkas(fk) API and newResultterminology.