chore: harden #189 — deps, docs, and the claims they back by FBumann · Pull Request #233 · fluxopt/lpspec

FBumann · 2026-07-28T08:46:14Z

Stacked on #189. Nothing here changes engine behaviour: the whole diff is metadata, prose, two test assertions and one CI step. Six commits, each standalone.

The two findings worth the PR

1. The floors gate was not gating. .github/workflows/ci.yml carries the comment "the lower bounds in pyproject.toml are real. Without this they are decoration" — and it was not testing them. uv locks universally, across every declared extra whether or not it is being installed, so uv sync --no-default-groups --resolution lowest-direct still resolves under the [linopy] extra's constraints. linopy 0.9.0 requires polars>=1.31.1, so the bare job installed polars 1.32 on every run and our own polars>=1.30 was never once exercised. That is precisely the failure the comment describes, occurring inside the gate written to prevent it.

uv venv + uv pip install --resolution lowest-direct . resolves the requested set and nothing else. Rehearsed locally end to end — it now installs


resolved	polars 1.30.0 · numpy 1.26.0 · highspy 1.10.0 · pydantic 2.0 · pyparsing 3.1.0 · pyyaml 6.0
absent	linopy · xarray · pandas · pyarrow
result	`214 passed, 20 skipped`

The floors are genuine — this was a hole in the gate, not a wrong bound. Also recorded there: the floors are a 3.11 claim only (pyyaml 6.0 and numpy 1.26.0 have no 3.13 wheel, so lowest-direct on 3.13 attempts a source build and fails), and why pytest goes in as a second unconstrained call — under lowest-direct an unpinned direct requirement resolves to pytest 2.0.0, a 2011 sdist that tries to build itself over plain HTTP.

2. Both lead claims were unbacked. README.md and ROADMAP.md led on end-to-end peak against the floor under the LP-file route and marginal cost per model in a loop, citing docs/benchmarks.md for both. Neither is measured in that file — not on this branch and not on main; I checked both. This PR then added an intro promising both "live further down", which nothing further down delivers.

They are good claims, and they are the two a reader is most likely to check, which is the worst place to be unbacked. The lead becomes what the tables settle — cost to a loaded solver, 2–4x faster on four of five cases and lower peak on all five, with profiled named as the one we lose and the LP-file route pointed at rather than averaged in. "Nothing accumulates between builds" stays, relabelled as architectural rather than measured. docs/benchmarks.md gets its Not measured yet section back (dropped in the re-measure) holding both, plus storage/roll, a MILP, and a hand-written highspy arm as the missing denominator.

The rest

Prose that did not follow the engine. CLAUDE.md was untouched by #189 and is the file an agent reads first: it opened by calling the package a definition layer for linopy that builds linopy.Model objects, said relational/ (duckdb), omitted frames.py / chunking.py / status.py / resolution.py, and told the reader to manage a lifetime that no longer exists. ARCHITECTURE.md named DuckdbExecutor._label_frame for a class called PolarsExecutor. "memory-bounded" survived in the architecture diagram and in ROADMAP's ahead-of-comparable list — the configured ceiling went with duckdb, and Track 5 says so two hundred lines below. chunking.py and test_relational.py both cited hard rule 4 for a claim about peak; rule 4 is "backend-visible YAML files are self-contained", and the rule they meant was deleted with the budget.

A test that can now be stronger. test_runtime_is_linopy_free explained that pandas could not be asserted absent, because duckdb imported it opportunistically when registering any Python object. polars does not, so pandas and pyarrow are on the list now. That is what keeps a bridge out (to_pandas, to_dataarray) from becoming something the build path walks over on its own.

pyproject.toml. dev repeated the [linopy] extra's three pins verbatim, so what CI tests the lane against could drift from what a user of the extra gets — it installs farkas[linopy] by name now. The description sold "at any scale" on "a streaming relational engine"; both were true of an engine that built under a ceiling and could exceed RAM, and this one holds the model it builds. The bench group's comment named two packages the group does not contain.

bench/. workdir_bytes was returning a hardcoded 0 — a column that always reads 0 is read as "measured and fine", which is the same failure it existed to prevent, so it is gone with a note saying where to put it back. The phase table listed only the LP sink and a count(*). And a footgun, from walking into it: bench.run writes the committed results/latest.jsonl by default and replaces it, so a one-rung smoke test turns 294 records of published provenance into 4. --out already exists; the narrow-run examples use it now.

pr-189-review.md — a review of this branch, committed to its root — is deleted.

Checks

ruff check · ruff format --check · pyrefly 0 errors (2 suppressed) · 404 passed, 1 xfailed on dev, and 214 passed, 20 skipped on the rehearsed bare install at the true floors.

Not done, deliberately

