Repository navigation
perf(gurobi): one call, and what the remaining gap actually is - #434
Merged
Merged
Conversation
`addMConstr` costs about 42 ns per *model* column per call, whatever the
block holds — so batching the matrix charges the model's full width again
on every block, and the wider the model the worse it gets. Splitting 2M
nonzeros into 20 blocks took 0.55s against 0.23s in one; 40 blocks took
0.89s.
Batching bought nothing back. It cannot bound the peak here, because
Gurobi accumulates the whole model as it goes: the only thing a block
saves is one block-sized temporary, measured at 0.62 GB against 0.65 GB
for twice the hand-off time. That is the opposite trade from the HiGHS
sink, where the budget is measured and stays.
1M cols/rows, 5M nnz 3.23s -> 1.14s
400k, 2M nnz 0.78s -> 0.45s
100k, 500k nnz 0.14s -> 0.11s
`batch_rows` still splits when a caller asks, which is how the tests
exercise block boundaries — indptr arithmetic is worth a test even when
nothing runs it by default.
Also skips the `vtype` array for a model with no integers, which is
linopy's own trick: an LP was paying 17% of the column hand-off to send
one repeated letter (0.46s against 0.38s at 10^6 columns).
The remaining ~10x against HiGHS is inside gurobipy's own API, on the
two calls linopy makes too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
added
area:engine
|
Warning Review limit reached
Next review available in: 28 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 (1)
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 |
FBumann
added a commit
that referenced
this pull request
Jul 31, 2026
#434 landed while this branch was open, and the two changes disagree about the same function for good reasons on both sides. #434 measured that Gurobi's `addMConstr` charges ~42 ns per *model column* per call whatever the block holds — 0.23 s in one call against 0.89 s in forty — and stopped batching it. This branch moves both solvers onto a shared `row_blocks` so they cannot come to disagree about the model they load. Resolved by putting #434's finding *into* the shared contract rather than around it: `row_blocks(budget=None)` yields one block, and its docstring says that whether splitting pays is a property of the API being fed and not of the model — HiGHS's budget bounds a temporary, Gurobi's per-call cost scales with the model's width. Both answers now come out of the same code, and `gurobi.py` passes `batch_rows` straight through un-defaulted. row_blocks(None) -> 1 block addMConstr calls: 1 row_blocks(5) -> 4 blocks addMConstr calls: 4 batch_rows=1 addMConstr calls: 20 objective 190.0 in every case Kept from this branch over main's: the Gurobi sense spelling derived from `SENSE_CODES` rather than written out beside it. main's version is a literal array in an order that has to match by hand, and a wrong order there is a model whose comparisons are silently permuted — which every solver answers confidently. 752 passed, 23 of them Gurobi's, ruff + pyrefly clean.
FBumann
changed the title
This was referenced Jul 31, 2026
FBumann
added a commit
that referenced
this pull request
Jul 31, 2026
…ug made (#441) * docs(bench): re-measure the published tables, and reverse a conclusion The committed results were taken at `ef8558c` — version `0.0.0a27`, before the package was even renamed. Eight perf changes have landed since (#408, #412, #413, #414, #415, #434, #421, #433), so every table on the page understated the engine, and one paragraph drew a conclusion the numbers no longer support. Re-run in one sitting on an idle machine: the ladder, the density sweep and the scaling run, so all three files describe the same tree. Parity gate passes on all six cases (`fleet` to 4.6e-16) before anything is timed. **What moved**, against the previously published numbers, `l` rung, our arm: wall -10% to -39% on nine of twelve rungs, peak -2% to -21% on nine. **What got worse, and is not buried.** `dispatch` build is 10-12% slower — that is `maintain_order='left'` from #433, which buys -21% peak on the same rung and is the trade that PR argued for. `fleet/highs` peak is up 8.2%, outside its 1.0% floor, and I have no explanation for it. **A conclusion is reversed.** `## Absurd sizes` read 1.07x peak at 120M variables and concluded that our memory headroom "is a small-and-sparse-model property, not a scaling one". Peak is now 0.73-0.83x across four orders of magnitude. Two things moved it and only one is us: #433 took ~20% off this case, and the earlier reading came from a run *that section itself flagged as noisy* — it recorded linopy at 38.5 s and 106.2 s for the same rung. The old number was a conclusion drawn across the noise floor. The section now says so rather than quietly presenting a better one. **`profiled` is no longer the case in the ladder we lose** (1.09x -> 0.74x through the highs sink), and `fleet/lp` has crossed on wall (1.03x -> 0.90x). Both paragraphs are rewritten to say what changed rather than to delete the sentences that admitted the losses. Two numbers still go against us and are still named: `profiled` 1.31x on wall through the LP sink, `fleet` 1.47x on peak. Not touched: `## Sink capabilities` still says Gurobi's row wants a spike before it is relied on. #418 shipped a real Gurobi sink since, so that caveat is probably obsolete — but exercising it is not this change, and asserting capabilities I have not run is how that table would come to lie. * docs(bench): re-measure at 98f382d, and correct a claim the harness bug made Every number on the page now comes from one run at `98f382d` on a clean tree, and the page says so — the fingerprint hash is what makes a published number traceable to the code that produced it, and the previous one recorded `782f76b-dirty`, which identifies nothing. **The `highs` comparison was not like for like, and it flattered us.** #438 found that linopy's `to_highspy()` was being timed *with* variable naming, which neither of our solver sinks does — 82% of its hand-off, and memory as well as time. Corrected: highs sink, l rung was now dispatch peak 0.87x 1.46x fleet peak 0.76x 1.10x profiled peak 0.82x 1.27x transport peak 0.75x 1.10x So the summary changes from "ahead on both, on all six" to **ahead on wall on five of six, and behind on peak on four of six through `highs`**. HiGHS's own dense model dominates both arms there and our COO triples are what is left. The `lp` sink barely moved, because that comparison was already like for like, and it is where the representation actually pays: ahead on peak on four of six. `profiled` is the case in the ladder we lose, on both sinks (1.27x, 1.07x). Said plainly rather than left to the tables. The scaling section's reversal stands and is re-measured: peak 0.68-0.78x from 10k to 120M variables, against the 1.07x this page used to conclude the advantage "runs out" from. That section now also says it is the LP sink, and therefore not evidence for the `highs` peak row.
FBumann
added a commit
that referenced
this pull request
Jul 31, 2026
A full run at f319cd1: six models, four rungs, both arms, and for the first time all three sinks — 192 measurements, 35 minutes, 192 passed. **Under pytest-benchmem 0.5**, whose isolated pass no longer warms up, so a `peak` here is the cold first call in a fresh process. That is the thing this page describes and the thing a caller building one model pays; a 0.4 peak is a different quantity wearing the same name, so the floor moved to `>=0.5` rather than letting the two mix. The release's headline fix — Linux `ru_maxrss` reading the parent's high-water mark — does not touch these numbers; macOS was never affected. **Peak reproduced; wall did not.** Against the previous run this one agrees on every peak cell to within 0.02x — the same 1.46x, 1.10x, 0.33x — and disagrees on wall by up to 40% relative. Peak is a property of what the code allocates; wall on a shared laptop is a property of what else was running. The page now says so, and says to read the wall column for its sign and order of magnitude rather than its second digit. **Gurobi is the slowest destination for both arms**, and the one where the lanes converge: gurobipy's per-row and per-column ingestion (#434) dominates the build in front of it. The fourth figure had to change with the harness. #448 made `test_emit` one number for build-and-hand-over and left the only other measurement a warm in-process rebuild, so there is no honest way to stack a phase bar any more; it is now the `l` rung through every sink, which is the question the third sink makes worth asking. `bench/conftest.py` tracks gurobipy and scipy from here on — a ratio through a solver has to say which solver. This file predates that by minutes, so the two versions are named in the page instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
added a commit
that referenced
this pull request
Aug 1, 2026
A full run at f319cd1: six models, four rungs, both arms, and for the first time all three sinks — 192 measurements, 35 minutes, 192 passed. **Under pytest-benchmem 0.5**, whose isolated pass no longer warms up, so a `peak` here is the cold first call in a fresh process. That is the thing this page describes and the thing a caller building one model pays; a 0.4 peak is a different quantity wearing the same name, so the floor moved to `>=0.5` rather than letting the two mix. The release's headline fix — Linux `ru_maxrss` reading the parent's high-water mark — does not touch these numbers; macOS was never affected. **Peak reproduced; wall did not.** Against the previous run this one agrees on every peak cell to within 0.02x — the same 1.46x, 1.10x, 0.33x — and disagrees on wall by up to 40% relative. Peak is a property of what the code allocates; wall on a shared laptop is a property of what else was running. The page now says so, and says to read the wall column for its sign and order of magnitude rather than its second digit. **Gurobi is the slowest destination for both arms**, and the one where the lanes converge: gurobipy's per-row and per-column ingestion (#434) dominates the build in front of it. The fourth figure had to change with the harness. #448 made `test_emit` one number for build-and-hand-over and left the only other measurement a warm in-process rebuild, so there is no honest way to stack a phase bar any more; it is now the `l` rung through every sink, which is the question the third sink makes worth asking. `bench/conftest.py` tracks gurobipy and scipy from here on — a ratio through a solver has to say which solver. This file predates that by minutes, so the two versions are named in the page instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
gurobisink was batching the matrix the way the HiGHS sink does. Thattrade does not transfer — and chasing the rest of the gap turned up what it is
actually made of.
The fix: one
addMConstraddMConstrcharges per model column per call, whatever the block holds —about 42 ns each, so every extra block pays the model's full width again. At
400k columns that is 17 ms of pure overhead per call:
And batching bought nothing back: it cannot bound the peak, because Gurobi
accumulates the whole model as it goes. The only thing a block saves is one
block-sized temporary — 0.62 GB against 0.65 GB, for twice the hand-off time.
The opposite of the HiGHS sink, where the budget is measured, real, and stays.
Plus two measured leftovers: a single sense character where a block's rows
all compare the same way (11% of the matrix call, and the ordinary case — every
balance an equality, every limit a cap), and one
update()instead of two(
optimizedoes its own;build_gurobikeeps one so the model it returnsstill reports what it holds).
batch_rowsstill splits when a caller asks — that is how the tests exerciseblock boundaries, and the
indptrarithmetic is worth a test even when nothingruns it by default.
What else was tried, and did not help
Every other documented way into gurobipy, at 400k×400k / 2M nnz:
x=None("use all variables")csc_matrixinstead ofcsr_matrixint32indices instead ofint64A @ x <= bexpression formgurobipy.read()What the remaining gap actually is
It is not per-nonzero. At a fixed 400k×400k shape:
Five times the data costs 26% more time. So Gurobi's ingestion is dominated by
a per-row and per-column term, and HiGHS's equivalent is roughly 5× cheaper
— which is the whole of the residual difference. It is not something we can
route around by handing the data over differently: we already make the minimum
two calls (
addMVar,addMConstr), which is also what linopy makes.The practical shape of it: a 1M×1M model loads in ~1s, in front of a
branch-and-bound measured in minutes. And the gap narrows as models get
denser, since the part that scales with nonzeros is the part Gurobi does well.
Follows #418. Correctness unchanged, still pinned by the ported models reaching
their published optima on this sink.
🤖 Generated with Claude Code