Repository navigation
Conversation
Measured and rejected; kept for the record. See the PR.
FBumann
added
area:engine
|
Closed as measured-and-rejected, not as abandoned. The peak win (8–16% on the write path, every case) is real and I want it; paying for it with a sort in the executor is not the way, and the table above is why. Picking it up from the other end — having the label paths emit in label order rather than sorting afterwards — as its own measured change. The test this uncovered is #427. |
|
Warning Review limit reached
Next review available in: 43 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 (3)
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 |
…an repaired (#433) `cols` holds one row per column, and labels are globally dense 0..n-1 — so if it arrives in label order its `col` column is the frame's own row number. 0.32 GB of index at 40M columns, held for as long as the model is, plus the work it caused: `lp_file` sorted it twice and `dense_columns` scattered three of its four vectors through it. The first attempt bought that order with a sort in the executor and cost 13-92% of the build (#428, closed with its numbers). This produces the order instead. **polars' streaming engine walks a cross join right-major.** Folding the coordinate product backwards therefore emits it in *declaration* row-major order, which is label order, at no cost — it is the same joins in the other sequence. The factored path's second cross join gets the same treatment, and the bounds join carries `maintain_order='left'`, which is +12 ms on a 10M-row join against the ~110 ms sort it replaces. on a quiet machine, minimum of three repeats peak build wall dispatch/l lp -20.8% +9.4% -7.4% transport/l lp -14.5% +1.0% -0.8% profiled/l lp -10.9% -9.2% -8.2% nodal/l lp -10.2% -4.7% -5.8% dispatch/l highs -5.6% +10.0% -13.1% That order is a property of a *dependency*, not of this package, and a label is arithmetic on ordinals rather than a position — so a change in polars would not corrupt a label, it would silently stop the frame being sorted while `cols` is still read positionally. `_in_label_order` therefore verifies rather than trusts: a linear scan over a column the frame already holds, with a sort behind it as the correctness floor. The test uses two dims because one dim is a scan and cannot be out of order, and distinct bounds per coordinate because a permuted frame still holds the right multiset. Removing either guard fails it.
Opening this to close it. The idea is right, the implementation is not, and the numbers are worth keeping where the next person will find them.
The idea
colshas exactly one row per column, and labels are globally dense0..n-1(verified on all six bench cases:sorted(cols['col']) == range(n)). So if it arrives in label order, itscolcolumn is the frame's own row number — 0.32 GB of pure index at 40M columns, held for the model's whole lifetime.Dropping it also collapses work at both sinks:
lp_filestops sortingcolstwice, andModelTables.dense_columnsstops scattering three of its four vectors, because they become the frame's own columns.What it measured
bench.run --cases dispatch nodal transport profiled --sizes m l --repeat 3, back to back.dispatch/llpprofiled/llptransport/llpnodal/llpdispatch/lhighsThe memory thesis holds — 8–16% off peak on the write path, on every case. The price does not. Paying for label order with a
sortin the executor costs 13–92% of the build, and the scatter it was supposed to buy back never showed.Why my estimate was wrong
A micro-benchmark said the sort (439 ms at 40M) would be cheaper than the scatter it removes (1330 ms). End to end it wasn't, for two reasons I can see:
.to_numpy()on a polars column is not always the zero-copy view I assumed, sonan_to_num(copy=True)may have added a copy where the old path already had one.collect, where it costs more than on the materialised frame the micro-benchmark timed.That is the harness doing its job. A
+92%build would have been an easy thing to miss by quoting the micro-benchmark.What survives
Two things, both already elsewhere:
dense_columnsmutates its vectors in place, which is safe only while they are freshly allocated. This change makes them views, and the moment they are views the projection rewrites the built model to suit whichever solver asked last. That trap is live onmaintoday withhighsandgurobiboth shipping, and nothing in the suite would have caught it.ROW_NUMBERimposes it, and the arithmetic and factored paths emit head-major cross joins that are close to it. An engine that guarantees the order instead of sorting at the end pays nothing for it, and the 8–16% stands on its own.That is the next piece of work, and it belongs in the label paths on both engines rather than at the sink contract. Closing this so it isn't mistaken for something waiting to merge.
🤖 Generated with Claude Code