feat!: rebuild the relational engine on polars - #189
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR migrates relational execution from DuckDB/SQL tables to Polars lazy frames, rewrites model assembly and solver sinks for Polars and direct HiGHS execution, updates result readers and dependencies, adds a sector benchmark case, and revises documentation and tests. ChangesPolars relational execution
Benchmark and documentation updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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 |
Correction: the headline was measured at the wrong pointPushed
A 2.8x build-memory deficit on Also worth knowing: linopy's LP path is not its cost. The same model through And against the duckdb engine, on the handoff
~8x faster handoff. duckdb still wins on peak, but its Measured headroom still on the tableFrames total 0.74 GB at 10M variables; build peaks at 2.12 GB. Most of the difference is the terminal
~25% peak and ~3x build time, for an aggregate that is a no-op in the common case. It is safe to skip when there is exactly one term fragment and no parameter table has duplicate rows for a coordinate — and the second condition is cheap to check (parameter tables are thousands of rows against a 10⁷-row matrix), which also turns a silent data bug into a named error. That is the same shape of argument as #179 does for the objective, and it deserves the same care. Say the word and I will land it here or as a follow-up. |
Landed the optimization —
|
l rung, LP path |
before | after |
|---|---|---|
| dispatch peak | 3.06 GB (1.40x linopy) | 2.28 GB (1.06x) |
| dispatch build | 2.12 GB / 1.82 s | 1.44 GB / 0.59 s |
| transport peak | 4.05 GB | 3.92 GB |
| nodal peak | 1.99 GB | 1.88 GB |
And at the handoff, which is the number that matters:
| build + handoff | farkas | linopy direct |
|---|---|---|
| dispatch 10M | 3.13 GB | 3.38 GB |
| nodal 3M @ 25% | 1.50 GB / 0.89 s | 2.01 GB / 1.26 s |
| transport 9.8M | 3.71 GB | 4.04 GB |
Ahead on all three now. transport still pays full price for the aggregate — three group_sum fragments land on every row, so _needs_aggregate is true and correctly so. That is the obvious next thing to look at.
The premise it rests on, which is worth having anyway
The skip is only sound if a parameter is keyed by its dims, so _check_one_row_per_coordinate now refuses a source carrying a coordinate twice. Two rows for one coordinate has no defined meaning, the eager lane refuses to lay such a source out at all, and the relational lane was silently resolving it into a sum — an actual divergence between two lanes that are supposed to accept the same thing. It costs one pass over the source, which is orders of magnitude smaller than the matrix it saves aggregating.
One thing to flag
My first draft had the condition inverted — it skipped the aggregate exactly when it was needed. The existing 338 tests did not catch it, because none of them writes a variable twice in one row. The two tests I added (x + 2 * x in a constraint, and in an objective) did. That gap is worth knowing about independently of this change.
|
271b0d7 |
fix: the HiGHS sink's column ingest was an unbounded global sort (#181) |
a31645b |
perf(relational): a label is a position, so compute it instead of counting it (#186) |
b1df538 |
perf(lp): render doubles with ::VARCHAR instead of printf('%.17g') (#190) |
787b1ce |
perf: skip the objective's GROUP BY where a column cannot repeat (#179) |
ac64f26 |
perf(sink): hand Arrow to numpy directly, not through to_pydict (#193) |
284df79 |
feat: expose duals on the solve path (sol.dual) (#156) |
Claims that are now void
"~8x faster handoff than duckdb" (2.81 s vs 22.44 s) — treat as withdrawn. 271b0d7 names precisely the bug I attributed that gap to. Until re-measured at alpha.24 it should not be relied on.
a31645b attacks label assignment, which was the other build-side cost I measured; 787b1ce is the same idea my keyed mechanism generalises. Every polars-vs-duckdb comparison here needs re-running.
Claims that still stand
Everything measured against linopy 0.9.0, which is identical in both runs:
- below ~1M variables, 2–3x faster and lower peak on all three cases;
- at the handoff (
Highs.runstubbed): 3.13 vs 3.38 GB on dispatch, 1.50 vs 2.01 GB on nodal, 3.71 vs 4.04 GB on transport — ahead on all three; io_api='lp'costs linopy 6.92 GB / 55 s against 3.38 GB / 2.9 s direct.
Blocking work before this is mergeable
- Rebase onto
v0.0.0-alpha.24. Not mechanical — the duckdb engine gained ~800 lines inexecutor.py/sinks/that this branch deletes wholesale. sol.dualis missing.284df79shipped duals on the solve path; the polars engine has no equivalent, so merging as-is is a feature regression.- Drop
e544400and653ef88.e92ba90fixed the bench parity gate upstream, so those two are now redundant and will conflict. - Re-measure duckdb at alpha.24 and rewrite
docs/benchmarks.md.
Until 1–4 are done this is a proposal with a stale baseline, not a merge candidate. Flagging rather than quietly rebasing, because (2) is a scope question and (4) may change the conclusion.
Re-measured against
|
| duckdb @ alpha.24 | polars (this branch) | linopy | |
|---|---|---|---|
| dispatch 10M | 7.38 s / 0.88 GB | 3.87 s / 2.28 GB | 3.02 s / 2.15 GB |
| nodal 3M @ 25% | 3.69 s / 0.59 GB | 1.99 s / 1.88 GB | 1.35 s / 1.57 GB |
| transport 9.8M | 6.31 s / 1.16 GB | 5.49 s / 3.92 GB | 3.46 s / 1.89 GB |
Build + COO handoff to HiGHS (run stubbed)
| duckdb @ alpha.24 | polars | linopy direct |
|
|---|---|---|---|
| dispatch 10M | 2.00 GB / 3.72 s | 3.13 GB / 3.55 s | 3.38 GB / 2.92 s |
| nodal 3M @ 25% | 0.75 GB / 2.36 s | 1.50 GB / 0.89 s | 2.01 GB / 1.26 s |
| transport 9.8M | 1.90 GB / 4.02 s | 3.71 GB / 2.69 s | 4.04 GB / 2.31 s |
What this actually says
The "duckdb is slow" case is gone. 271b0d7 did fix the unbounded sort: the dispatch handoff went 22.44 s → 3.72 s, against polars' 3.55 s. That is level, not 8x. I withdraw that claim in full.
The trade at alpha.24 is: polars ~1.5–2x faster, duckdb ~2–3x lighter.
- Speed, polars: 1.9x on dispatch LP, 1.9x on nodal LP, 1.15x on transport LP; at the handoff, level on dispatch, 2.6x on nodal, 1.5x on transport.
- Memory, duckdb: 2.6x less on dispatch LP, 3.2x on nodal, 3.4x on transport; 1.6–2x less at the handoff.
And duckdb at alpha.24 now beats the eager lane on memory outright — 0.88 GB against linopy's 2.15 GB on dispatch — which is the claim docs/benchmarks.md on main was already making and which this branch would give up.
Where that leaves the swap
The case I opened this PR on was largely a duckdb-performance problem that main has since fixed. What survives on the merits:
- the deletions —
sql.py, the identifier restriction on declared names, the chunking, the lifetime, two dependencies; - speed, consistently, most clearly on the sparse case;
- a smaller install.
Against a 2–3x memory regression that is structural (COO triples vs dense floats), not a bug I can find and fix.
That is a real decision, not a formality, and it is yours. I would not merge this on the numbers alone — the honest reading is that duckdb wins the axis this project leads on, and polars wins the one it does not currently claim.
Continuing with sol.dual on the polars engine either way, so the branch is a like-for-like comparison rather than one missing a feature.
Duals ported —
|
Replaces duckdb with polars as the engine the relational lane runs on, rebased
onto v0.0.0-alpha.24. The language does not move: plan.py, lowering.py, the
schema, both parsers, resolution, dimensions, piecewise and the whole linopy
lane are untouched, and the differential suite passes on all three benchmark
cases including transport, the group_sum shape.
BREAKING: `primal()` and `dual()` return a `polars.DataFrame`; the
`memory_limit`, `chunk_rows`, `threads` and `workdir` build knobs are gone.
What the swap deletes:
- `relational/sql.py` and every quoting rule in it. An identifier is a value
now, not syntax.
- The `[A-Za-z_][A-Za-z0-9_]*` restriction the engine imposed on every declared
name — a *language* restriction that existed only because names were spliced
into query text. A dimension may be called `from` or `order`.
- All hand-managed chunking, and the label window it existed for.
- The lifetime apparatus: workdir, tempdir, weakref.finalize, `_release`.
- Two dependencies: duckdb and pyarrow. The bare-install job proves the suite
runs with neither, nor pandas, nor linopy, nor xarray (210 tests).
What it costs, measured against the tag rather than the old base:
build + LP file duckdb a24 polars linopy
dispatch 10M 7.38s / 0.88 GB 3.87s / 2.28 GB 3.02s / 2.15 GB
nodal 3M @ 25% 3.69s / 0.59 GB 1.99s / 1.88 GB 1.35s / 1.57 GB
transport 9.8M 6.31s / 1.16 GB 5.49s / 3.92 GB 3.46s / 1.89 GB
build + handoff duckdb a24 polars linopy direct
dispatch 10M 2.00 GB / 3.72s 3.13 GB / 3.55s 3.38 GB / 2.92s
nodal 3M @ 25% 0.75 GB / 2.36s 1.50 GB / 0.89s 2.01 GB / 1.26s
transport 9.8M 1.90 GB / 4.02s 3.71 GB / 2.69s 4.04 GB / 2.31s
polars is 1.5-2x faster; duckdb is 2-3x lighter, and after #181/#186/#190/#179
it beats the eager lane on memory outright. The trade is real and the memory
side is structural — COO carries a (row, col, coeff) triple per nonzero where a
dense array carries one float — so hard rule 4 is rewritten to say what is true
now (peak tracks the model, not a configured number) and ROADMAP Track 5
becomes the memory axis, naming partition-wise execution as the way to get a
ceiling back.
Carried across from main and re-proved on this engine:
- `sol.dual` (#156), the same label join against the row frame, still refusing
rather than zero-filling and still naming the integer variable responsible.
- The LP-text round-trip properties (#190). Porting them caught a real bug: my
`_signed` prefixed `+` to whatever the cast produced, so `-0.0` became
`+-0.0`, which no LP parser accepts.
- The objective-aggregate tests (#179). Porting *those* caught a worse one — a
fragment's dims can equal its variable's while its rows do not, so a term now
tracks which of its dims its label actually determines (`label_dims`) rather
than assuming every reduction is safe.
- The regression suite and the build profiler, the latter rewritten to wrap
`LazyFrame.collect` instead of `DuckDBPyConnection.execute`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every explanatory comment inside a function body is gone from the engine.
Where one was load-bearing it became a docstring on the code it described, and
where the code needed the comment to be followed it became a named helper:
`TermFragment.survives_dropping`, `_relabel`, `_series_to_frame`, `_status_of`,
`_constraint_blocks`, `_sorted_terms`. Only section banners remain.
Docstrings trimmed with them — 939 lines to 769 across the six engine modules,
and the module headers cut hardest, since they were restating the architecture
docs rather than saying what the module does.
Tidying the compiler broke it once, which is worth recording: collapsing
condition = walk(pred)
return carrier, condition
into `return carrier, walk(pred)` reads better and is wrong. Walking the
predicate is what joins the mask's parameters onto `carrier`, and a tuple
evaluates left to right, so the one-expression form returns the frame as it was
before the walk. 30 tests caught it. The two-line form is back and its docstring
now says why it is two lines.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…one that failed
Build memory alone is out of the headlines. It is a small fraction of what
solving costs, so shrinking it further changes nothing a caller feels. What
replaces it: end-to-end peak against the floor under the LP-file route, and
marginal cost per model in a loop. Both are checkable in a command, and neither
moves when either library ships a release.
Also ran the density sweep, which refuses the prediction written into the case:
installed live wall farkas / linopy peak farkas / linopy
12/12 1.20M 0.58 s / 0.59 s 0.68 GB / 0.63 GB
6/12 0.60M 0.38 s / 0.45 s 0.52 GB / 0.60 GB
3/12 0.30M 0.35 s / 0.41 s 0.46 GB / 0.45 GB
1/12 0.10M 0.19 s / 0.32 s 0.40 GB / 0.35 GB
linopy's peak falls with density too, and at the sparsest rung ends up below
ours. So "an absent pair is a row we skip and a NaN they pay for" is not what
the memory does. Wall time is the axis that behaves — our margin grows 1.0x to
1.7x as the model thins.
Kept and written down rather than dropped: the case is the shape real
multi-node models have, and it is a throughput measurement, not evidence about
representations.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`nodal` is sparse everywhere it can be, `dispatch` dense everywhere, and
neither is what a sector-coupled model looks like. `sector` crosses them: 50
nodes x 12 technologies at 8% installed, 5 dense carriers, dense snapshots. The
shape that matters is `sum(p * produces, over=tech)` — a dense dim broadcast
onto a sparse frame, then the sparse one reduced away.
variables wall farkas / linopy peak farkas / linopy
4k 0.11 s / 0.32 s 0.17 GB / 0.21 GB
10k 0.10 s / 0.31 s 0.20 GB / 0.24 GB
100k 0.25 s / 0.44 s 0.42 GB / 0.53 GB
1M 1.58 s / 1.75 s 1.27 GB / 2.79 GB
2.2x less memory at the top rung, which is the clearest result in the file, and
it rescues the density sweep committed one change ago. That sweep holds the
coordinate product at 1.2M, where a dense array over it is ~10 MB and the
interpreter dominates — so it could not have shown the effect whatever the
density. `sector` runs the same 8% at a 12M product and it is unmistakable. The
claim needs both halves: low density *and* a product large enough for it to
cost anything. Both files now say so.
Two pre-existing divergences turned up while writing the model, neither caused
by nor fixed here, both worth their own issue:
1. `x * a + y * b` with disjoint dims in an objective. The relational lane sums
the fragments (32); the eager lane broadcasts them together (66). duckdb at
alpha.24 also answers 32, so this is lane-level, not engine-level — a hard
rule 3 violation.
2. A second `equations:` entry under one objective is silently dropped by both
lanes. It should sum or be refused, not ignored.
The case avoids both: one objective equation, one dim set, and `where: demand`
on the balance so an unreachable (node, carrier) pair is no row rather than a
vacuous one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… is not a solve Two defects, one visible symptom. Reported in review of #189, reproduced, and both fixed. **The wrong model.** `_group_fragment` passed `keyed` through unchanged. Grouping merges labels of `over` into one `into`, and whether that merges rows carrying the *same* `var_label` depends on where `over` came from. When the variable carries it the merged rows have distinct labels and the key survives; when it arrived by broadcast they do not. `group_sum(x * w, over=generator, by=bus)` with `x` indexed by snapshot alone is the reachable case: two generators on one bus put one column on a row twice, and `_needs_aggregate` had already decided not to collapse them. matrix, before matrix, after (0, 0, 1.0) (0, 0, 3.0) (0, 0, 2.0) (1, 0, 5.0) (1, 0, 5.0) **The silent answer.** HiGHS rejects a row list carrying a column twice — by return value, then carrying on with an empty model. `addCols`/`addRows` went unchecked, so on an LP that surfaced as an unrelated ShapeError when the dual read-back found no duals, and on a MIP as `ok / optimal / 20.0` for a model whose answer is 6.0: a confident solution to an unconstrained problem. The status is checked now and a refused batch raises. Unchecked on main too, but this branch made it load-bearing. Neither is covered by the differential suite. `transport` has three term fragments per balance, so `len(terms) != 1` forces the aggregate regardless and the elision never runs on the shape that breaks it. Three tests added: the broadcast case end to end, its `foreach` counterpart, and the flag itself at the compiler. All three fail with the fix reverted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
src/farkas/relational/sinks/highs.py (1)
99-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPer-chunk
filterrescans both frames once per batch.
ordered_rowsandordered_matrixare already sorted and eager, androwis dense0..n-1, so each chunk's slice boundaries are known without a predicate pass. As written a model withrow_count / batch_rowschunks pays a full scan of the matrix per chunk — the cost grows with the square of the model on the hot handoff path the PR is measuring.♻️ Slice instead of filter
ordered_rows = model.rows.sort('row') ordered_matrix = model.matrix.sort('row') + matrix_row = ordered_matrix['row'].to_numpy() for lo, hi in model.row_chunks(batch_rows): - rows = ordered_rows.filter(pl.col('row').is_between(lo, hi, closed='left')) - a = ordered_matrix.filter(pl.col('row').is_between(lo, hi, closed='left')) + rows = ordered_rows.slice(lo, hi - lo) # `row` is dense 0..n-1 + start, stop = np.searchsorted(matrix_row, [lo, hi]) + a = ordered_matrix.slice(int(start), int(stop - start))🤖 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/sinks/highs.py` around lines 99 - 108, Replace the per-chunk predicate filters in the row-processing loop with positional slices derived from each chunk’s dense row bounds, applying the same slice boundaries to ordered_rows and ordered_matrix. Preserve the existing rhs, sense, bound, and starts calculations while ensuring each chunk only scans its corresponding rows.tests/test_lp_text.py (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing type annotation on
renderparameter.
_rendered'srenderargument is untyped, unlike other newly-added helpers in this PR (e.g.columns/query/joinsintests/test_compiler.py). As per coding guidelines, "Keep Pyrefly strict type checking at zero errors; fix types rather than widening them."♻️ Suggested typing fix
-def _rendered(render, values: list[float]) -> list[str]: +def _rendered(render: Callable[[pl.Expr], pl.Expr], values: list[float]) -> list[str]:🤖 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_lp_text.py` around lines 42 - 45, Add an explicit callable type annotation to the render parameter of _rendered, matching the expression type accepted by frame.select and preserving strict Pyrefly checking without using an untyped or overly broad type.Source: Coding guidelines
🤖 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 `@bench/cases.py`:
- Around line 485-499: Update the explanatory comment in the sector Case
definition to remove the nonexistent `shed` reference and describe only the
model variables actually defined in sector.yaml, while preserving the stated
density characteristics for `p` and `balance`.
In `@bench/README.md`:
- Around line 58-60: Update the nearby “after the clock” row and “Memory is not
the whole cost” paragraph in bench/README.md to remove stale scratch-database,
SQL count, scratch-directory, and workdir_bytes disk-tracking references. Align
the documentation with bench/_run_case.py reporting workdir_bytes as 0 and the
current Polars engine, without changing unrelated benchmark guidance.
In `@docs/benchmarks.md`:
- Around line 14-21: The benchmark documentation and committed fixture must
describe the same complete run. In docs/benchmarks.md lines 14-21, include
sector in the reproduction command and update the parity-gate summary to match
all cases and the regenerated results; in bench/results/latest.jsonl lines 1-18,
regenerate and commit a full benchmark run containing gate and timing records
for every case shown in the documentation, including dispatch, nodal, transport,
and sector.
- Around line 45-58: Update the first variables label in the sector benchmark
table from “4k” to “1k”; leave its measured wall-time and peak-RSS values
unchanged, since they correspond to the 1000-column benchmark record. Preserve
the existing “10k”, “100k”, and “1M” labels.
In `@src/farkas/relational/compiler.py`:
- Around line 377-383: Add compile-time validation in the group_sum flow before
building the mapping or join: reject cases where g.into already exists in
p.dims. Raise a clear LanguageError naming the GroupSum construct and
conflicting target dimension, alongside the existing shape validation, so the
duplicate-column path is never reached.
In `@src/farkas/relational/executor.py`:
- Around line 421-426: Update _check_coordinates_single_valued so its per-column
distinct-value count excludes null coordinates before applying the n > 1
disagreement check. Preserve the existing validation for multiple distinct
non-null values while allowing labels with real values mixed with null rows,
consistent with _check_coordinate_containment.
- Around line 600-606: Update the objective aggregation decision in the shown
executor flow to account for the dimensions dropped before returning the
collected frame: use each TermFragment’s survives_dropping behavior on the
implicit-sum dimensions rather than relying only on
_needs_aggregate(comp.terms). Ensure objectives such as x over i multiplied by w
over (i, j) aggregate duplicate var_label rows before solve_direct joins them,
and add the corresponding regression test beside
test_an_objective_naming_a_variable_twice_sums_its_coefficients.
- Around line 365-366: Update the derived-dimension construction in the executor
around stacked, unique, and row indexing to filter out null values from the
selected dimension column before deduplication, sorting, and ordinal assignment,
matching _check_coordinate_containment’s absent-value behavior. Also validate
contributing parameter dimension columns at load time for compatible integer
widths and raise an actionable error naming both the dimension and conflicting
parameter names instead of allowing pl.concat to emit a generic schema error.
In `@src/farkas/relational/sinks/highs.py`:
- Around line 89-90: The assembly validation must reject null and NaN values in
cols, rows, and matrix, naming the relevant declaration; update _build_variable
and the corresponding assembly path so coeff and rhs are validated as well as
lb/ub. At src/farkas/relational/sinks/highs.py lines 89-90, preserve NaN by
passing nan=np.nan to np.nan_to_num so only infinities are substituted. At
src/farkas/relational/sinks/lp_file.py lines 144-163, make no direct change
because assembly validation makes the existing null/inf/NaN rendering paths
unreachable.
- Around line 93-97: Capture the return status from changeColsIntegrality in the
noncontinuous-column branch and combine it with the existing _loaded state, so a
rejected integrality update marks the model as not successfully loaded. Preserve
the current column selection and integrality values, and ensure subsequent
solve/reporting follows the existing _loaded failure path.
- Around line 163-167: Update the SolveStatus construction after
h.getModelStatus() so _CONDITION_OF_HIGHS_STATUS is keyed using the canonical
value returned by h.modelStatusToString(model_status), reusing that value for
solver_wording instead of parsing str(model_status). Preserve the existing
unknown fallback and primal-solution handling.
In `@src/farkas/relational/sinks/lp_file.py`:
- Around line 98-107: Update the row aggregation in the model-writing flow to
call group_by('row', maintain_order=True), preserving the `_sorted_terms` column
order when constructing the `body` string while retaining the existing final row
sort.
---
Nitpick comments:
In `@src/farkas/relational/sinks/highs.py`:
- Around line 99-108: Replace the per-chunk predicate filters in the
row-processing loop with positional slices derived from each chunk’s dense row
bounds, applying the same slice boundaries to ordered_rows and ordered_matrix.
Preserve the existing rhs, sense, bound, and starts calculations while ensuring
each chunk only scans its corresponding rows.
In `@tests/test_lp_text.py`:
- Around line 42-45: Add an explicit callable type annotation to the render
parameter of _rendered, matching the expression type accepted by frame.select
and preserving strict Pyrefly checking without using an untyped or overly broad
type.
🪄 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: acafe344-b1cb-4e28-b2e0-2536615e84de
⛔ Files ignored due to path filters (1)
examples/walkthrough.outis excluded by!**/*.out
📒 Files selected for processing (49)
ARCHITECTURE.mdREADME.mdROADMAP.mdSPEC.mdbench/README.mdbench/_run_case.pybench/cases.pybench/models/sector.yamlbench/profile_build.pybench/regressions/test_build.pybench/report.pybench/results/latest.jsonlbench/run.pydocs/benchmarks.mdexamples/walkthrough.pypyproject.tomlsrc/farkas/__init__.pysrc/farkas/api.pysrc/farkas/piecewise.pysrc/farkas/relational/__init__.pysrc/farkas/relational/arrow.pysrc/farkas/relational/compiler.pysrc/farkas/relational/executor.pysrc/farkas/relational/frames.pysrc/farkas/relational/sinks/highs.pysrc/farkas/relational/sinks/lp_file.pysrc/farkas/relational/sinks/tables.pysrc/farkas/relational/sql.pysrc/farkas/sources.pytests/conftest.pytests/differential.pytests/oracle.pytests/test_api.pytests/test_compiler.pytests/test_doc_examples.pytests/test_duals.pytests/test_expansion.pytests/test_group_sum.pytests/test_linopy_lane.pytests/test_lowering.pytests/test_lp_text.pytests/test_milp.pytests/test_piecewise.pytests/test_relational.pytests/test_resolution_parity.pytests/test_roll.pytests/test_solve_status.pytests/test_validation.pytests/test_walkthrough.py
💤 Files with no reviewable changes (2)
- src/farkas/relational/sql.py
- src/farkas/relational/arrow.py
| if g.over not in p.dims: | ||
| raise LanguageError(f"in {context}: GroupSum over '{g.over}' but the expression has dims {list(p.dims)}") | ||
| keep = tuple(x for x in p.dims if x != g.over) | ||
| valcols = 't.var_label, t.coeff' if p.is_term else 't.cval' | ||
| keepcols = ', '.join([*(f't.{x}' for x in keep), f'g."{g.coordinate}" AS {g.into}', valcols]) | ||
| sql = f'SELECT {keepcols} FROM ({p.sql}) t JOIN dim_{g.over} g ON g.val = t.{g.over}' | ||
| return TermFragment((*keep, g.into), sql, p.is_term) | ||
| mapping = self.dimensions[g.over].select(pl.col('val').alias(g.over), pl.col(g.coordinate).alias(g.into)) | ||
| frame = p.frame.join(mapping, on=g.over, how='inner').select(*keep, g.into, *p.carried) | ||
| keyed = p.keyed and g.over in p.label_dims | ||
| return TermFragment((*keep, g.into), frame, p.is_term, keyed, _relabel(p.label_dims, g.over, g.into)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
group_sum breaks when the fragment already carries the target dim.
If g.into is already in p.dims (e.g. group_sum(f, over=line, by=bus) on a fragment indexed by (line, bus)), keep retains bus, the joined mapping also contributes a bus (suffixed bus_right by the join), and select(*keep, g.into, ...) then names bus twice — polars raises a duplicate-column error with no reference to the model. Either reject this at compile time with a message naming the construct, or decide and implement the merge semantics explicitly.
As per coding guidelines, "Perform all validation at load time and provide clear, actionable error messages."
🛡️ Reject it where the other shape errors are raised
if g.over not in p.dims:
raise LanguageError(f"in {context}: GroupSum over '{g.over}' but the expression has dims {list(p.dims)}")
+ if g.into in p.dims:
+ raise LanguageError(
+ f"in {context}: GroupSum over '{g.over}' targets '{g.into}', which the expression "
+ f'already carries {list(p.dims)} — sum over one of them first'
+ )
keep = tuple(x for x in p.dims if x != g.over)📝 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.
| if g.over not in p.dims: | |
| raise LanguageError(f"in {context}: GroupSum over '{g.over}' but the expression has dims {list(p.dims)}") | |
| keep = tuple(x for x in p.dims if x != g.over) | |
| valcols = 't.var_label, t.coeff' if p.is_term else 't.cval' | |
| keepcols = ', '.join([*(f't.{x}' for x in keep), f'g."{g.coordinate}" AS {g.into}', valcols]) | |
| sql = f'SELECT {keepcols} FROM ({p.sql}) t JOIN dim_{g.over} g ON g.val = t.{g.over}' | |
| return TermFragment((*keep, g.into), sql, p.is_term) | |
| mapping = self.dimensions[g.over].select(pl.col('val').alias(g.over), pl.col(g.coordinate).alias(g.into)) | |
| frame = p.frame.join(mapping, on=g.over, how='inner').select(*keep, g.into, *p.carried) | |
| keyed = p.keyed and g.over in p.label_dims | |
| return TermFragment((*keep, g.into), frame, p.is_term, keyed, _relabel(p.label_dims, g.over, g.into)) | |
| if g.over not in p.dims: | |
| raise LanguageError(f"in {context}: GroupSum over '{g.over}' but the expression has dims {list(p.dims)}") | |
| if g.into in p.dims: | |
| raise LanguageError( | |
| f"in {context}: GroupSum over '{g.over}' targets '{g.into}', which the expression " | |
| f"already carries {list(p.dims)} — sum over one of them first" | |
| ) | |
| keep = tuple(x for x in p.dims if x != g.over) | |
| mapping = self.dimensions[g.over].select(pl.col('val').alias(g.over), pl.col(g.coordinate).alias(g.into)) | |
| frame = p.frame.join(mapping, on=g.over, how='inner').select(*keep, g.into, *p.carried) | |
| keyed = p.keyed and g.over in p.label_dims | |
| return TermFragment((*keep, g.into), frame, p.is_term, keyed, _relabel(p.label_dims, g.over, g.into)) |
🤖 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/compiler.py` around lines 377 - 383, Add compile-time
validation in the group_sum flow before building the mapping or join: reject
cases where g.into already exists in p.dims. Raise a clear LanguageError naming
the GroupSum construct and conflicting target dimension, alongside the existing
shape validation, so the duplicate-column path is never reached.
Source: Coding guidelines
| noncontinuous = np.flatnonzero(batch['vtype'].to_numpy() != 'continuous') | ||
| if len(noncontinuous): | ||
| cols_idx = batch['col'].to_numpy().astype(np.int32)[noncontinuous] | ||
| integrality = np.full(len(noncontinuous), int(highspy.HighsVarType.kInteger), dtype=np.uint8) | ||
| h.changeColsIntegrality(len(noncontinuous), cols_idx, integrality) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
changeColsIntegrality is the one load whose status is dropped.
_loaded exists precisely because HiGHS reports a rejected batch by return value; if this call fails the model quietly solves as its LP relaxation and reports optimal, which is the failure mode the helper's own docstring describes.
🛡️ Route it through `_loaded` too
- h.changeColsIntegrality(len(noncontinuous), cols_idx, integrality)
+ _loaded(
+ h,
+ h.changeColsIntegrality(len(noncontinuous), cols_idx, integrality),
+ 'column integrality',
+ )📝 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.
| noncontinuous = np.flatnonzero(batch['vtype'].to_numpy() != 'continuous') | |
| if len(noncontinuous): | |
| cols_idx = batch['col'].to_numpy().astype(np.int32)[noncontinuous] | |
| integrality = np.full(len(noncontinuous), int(highspy.HighsVarType.kInteger), dtype=np.uint8) | |
| h.changeColsIntegrality(len(noncontinuous), cols_idx, integrality) | |
| noncontinuous = np.flatnonzero(batch['vtype'].to_numpy() != 'continuous') | |
| if len(noncontinuous): | |
| cols_idx = batch['col'].to_numpy().astype(np.int32)[noncontinuous] | |
| integrality = np.full(len(noncontinuous), int(highspy.HighsVarType.kInteger), dtype=np.uint8) | |
| _loaded( | |
| h, | |
| h.changeColsIntegrality(len(noncontinuous), cols_idx, integrality), | |
| 'column integrality', | |
| ) |
🤖 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/sinks/highs.py` around lines 93 - 97, Capture the
return status from changeColsIntegrality in the noncontinuous-column branch and
combine it with the existing _loaded state, so a rejected integrality update
marks the model as not successfully loaded. Preserve the current column
selection and integrality values, and ensure subsequent solve/reporting follows
the existing _loaded failure path.
| model_status = h.getModelStatus() | ||
| return SolveStatus( | ||
| termination_condition=_CONDITION_OF_HIGHS_STATUS.get(str(model_status).rsplit('.', 1)[-1], 'unknown'), | ||
| solver_wording=h.modelStatusToString(model_status), | ||
| has_primal=h.getInfo().primal_solution_status == int(highspy.SolutionStatus.kSolutionStatusFeasible), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
highspy HighsModelStatus enum .name attribute str representation
💡 Result:
In the highspy library, HighsModelStatus is an enumeration [1][2]. Because it is implemented as a C++ enum exposed to Python, it does not behave exactly like a standard Python 3.4+ enum.Member; consequently, accessing the.name attribute directly on an instance of HighsModelStatus will generally fail as it is not a native Python Enum type [3]. To obtain a string representation of a HighsModelStatus value, you should use the designated utility function provided by the highspy library rather than relying on.name or direct string conversion [4]. Specifically: 1. Use the method highspy.Highs.modelStatusToString(status_value) to retrieve the human-readable string representation of a model status [4]. Example usage: import highspy h = highspy.Highs #... solve model... model_status = h.getModelStatus status_str = h.modelStatusToString(model_status) print(status_str) Attempting to access.name or use str directly on the enum member may not provide the expected string name associated with the HiGHS status constants (e.g., 'kOptimal', 'kInfeasible') [3].
Citations:
- 1: https://ergo-code.github.io/HiGHS/stable/structures/enums/
- 2: https://ergo-code.github.io/HiGHS/dev/structures/enums/
- 3: Fix: handle HighsModelStatus.kSolutionLimit like kIterationLimit Pyomo/pyomo#3634
- 4: https://ergo-code.github.io/HiGHS/stable/interfaces/python/example-py/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant file snippet =="
sed -n '120,185p' src/farkas/relational/sinks/highs.py
echo
echo "== highspy dependency declarations =="
rg -n "highspy|highs" pyproject.toml setup.cfg setup.py requirements* .github workflow 2>/dev/null || true
echo
echo "== package availability and enum attrs =="
python3 - <<'PY'
try:
import highspy
print("highspy_version=", getattr(highspy, "__version__", "unknown"))
statuses = [
highspy.HighsModelStatus.kOptimal,
highspy.HighsModelStatus.kInfeasible,
]
for s in statuses:
print(type(s), repr(s), str(s), int(s))
for a in ["name", "value", "name()"]:
try:
print(f" attr={a}: {getattr(s, a)() if callable(getattr(s, a)) else getattr(s, a)}")
except Exception as e:
print(f" attr={a}: {e.__class__.__name__}: {e}")
except Exception as e:
print(f"FAILED: {e.__class__.__name__}: {e}")
PYRepository: FBumann/farkas
Length of output: 2946
🌐 Web query:
highspy Python HighspySource.cpp HighsModelStatus enum __str__ pybind11
💡 Result:
In the highspy Python package, the HighsModelStatus enum does not automatically provide a human-readable string representation via a built-in str method on the enum itself [1][2]. Instead, the package provides explicit helper functions to convert status codes into descriptive strings [3][4]. To get a human-readable string for a HighsModelStatus instance, you should use the corresponding conversion method provided by the Highs object [3][4]. Example usage: import highspy h = highspy.Highs #... after running the solver... model_status = h.getModelStatus print(h.modelStatusToString(model_status)) While pybind11 supports defining custom str methods for C++ enums exposed to Python, highspy follows a pattern of using explicit conversion functions like modelStatusToString (and similar methods for other statuses like solutionStatusToString or basisValidityToString) rather than relying on the default enum string representation [3][4][5]. If you were to implement this in a custom pybind11 binding, you would typically use.def("str",...) on the py::enum_ object, provided the pybind11 version supports the desired override behavior [6]. In the context of the official highspy package, the conversion utility is the intended approach [3][4].
Citations:
- 1: https://ergo-code.github.io/HiGHS/stable/structures/enums/
- 2: https://ergo-code.github.io/HiGHS/dev/structures/enums/
- 3: https://ergo-code.github.io/HiGHS/stable/interfaces/python/example-py/
- 4: https://github.com/ERGO-Code/HiGHS/blob/master/examples/call_highs_from_python_highspy.py
- 5: https://github.com/SUM156/Product-Recommendation-Engine/blob/main/venv/Lib/site-packages/scipy/optimize/_highspy/_highs_wrapper.py
- 6: [BUG] Allow for __str__ to be usable with enums (again) pybind/pybind11#2537
🌐 Web query:
PyPI highspy 1.10 HighspySource.cpp HighsModelStatus.py
💡 Result:
Highspy is the Python interface for the HiGHS optimization solver [1][2]. Version 1.10.0 of highspy was released on March 20, 2025 [2]. The terms you mentioned relate to the internal structure and usage of the library: 1. HighspySource.cpp: This is likely a reference to the C++ source files within the HiGHS project that highspy wraps. Highspy uses pybind11 to create a thin Python wrapper around the native HiGHS C++ codebase [1][3][4]. 2. HighsModelStatus.py: This refers to the Python-exposed enumeration used to check the status of an optimization model after it has been solved [5][6]. In practice, you typically access this status via the highspy package (e.g., highspy.HighsModelStatus) to determine if a model is optimal, infeasible, unbounded, etc. [5][6]. For example, when using highspy, you can check the model status as follows [6]: import highspy h = highspy.Highs #... (define and solve model) model_status = h.getModelStatus if model_status == highspy.HighsModelStatus.kOptimal: print("Model solved to optimality.")
Citations:
- 1: https://pypi.org/project/highspy/1.10.0/
- 2: https://pypi.org/project/highspy/
- 3: https://pypi.org/project/highspy/1.11.0/
- 4: https://workshop24.highs.dev/tutorial
- 5: https://ergo-code.github.io/HiGHS/dev/structures/enums/
- 6: https://github.com/ERGO-Code/HiGHS/blob/master/examples/call_highs_from_python.py
🌐 Web query:
"highspy" "class HighsModelStatus" "__repr__"
💡 Result:
In the highspy library, HighsModelStatus is an enumeration that represents the status of a model after the solver has been run [1][2]. While Python enums typically have a repr method that returns a descriptive string representation of the enum member, the highspy documentation does not explicitly document a custom repr implementation for HighsModelStatus [1]. In practice, users generally interact with HighsModelStatus by converting the status to a human-readable string using the modelStatusToString method provided by the Highs class [3][4]. For example: import highspy h = highspy.Highs #... (model configuration and run) model_status = h.getModelStatus print(h.modelStatusToString(model_status)) This approach is the standard and recommended way to output or log the status of a model, as it returns a clear, descriptive name (e.g., kOptimal, kInfeasible) corresponding to the enum value [1][3].
Citations:
- 1: https://ergo-code.github.io/HiGHS/stable/structures/enums/
- 2: https://ergo-code.github.io/HiGHS/stable/guide/basic/
- 3: https://ergo-code.github.io/HiGHS/stable/interfaces/python/example-py/
- 4: https://github.com/ERGO-Code/HiGHS/blob/master/examples/call_highs_from_python_highspy.py
🌐 Web query:
"highspy" "enum class HighsModelStatus" "__str__"
💡 Result:
In the highspy Python interface, the HighsModelStatus enum class does not provide a direct str method to convert its members to human-readable strings [1][2]. Instead, to convert a HighsModelStatus value to a string, you should use the modelStatusToString method provided by the Highs class instance [3][4][5]. Example usage: import highspy h = highspy.Highs #... solve the model... model_status = h.getModelStatus print(h.modelStatusToString(model_status)) Using h.modelStatusToString is the standard approach for obtaining descriptive, string-based representations of model status enum values within the highspy library [3][4].
Citations:
- 1: https://ergo-code.github.io/HiGHS/stable/structures/enums/
- 2: https://ergo-code.github.io/HiGHS/dev/structures/enums/
- 3: https://ergo-code.github.io/HiGHS/stable/interfaces/python/example-py/
- 4: https://github.com/ERGO-Code/HiGHS/blob/master/examples/call_highs_from_python.py
- 5: https://github.com/ERGO-Code/HiGHS/blob/master/examples/call_highs_from_python_highspy.py
🌐 Web query:
repository: ERGO-Code/HiGHS HighspySource.cpp py::enum_ HighsModelStatus modelStatusToString
💡 Result:
In the HiGHS optimization software project, HighspySource.cpp is a C++ source file that facilitates the Python interface (highspy) using pybind11 [1][2][3]. The code snippet py::enum_(m, "HighsModelStatus") is used to bind the C++ HighsModelStatus enumeration to Python, allowing users to access model status constants directly within their Python code [4][5]. The function modelStatusToString is a utility exposed through this interface that converts a HighsModelStatus enum value into its corresponding human-readable string representation (e.g., kOptimal becomes 'Optimal') [6]. Users can call this function from Python—typically as h.modelStatusToString(status)—to interpret the numerical or enum-based status returned by the solver [6]. Key points: - Purpose: Provides a bridge between C++ internal model status codes and Python-readable strings for debugging and result interpretation [6]. - Binding: The py::enum_ construct is part of pybind11, which creates a Python-compatible wrapper for the C++ HighsModelStatus type [5][3]. - Usage: It is commonly used in conjunction with h.getModelStatus to report the solver's outcome [6]. Technical documentation for the HiGHS project and its enums is available on the official HiGHS documentation site [4][2].
Citations:
- 1: https://github.com/ERGO-Code/HiGHS
- 2: https://ergo-code.github.io/HiGHS/dev/
- 3: https://pypi.org/project/highspy/
- 4: https://ergo-code.github.io/HiGHS/dev/structures/enums/
- 5: https://repology.org/project/python%3Ahighspy/information
- 6: https://ergo-code.github.io/HiGHS/dev/interfaces/python/example-py/
🌐 Web query:
github highspy 1.10.0 source Highspysources.pyi enum.__str__
💡 Result:
In the highspy Python package, including version 1.10.0, the codebase interacts with HiGHS enums by wrapping C++ structures [1][2][3]. The specific file Highspysources.pyi (often generated or located within the package's type stub files) provides type information for these interfaces [4]. Regarding enum.str, this refers to the standard Python Enum behavior or a specific implementation override [5][6]. Python's enum types define str to provide a human-readable string representation of the member (e.g., EnumName.MemberName) [5][6]. If you are encountering issues related to enum.str in highspy or are looking to modify how these enums are represented, note that: 1. Standard Behavior: By default, calling str on a Python enum member returns the Enum name and member name [5][6]. 2. Overrides: If a specific version or implementation of highspy requires different behavior, developers sometimes explicitly reassign the str method to the base Enum.str (or similar) to change output, often to avoid breaking changes or to align with expected serialization [5][7]. 3. Context: HiGHS enums in highspy are mappings of the underlying C++ structures [8][3]. If you are seeing references to enum.str in type stubs (.pyi) or code, it typically reflects how the Python wrappers maintain compatibility with the standard library's Enum type system [5][6]. For precise implementation details, you can inspect the highspy source code on the official ERGO-Code/HiGHS GitHub repository [2]. If you are troubleshooting a specific error, ensure your Python environment version is compatible, as Python 3.11+ introduced changes to how enum string representations are handled [7].
Citations:
- 1: https://pypi.org/project/highspy/
- 2: https://github.com/ERGO-Code/HiGHS
- 3: https://ergo-code.github.io/HiGHS/stable/
- 4: https://github.com/ERGO-Code/HiGHS/blob/HEAD/pyproject.toml
- 5: https://docs.python.org/3/library/enum.html
- 6: https://docs.python.org/3/howto/enum.html
- 7: Enum with
strorintMixin Breaking Change in Python 3.11 python/cpython#100458 - 8: https://ergo-code.github.io/HiGHS/dev/structures/enums/
🌐 Web query:
site:github.com/ERGO-Code/HiGHS "def(str" "py::enum" enum
💡 Result:
In pybind11, the py::enum_ class is used to bind C++ enumerations to Python. The syntax for defining an enum and its values involves creating a py::enum_ object and chaining the.value method to register each enumeration member. The standard pattern for this definition is: py::enum_(m, "EnumType").value("NAME1", EnumType::NAME1).value("NAME2", EnumType::NAME2).export_values; In this structure: 1. py::enum_(m, "EnumType"): This initializes the binding for the C++ enumeration TypeName, exposing it in the Python module m as "EnumType". 2..value("NAME1", EnumType::NAME1): This registers a specific enum member, providing its Python-side string name ("NAME1") and the corresponding C++ value (EnumType::NAME1). 3..export_values: This optional method exports the enum members directly into the parent scope (e.g., the module level), allowing them to be accessed as EnumType.NAME1 rather than requiring the full qualified path if desired. While the query mentioned def(str py::enum enum, pybind11 bindings typically use the py::enum_ constructor rather than a function named def for the enum object itself. The py::enum_ object is usually instantiated within a module or class binding block.
Citations:
- 1: highspy (direct calls via pybind11) incorrectly handles sliced numpy arrays ERGO-Code/HiGHS#1903
- 2: https://github.com/ERGO-Code/HiGHS/blob/81f30146713501a25c04058aaf51322172c24a30/pyproject.toml
- 3: RFC,ENH: On `C++` exceptions ERGO-Code/HiGHS#1368
- 4: https://github.com/ERGO-Code/HiGHS/blob/latest/docs/src/structures/index.md
- 5: Python documentation improvements ERGO-Code/HiGHS#1419
Use modelStatusToString() instead of parsing str(model_status).
str(model_status).rsplit('.', 1)[-1] depends on an undocumented str() format. Since h.modelStatusToString(model_status) returns the stable solver status text and the code already calls it for solver_wording, map _CONDITION_OF_HIGHS_STATUS by that canonical value instead so future enum repr changes don’t silently fall back to unknown.
🤖 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/sinks/highs.py` around lines 163 - 167, Update the
SolveStatus construction after h.getModelStatus() so _CONDITION_OF_HIGHS_STATUS
is keyed using the canonical value returned by
h.modelStatusToString(model_status), reusing that value for solver_wording
instead of parsing str(model_status). Preserve the existing unknown fallback and
primal-solution handling.
Ported from main (#186), which this branch had dropped on the floor when the executor was rewritten. An unmasked coordinate product needs no sort: every coordinate exists, so a row's label is its row-major position, which is a dot product against the dim ordinals the frame already carries. l rung build before build after transport 2.57 s 0.87 s dispatch 0.97 s 0.49 s transport gains most because neither of its variables carries a mask; dispatch keeps the sort for `p` and gains on its constraint rows. Total wall at the l rung: transport 8.8 s -> 4.2 s. Both paths now return `(dims…, label)` in that column order. They did not before — `with_row_index` puts the index first — and the ported upstream test `test_a_mask_that_removes_nothing_labels_exactly_like_no_mask` caught it, which is exactly the property it exists to pin: a mask that removes nothing has to be indistinguishable from no mask, down to the schema. Benchmarks re-run across all four cases and the tables updated. What is left at scale is the LP writer — 3.6 s of dispatch's 4.1 s at 10M — which is now the obvious next target and is also engine-independent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-measured: supersedes every polars-vs-duckdb number earlier in this threadThree changes since the last comparison, two of which move the numbers: the Build + LP file
Build + COO handoff to HiGHS (
|
| polars | duckdb a24 | |
|---|---|---|
| dispatch 10M | 0.79 s build, 2.99 GB total | 3.48 s build, 2.46 GB total |
| nodal 3M | 0.46 s build, 1.50 GB | 2.48 s build, 0.80 GB |
| transport 9.8M | 0.94 s build, 3.68 GB | 3.07 s build, 1.95 GB |
What changed, and what did not
The speed gap widened from ~1.9x to ~4x on the build phase alone — 0.79 s against 3.48 s on dispatch. Both engines now compute unmasked labels arithmetically rather than sorting for them, so what is left is everything else: parameter binding, dim frames, matrix assembly. That is a straight engine comparison with the biggest algorithmic difference removed from both sides.
The memory gap did not move. duckdb is 2.7-3.2x lighter on the write path and 1.6-1.9x lighter at the handoff, on every case. It was structural before and it is structural now: COO carries a triple per nonzero where a dense array carries a float, and there is no budget to set.
Nothing here contradicts the earlier reading, it sharpens it. polars is faster, duckdb is lighter, and the crossover is still wherever a model stops fitting.
Corrections to earlier comments in this thread
- "~8x faster handoff than duckdb" — withdrawn already, and still withdrawn. It was an artifact of fix: the HiGHS sink's column ingest was an unbounded global sort #181, fixed on main.
- "ahead on all three at the handoff" — that was against linopy
io_api='direct', not against duckdb. duckdb was lighter than this branch at the handoff then and is now. The comparison stands but the sentence read as if it were against duckdb. sectorhas no duckdb column anywhere, because the case only exists on this branch. Its 0.42x peak ratio is against linopy, not against main.
…t out of the rules Two changes, both about what this PR should be carrying. **The constraint-matrix elision comes out.** It was new here, it is worth ~30% of build peak, and it had two bugs during this PR that the differential suite structurally cannot catch. The objective elision stays, because main already ships it (#179) and dropping it would be a regression rather than a split. The mechanism (`keyed`, `label_dims`) stays with it and the tests stay in full; extending it to the matrix is #161's, on its own evidence. Measured cost of holding it back, l rung: dispatch build 0.49s -> ~2.1s, transport 0.87s -> ~1.8s. Tables re-run and updated. That is the size of the prize #161 is now carrying. **Hard rule 4 is no longer a rule.** "Peak memory tracks the model" is a property of the engine, not of the language: it was phrased around a `memory_limit` only one engine had, and promoting it made an implementation choice load-bearing in the rulebook — which is the only reason changing the implementation reads as a decision about what the package is. Rules 0-3 (layer order, engine isolation, one language two lanes, and the closure) survive any engine and decide what enters SPEC.md; cost is settled by measurement and lives in docs/benchmarks.md, which now says so at the top. Rules renumbered, and every reference to "hard rule 4" in ARCHITECTURE, bench/README and the sinks README rewritten to say what it actually meant. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ARCHITECTURE.md (1)
88-100: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNarrow the “engine knows nothing about xarray” rule.
src/farkas/relational/frames.pyintentionally detects and unwrapsxarray.DataArray, so stating thatsrc/farkas/relational/never sees xarray contradicts the implemented boundary adapter. Limit this rule to the compiler/solver core or explicitly documentframes.pyas the permitted input-boundary exception.🤖 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 `@ARCHITECTURE.md` around lines 88 - 100, Revise the architecture rule around “The engine knows nothing about linopy, xarray or YAML” to acknowledge the intentional xarray handling in frames.py. Limit the prohibition to the compiler/solver core, or explicitly identify frames.py as the permitted input-boundary adapter exception while preserving the existing dependency-boundary guidance.
🤖 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/sinks/README.md`:
- Around line 33-34: Update the streaming guidance in the sinks README to remove
the obsolete DuckDB aggregation instruction and state that sinks should consume
the already-built Polars frames directly, without materializing the model a
second time.
---
Outside diff comments:
In `@ARCHITECTURE.md`:
- Around line 88-100: Revise the architecture rule around “The engine knows
nothing about linopy, xarray or YAML” to acknowledge the intentional xarray
handling in frames.py. Limit the prohibition to the compiler/solver core, or
explicitly identify frames.py as the permitted input-boundary adapter exception
while preserving the existing dependency-boundary guidance.
🪄 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: 7568bdaa-0c55-49a5-8995-29d0d1ba37e4
⛔ Files ignored due to path filters (1)
examples/walkthrough.outis excluded by!**/*.out
📒 Files selected for processing (6)
ARCHITECTURE.mdbench/README.mdbench/results/latest.jsonldocs/benchmarks.mdsrc/farkas/relational/executor.pysrc/farkas/relational/sinks/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- bench/README.md
- docs/benchmarks.md
- src/farkas/relational/executor.py
Restores what the previous commit held back, for a measured reason rather than a procedural one. Ported to the duckdb engine on its own branch, it moved nothing: dispatch build 4.17s against 4.16s, transport 3.80s against 3.64s, peak unchanged — inside the noise on a back-to-back run. On this engine the same change is worth 2-4x of build time. So the reasoning is engine-independent and the value is not. duckdb's `GROUP BY row, col` is a spilling hash aggregate that costs little beside everything else it does; polars was materialising it. That is now on #161, which is closed against the duckdb path. What made the split worth doing anyway: the port produced the tests. Both guards are verified load-bearing by reinstating each bug and watching the right test fail — the broadcast `group_sum` case and the constant-part reduction. Those stay. The premise the elision rests on — every parameter keyed by its dims — went to main separately as #201, where it stands on its own as a lane-parity fix rather than as this optimisation's precondition. Benchmark tables re-run. The machine was busy, so the note about reading ratios rather than seconds is now in the doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_constraint_blocks` joined the matrix onto the rows, grouped, and pasted each row's terms together with `str.join`. The join is where reproducibility went: it hands back a group in whatever order it finished it, and ordering the *rows* afterwards cannot repair the order *within* one. Two writes of the same model gave different bytes — which #109 asks for and the branch already claimed. So build the lines independently and interleave them by sorting. Header, terms and footer each carry `(row, ord, line)`; concat, `.sort('row','ord')`, sink. Nothing gathers a row's terms into a string, and the order is a total order on two integers rather than a side effect of a join. `_sorted_terms` folds into the terms arm. The anti-join is the one thing the old shape got free: a row with no terms still needs a line a solver can parse, and a group-by emits one per group whether or not the join matched. Emit at the `l` rung, best of three, the two arms back to back on a quiet machine with the build phase matched as a control: sector 0.52s -> 0.22s -57% nodal 0.80s -> 0.48s -40% transport 2.72s -> 2.05s -25% dispatch 1.89s -> 2.15s +13% **`dispatch` pays for this and the others are paid.** It is the one case with few rows and many terms each, so the old group-by had little to group and the new sort has everything to sort; the other three invert that. Peak falls on all four either way — 3.16 -> 2.46 GB on `dispatch`, 1.91 -> 1.35 on `nodal`, 1.26 -> 1.00 on `sector`, 3.89 -> 3.40 on `transport` — because nothing builds a row's text before writing it. Reproducible bytes are the point; the emit time is a trade that goes our way on three cases of four, and the memory goes our way on all of them. Two shapes that look like cleanups are not. Dropping the `.sort('row','col')` on the matrix is slower on every case even though the order does not survive into output that is sorted again anyway. Sorting *after* the join is the other way to fix the bytes; it works and costs 30-50%, because it orders rendered strings instead of the two integers behind them. The test is new because there was none: it writes one model three times and compares digests. The old sink fails it on every attempt — at 60 variables, not just at scale, so the size is for realism rather than to provoke it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rting
The sink opened by joining `cols` to `obj`, sorting the result by `col` and
collecting the whole frame, then sliced it batch by batch. Every part of that
is work the labels already did: `col` is dense `0..n-1` (`ModelTables`), so it
*is* the position each value belongs at. The objective's gaps are a fill, not a
join, and lining the bounds up is a write, not a sort — and neither needs an
n-row frame to exist before the first batch can be handed over.
The column half alone, `dispatch/l` (10M columns), both shapes on the same
tables with HiGHS taken out so what is left is the sink:
join + sort + collect 2.22s +1.16 GB
scatter 0.37s +0.49 GB
End to end with `Highs.run` stubbed — so HiGHS's own copy of the model is in
the number and the simplex workspace is not — best of two at the `l` rung, the
build peak matched as a control:
dispatch 1.02s -> 0.55s
transport 1.18s -> 0.72s
nodal 0.34s -> 0.16s
**Peak barely moves there, and that is the honest reading**: 3.51 -> 2.59 GB on
`dispatch`, 4.04 -> 3.97 on `transport`, 1.50 -> 1.49 on `nodal`. HiGHS's own
copy dominates once the model is loaded, so saving 0.67 GB on our side of the
handoff is most of a gigabyte that the process never gets back to show. The
time is what a caller feels.
Both shapes hand the solver the same numbers — the two arms agree to the bit on
a checksum over every bound, cost and integrality flag. Solving dispatch,
transport, sector and nodal at `s` gives the same objective, the same primal
digest and the same row and column counts as before the patch.
Nothing textual crosses into numpy any more. `vtype` and `sense` were compared
in numpy, which means a polars `String` column converts by boxing 10M Python
strings: 0.95s where the same test in polars, handing back a bool, costs 0.04s.
Both comparisons move to the frame side.
`matrix.sort('row')` stays. `row` is not dense *per matrix entry*, so unlike
the columns that is a real sort, and it is what `searchsorted` reads.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Left behind by the rebuild: `COPY`, `printf`, `DuckdbExecutor`, "aggregate inside duckdb", and a connection where there are now frames. The known-issue section goes with it. #109 is closed, so what is worth writing down is not the bug but why stable output is easy to lose again — which is the one thing a future sink has to know before it reaches for a group-by. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The committed numbers were taken under load — visible in the fact that the *linopy* arm, whose code nobody touched, ran 30-50% faster on the re-run. A number measured against a busy machine is not wrong so much as unattributable, and two changes landed on the sinks since. Re-run with nothing else above 40% CPU. What actually moved, rather than what the machine was doing: - Emit: -57% on `sector`, -40% on `nodal`, -25% on `transport`, +13% on `dispatch`, from the one-sorted-stream LP sink. Recorded as a trade rather than a win, since one case pays. - Handoff: 2-5x faster on all three cases in the `solver_direct` table, from scattering the columns instead of joining and sorting them. - `sector` peak reads 0.33x rather than 0.51x, and the prose that quoted 1.27 GB against 2.79 GB now quotes what the table says. Also corrected two things that were stale rather than re-measured: the run command omitted `sector`, which has been a case for a while, and the parity gate covers four cases, not three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
added
area:engine
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/farkas/relational/sinks/lp_file.py (2)
121-129: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant pre-sort of
termsbefore the final global sort.The concatenated frame is re-sorted by
('row', 'ord')at line 135, which already fully determines term order (termordequalscol). The.sort('row', 'col')at line 123 does no useful work for correctness since the subsequent sort supersedes it, but it does add an extra sort pass overmatrix— likely the largest table in the model and the one dominating theemitphase at scale.♻️ Proposed fix
terms = ( model.matrix.lazy() - .sort('row', 'col') .select( 'row', pl.col('col').cast(pl.Int64).alias('ord'), (_signed(pl.col('coeff')) + pl.lit(' x') + _digits(pl.col('col'))).alias('line'), ) )🤖 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/sinks/lp_file.py` around lines 121 - 129, Remove the redundant .sort('row', 'col') call from the terms construction chain in the model emission flow. Keep the existing row, ord, and line selections unchanged, relying on the later global sort by ('row', 'ord') to determine term order.
65-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBinary/integer existence check filters
colstwice.
chosen.select(pl.len()).collect()materializes the filtered-and-sorted plan just to check emptiness, then_sink(chosen.select(...), part)re-derives the same filter from scratch. For models with largecolstables this doubles the scan/filter cost per variable-type check.♻️ Proposed fix
integrality = [] for variable_type, keyword in (('binary', 'binary'), ('integer', 'general')): - chosen = model.cols.lazy().filter(pl.col('vtype') == variable_type).sort('col') - if chosen.select(pl.len()).collect().item() == 0: - continue - part = parts / keyword - integrality.append((keyword, part)) - _sink(chosen.select(pl.lit('x') + _digits(pl.col('col'))), part) + lines = ( + model.cols.lazy() + .filter(pl.col('vtype') == variable_type) + .sort('col') + .select(pl.lit('x') + _digits(pl.col('col'))) + .collect() + ) + if lines.is_empty(): + continue + part = parts / keyword + integrality.append((keyword, part)) + _sink(lines.lazy(), part)🤖 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/sinks/lp_file.py` around lines 65 - 72, Update the integrality loop to avoid materializing and scanning each filtered LazyFrame twice: reuse a single collected result from chosen for both the emptiness check and the _sink output. Preserve the existing binary/integer filtering, sorting, part naming, and integrality entries while ensuring each variable type scans cols only once.
🤖 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.
Nitpick comments:
In `@src/farkas/relational/sinks/lp_file.py`:
- Around line 121-129: Remove the redundant .sort('row', 'col') call from the
terms construction chain in the model emission flow. Keep the existing row, ord,
and line selections unchanged, relying on the later global sort by ('row',
'ord') to determine term order.
- Around line 65-72: Update the integrality loop to avoid materializing and
scanning each filtered LazyFrame twice: reuse a single collected result from
chosen for both the emptiness check and the _sink output. Preserve the existing
binary/integer filtering, sorting, part naming, and integrality entries while
ensuring each variable type scans cols only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4437cf02-fe5c-48ce-8ac3-9e53f679c2fd
📒 Files selected for processing (7)
bench/results/latest.jsonldocs/benchmarks.mdsrc/farkas/relational/executor.pysrc/farkas/relational/sinks/README.mdsrc/farkas/relational/sinks/highs.pysrc/farkas/relational/sinks/lp_file.pytests/test_lp_text.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/farkas/relational/sinks/highs.py
- docs/benchmarks.md
- src/farkas/relational/executor.py
Conflicts everywhere the two lines touched the same idea from opposite sides: main improved the duckdb sinks (#195) and this branch deletes duckdb. The SQL does not survive; the rule it encodes does, and is worth having. `chunking.py` comes over intact — it is a budget divided by the width of one unit, which has nothing to do with which engine spends it. `ModelTables` keeps `row_chunks_by_nonzeros` and `col_chunks`, reimplemented over frames: `matrix.height` is the nonzero count that `SELECT count(*) FROM A` was there to get, and `scalar()` goes with the connection it queried. The hand-off spends the budget through both. Resolved to this branch's side, with reasons: - `executor.py` — `_chunk_starts` sized label assignment over a leading dim. That pass no longer exists: labels are positional arithmetic, so there is no window to chunk. - `lp_file.py` — main chunked the constraint text by nonzeros because each chunk was a `COPY` of a `string_agg`. Here it is one lazy frame handed to `sink_csv`, which streams it, so the chunking has no counterpart. - `docs/benchmarks.md` — the operational findings were duckdb's (`memory_limit`, the sort that would not stay inside it). #188's methodology lesson is the part that generalises, and it lives in `bench/README.md`, which merged clean. **The budget is 1e5 elements here, not the 2e6 main settled on**, and that is a measurement rather than an oversight. On duckdb a wider chunk was worth a third of the hand-off because every chunk re-ran an ordered query. On this lane the columns are numpy slices and the rows a filter over an already-sorted frame, so more chunks cost almost nothing and only residency scales with the budget. At the `l` rung, 2e6 against 1e5 is 0.50s against 0.59s on `dispatch` and 0.71 against 0.75 on `transport` — for 0.6 GB and 0.2 GB more peak. A tenth of a second in front of a minute of simplex does not buy half a gigabyte of hard rule 4. Both of #195's tests come over, one ported to frames. Solving dispatch, nodal, sector and transport at `s` gives the same objective, the same primal digest and the same counts as before the merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#253) duckdb stopped being a dependency when #189 landed. The arm that measured it was there to answer one question — was replacing it worth doing — and that question is closed, so what is left is a column nobody can re-run without keeping a foreign checkout synced indefinitely. The comparison itself survives in #189 and in git, which is where a decided decision belongs. **Harness.** `FOREIGN_ARMS`, `--duckdb-root`, `--duckdb-limits`, the foreign-interpreter plumbing in `_child`, and `--memory-limit` in `_run_case` all go. `_child` is now four lines of `subprocess` with no notion of running another checkout's code under another checkout's python. The report drops to two arms, which takes the ladder tables from fourteen columns to ten. **docs/benchmarks.md: 649 -> 419 lines.** It had become the largest file in the repo — larger than SPEC.md — and most of the excess was argument for the engine choice rather than measurement of the engine we have. The tables are the same measurements with the duckdb columns dropped, not a re-run. What that argument was hiding is worth keeping, and is now stated plainly: at 120M variables the peak ratio is **1.07x**. Past a certain size both lanes hold the same model and the representation stops deciding anything, so the memory headroom this lane has is a small-and-sparse-model property rather than a scaling one. The old section reached the opposite impression by reading duckdb's budgeted column as an engine measurement. **The chart page** loses its third and fourth series with the arm. The band section changes question with it — from "is the ordering between three engines robust to model shape" to "does the advantage over linopy hold whatever the model looks like" — which the two-arm data answers directly: clear of 1.0 at every rung but `profiled/l` on time, straddling it on peak with a threefold spread between `sector` and `dispatch`. Palette re-validated at two slots (worst CVD dE 24.7 light / 26.8 dark, no contrast warning). **SPEC.md** still justified refusing a stray dim with "under a memory budget the first quietly builds a bigger model" — there is no budget now, and the clause is stronger without it. 423 passed / 1 xfailed, ruff and pyrefly clean.
The docs described one engine because until this branch there was one. What needed saying is not "duckdb exists" but the distinction it forces. **Lane and engine are different axes**, and hard rule 3 now says so where the confusion actually lands. A *lane* consumes the AST and owns everything below it; an *engine* consumes the plan and fills `ModelTables`. The relational lane has two engines; linopy is a lane and could not be an engine — it never sees the plan, produces no `ModelTables`, and `extend` attaches to a model that already exists. Reading rule 3 without that, "the streaming engine" and "the linopy lane" look like the same kind of thing named twice. The consumers diagram carries it: `engines/` holds two boxes, and `engine.py` feeds them, because the shared base is *why* there can be two. An engine that had to write its own sinks and read-back would be a lane's worth of work. `bench/duckdb-spike.md` is retitled from a spike to what it now is — the provenance for numbers the package states in `pyproject.toml`, in the engine's docstring and in SPEC. Its conclusion is rewritten: the question was "reverse #189 or not", and the answer was neither. Both engines ship and the caller chooses, because which path a workload is on decides the answer and nothing in the library can know that. The ~2,300-line estimate priced a *replacement*; a second engine cost far less, because most of an executor is not engine work. ROADMAP's memory axis records duckdb as a partial answer and is careful about which: it is *smaller*, not *bounded*. A caller still cannot name a number, and partition-wise execution on polars floors at 23-36% off peak. Worth knowing before that track is picked up, rather than after.
`build_and_hand_over` passed `memory_limit='1GB'` to `lps.build`, which has had no such parameter since #189 retired the engine that took one. Half the regression workloads have been raising `TypeError` ever since — invisibly, because `bench/regressions` is outside `testpaths` and nothing in CI runs it. The docstring now also says what the harness measures: *whatever `lps.build` builds with*. Nothing here names an engine, so `LPSPEC_ENGINE` selects one and the same two commands answer the same question for either — with the caveat that matters, that a stored baseline does not record which engine produced it, so comparing across engines measures the engine rather than the change. Verified on `dispatch-s`: both workloads run, and the memray peak separates the engines cleanly (19.7 MB against 118.5 MB), which is the signal perf work needs.
`build_and_hand_over` passed `memory_limit='1GB'` to `lps.build`, which has had no such parameter since #189 retired the engine that took one. Half the regression workloads have been raising `TypeError` ever since — invisibly, because `bench/regressions` is outside `testpaths` and nothing in CI runs it. The docstring now also says what the harness measures: *whatever `lps.build` builds with*. Nothing here names an engine, so `LPSPEC_ENGINE` selects one and the same two commands answer the same question for either — with the caveat that matters, that a stored baseline does not record which engine produced it, so comparing across engines measures the engine rather than the change. Verified on `dispatch-s`: both workloads run, and the memray peak separates the engines cleanly (19.7 MB against 118.5 MB), which is the signal perf work needs.
The docs described one engine because until this branch there was one. What needed saying is not "duckdb exists" but the distinction it forces. **Lane and engine are different axes**, and hard rule 3 now says so where the confusion actually lands. A *lane* consumes the AST and owns everything below it; an *engine* consumes the plan and fills `ModelTables`. The relational lane has two engines; linopy is a lane and could not be an engine — it never sees the plan, produces no `ModelTables`, and `extend` attaches to a model that already exists. Reading rule 3 without that, "the streaming engine" and "the linopy lane" look like the same kind of thing named twice. The consumers diagram carries it: `engines/` holds two boxes, and `engine.py` feeds them, because the shared base is *why* there can be two. An engine that had to write its own sinks and read-back would be a lane's worth of work. `bench/duckdb-spike.md` is retitled from a spike to what it now is — the provenance for numbers the package states in `pyproject.toml`, in the engine's docstring and in SPEC. Its conclusion is rewritten: the question was "reverse #189 or not", and the answer was neither. Both engines ship and the caller chooses, because which path a workload is on decides the answer and nothing in the library can know that. The ~2,300-line estimate priced a *replacement*; a second engine cost far less, because most of an executor is not engine work. ROADMAP's memory axis records duckdb as a partial answer and is careful about which: it is *smaller*, not *bounded*. A caller still cannot name a number, and partition-wise execution on polars floors at 23-36% off peak. Worth knowing before that track is picked up, rather than after. (cherry picked from commit ef463e5e86103cccf22cf0747e291451f33c6971)
The docs described one engine because until this branch there was one. What needed saying is not "duckdb exists" but the distinction it forces. **Lane and engine are different axes**, and hard rule 3 now says so where the confusion actually lands. A *lane* consumes the AST and owns everything below it; an *engine* consumes the plan and fills `ModelTables`. The relational lane has two engines; linopy is a lane and could not be an engine — it never sees the plan, produces no `ModelTables`, and `extend` attaches to a model that already exists. Reading rule 3 without that, "the streaming engine" and "the linopy lane" look like the same kind of thing named twice. The consumers diagram carries it: `engines/` holds two boxes, and `engine.py` feeds them, because the shared base is *why* there can be two. An engine that had to write its own sinks and read-back would be a lane's worth of work. `bench/duckdb-spike.md` is retitled from a spike to what it now is — the provenance for numbers the package states in `pyproject.toml`, in the engine's docstring and in SPEC. Its conclusion is rewritten: the question was "reverse #189 or not", and the answer was neither. Both engines ship and the caller chooses, because which path a workload is on decides the answer and nothing in the library can know that. The ~2,300-line estimate priced a *replacement*; a second engine cost far less, because most of an executor is not engine work. ROADMAP's memory axis records duckdb as a partial answer and is careful about which: it is *smaller*, not *bounded*. A caller still cannot name a number, and partition-wise execution on polars floors at 23-36% off peak. Worth knowing before that track is picked up, rather than after. (cherry picked from commit ef463e5e86103cccf22cf0747e291451f33c6971)
2 participants
Replaces duckdb with polars as the engine the relational lane runs on.
The decision this PR is asking for
polars — faster, and code you can read — or duckdb, which lets you name a memory ceiling and holds it.
That is the whole of it. Everything below is evidence, and both sides are real:
l)sql.pyto quote themWhat tips it, for me: the sink most callers use is the solver hand-off, and there this lane is ahead on peak on all six cases and on wall on five of six. duckdb's remaining memory advantage is real but smaller than it first looks, and it is bought with a knob most callers will never set.
What should give you pause: duckdb's budget is close to free. On
dispatcha 1 GB ceiling costs it 0.99x, 0.89x and 1.08x of wall time at the top three rungs while saving up to 2.5x of peak. That is a genuinely good property and this PR gives it up:ROADMAP.mdTrack 5 becomes partition-wise emission, a promise this branch has not yet kept.A correction worth reading before the tables. Every duckdb number this PR published before today was measured at that engine's 1 GB default, unstated. Re-run at its own unlimited, two things changed: duckdb needs more memory than it appeared to (5.28 GB, not 2.11, at 120M), and the budget turned out not to be the wall-time handicap I assumed. The arm now runs at both settings because a single number is whichever one the reader assumed.
The language does not move either way.
plan.pyis frozen engine-agnostic dataclasses, which is what made this a rewrite of one module rather than of the project — and what makes the decision reversible if it is wrong.Scale, to 120M variables
dispatch, LP sink, best of two, on a 25 GB machine. Charts:docs/benchmarks-scaling.html.Nothing falls over, and polars is fastest at every rung. The
2xlrow isnoisy between runs (polars has read 20.6 s and 35.4 s; linopy 38.5 s and
106.2 s) because 8-10 GB of peak on a 25 GB machine is where other things
start to matter — read the ordering there, not the seconds.
The budget is a real ceiling and it can be too low. On
fleet— twelvevariable declarations rather than one — duckdb at 1 GB fails outright at the
xlrung, and builds in 26.4 s inside 2.40 GB when given 8 GB. No amount ofspilling fixes a budget below what the shape needs.
What it costs and buys, measured
Three engines in one run, on the current branch, against the linopy this
branch pins — the v1-semantics build from PyPSA/linopy#717, not the released
0.9.0. The duckdb arm runs
main's own code from its own checkout andinterpreter, interleaved against one shared parquet cache. The parity gate
spans all three and they agree exactly on every case they share, which is
also what shows this branch's #234 and
main's #239 build the same model.Six cases, two sinks, four arms — duckdb runs at its own unlimited and
at its 1 GB default, because the two are different claims. Best of three, on
an idle machine. 655 timings, no failures.
farkasef8558clinopyduckdb4a13d38onmainBuild → a loaded solver
The path a caller takes to get an answer, and what
solver_directexists for.lrung, build + hand-off,run()never called:lrungAhead of linopy on peak on all six and on wall on five — 1.7-3.3x faster
to a solver-ready model on those five.
profiledis level at 1.09x, and it isthe case in the ladder to be lost: a parameter dense over the whole variable
product is the array xarray already wants and a full-size join for us. It is
also the noisiest case here, its own repeats spanning 2.33-2.84 s at ~4 GB
resident, so read it as level rather than as either result.
fleetis new and immediately earned its place — twelve variabledeclarations rather than one, which is the shape a real unit-commitment model
has. It is 0.44x here and the worst peak ratio in the set through the LP
sink (1.64x), which none of the single-declaration cases showed.
Both phases are shown because only their sum is comparable: linopy defers
~82% of its work to whatever consumes the model, so its
buildcolumn is aplaceholder where ours is a finished matrix. Read either alone and it flatters
whichever lane defers more.
Against the engine this replaces
duckdb here is that arm unbounded, not at its 1 GB default — see the
correction above.
lrungpolars is 1.7-5.2x faster than duckdb on every case and both sinks; duckdb
is 1.29-2.8x lighter. That is the trade this PR makes, and neither half is
marginal. The memory half is much smaller on the sink most callers use, since
HiGHS's own dense model is resident either way — 1.29-1.49x there against
1.6-2.8x on the LP path.
And plainly, now the table shows it: duckdb is slower than the eager lane
from the
mrung up. The streaming engine's advantage was never wall time.The LP-file path
The secondary route — a file for another tool rather than an answer.
lrungThis is where the losses are.
profiled1.46x andfleet1.03x on wall;fleet1.64x andtransport1.19x on peak.transportwas 1.69x until theconstraint section stopped sorting a whole model's worth of rendered text at
once (#245).
What is left on this route is structural in two ways. Most of an LP write is
turning doubles into text, work neither lane can avoid, so the wall ratio
compresses toward 1.00 however fast the build gets. And COO carries a
(row, col, coeff)triple per nonzero where a dense array carries one float,which is
fleet's 1.64x — the many-declaration shape is the one that shows it,which is why that case is in the ladder now.
profiledis the shape the eagerlane handles best — a parameter dense over the whole variable product is the
array xarray already wants, and a full-size join for us — and it too is in the
ladder to be lost.
Two costs, and both are real
The harness spawns one process per measurement, so every number above is a
first build. That is right for a caller who builds one model and solves it,
and wrong for a rolling horizon. Measured both ways to the same end point —
build and hand-off, because a build-only comparison would measure our
finished matrix against linopy's placeholder:
138 ms of the eager lane's 180 is linopy and xarray's own first-call
machinery, measured with no farkas anywhere; our shim adds a constant ~2.3 ms.
At
xsa first build reads 0.05-0.10x of linopy and steady reads ~0.4x — bothtrue, answering different questions.
What is in the number
import· the simplex — the hand-off stops beforerun()· reading the solution backsectorpostdates the duckdb checkout, so it has no duckdb column rather thana lost one.
What it deletes
relational/sql.pyand the quoting it existed for, plus the[A-Za-z_][A-Za-z0-9_]*restriction on declared names. This is a simplification, not a capability — finishing the quoting pass would lift the same restriction on duckdb.with
main(perf(sink): size the hand-off by nonzeros, under one budget and one chunking rule #195):chunking.pyis a budget over the width of one unit,which is engine-agnostic, and the hand-off spends it. The budget here is
1e5 elements, not the 2e6
mainsettled on — on duckdb a wider chunk wasworth a third of the hand-off because each one re-ran an ordered query; here
the columns are numpy slices, so 2e6 buys 0.09 s and costs 0.6 GB of peak.
weakref.finalize.close()andwithremain but nothing breaks without them.What it gives up
A settable memory ceiling, and models that exceed RAM. Hard rule 4 is rewritten from "peak is a function of the configured budget" to "peak tracks the model", and ROADMAP Track 5 becomes the memory axis with partition-wise emission named as the way to get a bound back without a database.
Whether that matters is a positioning question, not an engineering one — see the thread. The short version: build peak is ~10% of build-and-solve, so the ceiling pays on the write path and little else.
Riding along, and probably should not be
The terminal-aggregate elision (
TermFragment.keyed,label_dims,_needs_aggregate) is new here and is not part of the swap. It is worth ~30% of build peak and belongs to #161. It has had two bugs during this PR — an inverted predicate, thengroup_sumover a broadcast dim producing duplicate(row, col)entries — and the differential suite caught neither, becausetransporthas three term fragments and so forces the aggregate regardless. Both are fixed and covered now, but I would split it out and land it against #161 on its own evidence. Say the word.Also in here
sol.dualported from feat: expose duals on the solve path (sol.dual) #156, with its differential test.sectorbench case — dense snapshots and carriers crossed with an 8% node/tech portfolio, which is where the sparsity claim is actually visible (0.42x peak against linopy).transport's build went 2.57 s → 0.87 s. Engine-independent, and the largest single win measured here.The constraint section came out of a join, and a join hands back a group in
whatever order it finished it — two writes of one model gave different bytes.
The sink now emits one sorted stream of
(row, ord, line). There is a test,and it fails on the old sink at 60 variables. Emit moved with it: -57% on
sector, -40%nodal, -25%transport, and +13% ondispatch— the onecase with few rows and many terms each, where the old group-by had little to
group and the new sort has everything to sort. That regression is gone: perf(lp): write each section once, and build each line once #203
took another 30–35% off all four, and
dispatchnow emits faster than it didbefore either change. Peak falls on all four.
colis dense0..n-1, so the sink writes each valueat its own position instead of joining
objon and sorting bycol. Thehand-off is 2–5x faster (
dispatch1.02 s → 0.55 s); peak barely moves,because HiGHS's own copy dominates once the model is loaded.
addCols/addRowswere unchecked on main too, so a refused batch produced a confident answer to an unconstrained model.Found on main while doing this, filed separately
#197 (objective terms with disjoint dims diverge between lanes), #198 (a second objective
equations:entry is silently dropped), #199 (fk.buildinto a used workdir raises a raw duckdb exception). None caused or fixed here.Open questions before merge
primal()returnspolars.DataFrame. Should it return pandas, given the audience? Decoupled from the engine — one line either way.🤖 Generated with Claude Code
Summary by CodeRabbit
where/row masking, bounds propagation (including infinities), and correct aggregation when variables repeat in objectives.