Replace good_lp with direct highs-sys calls + add LP export by isPANN · Pull Request #790 · CodingThrust/problem-reductions

isPANN · 2026-03-27T16:04:20Z

Summary

Replace good_lp with direct calls to the HiGHS C library via highs-sys (auto-generated FFI bindings), removing two unnecessary middleman layers
Add LP-format export (ILP::to_lp_format() + pred export CLI) so users can feed problems to any external solver (Gurobi, CPLEX, GLPK, etc.)
All 3486 tests pass (3484 existing + 2 new roundtrip)

Key changes

highs_raw.rs: ~220 line safe RAII wrapper around highs-sys C API, with debug_assert on option errors and null-ptr optimization for unused solution arrays
solver.rs: CSC sparse matrix built with flat two-pass allocation (no per-column Vec) + duplicate-index dedup (HiGHS rejects duplicate row indices, which good_lp silently merged)
ilp.rs: to_lp_format() exports standard LP format
pred export: new CLI subcommand for LP-format output

Test plan

All 3486 tests pass (make check)
8 LP-format export tests (maximize, minimize, equality, binary, generals, negative coefficients, empty, multiple constraints)
2 roundtrip tests: export LP file → Highs_readModel read back → solve → verify objective value matches built-in solver (covers ILP<bool> and ILP<i32>)
good_lp fully removed from source and lockfile
pred export end-to-end test with piped JSON input

Closes #787

🤖 Generated with Claude Code

…#787) Replace `good_lp` with direct calls to the HiGHS C library via its auto-generated low-level bindings (`highs-sys`), wrapped in ~200 lines of safe Rust — removing two unnecessary middleman layers. Add LP-format export (`ILP::to_lp_format()` + `pred export` CLI) so users can also use any external solver of their choice. - Remove `good_lp` dependency and all non-HiGHS solver features - Add `src/solvers/ilp/highs_raw.rs`: thin safe RAII wrapper around `highs-sys` C API (Highs_create/passMip/run/getSolution/destroy) - Rewrite `solver.rs` to build CSC sparse matrix directly with flat two-pass allocation and duplicate-index dedup - Add `ILP::to_lp_format()` for LP-format export (CPLEX LP format) - Add `pred export` CLI subcommand - 8 new LP-format export tests Public API unchanged, all 3484 existing tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…e consistent solutions Export ILP to LP file → read back with HiGHS via Highs_readModel → solve → compare objective value against built-in solver. Covers both bool and i32 variables. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

codecov · 2026-03-27T16:16:26Z

Codecov Report

❌ Patch coverage is 94.11765% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.82%. Comparing base (f29cfb1) to head (ae5f8ac).

Files with missing lines	Patch %	Lines
src/solvers/ilp/highs_raw.rs	84.72%	22 Missing ⚠️
src/solvers/ilp/solver.rs	95.49%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #790      +/-   ##
==========================================
- Coverage   97.85%   97.82%   -0.03%     
==========================================
  Files         625      626       +1     
  Lines       68311    68725     +414     
==========================================
+ Hits        66846    67232     +386     
- Misses       1465     1493      +28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR replaces the good_lp-based ILP solving path with a direct highs-sys (HiGHS C API) integration, and adds LP (CPLEX LP) export support to enable interoperability with external solvers via the CLI.

Changes:

Implement a safe(ish) RAII wrapper around highs-sys and migrate the ILP solver to call the HiGHS C API directly.
Build the constraint matrix in CSC format (two-pass allocation) and add per-column duplicate-index handling for HiGHS compatibility.
Add ILP::to_lp_format() plus a new pred export subcommand and accompanying tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
`src/solvers/ilp/highs_raw.rs`	Adds a minimal wrapper around the HiGHS C API (model lifecycle, options, solve, solution extraction).
`src/solvers/ilp/solver.rs`	Replaces `good_lp` modeling with direct `Highs_passMip` calls and CSC matrix construction + dedup.
`src/solvers/ilp/mod.rs`	Updates module docs and exposes the new `highs_raw` module internally.
`src/models/algebraic/ilp.rs`	Adds `to_lp_format()` for exporting ILPs in LP format.
`src/unit_tests/models/algebraic/ilp.rs`	Adds LP-format export tests and HiGHS roundtrip tests.
`problemreductions-cli/src/commands/export.rs`	Introduces `pred export` command implementation for LP output.
`problemreductions-cli/src/commands/mod.rs`	Registers the new `export` command module.
`problemreductions-cli/src/dispatch.rs`	Adds type-erased `export_lp()` support for loaded problems (ILP-only).
`problemreductions-cli/src/main.rs`	Wires the new `Export` command into dispatch and auto-JSON behavior.
`problemreductions-cli/src/cli.rs`	Adds clap wiring + help text for `pred export`; updates solve help to reflect HiGHS-only backend.
`Cargo.toml`	Removes `good_lp` feature wiring; adds `highs-sys` dependency and `ilp-highs` feature mapping.
`problemreductions-cli/Cargo.toml`	Removes non-HiGHS ILP backend feature flags; keeps `highs` feature only.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-27T16:17:49Z