The "streaming lane" vocabulary is still used throughout SPEC.md and ARCHITECTURE.md. Emission genuinely is batched, so it is not false, but it reads as out-of-core to someone who has not read Track 5. A rename is a sweep across two specs and does not belong in a hardening PR stacked under a live one — worth its own, and worth deciding rather than drifting.

🤖 Generated with Claude Code

`pr-189-review.md` was a review *of* this branch, committed to its root. Review lives on the PR; the repository keeps the four docs `CLAUDE.md` names and nothing else at that level. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The swap rewrote the engine and left the sentences describing the old one. * `CLAUDE.md` was not touched by #189 at all, and it is the file an agent reads first. It opened by calling the package "a YAML-based math definition layer for linopy" that builds `linopy.Model` objects — the opt-in lane described as the whole product, with the relational path absent. Its module map said `relational/ (duckdb)` and omitted `frames.py`, `chunking.py`, `status.py` and `resolution.py`; its API snippet told the reader to manage a lifetime that no longer exists. * `ARCHITECTURE.md` named `DuckdbExecutor._label_frame` for a class called `PolarsExecutor`, and drew both sinks as `sinks.py` after they became a package. * **"memory-bounded" is no longer true and is the one that could mislead.** The configured ceiling went with duckdb; peak tracks the model now, which ROADMAP Track 5 states plainly while the architecture diagram still promised a bound. * `chunking.py` and `test_relational.py` both cited "hard rule 4" for a claim about peak. Rule 4 is "backend-visible YAML files are self-contained", and no hard rule makes a claim about peak at all — the rule they meant was deleted with the budget. Both now state the narrower thing that is still true: a batched pass holds a bounded slice, whatever the whole engine peaks at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`test_runtime_is_linopy_free` asserted only that linopy and xarray stay out of `sys.modules`, and its docstring explained why pandas could not be on the list: duckdb imported it opportunistically when registering any Python object, so "not in `sys.modules`" was a claim this package could not keep. polars imports neither pandas nor pyarrow until asked, so the stronger claim is available now and this takes it. It is what stops a bridge *out* (`to_pandas`, `to_dataarray`) from quietly becoming something the build path walks over on its own — a regression the bare-install job would catch only if the import also happened to be unconditional. Distinct from, and stronger than, "need not be installed", which the bare-install job proves and this does not replace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`README.md` and `ROADMAP.md` both led on two claims and cited `docs/benchmarks.md` for them: end-to-end peak against the floor under the LP-file route, and marginal cost per model in a loop. **Neither is measured in that file, and neither ever was** — not on this branch and not on `main`. #189 then added an intro promising both "live further down", which nothing further down delivers. They are good claims. They are also the two a reader is most likely to check, which is the worst place to be unbacked. So: * The lead becomes the claim the tables settle — cost to a loaded solver, wall and peak, on the sink most callers reach for, against the eager lane's own best path to the same place. 2-4x faster on four of five cases, lower peak on all five, with the case we lose named. The LP-file route is pointed at rather than folded in, because on one case it is behind on peak and averaging that away is how the first version of this section went wrong. * "Nothing accumulates between builds" stays, relabelled as architectural rather than measured, which is what it is. * `docs/benchmarks.md` gets its "Not measured yet" section back — dropped in the re-measure — with both claims in it, plus `storage`/`roll`, a MILP, and a hand-written highspy arm as the missing denominator. A claim with no table under it should be visible as one. Also here: ROADMAP's ahead-of-comparable list still sold "memory-bounded streaming build" after the ceiling was deleted, and README's install comment still called it the streaming engine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Written for one sink and a scratch database, both of which have moved: * `workdir_bytes` was there so a peak-RSS win could not hide a multi-gigabyte temp file. Nothing spills any more, so it was returning a hardcoded `0` — and a column that always reads 0 is read as "measured and fine", which is the same failure the field existed to prevent. Removed from `_run_case.py`, with a note saying where to put it back. * Teardown is still timed, but it is a tripwire now rather than a cost: if it stops reading ~0, something acquired a lifetime again. * The phase table listed only the LP sink, and "after the clock" listed a `count(*)` and a scratch-dir size. And the footgun, from walking into it: `bench.run` writes the committed `results/latest.jsonl` by default and *replaces* it, so a one-rung smoke test turns 294 records of published provenance into 4. `--out` already exists — the narrow-run examples now use it, and the warning says what the default costs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The bare-install job carries the comment "the lower bounds in pyproject.toml are real. Without this they are decoration" — and it was not testing them. uv locks **universally**, across every declared extra whether or not it is being installed. `uv sync --no-default-groups` therefore still resolves under the `[linopy]` extra's constraints, and linopy 0.9.0 requires `polars>=1.31.1`. The job installed polars 1.32 every time; our own `polars>=1.30` was never once exercised. Exactly the failure mode the comment describes, in the gate that exists to catch it. `uv venv` + `uv pip install --resolution lowest-direct .` resolves the requested set and nothing else. Verified locally: it now installs polars 1.30.0 / numpy 1.26.0 / highspy 1.10.0 / pydantic 2.0 / pyparsing 3.1.0 / pyyaml 6.0, with no linopy, xarray, pandas or pyarrow present, and the suite passes on it. The floors are genuine — this was a hole in the gate, not a wrong bound. Also recorded there: the floors are a 3.11 claim only (pyyaml 6.0 and numpy 1.26.0 have no 3.13 wheel and fall back to a source build that fails), and why pytest goes in as a second unconstrained call rather than alongside the project. In `pyproject.toml`: * `dev` repeated the `[linopy]` extra's three pins verbatim, so what CI tests the lane against could drift from what a user installing the extra gets. It installs `farkas[linopy]` by name now — one place for the pins. * The description sold "at any scale" on "a streaming relational engine". Both were true of the duckdb engine, which built under a configured ceiling and could exceed RAM. This one holds the model it builds, so the description says what is still true. * The `bench` group's comment named memray *and* pytest-benchmark; the group is one package, `pytest-benchmem`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-07-28T08:46:22Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 97032ada-b9f2-4752-bb6f-89d8da27139b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch chore/harden-polars-engine

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

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

