Presolve work and iteration limits - #1656
Open
akifcorduk wants to merge 20 commits into
Open
Conversation
Papilo and the probing cache were bounded only by a fraction of the wall clock (presolve_time_ratio * time_limit, and max_time_on_probing). On wide instances that fraction expires mid-round, so which reductions survive depends on how fast the machine is: bab2 at a 15s budget got 1.5s of Papilo and a reduced problem of 1.82M nonzeros, versus 1.17M when allowed to finish. Bound presolve by structure instead. Papilo takes a round cap and a ceiling on probing.minbadgesize; the probing cache takes a budget in work units, counted exactly from propagation iterations and probes and folded in at the step barrier in a fixed order so it does not depend on thread count. The timers now carry the time actually remaining in the solve, which keeps presolve from overrunning the whole budget without truncating it early. presolve_budget_policy.hpp maps structural features onto these budgets, with one policy per hypothesis so a sweep isolates the mapping itself; `legacy` reproduces the previous behaviour as a baseline. The `size` policy encodes what measurement showed: probing.minbadgesize, not the round count, drives Papilo's cost, and on wide problems a large badge buys almost nothing (capping it at 32 left square41, supportcase6, nw04 and rail507 bit-identical while cutting presolve 2-26x), whereas narrow problems reduce measurably worse when capped. Add PRESOLVE_BUDGET, PRESOLVE_PAPILO, PRESOLVE_PAPILO_REDUCED and PRESOLVE_PROBING log lines carrying the features that went in, the budgets that came out, and the realised cost, so budgets can be refit offline. A work budget is worth 0.3s on one instance and 16s on another, so the log records units_per_s to translate one back into the other. Signed-off-by: akif <akifcorduk@gmail.com>
The probing-cache budget was an absolute number of work units, and every derived policy was additionally clamped to at most 120. Probing cost is close to linear in the candidate count, so a constant budget probes a small problem exhaustively and a large one barely at all: at the default of 30 units that was 21% of 30n20b8 but 0.8% of netdiversion. On the 240-instance benchmark this cost four instances their feasible solution (physiciansched3-3, neos-3216931-puriri, 30n20b8, neos-5104907-jarama), which is most of the 10.97 -> 12.33 mean error regression. 169 of 239 instances exhausted the budget, mean error delta +2.18, against -0.59 for the 70 that did not. None of the 169 were limited by the clock: probing stopped after 0.2-1.8s with ~590s still available. Policies now state the fraction of candidates they want probed, converted with the same cost model the probing loop charges. A fraction >= 1 is carried as infinity rather than a large finite number, since the per-candidate cost is an average and a budget sized for full coverage would still truncate an instance whose probes are dearer than average. probing_step_size drops from 512 to 128, because at 512 the first step already overshot a 30-unit budget by 2-2.7x and probe counts were pinned to multiples of the step. `fixed` becomes the Papilo-only arm (probing unbounded, rounds and badge capped), and `size` keeps the measured badge rule at a quarter coverage, so benchmarking the two separates the Papilo caps from the probing budget. Analysis and numbers in design_summaries/presolve_budget/. Signed-off-by: akif <akifcorduk@gmail.com>
The 240-instance run could not separate the Papilo round/badge caps from the probing coverage, because a single policy fixes both at once. Add a 2x3 factorial over the two -- two Papilo rules against three probing coverages -- selected by CUOPT_CONFIG_ID so one build covers the whole sweep: 0/1/2 fixed (rounds 30, badge 1024) x 1.00 / 0.25 / 0.05 3/4/5 size (wide 50/32, narrow 20/uncapped) x 1.00 / 0.25 / 0.05 Configs 0 and 3 leave probing unbounded and so isolate the Papilo rule. Coverage is spaced geometrically rather than evenly because truncating probing won at 0.5-4.3% of candidates (bab6, square41, square47) and lost at 9.1-21.6% (physiciansched3-3, 30n20b8), which puts the crossover below 10%. The id is resolved once inside evaluate_presolve_budget rather than at each call site, so the Papilo stage and the probing stage cannot end up running different points of the sweep. It overrides the policy hyper-parameter, and the effective policy and config now travel on presolve_budget_t so PRESOLVE_BUDGET stays attributable instead of reporting the hyper-parameter that was overridden. Verified on 30n20b8: realised coverage 100 / 24.3 / 5.4% for configs 0-2 and 100 / 24.9 / 5.5% for 3-5. Signed-off-by: akif <akifcorduk@gmail.com>
Commit 8de5112 made both presolve timers carry remaining_time(), dropping min(presolve_time_ratio * time_limit, presolve_max_time) for Papilo and max_time_on_probing for the probing cache. Those were the only bounds on presolve in time, and a work budget does not substitute for one: realised probing throughput spans 12 to 450 work units per second, so on s100 even a 25% coverage budget ran 583-593s of a 600s solve. ns1760995 converges in well under 30 rounds, so its round cap never binds and Papilo ran 243-403s in every config. Presolve starving the solve is what drove the inf mip_gaps up: 16 of the 21 across the six configs have presolve taking 40-99.8% of the budget, and the probing-unbounded configs show 7 each against 1-3 for the truncated ones. With no time left the root LP never produces a dual bound. Restore both as min(cap, remaining_time()), so the cap is a hard ceiling and remaining_time only stops presolve reaching past the end of the solve. The structural budgets still shape presolve; the ceiling only stops it starving the solve. Verified at a 180s limit: ns1760995 Papilo 18.2s against tlim=18 with hit_tlim=1, supportcase6 probing 60.2s against wall_limit=60. Signed-off-by: akif <akifcorduk@gmail.com>
…ceiling Restoring the wall ceilings stopped presolve starving the solve, but only by truncating mid-round, which pays for reduction and then discards it. Size each stage's own limit so it finishes well inside 60s and the ceiling never fires. Probing throughput spans 1.9 to 689 work units/s over 660 measured runs, so no coverage fraction can bound time. nnz + n_cand * avg_col_len tracks the cost of a propagation sweep best of the proxies tried, and a ceiling of 1.5e8 over it caps the slowest measured run at 44s with none above 60s, turning today's 597s runaways into 3-34s. For Papilo, rounds never bind: all 240 instances converged inside 30 without hitting a round or time cap, and minbadgesize is the whole cost. Clamp it to 32 above n_bin * avg_col_len of 2e5, where a large badge stops paying for itself -- square47 40.6s to 8.1s and sorrell3 42.9s to 14.0s, both bit-identical reduced problems. ns1760995 is the exception where the badge does buy reduction, but only at 1024 and at 274s, and that is what loses the instance: truncating Papilo at 60s closed it to 1.37% while every run that let it finish ended with no dual bound.
…thodology to cuopt-developer Bounding MIP presolve by work units regressed the benchmark, and three separate assumptions had to be falsified by measurement before the fix landed: that a deterministic work counter bounds wall time, that the plausible-looking knob (Papilo rounds) drives cost, and that reduced probing coverage caused a given instance's feasibility loss. The generalizable part is not the presolve numbers but the method: measure the throughput spread before trusting a work budget, size a limit so the stage concludes before its ceiling rather than being truncated by it, check whether a cap ever binds before tuning it, and run the ordered attribution checks -- did the limit bind, did the stage starve a later one, does the failure reproduce unbounded -- before changing a limit's value.
The cost policy capped Papilo rounds at 30 and clamped the probing badge above a 2e5 cost proxy. Measuring both directly shows neither limit was buying time that was actually scarce: - mzzv11 keeps reducing past 30 rounds (1962 rows against 1576) for 6s more, while triptim1 is bit-identical at 30 and unlimited. A round cap costs reduction where it binds and buys nothing where it does not, so rounds are now uncapped and the wall ceiling bounds the cost. - triptim1 (proxy 3.5e5) removes 669 rows at badge 1024 against 545 at 32, for 17s instead of 10s against a 60s ceiling. The threshold moves to 5e5 so the clamp only applies where the badge genuinely cannot fit, as on ns1760995 which needs 100s+ to make it pay. The structural probing ceiling is demoted to a backstop. Its cost proxy predicts throughput only to within ~700x, so no scale both bounds the wall time and leaves useful coverage: the value that kept every run under 60s also truncated the large instances to 1-9% of their candidates, which cost netdiversion and roi5alpha10n8 their solutions. Stopping at a measured wall target dominates it, so max_time_on_probing becomes the real bound and the ceiling only binds where the proxy is extreme. Verified at 600s: mzzv11 3.48 -> 0.26 error, triptim1 1.63 -> 0.00, netdiversion 41.70 -> 0.00, all better than baseline. The probing change is not yet settled on the noisier instances; configs 1, 2 and 3 span it for the full run.
The reference recommended reading hit_tlim to decide whether a round cap binds. That field reports the time limit only, so it stays clear on a stage the round cap stopped early -- following the advice exonerated a cap that was in fact costing reduction. Replace it with an A/B against the cap removed, and note that a binding round cap is better deleted than tuned since the wall ceiling bounds cost directly. Also add a noise-band check as the first attribution step. Using the spread between repeats of the same build as a per-instance threshold removed 18 of 26 apparent regressions on a 240-instance run.
Two findings from the six-config sweep on 3a2ba29. Exempting an instance from the badge clamp handed Papilo an unbounded badge (ncols/2), which on wide problems is enormous: rail01 ran at badge 58763 and Papilo 12.1s -> 49.2s, tbfp-network at 36373 and 4.8s -> 47.7s, neos-5114902-kasavu at 355082 and 7.3s -> 44.6s. Eight instances lost 12-43s of their presolve budget that way. Exempt now means 1024, which still keeps the reduction that badge 1024 bought on triptim1 and mzzv11. This also corrects the previous attribution. Configs 0 and 1 moved the round cap and the badge together, so the 231-instance net of +0.02 read as "the Papilo change does nothing" when the badge was doing the damage and the rounds were doing the good -- mzzv11 is 0.07 error uncapped against 3.41 capped, and none of the eight blowups came from rounds. Rounds stay uncapped and each sweep point now moves one knob. Probing coverage drops to a quarter, which beat full coverage by 0.615 mean error over 231 instances. Probing takes its time from branch and bound, and the wins (satellites2-40 100 -> 31.6, brazil3, rail01, physiciansched3-3) outweigh the losses on 30n20b8 and netdiversion. Not addressed here: square47 runs 2250s against a 600s limit in every config while presolve costs it under 40s, exploring 4-10 nodes where the baseline managed 2 in 600s, and its error is identical at 6.25 throughout. Cheap presolve just lets it start more giant node LPs with no deadline check inside one. That belongs in the node-LP time check.
The probing cap was min(max_time_on_probing, global_timer.remaining_time()), dropping the time_limit argument that main bounds it with. That argument is the presolve share of the solve (presolve_time_ratio, a tenth by default), so without it probing was bounded only by its own 45s ceiling and a short solve could spend all of itself there: at --time-limit 10 it could take the whole 10s where it should get 1s. At 600s the ceiling was tighter than main's 60s either way, which is why the benchmarks did not show it. Measured on supportcase6, probing now gets 0.63s at a 10s limit and 5.61s at 60s, and still reaches the 45s ceiling at 600s.
The harness runs every config id once and cannot repeat one, so the three settings worth measuring are duplicated across adjacent id pairs. In opportunistic mode a duplicated pair does not repeat itself; it measures the run-to-run spread, which is what the previous sweep lacked. The effects here are worth roughly 0.6 mean error while two baseline repeats differ by 0.53, so one run per setting cannot separate signal from noise. 0, 1 the defaults 2, 3 full coverage, against the quarter that beat it by 0.615 4, 5 round cap restored, now that the badge no longer confounds it Dropped from the previous table: the no-clamp arm, which square47 already settles with a bit-identical reduced problem at badge 32 against 1024, and the 0.10 coverage and no-ceiling arms, which only matter once the default holds up.
The 45s cap on probing was a stand-in for a bound the work budget was meant to provide, so let a config remove it and size the ceiling instead. The presolve share of the solve and the global timer stay: they bound probing against the rest of the solve rather than tuning it. Three arms set the coverage target to 1, which carries no target at all, so the work limit is exactly the structural ceiling and the scale-to-time curve is readable without the fraction masking which of the two bound an instance. Two more pair the ceiling with the usual quarter coverage as default candidates. Papilo is pinned across all of them so the probing bound is the only thing moving.
Adopts the sweep's tightest arm as the default. Over 240 instances a scale of 1.5e8 stopped every probing run before the wall cap could fire, worst case 42.8s, where 4e8 and 1e9 still needed the wall on 2 and 8 instances. It also spends the least time doing it -- 582s of probing across the set against 1136s at 4e8 -- and solves that overran their own limit fell from 17 to 7. max_time_on_probing goes to 120s as a backstop for an instance the cost proxy badly mispredicts. Leaving it tight would keep it shaping the common case, which is what the work ceiling now does. Drops the coverage fraction so the ceiling is the sole work bound. A fraction cuts every instance by the same proportion regardless of cost, which both obscured attribution and penalised cheap instances; probing now runs to completion on those while the ceiling truncates by cost. Mean error is unchanged within noise (11.78 against 12.01, with 0.53 between repeats of one build), so this was decided on bounding behaviour and time.
rd-rplusc-21 was the 239-of-240 instance, dying in the concurrent barrier LP with "Requested size overflows device_uvector storage" and taking the whole MIP solve with it, since the error escaped run_mip and suppressed the result row. It was never a large allocation, it was a negative one. The instance is 125899 x 622 with four columns each touching ~119000 rows, so A*A^T is 90.5% dense at 1.44e10 nonzeros. Dense-column elimination does its job -- the barrier finds 8, and dropping 8 brings A*A^T to 1.0e9 -- but the residual system still needs 2311527529 nonzeros, clearing INT32_MAX by 7.6%. cuSPARSE reports that count as int64_t while resize_to_nnz takes i_t, so it narrowed to -1983439767 and reached RMM as ~1.8e19. That 7.6% margin is why the failure looked nondeterministic. Small differences in what presolve leaves behind put the count either side of the limit, and below it the same instance instead asks for ~28 GB and fails as an already-handled out_of_memory. Guarding the narrowing reports the real count and cause, and the barrier's handler now catches std::bad_alloc rather than only rmm::out_of_memory so any allocation failure there degrades to NUMERICAL_ISSUES, which is how the barrier already treats every other shape it cannot hold. The instance is still hard -- 0 nodes explored in 600s -- but it now terminates normally and reports a result rather than vanishing from the set. Not addressed here: the augmented-system fallback cannot fire for this shape, needing estimated_nz_AAT > 1e10 and max_row_nz > 5000 while max row length is 100 and m^2 caps the estimate at 2.9e9.
The policy menu existed to choose between competing budget rules on a benchmark; that question is settled, so the enum, the six-arm config table and the CUOPT_CONFIG_ID selector go, leaving the one rule that was adopted. Default behaviour is unchanged by construction: rounds uncapped, badge 32 or 1024 on the same n_bin * avg_col_len > 5e5 threshold, work limit 1.5e8 / (nnz + n_cand * avg_col_len), step 128, wall 120s -- the wall now set unconditionally rather than gated on the config id. The work model moves from tunable hyper-parameters to constants beside the scale they calibrate, since the ceiling is only meaningful against the costs the probing loop actually charges. Removes the settings for the policy, the step size, both work weights, the work-time scale, and cuopt_presolve_work_limit along with the coverage multiplier it drove, so the ceiling is the only work bound. presolve_max_rounds and papilo_probing_max_badgesize stay as overrides, now defaulting to -1 meaning "derive from the problem", where an explicit 0 removes the cap. The per-stage Papilo telemetry drops to DEBUG now that it is no longer being regressed offline, and papilo_presolve_features moves next to the Papilo code it feeds rather than sitting in solve.cu.
presolve_time_ratio and presolve_max_time gave Papilo and cuOpt presolve a wall budget of min(0.1 * time_limit, 60s) each. Neither was binding at the 600s limit the set is measured at: across two repeats no Papilo call hit its tlim, the slowest finishing at 46.9s of 60 (ns1760995, whose old 243-403s blowup the badge clamp already fixed), and probing's worst was 44.8s (nw04). Both settings are removed outright, along with the 120s max_time_on_probing backstop and the dead time_ratio_of_probing_cache beside it, so the badge clamp and the probing work ceiling are what shape presolve cost while remaining_time only stops it reaching past the end of the solve. The caps did bind on short solves, where the ratio rather than the 60s term set them, so that is where this changes behaviour. Removing the settings takes their C constants, registrations, proto fields 37 and 38, and test assertions with them; the registry notes those numbers as reserved since an older client still sends them there. Every other field number is unchanged, so the wire stays compatible. Also drops the probing_wall_limit field and its log column, and the deterministic-mode branch that disabled probing when the work limit was infinite, which the previous commit made unreachable. probing_work_time_scale becomes probing_work_scale, since with no wall left it is a work coefficient and nothing converts it to seconds. CUOPT_CONFIG_ID returns with two arms so one build covers the sweep: unset or 0 is the shipping ceiling, 1 raises it 25% to measure what the extra probing buys now that no wall clips the cost of a mispredicted instance.
Two repeats of CUOPT_CONFIG_ID=1 against the 0b77cd1 baseline moved the set mean from 12.34 to 12.19, inside the 0.73 spread between the arm's own repeats. The arm reached only 67 of 240 instances, since work folds in at the step-128 barrier and a budget a few steps deep cannot resolve a 25% change; on those 67 it spent 23% more probing time for a median error delta of exactly 0.00, with the -0.83 mean coming from large swings that cancel in sign. The selector goes and probing_work_limit reads probing_work_scale directly again, which restores the values the default already produced. The measurement is folded into the comment on the scale, since it settles by experiment what the proxy fit had only suggested: a larger scale is not how to buy coverage back.
akifcorduk
commented
Aug 4, 2026
| inline constexpr double probing_probe_work = 0.02; // per probed variable, host overhead | ||
| inline constexpr double probing_iter_work = 0.01; // per multi-probe propagation iteration | ||
|
|
||
| // Probing work allowed per unit of the cost proxy below; dividing by that proxy is what turns it |
Contributor
Author
There was a problem hiding this comment.
Note for reviewer: I think these commands are useful for future tuning and improvements.
akifcorduk
added
improvement
The budget rule is settled and measured, so the per-stage telemetry no longer needs to be on by default: PRESOLVE_BUDGET, PRESOLVE_PROBING, PRESOLVE_PROBING_WALL and the probing-cache start line all move to DEBUG. The comments recording how the ceiling was arrived at go with them, since what they argued is now either settled in the comment on probing_work_scale or recoverable from the history. Demoting is not free here. Builds set CUOPT_LOG_ACTIVE_LEVEL=INFO, so CUOPT_LOG_DEBUG compiles to nothing and diversity_manager's probing_t0 became unused-but-set under -Wall -Werror; it is marked maybe_unused. The equivalent locals in probing_cache.cu are left unannotated because they are genuinely used: probing_t0 is read by probing_wall's initializer, which still runs, and probing_wall compiles clean without a marker. Also compacts the ADAT narrowing comment to the two lines that explain the check and drops the note reserving proto fields 37 and 38.
📝 WalkthroughWalkthroughThe PR replaces heuristic presolve time caps with feature-derived Papilo and probing work budgets. It updates public parameters, serialization, solver integration, deterministic probing accounting, diagnostics, tests, and developer guidance. It also broadens allocation failure handling and adds sparse matrix overflow checks. ChangesPresolve budgeting
Allocation safety
Developer guidance
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip_heuristics/solve.cu (1)
563-588: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd unit tests for presolve feature extraction and budget evaluation.
The new functions
papilo_presolve_featuresandevaluate_presolve_budgetin solve.cu (lines 566–567) have no direct unit test coverage. Existing presolve tests hardcodeapply_presolve_from_op_problemparameters instead of calling the feature extraction and budget derivation path. Add tests that verify:
- Feature extraction from diverse problem structures (small, large, sparse, dense, different integer fractions)
- Budget values derived from features and configured hyper-parameters (both default and explicit)
- Deterministic mode interaction with presolve budgets (infinity time limit vs. remaining time)
- Round and badge-size limits applied to Papilo from the computed budget
As per coding guidelines, contributors must add unit tests for code changes.
🤖 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 `@cpp/src/mip_heuristics/solve.cu` around lines 563 - 588, Add unit test coverage for the functions papilo_presolve_features and evaluate_presolve_budget that were introduced in solve.cu. Create tests that verify feature extraction from problem instances with varying characteristics (small, large, sparse, dense, different integer fractions), validate that budget values are correctly derived from extracted features and hyperparameter settings (both defaults and explicit configurations), confirm that deterministic mode properly applies infinity time limits while non-deterministic mode uses remaining time, and ensure that the computed papilo_max_rounds and papilo_max_badgesize values are correctly passed to the presolver as shown in the apply_presolve_from_op_problem call.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (1)
cpp/src/mip_heuristics/presolve/multi_probe.cuh (1)
78-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the accumulator pointer private.
Line 83 adds a mutable public data member. Expose a setter for the non-owning accumulator and store the pointer privately. This keeps the required injection point public without allowing unrelated callers to replace the counter.
As per coding guidelines, “keep data members private.”
🤖 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 `@cpp/src/mip_heuristics/presolve/multi_probe.cuh` around lines 78 - 83, Make the accumulator pointer in multi_probe_t private, and add a public setter for the non-owning accumulator injection. Update existing initialization or assignment sites to use that setter, preserving the current accumulation behavior while preventing unrelated callers from directly replacing local_iter_accumulator.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 `@cpp/src/grpc/codegen/field_registry.yaml`:
- Around line 758-759: Keep field tags 37 and 38 reserved in
cpp/src/grpc/codegen/field_registry.yaml lines 758-759, then add optional int32
entries for presolve_max_rounds and papilo_probing_max_badgesize using fresh
field numbers. In cpp/tests/linear_programming/grpc/grpc_client_test.cpp lines
2495-2498, extend the gRPC client coverage with default-preservation and
explicit round-trip assertions for both settings.
In `@cpp/src/mip_heuristics/presolve/presolve_budget_policy.hpp`:
- Around line 101-139: Add unit tests covering evaluate_presolve_budget with
zero-sized features, papilo probing cost exactly equal to
papilo_badge_cost_threshold, and papilo_max_rounds and
papilo_probing_max_badgesize overrides set to -1, 0, and positive values. Assert
the resulting budgets, including default boundary behavior and explicit override
preservation.
- Around line 136-138: Update the presolve budget construction around
probing_work_limit to retain the deterministic work limit and also derive a
separate probing wall-time ceiling with margin; pass both limits to
compute_probing_cache instead of using remaining solve time as its timer limit.
Track and record whether probing stopped due to the work limit or wall ceiling,
following the stage budget guidance and existing budget/status symbols.
In `@cpp/src/mip_heuristics/presolve/probing_cache.cu`:
- Around line 903-905: Update the probing-step initialization around
step_size_hint and the empty-step construction near the step-processing loop so
a nonempty priority_indices collection never produces a zero-sized step;
validate or clamp a zero hint to at least one while preserving the
empty-candidate behavior. Add a regression unit test covering step_size_hint ==
0 with available candidates and verify that probing proceeds instead of setting
early_exit before any candidate is processed.
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 874-883: Rename the logged termination field in the
PRESOLVE_PAPILO telemetry from hit_tlim to near_tlim, and update its value label
consistently with the elapsed-time threshold. Do not present this proximity
metric as an actual time-limit termination cause; preserve the existing timing
calculation and other telemetry fields.
---
Outside diff comments:
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 563-588: Add unit test coverage for the functions
papilo_presolve_features and evaluate_presolve_budget that were introduced in
solve.cu. Create tests that verify feature extraction from problem instances
with varying characteristics (small, large, sparse, dense, different integer
fractions), validate that budget values are correctly derived from extracted
features and hyperparameter settings (both defaults and explicit
configurations), confirm that deterministic mode properly applies infinity time
limits while non-deterministic mode uses remaining time, and ensure that the
computed papilo_max_rounds and papilo_max_badgesize values are correctly passed
to the presolver as shown in the apply_presolve_from_op_problem call.
---
Nitpick comments:
In `@cpp/src/mip_heuristics/presolve/multi_probe.cuh`:
- Around line 78-83: Make the accumulator pointer in multi_probe_t private, and
add a public setter for the non-owning accumulator injection. Update existing
initialization or assignment sites to use that setter, preserving the current
accumulation behavior while preventing unrelated callers from directly replacing
local_iter_accumulator.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2b786cb-8434-454a-8a5f-91096de7f6e2
⛔ Files ignored due to path filters (3)
cpp/src/grpc/codegen/generated/cuopt_remote_data.protois excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_mip_settings_to_proto.incis excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_proto_to_mip_settings.incis excluded by!**/generated/**
📒 Files selected for processing (21)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/mip/heuristics_hyper_params.hppcpp/src/barrier/barrier.cucpp/src/barrier/sparse_matrix_kernels.cuhcpp/src/grpc/codegen/field_registry.yamlcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/diversity/diversity_config.hppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/presolve/multi_probe.cucpp/src/mip_heuristics/presolve/multi_probe.cuhcpp/src/mip_heuristics/presolve/presolve_budget_policy.hppcpp/src/mip_heuristics/presolve/probing_cache.cucpp/src/mip_heuristics/presolve/probing_cache.cuhcpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/presolve/third_party_presolve.hppcpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/tests/linear_programming/grpc/grpc_client_test.cppcpp/tests/mip/heuristics_hyper_params_test.cuskills/cuopt-developer/SKILL.mdskills/cuopt-developer/references/stage_budgets.md
💤 Files with no reviewable changes (1)
- cpp/src/mip_heuristics/diversity/diversity_config.hpp
CI Test Summary✅ All 31 test job(s) passed. |
akifcorduk
added
breaking
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cpp/src/grpc/codegen/field_registry.yaml`:
- Around line 805-812: Validate external presolve override values before
evaluate_presolve_budget() passes them to Papilo, treating omitted and -1 as the
existing unset behavior and rejecting or clamping 0 and values above the safe
maximum, including INT32_MAX. Add boundary tests covering omitted, -1, 0, the
safe maximum, and INT32_MAX, while preserving valid bounded overrides.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 65bfd6a7-9d2a-4838-b61a-277821739adf
⛔ Files ignored due to path filters (3)
cpp/src/grpc/codegen/generated/cuopt_remote_data.protois excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_mip_settings_to_proto.incis excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_proto_to_mip_settings.incis excluded by!**/generated/**
📒 Files selected for processing (2)
cpp/src/grpc/codegen/field_registry.yamlcpp/tests/linear_programming/grpc/grpc_client_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/tests/linear_programming/grpc/grpc_client_test.cpp
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
1 participant
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.
This PR introduces determinsitic iteration and work limits to papilo and cuopt presolve (mainly probing). With this PR, we remove the time limits. This might impact very short runtime runs, so we might need more iterations depending on feedback. Slight regression on average error gap, possibly due to varying probing time. However, we have more wins vs loses in terms of number of inttances. Few instances are impacted more from the probing time. All other metrics are improving. Both papilo and cuopt probing execute under 60s cap that was there previously.
On MIPLIB2017 benchmarks on H100, improvements are as follows:
main (avg of 2 runs):
Optimal: 74.5
Error gap: 12.25
MIP gap(geomean): 20.0
<0.1% error gap: 120.5
This branch(avg of 2 runs):
Optimal: 75.5
Error gap: 12.34
MIP gap(geomean): 19.67
<0.1% error gap: 122.5
closes #1068