src/solvers/ilp/solver.rs

+                .map(|(&i, &v)| (i, v))
+                .collect();
+            pairs.sort_by_key(|&(row, _)| row);
+            pairs.dedup_by(|b, a| {


Vec::dedup_by keeps the first element and removes the second when the predicate returns true. This closure accumulates into the second argument (a.1 += b.1), so the summed coefficient is discarded and duplicates are effectively dropped instead of merged. Swap the accumulation to update the kept element (or rename args to avoid confusion).

Suggested change

pairs.dedup_by(|b, a| {

pairs.dedup_by(|a, b| {

Copilot · 2026-03-27T16:17:50Z

src/solvers/ilp/solver.rs

+            // Sort entries by row index using paired key
+            let mut pairs: Vec<(i32, f64)> = a_index[start..end]
                .iter()
-                .map(|&(var_idx, coef)| coef * vars[var_idx])
-                .sum();
+                .zip(&a_value[start..end])
+                .map(|(&i, &v)| (i, v))
+                .collect();
+            pairs.sort_by_key(|&(row, _)| row);


The duplicate-merge step allocates a fresh Vec<(i32, f64)> for every column. If this code is performance-critical (large n), consider sorting/deduping in-place on the a_index/a_value slices (or reusing a scratch buffer) to preserve the “no per-column allocation” benefit mentioned above.

Copilot · 2026-03-27T16:17:50Z

src/solvers/ilp/highs_raw.rs

+        let nnz = a_value.len();
+        let status = unsafe {
+            highs_sys::Highs_passMip(
+                self.ptr,
+                num_vars as c_int,
+                num_constraints as c_int,
+                nnz as c_int,


Highs_passMip receives c_int sizes/nnz, but usize as c_int will truncate on 64-bit platforms if the problem is large. This is a safe wrapper, so these casts should be checked (e.g., try_from) and return HiGHSStatus::Error (or panic) on overflow to avoid UB in the FFI call.

Copilot · 2026-03-27T16:17:50Z

src/solvers/ilp/highs_raw.rs

+    pub(crate) fn new() -> Self {
+        let ptr = unsafe { highs_sys::Highs_create() };
+        let mut model = Self { ptr };
+        model.set_bool_option("output_flag", false);
+        model.set_bool_option("log_to_console", false);
+        model


Highs_create() can theoretically return a null pointer (e.g., allocation failure). Since this wrapper is meant to be safe, add a null check and fail fast (panic/Result) before calling any other HiGHS API functions on ptr.

Copilot · 2026-03-27T16:17:51Z

problemreductions-cli/src/main.rs

+            | Commands::Solve(_)
+            | Commands::Evaluate(_)
+            | Commands::Inspect(_)
+            | Commands::Export(_)


Including Commands::Export(_) in the “auto-output JSON when piped” list means pred export ... > problem.lp will emit JSON (not raw LP), which undermines the primary use-case of feeding the output to external solvers via piping/redirection. Consider excluding Export from auto_json, or making pred export always emit raw LP unless --json is explicitly requested.

Suggested change

| Commands::Export(_)

Copilot · 2026-03-27T16:17:51Z

problemreductions-cli/src/cli.rs

+Examples:
+  pred export ilp.json                           # export ILP problem in LP format
+  pred export ilp.json -o problem.lp             # save to file
+  pred create ILP --example | pred export -      # pipe from create
+
+Input: a problem JSON from `pred create`. Currently only ILP problems support direct export.
+For other problems, reduce to ILP first with `pred reduce ... --to ILP`, then export.


The help example suggests -o problem.lp writes an LP file, but OutputConfig currently writes JSON when -o is set. Either update the help text to reflect that -o saves a JSON wrapper (with LP in a field), or change pred export to write the raw LP text to the -o path.

Copilot · 2026-03-27T16:17:51Z

problemreductions-cli/src/commands/export.rs

+    let json = serde_json::json!({
+        "format": format,
+        "problem": pj.problem_type,
+        "content": &lp_string,
+    });
+    out.emit_with_default_name("", &lp_string, &json)
+}


out.emit_with_default_name writes JSON when -o is provided, so pred export ... -o problem.lp will produce a JSON file rather than a solver-readable LP file. If the intent is to support exporting a real LP file, this command should write lp_string directly to the output path when out.output is set (and optionally provide a separate --json mode for the wrapper).

Copilot · 2026-03-27T16:17:52Z

Cargo.toml

@@ -16,12 +16,7 @@ default = ["ilp-highs"]
 example-db = []
 ilp = ["ilp-highs"]  # backward compat shorthand
 ilp-solver = []      # marker: enables ILP solver code


Feature gating looks inconsistent: ilp-solver enables compiling the ILP solver module, but it no longer pulls in any backend dependency. Since the solver implementation now unconditionally depends on highs-sys, enabling --features ilp-solver without ilp-highs will fail to compile. Consider either (a) making ilp-solver depend on dep:highs-sys, or (b) gating the ILP solver module on ilp-highs instead of ilp-solver.

Suggested change

ilp-solver = [] # marker: enables ILP solver code

ilp-solver = ["dep:highs-sys"] # marker: enables ILP solver code

Copilot · 2026-03-27T16:17:52Z

src/solvers/ilp/mod.rs

+//! This module provides an ILP solver using the HiGHS solver via direct `highs-sys` calls.
+//! It is only available when the `ilp-highs` feature is enabled.


This module-level doc says the solver is only available with ilp-highs, but the module is compiled under the ilp-solver feature (see src/solvers/mod.rs). Either the doc or the cfg gating should be updated so feature behavior is unambiguous.

Copilot · 2026-03-27T16:17:52Z

src/unit_tests/models/algebraic/ilp.rs

+    let tmp_path = std::env::temp_dir().join("test_roundtrip_bool.lp");
+    std::fs::write(&tmp_path, &lp_text).expect("write LP file");
+
+    let mut model = HiGHSModel::new();
+    model.set_int_option("random_seed", 0);
+    model.set_string_option("presolve", "off");
+    let file_sol = model
+        .read_and_solve(tmp_path.to_str().unwrap())
+        .expect("HiGHS should solve the LP file");
+    std::fs::remove_file(&tmp_path).ok();


These roundtrip tests write to fixed filenames in the shared temp directory. If tests run concurrently across processes (or a previous run leaves a stale file), this can cause flaky failures. Prefer a unique per-test filename (timestamp/random) or a tempdir helper.

isPANN and others added 2 commits March 28, 2026 00:04

isPANN requested a review from Copilot March 27, 2026 16:10

Copilot started reviewing on behalf of isPANN March 27, 2026 16:11 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

isPANN closed this Mar 28, 2026

GiggleLiu deleted the 787-replace-good-lp-with-highs-sys branch April 12, 2026 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace good_lp with direct highs-sys calls + add LP export#790

Replace good_lp with direct highs-sys calls + add LP export#790
isPANN wants to merge 2 commits intomainfrom
787-replace-good-lp-with-highs-sys

isPANN commented Mar 27, 2026 •

edited

Loading

Uh oh!

codecov bot commented Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	ilp-solver = [] # marker: enables ILP solver code
	ilp-solver = ["dep:highs-sys"] # marker: enables ILP solver code

		//! This module provides an ILP solver using the HiGHS solver via direct `highs-sys` calls.
		//! It is only available when the `ilp-highs` feature is enabled.

Conversation

isPANN commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key changes

Test plan

Uh oh!

codecov bot commented Mar 27, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

isPANN commented Mar 27, 2026 •

edited

Loading