feat: forward solver_options, and gate reads on an actual incumbent - #169
Conversation
|
Warning Review limit reached
Next review available in: 5 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 selected for processing (6)
✨ 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 |
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>
906fb84 to
6b2c0cf
Compare
Review finding on #148, verified and valid — with a correction to my own first assessment.
I was wrong that it was unreachable
My initial read was "a time limit isn't settable, so this is latent — defer to #28". That was wrong, and for an embarrassing reason: the time limit isn't settable because we forgot to forward solver options at all.
linopy takes a dict and applies it:
We forwarded nothing. Every kwarg on
farkas.solvestops atDuckdbExecutor.__init__—memory_limit,chunk_rows,threads,workdir— and none of those reach HiGHS. No time limit, no MIP gap, no solver knob whatsoever.So the finding wasn't blocked by unreachability; it was blocked by a missing feature. Both halves are here because neither is testable without the other.
is_okwas too coarseis_okis linopy's rollup, andokcoverstime_limit,iteration_limit,terminated_by_limit,suboptimal,imprecise— all "stopped early", where an incumbent may or may not exist. Onlyoptimalguarantees one.HiGHS answers that separately (
Info.primal_solution_status), soSolveStatuscarries it and readers gate on:Deterministic once options work:
Reading the primal of the first raises, instead of returning 60 zeros.
Stating it loudly, because we just made "lean on linopy" a written convention.
I checked all four signals —
primal_solution_status,SolutionStatus,has_solution,info.valid: zero hits in linopy.safe_get_solutiongates onis_okalone, so linopy has this exact bug. A MIP stopped before finding a feasible point has its zero-filledcol_valueread as an answer.Copying that would reintroduce #115 one level up, so I didn't. The vocabulary stays linopy's —
is_okstill means precisely what linopy means. We add a second, narrower predicate rather than redefining theirs.If you'd rather match linopy exactly and accept the gap, say so — it's a one-line change to make
is_readablereturnis_ok. I think that would be wrong, but it's your call and it is a real deviation from the convention.Scope
solver_optionsthreaded throughapi.solve→executor.solve→sinks.solve_direct, kept separate from build knobs.SPEC.mddocuments both the option and whyis_okandhas_primaldiffer.Three new tests: options reach the solver, time-limit-without-incumbent, time-limit-with-incumbent.
337 passed, 1 xfailed ·
ruff/pyreflyclean.🤖 Generated with Claude Code