bench: a case where I/O is not noise, and a sink axis to measure it on - #205
Conversation
Two changes to the harness, both about measuring what users actually do. **`profiled`, the case whose input is the same size as its model.** Every existing case builds its coordinate product from parameters far smaller than the product they explode into — 2-15% of it — so reading the parquet is 2-9% of the build and the read path is lost in the margin. `profiled` is `nodal`'s cardinalities with the mask removed and `availability` dense over `(snapshot, node, tech)`: at the `l` rung, a 12M-row table against a 12M coordinate product. That makes the read path, the join, and the in-memory-vs-parquet question measurable rather than noise. It is also the shape the eager lane handles best — a parameter already dense over the variable product is the array xarray wants, no broadcast and no alignment — while we join a full-size frame against a full-size coordinate product. `nodal` is the mirror image and our clearest win. Carrying both is what keeps the ladder from holding only shapes that suit one engine. Closes #202. **Both sinks, not just the LP file.** `--sinks lp highs`, defaulting to both. The LP file is the artifact fewest callers want, and it is not the same comparison: at `profiled/m` the peak ratio is 0.60x through the LP writer and 0.94x through the handoff, because HiGHS's own dense model is resident in both arms and swamps the difference between them. The `highs` sink stops at the handoff — `run()` is never called. The simplex is the same work whoever filled the model, so including it would swamp the phase this harness exists to measure. `solve_direct` is split into `build_highs` + the solve so there is a seam to stop at; linopy's `Model.to_highspy()` is the same seam on the other side, which is what makes the two arms comparable. A rung the sink is capped out of is written to the JSONL as a `skipped` record and footnoted in the report, so a coverage hole cannot read as a result. Measured, `profiled` at the `l` rung (12M variables): LP file, farkas 8.42 s / 1.49 GB against linopy 2.44 s / 3.26 GB; HiGHS handoff, farkas 7.24 s / 2.60 GB against linopy 2.93 s / 3.23 GB. The parity gate passes on the new case. No published numbers change here: `bench/results/latest.jsonl` is left as it was, since re-running the ladder is its own act. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✨ 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 |
…e it on Ports the two ideas from #205, which was written against the duckdb harness and cannot cherry-pick: that commit also carries `--memory-limits`, `--chunk-rows` and `workdir_bytes`, the budget apparatus #189 deleted. `profiled` is the one case whose input is the same order as its model. Every other case builds its coordinate product from parameters far smaller than the product they explode into, so reading the parquet is noise; here `availability` is dense over `(snapshot, node, tech)`, which at the `l` rung is a 12M-row table against a 12M coordinate product. It is also the shape the eager lane handles best, and the case we lose — which is the point, because a ladder holding only shapes that suit one engine proves nothing. `--sinks lp highs`, both by default. The LP file is the artifact fewest callers want, and it is not the same comparison: HiGHS's own dense model is resident in both arms and narrows the gap. It is also what makes the build-and-hand-off table reproducible, where before it was hand-made with no script behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…easure on both (#215) * fix: a dictionary-encoded source column binds like a plain one A `Categorical` dim column is a source encoding, not a different model. pandas writes one for any `Categorical`, and any writer will dictionary-encode a 12M-row table of repeated node names. polars will not join `Categorical` against `String`, and the dim frames are built from declared coordinate values and are plain — so binding such a source failed with a schema error from inside a join, a long way from anything a caller can act on. Source dim columns are cast to `String` at bind time. Casting the source side rather than the dim side is deliberate: the dim frame is the authority on what a coordinate is, and a source is whatever a caller happened to hand over. duckdb coerced this silently, so it arrived with the polars rewrite rather than being found by it. Found by the `profiled` bench case, which writes its dim columns exactly this way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(sinks): split build_highs out of solve_direct The hand-off without the simplex. `solve_direct` is now this plus `run()`, and the seam exists because the two are different questions: the simplex is the same work whoever filled the model, so a measurement that includes it says nothing about the lane that filled it. `bench/` ends here, and linopy's `Model.to_highspy()` is the same seam on that side of the fence — which is the only reason the two arms are comparable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(bench): a case where I/O is not noise, and a sink axis to measure it on Ports the two ideas from #205, which was written against the duckdb harness and cannot cherry-pick: that commit also carries `--memory-limits`, `--chunk-rows` and `workdir_bytes`, the budget apparatus #189 deleted. `profiled` is the one case whose input is the same order as its model. Every other case builds its coordinate product from parameters far smaller than the product they explode into, so reading the parquet is noise; here `availability` is dense over `(snapshot, node, tech)`, which at the `l` rung is a 12M-row table against a 12M coordinate product. It is also the shape the eager lane handles best, and the case we lose — which is the point, because a ladder holding only shapes that suit one engine proves nothing. `--sinks lp highs`, both by default. The LP file is the artifact fewest callers want, and it is not the same comparison: HiGHS's own dense model is resident in both arms and narrows the gap. It is also what makes the build-and-hand-off table reproducible, where before it was hand-made with no script behind it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(bench): re-measure on both sinks, and let the sink decide the answer Full ladder on a quiet machine: five cases, two sinks, both arms, best of three, parity gate green on all five at 0.0e+00. 294 records, no failures. `transport`'s peak is 1.62x through the LP writer and 0.78x through the hand-off. Same build, same rung — the LP path spends its peak turning doubles into text, while HiGHS's own dense model is resident in both arms and dwarfs the difference between the lanes that filled it. This file has called that row the one open problem at scale; on the sink most callers use, we are ahead of linopy on it. "What this says" now reads per sink, because the two disagree by more than the noise between them. The density sweep no longer refuses its own claim: peak improves at every rung (0.87x -> 0.74x) where linopy used to win the sparsest one. Not because the prediction changed but because a mask got cheaper for us — #212 made masked labels arithmetic, and the sweep had been measuring our own cost of being sparse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
main gained the three duckdb perf PRs (#152, #174, #178), two language-level fixes (#201, #223) and the bench harness from #205. Resolved by lane: **Engine files take the polars side.** #152, #174 and #178 are `ROW_NUMBER() OVER`, `CREATE TYPE ... AS ENUM` and `CREATE TABLE surv_*` against the engine this branch deletes. Their *ideas* are already here in polars form — #152's as `_positional`, #174's as #211, #178's as #212 — so taking main's side would have re-added SQL against a connection that no longer exists. **Language and validation take main's.** #223's `lowering.py`, `validation.py` and `linopy/builder.py` changes merged clean and are engine independent; its executor half and #201's are already here (as #225 and `_check_one_row_per_coordinate`). `test_objective.py` and `test_validation.py` arrived whole and pass on this lane unmodified — 404 tests to 414. **bench takes ours**, which carries #205's two ideas ported rather than merged (#215): the duckdb commit also brings `--memory-limits`, `--chunk-rows` and `workdir_bytes`, the budget apparatus this branch removed. Two tests were combined rather than picked. `test_group_sum`'s docstring says both true things now — the divergence between lanes, and the aggregate-skip premise that rests on it. `test_milp` keeps the polars Enum test and gains main's ragged-chunk test, which is API-level and applies to this sink too. Main's duckdb label tests are dropped, not ported: they drive `DuckdbExecutor` and `ex._con` directly. The properties they pin are covered here by `test_a_mask_that_removes_nothing_labels_exactly_like_no_mask` and `test_a_factored_mask_labels_exactly_like_the_counted_path`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sult (#231) * bench: a case where I/O is not noise, and a sink axis to measure it on (#205) Two changes to the harness, both about measuring what users actually do. **`profiled`, the case whose input is the same size as its model.** Every existing case builds its coordinate product from parameters far smaller than the product they explode into — 2-15% of it — so reading the parquet is 2-9% of the build and the read path is lost in the margin. `profiled` is `nodal`'s cardinalities with the mask removed and `availability` dense over `(snapshot, node, tech)`: at the `l` rung, a 12M-row table against a 12M coordinate product. That makes the read path, the join, and the in-memory-vs-parquet question measurable rather than noise. It is also the shape the eager lane handles best — a parameter already dense over the variable product is the array xarray wants, no broadcast and no alignment — while we join a full-size frame against a full-size coordinate product. `nodal` is the mirror image and our clearest win. Carrying both is what keeps the ladder from holding only shapes that suit one engine. Closes #202. **Both sinks, not just the LP file.** `--sinks lp highs`, defaulting to both. The LP file is the artifact fewest callers want, and it is not the same comparison: at `profiled/m` the peak ratio is 0.60x through the LP writer and 0.94x through the handoff, because HiGHS's own dense model is resident in both arms and swamps the difference between them. The `highs` sink stops at the handoff — `run()` is never called. The simplex is the same work whoever filled the model, so including it would swamp the phase this harness exists to measure. `solve_direct` is split into `build_highs` + the solve so there is a seam to stop at; linopy's `Model.to_highspy()` is the same seam on the other side, which is what makes the two arms comparable. A rung the sink is capped out of is written to the JSONL as a `skipped` record and footnoted in the report, so a coverage hole cannot read as a result. Measured, `profiled` at the `l` rung (12M variables): LP file, farkas 8.42 s / 1.49 GB against linopy 2.44 s / 3.26 GB; HiGHS handoff, farkas 7.24 s / 2.60 GB against linopy 2.93 s / 3.23 GB. The parity gate passes on the new case. No published numbers change here: `bench/results/latest.jsonl` is left as it was, since re-running the ladder is its own act. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: refuse a parameter source that carries a coordinate twice (#201) A parameter is a function of its dims, so two rows for one coordinate has no defined answer. The eager lane already refuses such a source — `xarray` will not lay out a duplicated index — while the relational lane quietly resolved it into a sum. Two lanes that accept the same language were accepting different data, and the one that accepted more gave an answer nobody asked for. Now it raises a `DataError` naming the parameter and up to three offending coordinates. One `GROUP BY … HAVING count(*) > 1` per parameter, over a source orders of magnitude smaller than the model built from it. `test_a_coordinate_must_be_single_valued` had to be narrowed: it doubled the whole generator frame, which feeds both the coordinate index *and* two parameters, so the new check now fires first and correctly. It doubles only the index; the parameter case is its own test beside it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(main): release 0.0.0-alpha.27 (#210) Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com> * fix: three ways an objective or a scalar parameter answered quietly (#223) * fix: an objective sums the terms it names, once each Two ways an objective quietly answered a question nobody asked. `x * a + y * b`, with `x, a` on `i` and `y, b` on `j`, is `|i| + |j|` summands. The relational lane knew that — an expression there is a set of term fragments, each keeping its own dims until the objective sums it. The eager lane added the two operands first, which is linopy's `+`, which broadcasts: every term counted once per coordinate of its sibling, `|i| * |j|` summands, 8.8x apart on the mixed-density case that found it (#197). Hard rule 3 says the lanes mean the same thing, and here the relational one was right. So the eager lane distributes the sum over addition and sums each product on its own. The distribution has to survive an operator applied to the group — `(x * a + y * b) * c` was wrong by the same factor — so `*`, `/` and unary minus are walked too, and everything else stays one opaque term. Separately, `objectives.<name>.equations` is a list of which only the first entry was ever read. It reads as "several terms in one objective", because that is what the key means under a constraint, and the answer was wrong by whatever the ignored entries were worth with nothing to see (#198). It is now a load error naming the rewrite, spelled from what was written. SPEC said "only equations[0] is used" in §2 and "the last defined wins" for multi-objective in §11 — the second was already false, since a second objective has been a load error for a while. Closes #197 Closes #198 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: a dimensionless parameter is one row, and now has to be `_check_one_row_per_coordinate` returned early when a parameter had no dims, on the reading that there was no coordinate to group by. But a parameter with no dims has exactly one coordinate — the empty one — so the rule it was skipping is the rule that matters most there. The join that broadcasts a dimensionless parameter is `ON TRUE`, which is correct broadcast semantics for one row and a silent row multiplication for two: duplicate `cols` entries for one variable in a bound, duplicate mask rows in a where. Both built and solved without a word. A user who gets here has usually passed a column they thought was indexed, so the message says what a dimensionless parameter means rather than only what is wrong with the source. Zero rows is caught by the same count, and was the other half of the hole. One `count(*)` per scalar parameter, at metadata level. Closes #166 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(main): release 0.0.0-alpha.28 (#226) Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com> * perf: compute variable labels arithmetically when the mask factors (#178) The label frame was the single most expensive statement in build, and the cost was the sort inside ROW_NUMBER() OVER (ORDER BY t_snapshot.ord, t_generator.ord) When the mask does not read the leading dim, that sort is avoidable without changing a single label. `where: "p_max > 0"` depends only on generator, so the surviving trailing coordinates are identical for every snapshot: rank them once over a table of size prod(rest), and the label is `(lead.ord - lo) * width + rank` — no window function, no per-chunk sort. The general path stays for masks that do read the leading dim. Measured with bench/run.py, best of the repeats, on this machine: case/size build before build after vs linopy dispatch/m 1.27s 0.88s 2.20x -> 1.67x dispatch/l 8.66s 5.01s 1.35x -> 0.90x transport/m 1.56s 1.10s 2.03x -> 1.65x transport/l 10.67s 7.27s 1.65x -> 1.31x dispatch/l crosses over: farkas is now faster than linopy end to end at 10M variables. The absolute ratios are machine-specific and differ from the committed macOS numbers, so the delta is the reliable part; docs/benchmarks.md is left for a run on the machine its ladder was measured on. Labels are unchanged, and that is checked rather than assumed: building the same 10M-row frame both ways and diffing gives 0 differing rows, and the same holds on the walkthrough model. One test moved. examples/walkthrough.py printed `sol.primal('p').head(6)` unsorted, so its golden file pinned the physical storage order of var_p — which this change alters even though the labels do not. primal() is a label join and a join has no inherent row order, so the example now sorts explicitly. That makes the golden stable under any future storage change (chunking, parallelism, a duckdb upgrade), which it was not before. Left alone deliberately: `primal()` itself still returns rows in whatever order the join produces. Adding an ORDER BY there would also hit _solution_to_parquet, which streams, and a global sort is exactly what that path exists to avoid. Claude-Session: https://claude.ai/code/session_0145rKirxsCRGodhkKozjaYW Co-authored-by: Claude <noreply@anthropic.com> * perf: cols.vtype is an ENUM, not a VARCHAR per column (#174) `vtype` is one of three literals written once per column, which as VARCHAR made it the widest thing on the row — and the row count is the model's column count. An ENUM stores the tag instead. Nothing downstream moves: `vtype = '...'` comparisons in both sinks keep working against an ENUM unchanged. Measured on the branch this was split out of (#153): 0.87s -> 0.48s writing `cols` at 10M columns. The members come off `plan.VariableType` rather than being typed out, so the Literal stays the single declaration. The test pins that: an ENUM rejects a value it does not know, so a fourth variable type added to the plan and not here would otherwise fail at insert with a duckdb cast error a long way from its cause. Closes #170. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(main): release 0.0.0-alpha.29 (#227) Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com> * perf: unmasked label assignment needs no sort (#152) `_label_frame` numbered every coordinate with `ROW_NUMBER() OVER (ORDER BY ord, ...)`, a full sort of every chunk. It was 71% of build at 10M variables (#146), and the sort is the whole cost: the same statement without the ORDER BY runs at the join floor. When nothing is masked out, that rank has a closed form -- it is the mixed-radix value of the `ord` tuple -- so the label can be computed instead of sorted for. Isolated at 10M rows: 4.37s -> 0.56s. End to end, interleaved best-of-three, `transport` at 9.8M variables: build 10.19s -> 6.52s (-36%). The labels are *the same integers*, not merely valid ones, which is what makes this a pure optimisation rather than a decision. #151 weighed dropping the ordering guarantee instead and had to leave three questions open -- `solver_direct`'s batching, reproducibility (#109), and the locality of `A`. None of them arise here: nothing downstream can tell the two apart, and the walkthrough golden that a renumbering would have forced to regenerate is untouched. Masked frames keep the window. Denseness there depends on which rows survive the predicate, and no closed form knows that -- so `dispatch`, whose variable is masked by `where: p_max > 0`, is unchanged by this commit. Verified: HiGHS `addRows` is insensitive to column-index order within a row (bit-identical objective and primal under reversed and shuffled indices), so the `ORDER BY` was not load-bearing for the shipped path either. 332 passed; ruff and pyrefly clean. Claude-Session: https://claude.ai/code/session_013DtGqM3z2DTtT556178iXi Co-authored-by: Claude <noreply@anthropic.com> * chore(main): release 0.0.0-alpha.30 (#228) Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com> * docs(bench): re-run the ladder on the merged tree Full ladder after merging main: five cases, two sinks, both arms, best of three, parity gate green on all five. 288 records, no failures. **Every ratio is unchanged within noise, and that is the expected result.** main's three duckdb perf PRs cannot move this comparison: neither arm here is duckdb. The farkas arm is the polars engine and the linopy arm is `farkas.linopy.build`, so the work in #152, #174 and #178 has nothing to act on — their ideas already arrived on this branch as `_positional`, #211 and #212, and it is those that the numbers already reflected. `transport` reads 1.59x / 0.81x where it read 1.62x / 0.78x, `sector` 0.31x where it read 0.32x, and so on. The tables and every figure quoted in the prose are refreshed off the new file so the doc and its provenance agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: route the objective tests' pandas through the oracle guard `test_objective.py` arrived from main with a bare `import pandas as pd`. It merged without a conflict, so nothing flagged it — and on this branch pandas is not a core dependency, so the bare-install CI job died collecting it. Every other test module on this branch takes pandas from `tests.oracle`, which is a `pytest.importorskip` behind the `[linopy]` extra. `test_duals.py` even carries the comment this file needed: "through the guard: a bare import would beat it". Reproduced against an install with no pandas, linopy or xarray: 11 skipped and a collection error before, 214 passed and 21 skipped after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: fluxopt-release[bot] <267260463+fluxopt-release[bot]@users.noreply.github.com>
Two changes to
bench/, both about measuring what callers actually do. Closes #202.1.
profiled— the case whose input is the same size as its modelEvery existing case builds its coordinate product from parameters far smaller than the product they explode into, so reading the parquet is 2-9% of the build and the read path is noise.
profiledisnodal's cardinalities with the mask removed andavailabilitydense over(snapshot, node, tech)— at thelrung, a 12M-row table against a 12M coordinate product. The read path, the join, and the in-memory-vs-parquet question all become measurable.It is also the shape the eager lane handles best: a parameter already dense over the variable product is the array xarray wants, with no broadcast and no alignment, while we join a full-size frame against a full-size coordinate product.
nodalis the mirror image and our clearest win. Carrying both is what keeps the ladder from holding only shapes that suit one engine.Density stays 1.0 and there is no
where— sparsity isnodal's axis, and sweeping two at once leaves no way to attribute an effect.2. Both sinks, not just the LP file
--sinks lp highs, defaulting to both. The LP file is the artifact fewest callers want, and it is not the same comparison:profiled/mHiGHS's own dense model is resident in both arms and swamps the difference between them. Measuring only the LP path reported the wrong number for the common case.
The
highssink stops at the handoff —run()is never called. The simplex is the same work whoever filled the model, so including it would swamp the phase this harness exists to measure and publish a number about HiGHS under our name.solve_directis split intobuild_highs+ the solve so there is a seam to stop at; linopy'sModel.to_highspy()is the same seam on the other side, which is what makes the two arms comparable rather than merely both slow.Case.highs_max_variablescaps the sink, because the ceiling is the solver's model rather than either engine's. A capped rung lands in the JSONL as askippedrecord and is footnoted under the table, since a rung nobody ran and a rung with no row look identical otherwise — that is how a coverage hole gets published as a result.Measured
profiledatl(12M variables, 12M-row availability parquet):Our worst wall ratio of any case, which is the point of adding it — and still a peak win. The parity gate passes: objectives agree exactly.
Scope
run.py's process model, the gate, or how data is generated and cached.bench/results/latest.jsonlis unchanged. Re-running the ladder is its own act, and the README's own advice about drift says not to mix it into a change.docs/benchmarks.mdis untouched for the same reason — nothing published here.bench/regressions/gains atest_build_and_hand_oversibling andprofiled/m. Deliberately a new test rather than a[lp]/[highs]parametrisation of the existing one:--benchmark-memory-comparematches baselines by test id, and adding an axis would orphan every baseline already recorded.Notes for review
build_highsis exported from the module but not fromsinks/__init__.py, and stays private-by-convention. Making the handoff a first-class part of the public surface is A Solver session, and value rebinding, as one feature #204, where it belongs with the session object that justifies it.profiledeager arm reshapes rather than going throughfrom_series. That is deliberate — the file already carries the layout xarray wants, and routing it through a sort the harness imposed would make the comparison dishonest in our favour. Order is load-bearing, and a permuted file would change the objective and be caught by the gate before anything is timed.🤖 Generated with Claude Code