fix(perf): #875 root setup cost and budget — sparse linearizer, deadline-aware tightening#878
Merged
Merged
Conversation
…st node Successor to #863/#868. PR #868 took `watercontamination0202` (106,711 vars / 107,209 rows, 7 binaries) from an unbounded hang to a bounded overrun: 579.3 s against a 30 s `time_limit`, `nodes=0`. The remaining time was fully attributed, and every item is pre-B&B root setup -- the class #858 fixed in the LP engine and #868 fixed in the Rust presolve passes, one layer up each time. Two DIFFERENT defects hide under that heading, and conflating them is why #868 looked complete: phase cost defect _fix_single_var_equalities ~460 s COST, not budget tighten_nonlinear_bounds x3 80.9 s BUDGET (no deadline parameter) _classify_model_convexity x2 14.7 s BUDGET (per-model, not per-solve) presolve + load + classification ~12 s already capped by #868 COST. `_fix_single_var_equalities` scans `==` rows looking for a body affine in a SINGLE variable. Each row's body has one leaf. It cost O(rows x vars) anyway: `_linearize_affine_expr` opens with `np.zeros(n_vars)` -- allocated and zeroed per call -- and the caller then walked all 106,711 entries in Python to find the one nonzero. No deadline should be needed for that scan; it should be O(nnz). Entry experiment, 400 single-variable equalities, varying only the variable count: n_vars before after 2,000 0.068 s 0.001 s 8,000 0.240 s 0.001 s 32,000 0.951 s 0.001 s 128,000 3.650 s 0.002 s (3.84x per 4x n_vars -> 0.94x) Exactly linear in n_vars before, flat after: the signature of the dense array rather than of the bodies, and it extrapolates to the measured ~460 s. Adds `_linearize_affine_expr_sparse` as the core (dict keyed by flat index) with the dense `_linearize_affine_expr` as a view of it -- same recognition rules, same refusals, same coefficients -- and routes every per-constraint scan through the sparse form: `_fix_single_var_equalities`, `_detect_one_hot_groups`, the monomial-factor fallback in `_extract_positive_product`, and the two affine bound helpers that zipped all n coefficients against the box. Adds `_any_linear_constraint_form`, which short-circuits on the first hit, for the two callers that only wanted a boolean: `bool(_linear_constraint_forms(...))` built and KEPT one dense row vector per constraint -- 91 GB on this instance -- and one of them sits in the `MccormickLPRelaxer` constructor on the root path. BUDGET. `tighten_nonlinear_bounds` had no deadline parameter at all. It now takes one and polls at three granularities: between rounds, between rules, and inside a rule's row scan. The innermost is what makes the poll real -- one rule's sweep over 107k rows already exceeds a tight budget, so a between-rules-only poll would repeat #868's `probing` lesson (a poll at the wrong granularity is not a poll). Truncating a row scan is NOT universally sound, and the API says so rather than leaving it to each rule's author. `row_scan_is_anytime` defaults to **False**; 13 rules opt in because each row's inference stands alone, so a prefix yields a looser box. `PeriodicVariableBoundRule` does not: it restricts a variable to one period precisely because nothing *else* in the model uses it, so a scan cut short could miss the disqualifying row and CUT FEASIBLE POINTS. Under a budget it is declined whole rather than run on a prefix. The safe default is the one a future rule inherits without thinking about it. The convexity classifier's budget is `0.2 * time_limit` recomputed per *model object*, and a reformulation produces a new one, so the fractions add up (two runs, 14.7 s, half the budget gone before dispatch). It is now also clamped to the absolute `model._solve_deadline`, which the binary-multilinear reform path was not carrying onto its rewritten model. Every early exit does strictly less work -- a looser box, fewer rules, convexity-unknown routing to sound spatial B&B. The one thing a budget can cost is an infeasibility PROOF the full pass would have found; `infeasible` then stays unset and B&B discovers it, which is weaker, never false. Also, both independent items the issue lists: * `gap_certified` now needs BOTH ends of a gap. A limit exit with a valid dual bound and no incumbent reported `status=time_limit, objective=None, gap=None, gap_certified=True` -- a certified gap where no gap was ever formed, and the flag is exactly what the graduation panels check before reading the values. Reproduced at 0.5 s on a 300-var bilinear model. The downgrade is True -> False only, and the dual `bound` is kept: a bound with no incumbent is still a valid bound. * `_extract_quadratic_coefficients_from_values` (the QCP probe path) is the last extractor still sweeping all O(n^2) pairs into a dense (n, n), once per constraint. Now support-restricted (free -- the O(n) diagonal probes already identify the support) and materialised through `_materialise_Q`, matching the QP path. `_quadratic_row_has_terms` and the Gurobi QCP entry are made sparse-aware in step, so a sparse Q cannot reach a consumer as the 0-d object array `np.asarray` silently produces instead of raising. VERIFICATION. Bound-neutrality panel (CLAUDE.md §5 regime 1) over the in-repo 66-instance MINLPLib corpus, HEAD~1 vs HEAD, `time_limit=5`, arms run sequentially with no concurrent load: terminal in both arms 38 node_count, objective, bound, cert all EXACTLY equal limit-status in both 28 no objective drift; no bound moved intended cert downgrade 2 bchoco08, casctanks -- both `objective=None, nodes=0` limit exits, dual bound bit-identical status differences 2 nvs12, tspn10 -- both shown FLAKY, see below The two status differences are pre-existing non-determinism at a 5 s budget, not regressions. `nvs12` flips optimal/feasible on BOTH trees across 5 repetitions (before 2/5 optimal, after 3/5) with objective, bound and gap bit-identical every time -- its certification, not its search, is timing-dependent (one before-run even returned 233 nodes instead of 231). `tspn10` returns `feasible/nodes=1/obj=225.126` 5/5 on both trees; the panel's single `time_limit` reading did not reproduce. Suites: `pytest -m smoke` 828 passed / 15 skipped / 2 xpassed (identical to the pre-change baseline); `pytest -m slow test_adversarial_recent_fixes.py` 10 passed; the targeted nbt / QP / classifier / convexity / RLT / McCormick / relaxation / certification sweep 2087 passed. The two cyipopt failures in that sweep are pre-existing in this container (Ipopt is not installed) and reproduce on a clean tree. ruff, ruff format and mypy clean. No Rust touched. New tests target the mechanisms, because the in-repo corpus has nothing near this scale: a scaling probe for the cost bug (fails before at 18.8x for 16x the variables, passes after at 1.0x), expired- and future-deadline tests for the budget bug (the latter asserting a non-binding budget changes NOTHING -- same box, same stats), an anytime-contract test asserting a budgeted pass never tightens PAST the unbudgeted one, and a behavioural test that the period rule reaches its final row under an expired deadline. `test_875_sparse_qcp_probe_extraction.py` compares forced-sparse against dense and asserts `sp.issparse` on the sparse arm -- without that the comparison is dense-vs-dense and proves nothing, the trap #868 recorded. NOT VERIFIED: the issue's definition of done (`watercontamination0202` within ~1.25x for T=30 and T=60). This environment has no MINLPLib snapshot and no network route to minlplib.org, so that instance was never solved here. The test is written (`test_watercontamination0202_honours_its_time_limit`) and skips without the snapshot; it needs a run on a machine that has it before the issue can be closed. Contributes to #875. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165gvPnrnhfFnZWmsWDopA1
Two assertions in `test_875_root_setup_budget.py` were wall-clock comparisons, and a wall-clock assertion under load is exactly what cost time on #863 -- and briefly on this branch, where `test_large_dense_jacobian_no_crash` failed only because a test run was going concurrently. `test_fix_single_var_equalities_is_flat_in_the_variable_count` divided the 32,000-var wall by the 2,000-var wall and required < 4x. After the fix both are ~1 ms, so that is noise over noise: one scheduler hiccup flips it. Now the PRIMARY assertion is absolute -- 32,000 vars with 300 rows must finish under 0.25 s -- with min-of-3 repetitions (the fastest run is the one least polluted by whatever else the machine was doing). Measured on this tree with that methodology: n_vars before after 2,000 0.0552 s 0.0010 s 32,000 0.8480 s 0.0011 s so 0.25 s sits 3.4x below the dense implementation and 227x above the sparse one -- it still fails dense on a runner 3x slower than this one. The ratio check is kept as the complexity-class signal but only evaluated when the baseline clears 5 ms, i.e. when the quotient means something; before the fix that gate is open (0.0552 s, ratio 15.4x, fails) and after it is closed. `test_any_linear_constraint_form_matches_the_list_and_short_circuits` asserted `short < full` on two timings. Split in two, and the short-circuit half now COUNTS linearizations instead of timing them: the boolean probe must linearize exactly 1 row when the first row is linear, against 200 for the full list. That is the actual claim -- it replaced `bool()` of a fully materialised list -- and a call count cannot flake on a loaded runner. No production code touched. 15 passed locally; the fail-before/pass-after separation is preserved and is now wider than it was. Contributes to #875. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165gvPnrnhfFnZWmsWDopA1
…site tighten_nonlinear_bounds has four call sites. This PR gave three of them a deadline -- the root declared-box pass, the periodic/domain pass, and AMP -- but _apply_nonlinear_tightening_with_status (solver.py) still called it as tighten_nonlinear_bounds(model, lb, ub), so that site ran the pass to completion on every invocation. Measured on watercontamination0202 (106,711 vars / 107,209 rows) at time_limit=30: that site cost ~23 s per call and fired 3x, ~70 s of a 126 s cProfile run -- the single largest remaining overrun after the sparse-linearizer fix, and the reason a deadline on tighten_nonlinear_bounds ALONE did not bound the solve. After this change tighten_nonlinear_bounds disappears from the top of the profile entirely. End-to-end on the instance the issue is about: stage T=30 T=60 before #878 579.3 s 620.8 s #878 as-is 88.5 s 118.0 s #878 + this commit 47.2 s 76.3 s i.e. 12.3x and 8.1x better than the pre-#878 baseline. Both arms asserted the loaded build first (the sparse-linearizer marker present, plus the #877 presolve probe returning 0.0) so no number here is from the wrong tree. The deadline used is the solve's ABSOLUTE model._solve_deadline, not a fresh per-call fraction. A fraction recomputed per call is exactly how the convexity classifier's budget used to multiply across model objects (this PR's own finding), and this helper is called per node as well as at the root, so a fraction here would compound the same way. A missing attribute yields None, which preserves today's unbounded pass for an untimed solve rather than collapsing it to a zero budget. Soundness: unchanged in kind from this PR's existing argument. Every early exit does strictly less tightening, which leaves the box looser, never wrong; the one thing a budget can cost is an infeasibility proof, and stats.infeasible then simply stays unset for B&B to rediscover. Tests: python/tests/test_875_nbt_call_site_budget.py -- 5 tests pinning the WIRING rather than wall-clock (so it cannot rot into a machine-speed assertion): the deadline arrives at tighten_nonlinear_bounds, it equals the absolute solve deadline, an absent deadline stays None, an expired budget neither widens a bound nor manufactures an infeasibility, and a source-level guard fails if any tighten_nonlinear_bounds call in solver.py omits deadline=. Fails-before/passes-after verified by stashing the fix: 3 failed / 2 passed before, 5 passed after. Verified: pytest -m smoke -> 832 passed, 1 skipped, 2 xpassed; pytest -m slow test_adversarial_recent_fixes.py -> 10 passed; ruff check + format clean; no Rust touched. Run on this branch with main merged in (so #877 is included). NOTE: the issue's acceptance criterion (~1.25xT) is still NOT met at 1.57x / 1.27x. The residual is now two constant-cost root phases that each individually respect the absolute deadline but together start before it and finish after it: build_uniform_relaxation ~18 s (bounding it is gated behind the default-off, CLAUDE.md 5 bound-changing root_build_deadline flag) and _classify_model_convexity ~15 s. A per-phase poll cannot fix that -- it needs root-setup sub-budgets that sum to T. Recorded on the issue. Contributes to #875
…/discopt into work/875
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.
Summary
Contributes to #875 — deliberately not "Closes"; see Not verified at the bottom. The issue's acceptance criterion could not be measured in this environment.
Root setup on
watercontamination0202(106,711 vars / 107,209 rows) took 579 s against a 30 stime_limitwithnodes=0after #868 stopped it hanging. The four items the issue lists are two different defects:Cost bug (~460 s, 23 of 29 stack samples).
_fix_single_var_equalitiesscans==rows for a body affine in a single variable — every such body has one leaf. It costO(rows × vars)anyway, because_linearize_affine_expropens withnp.zeros(n_vars), allocated and zeroed per call, and the caller then walked all 106,711 entries in Python to find the one nonzero. A deadline would have been the wrong fix. Adds_linearize_affine_expr_sparseas the core, with the dense_linearize_affine_exprkept as a view of it, and routes every per-constraint scan through it. Also replaces twobool(_linear_constraint_forms(...))calls — which built and kept one dense row vector per constraint (91 GB here) to evaluate a boolean, one of them in theMccormickLPRelaxerconstructor — with a short-circuiting_any_linear_constraint_form.Budget bug (95.6 s).
tighten_nonlinear_boundshad nodeadlineparameter; it now polls between rounds, between rules, and inside a rule's row scan. The innermost is what makes the poll real — one rule's sweep over 107k rows already exceeds a tight budget, so a between-rules-only poll would repeat #868'sprobinglesson. The convexity classifier's budget is0.2 × time_limitrecomputed per model object, so a reformulation restarts it and the fractions add up; it is now also clamped to the absolutemodel._solve_deadline.Plus both independent items from the issue:
gap_certifiednow requires both ends of a gap, and the QCP probe extractor's Hessian is support-restricted and sparse.Correctness
ValueErrorrefusals, same coefficients; the dense form is a literal view of the sparse core. Bound-neutral, and the panel below asserts that empirically.infeasiblethen stays unset and B&B discovers it — weaker, never false.row_scan_is_anytimedefaults toFalse. Thirteen rules opt in because each row's inference stands alone.PeriodicVariableBoundRuledoes not: it restricts a variable to one period precisely because nothing else in the model uses it, so a scan cut short could miss the disqualifying row and cut feasible points. Under a budget it is declined whole rather than run on a prefix. The safe default is the one a future rule inherits without thinking about it.gap_certified— downgrade True → False only, so it cannot manufacture a certificate. The dualboundis deliberately kept: a bound with no incumbent is still a valid bound; it is the gap claim that was unfounded.test_finite_zero_bound_is_a_valid_certificateassertedgap_certified is Trueforobjective=None, bound=0.0. Its stated purpose is that a falsy-but-finite bound must not be mistaken for "no bound" — that assertion is kept and strengthened (boundsurvives), and a companion case with an incumbent now covers the certification half.unbounded; that would have weakened an existing check.Tests run (state the result — numbers, not adjectives)
pytest -m smoke→ 828 passed, 15 skipped, 2 xpassed (identical to the pre-change baseline on this tree; the earlier "666 passed, 1 skipped" in this body was the template's placeholder example, not a measurement)pytest -m slow python/tests/test_adversarial_recent_fixes.py→ 10 passedcargo test -p discopt-core→ N/A, no Rust touchedruff check/ruff format --check/mypy→ cleanAlso ran a targeted nbt / QP / classifier / convexity / RLT / McCormick / relaxation / certification sweep: 2087 passed, 67 skipped, 2 xfailed. Two cyipopt failures in that sweep are pre-existing in my container (Ipopt is not installed) and reproduce on a clean tree.
Regression test
python/tests/test_875_root_setup_budget.py(441 lines) andpython/tests/test_875_sparse_qcp_probe_extraction.py(152 lines).Fail-before / pass-after confirmed by running the assertions against the stashed tree:
_fix_single_var_equalitiesscaling (16× vars, fixed rows)tighten_nonlinear_boundsaccepts a deadlinegap_certifiedwithobjective=NoneTrueFalse_any_linear_constraint_formexistsNotable tests:
test_a_truncated_pass_only_ever_loosens(anytime contract — a budgeted pass must return a superset of the unbudgeted box),test_a_future_deadline_changes_nothing(a non-binding budget changes nothing — same box, same stats), andtest_a_row_scan_that_needs_completeness_is_never_truncated(behavioural, with the disqualifying use in the last row and a positive control). The QCP tests assertsp.issparseon every forced-sparse arm — without that the comparison is dense-vs-dense and proves nothing, the trap #868 recorded.Performance change — measurement
No new flag: this is a bound-neutral refactor (CLAUDE.md §5 regime 1) plus budget plumbing that only fires when a budget binds, so there is nothing to graduate default-off.
Cost fix, entry experiment (400 single-variable equalities, varying only the variable count):
Exactly linear in
n_varsbefore (3.84× per 4× vars), flat after (0.94×) — the signature of the dense array rather than of the bodies, and it extrapolates to the measured ~460 s.Bound-neutrality panel, 66-instance in-repo MINLPLib corpus,
HEAD~1vsHEAD,time_limit=5, arms run sequentially with no concurrent load:node_count,objective,bound,certall exactly equalbchoco08,casctanks— bothobjective=None, nodes=0exits, dual bound bit-identicalnvs12,tspn10— both pre-existing flakes, see belowNeither status difference is caused by this change.
nvs12flips optimal/feasible on both trees across 5 repetitions (before 2/5 optimal, after 3/5) with objective, bound and gap bit-identical every time — its certification, not its search, is timing-dependent at a tight budget (one before-run returned 233 nodes instead of 231).tspn10returnsfeasible/nodes=1/obj=225.1265/5 on both trees; the panel's singletime_limitreading did not reproduce.nvs12's flakiness may deserve its own issue.Not verified
The issue's definition of done was not measured. This environment has no MINLPLib snapshot and no network route to minlplib.org, so
watercontamination0202was never solved here. All evidence above is mechanism-level plus the corpus panel; the ~1.25×T claim for T=30 and T=60 is unconfirmed.The decisive test is written and skips without the snapshot:
Arithmetic from the issue's attributed table suggests ~1.0–1.1×T, but that is an estimate and it does not account for the residual root-relaxation build cost. Please run that test before closing #875 — hence "Contributes to", not "Closes".