Both were written against #231's branch and kept being committed there after that PR was squash-merged, so none of it reached `polars-engine`: `--duckdb-root`, `--builds`, the `loop` mode, `FOREIGN_ARMS` and the marginal cost table were all absent. This lands them on the current base. **The duckdb arm** runs the engine this branch replaces from its own checkout, its own interpreter and its own `bench/_run_case.py`, interleaved with the other two against one parquet cache, with the parity gate spanning all three. **The loop pass** answers the second question the timings cannot: the harness spawns one process per measurement, so every timing is a *first* build, and each lane does lazy first-call work — ~180 ms eager, ~21 ms duckdb, ~4 ms here. `first` is what a caller pays who builds one model and solves it; `steady` is what a rolling horizon pays for every model after. Both are real, so both are reported. It measures build *and* hand-off, not build alone: linopy defers ~82% of its work to whatever consumes the model, so a build-only loop compares our finished matrix against its placeholder. `bench/README.md` keeps #233's structure — narrow runs go to `/tmp` so they cannot clobber the published file — with the three-engine invocation added to the publishing block, which is what it is. No numbers are published here: the base has moved under them twice (#233, #234) and again with #238, and the linopy pin is now the v1-semantics build rather than 0.9.0. The ladder is re-run separately. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Every number in this file predated #233, #234 and #238, and was measured against linopy 0.9.0 rather than the v1-semantics build this branch now pins. Re-taken in one run: five cases, two sinks, three arms, best of three, plus the marginal-cost pass. 408 timings and 72 loop records, no failures. **Ahead on both axes on every case through the hand-off**, which is the sink most callers use: wall 0.35x, 0.32x, 0.25x, 0.45x, 0.86x and peak 0.95x, 0.84x, 0.32x, 0.76x, 0.88x. `profiled` was the one exception until #238 stopped the duplicate-coordinate check grouping 12M rows to answer a yes/no question; it is a win now like the rest. The LP file is the weaker route and stays that way — 0.71x to 1.35x on wall, and `transport` 1.61x on peak — because most of an LP write is float-to-text, work neither lane avoids. Against duckdb: 2.2-5.2x faster on every case and both sinks, and duckdb 1.2-2.9x lighter. duckdb is slower than the eager lane everywhere here. The parity gate agrees across all three arms at 0.0e+00 on every shared case, which is also what proves this branch's v1 semantics (#234) and `main`'s (#239) build the same model rather than merely claiming to. Provenance now names a commit per arm rather than a version, since two of the three arms have no meaningful version string — the duckdb arm is a checkout and the polars arm is an editable install that reports its sync point. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

FBumann and others added 6 commits July 28, 2026 10:44

FBumann merged commit 1fda16b into polars-engine Jul 28, 2026
3 checks passed

FBumann mentioned this pull request Jul 28, 2026

test(bench): recover the duckdb arm and the marginal-cost pass #240

Merged

FBumann mentioned this pull request Jul 28, 2026

docs(bench): re-run all three engines on the current base #243

Merged

FBumann deleted the chore/harden-polars-engine branch July 31, 2026 10:51

Navigation Menu

Uh oh!

chore: harden #189 — deps, docs, and the claims they back - #233

chore: harden #189 — deps, docs, and the claims they back#233
FBumann merged 6 commits into
polars-enginefrom
chore/harden-polars-engine

FBumann commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

FBumann commented Jul 28, 2026

The two findings worth the PR

The rest

Checks

Not done, deliberately

Uh oh!

coderabbitai Bot commented Jul 28, 2026

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant