Add stable QUBO C ABI wrapper - #10
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Review found two blocking issues. I attempted to submit this as REQUEST_CHANGES, but GitHub rejects request-changes reviews from the PR author. The inline comments below should be treated as blocking before merge.
| } else { | ||
| mi.reset(new MaxCutInstance(qi)); | ||
| std::string hyperheuristic_choice; | ||
| maxcut_heuristic.reset(new MaxCutHyperheuristic( |
There was a problem hiding this comment.
Blocking: The hyperheuristic path depends on upstream MaxCutHyperheuristic finding hhdata/*.rf relative to the process CWD, but this wrapper neither provides nor validates that data path. Running the new example from the package checkout instead of the upstream MQLib checkout returns MQLIB_STATUS_OK with selected_heuristic == "HH_", objective 0, and the all-zero solution, so shared-library consumers get a successful no-op whenever their CWD lacks hhdata. This breaks the issue requirement that the C ABI can run the hyperheuristic path. Please make the model directory explicit or robust for the shared library, and return a non-OK status if no hyperheuristic model is selected; add a regression test that runs from a CWD without hhdata.
There was a problem hiding this comment.
Addressed in f175e52. The wrapper now selects hyperheuristic models from MQLibCQUBOInput.hyperheuristic_data_dir, returns MQLIB_STATUS_HYPERHEURISTIC_DATA_NOT_FOUND when no model files are found, and test/c_api_smoke.c covers running from a CWD without hhdata.
There was a problem hiding this comment.
There was a problem hiding this comment.
|
|
||
| upstream_dir = get(ENV, "MQLIB_UPSTREAM_DIR", "") | ||
| cxx = first_available_tool(["c++", "g++", "clang++"]) | ||
| if isempty(upstream_dir) || !isdir(joinpath(upstream_dir, "include")) |
There was a problem hiding this comment.
Blocking: In normal CI MQLIB_UPSTREAM_DIR is not set, so this branch skips the only check that compiles c_api/src/mqlib_c_api.cpp; it also never links or runs c_api/examples/solve_qubo.c. The changed behavior is the native ABI implementation, but future C++ breakage, link errors, and runtime bugs still pass the default suite. Please add a CI-covered native smoke test/build target that compiles the wrapper against upstream MQLib sources and runs at least the explicit heuristic and hyperheuristic example, or otherwise fail when the implementation cannot be checked.
There was a problem hiding this comment.
Addressed in f175e52. Added test/native_c_api_smoke.sh plus test/c_api_smoke.c, and wired the smoke target into Linux CI so the wrapper is compiled, linked, and run for explicit heuristic, hyperheuristic, and missing-hhdata cases.
|
Pushed follow-up commit Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks / follow-up:
|
| return MQLIB_STATUS_INTERNAL_ERROR; | ||
| } | ||
|
|
||
| result->runtime_seconds = heuristic->Runtime(); |
There was a problem hiding this comment.
Blocking: This reports only the selected heuristic runtime. In the hyperheuristic path, graph metric computation and random-forest model selection happen before the selected heuristic is constructed, so heuristic->Runtime() omits selection overhead. That diverges from the upstream hyperheuristic wrapper/CLI semantics and underreports the C ABI runtime_seconds for HH_* runs. Please time the full hyperheuristic path, or explicitly document that the ABI reports selected-heuristic runtime only, and add a focused test for the intended semantics.
There was a problem hiding this comment.
Addressed in 8c1249e: hyperheuristic runtime_seconds now measures model selection plus the selected heuristic run, and test/c_api_smoke.c checks that the reported runtime does not omit elapsed hyperheuristic work.
| UPSTREAM_DIR="$MQLIB_UPSTREAM_DIR" | ||
| else | ||
| UPSTREAM_DIR="$BUILD_DIR/MQLib-upstream" | ||
| git clone --depth 1 https://github.com/MQLib/MQLib.git "$UPSTREAM_DIR" |
There was a problem hiding this comment.
Nonblocking: This clones upstream MQLib at the current default branch. That keeps smoke coverage fresh, but it makes CI nondeterministic: a future upstream change can break this PR without any local change. Consider adding an optional MQLIB_UPSTREAM_REF and defaulting CI to a known vetted commit or tag, while retaining HEAD as an opt-in local mode.
There was a problem hiding this comment.
Addressed in 8c1249e: the smoke script now defaults to upstream MQLib commit 585496274af5abb0849d0d47e135496b4688680b, CI passes that ref explicitly, and MQLIB_UPSTREAM_REF=HEAD remains available for opt-in current-upstream testing.
There was a problem hiding this comment.
|
Pushed follow-up commit Main changes:
Tests run:
Tests not passing locally:
Comments intentionally not addressed:
Remaining risks / follow-up:
|
| maxcut_heuristic.reset(factory.RunMaxCutHeuristic( | ||
| choice.code, | ||
| *mi, | ||
| input->runtime_limit_seconds, |
There was a problem hiding this comment.
Blocking: The selected heuristic still receives the full original runtime limit after hyperheuristic model selection has already consumed time. Upstream MaxCutHyperheuristic starts its timer before metrics/model selection and routes the selected heuristic through callbacks, so the runtime limit applies to the whole hyperheuristic run. Here result->runtime_seconds includes selection elapsed, but this branch and the QUBO branch below can run for an additional full budget. In a local targeted C ABI harness with runtime_limit_seconds = 0.75, the call returned runtime=1.273744. Please compute the remaining budget after select_hyperheuristic and pass that to the selected heuristic, and add a native smoke/contract test with a moderate hyperheuristic limit that would fail if total runtime can exceed the requested budget by the selection time. If history buffers are populated for the hyperheuristic path, also keep their times on the same total-run timeline.
There was a problem hiding this comment.
Addressed in a446018: after model selection, the wrapper computes the remaining runtime budget and passes that to the selected heuristic; copied hyperheuristic history times are shifted onto the total-run timeline. test/c_api_smoke.c now includes a 0.75-second hyperheuristic budget regression case.
There was a problem hiding this comment.
Follow-up in aeb2e5c: the budget regression still checks total-budget behavior, but now uses 1.50 seconds so slower CI runners have enough post-selection solve time to produce improvement history.
bernalde
left a comment
There was a problem hiding this comment.
Review found one blocking correctness issue in the hyperheuristic runtime-budget semantics. I attempted to submit this as REQUEST_CHANGES, but GitHub rejects request-changes reviews from the PR author, so the inline comment should be treated as blocking before merge.
Blocking issues:
- The C ABI hyperheuristic path reports total runtime but still gives the selected heuristic the full original runtime limit after selection has already consumed time.
Nonblocking issues:
- None.
Questions:
- None.
Tests run:
bash test/native_c_api_smoke.sh- passed against pinned upstream MQLib5854962.julia +1.12 --project=. test/runtests.jl- passed: QUBODrivers 124/124, C ABI contract 9/9.MQLIB_UPSTREAM_DIR=/tmp/MQLib-upstream julia +1.12 --project=. test/runtests.jl- passed: QUBODrivers 124/124, C ABI contract 10/10.git diff --check origin/master...HEAD- passed.- Temporary targeted native harness with
runtime_limit_seconds = 0.75- reproduced the budget issue: returnedruntime=1.273744. gh pr checks 10- Ubuntu and Windows checks passed.
Not passing locally:
julia +1.12 --project=. -e 'import Pkg; Pkg.test()'fails before tests because localQUBOToolsis fixed at0.12.0whileMQLibcompat restricts it to0.10.1 - 0.11; this is the existing resolver issue.
I would not merge this until the blocking issue above is addressed.
|
Pushed follow-up commit Main changes:
Tests run:
Tests not passing locally:
Comments intentionally not addressed:
Remaining risks / follow-up:
|
|
Pushed follow-up commit Main changes:
Tests run:
Comments intentionally not addressed:
Remaining risks / follow-up:
|
bernalde
left a comment
There was a problem hiding this comment.
Review outcome: merge-ready. I attempted to submit APPROVE, but GitHub rejected approval from the PR author account.
Blocking issues: None.
Nonblocking issues: None.
Questions: None.
Tests run and outcomes:
- git diff --check origin/master...HEAD: passed.
- bash test/native_c_api_smoke.sh: passed against pinned upstream MQLib 5854962 after rerunning with network access; initial sandboxed attempt failed only because github.com was not resolvable.
- julia +1.12 --project=. test/runtests.jl: passed; QUBODrivers 124/124, C ABI contract 9/9.
- MQLIB_UPSTREAM_DIR=/tmp/MQLib-upstream julia +1.12 --project=. test/runtests.jl: passed; QUBODrivers 124/124, C ABI contract 10/10.
- MQLIB_UPSTREAM_REF=HEAD bash test/native_c_api_smoke.sh: passed; current upstream HEAD resolved to 5854962.
- make bin/MQLib in /tmp/MQLib-upstream: passed; upstream warnings only.
- julia +1.12 --project=. -e 'import Pkg; Pkg.test()': did not run tests because the local environment has QUBOTools fixed at 0.12.0 while MQLib compat allows 0.10.1 - 0.11.
- gh pr checks 10: Ubuntu and Windows CI passed.
Merge assessment: I found no blocking issues and no actionable inline comments. The PR is merge-ready as-is. The remaining Linux-only native ABI execution coverage is acceptable here because Windows shared-library packaging is the MQLib_jll follow-up.
Summary
Tests run
cc -std=c99 -Ic_api/include -fsyntax-only c_api/examples/solve_qubo.c- passed.c++ -std=c++11 -Ic_api/include -I/tmp/MQLib-upstream/include -fsyntax-only c_api/src/mqlib_c_api.cpp- passed.c++ -std=c++11 -fPIC -Ic_api/include -I/tmp/MQLib-upstream/include $(find /tmp/MQLib-upstream/src -name '*.cpp' ! -name main.cpp) c_api/src/mqlib_c_api.cpp -shared -o /tmp/mqlib-c-api-build/libmqlib_c_api.so -lm- passed.cc -std=c99 -Ic_api/include c_api/examples/solve_qubo.c -L/tmp/mqlib-c-api-build -lmqlib_c_api -Wl,-rpath,/tmp/mqlib-c-api-build -o /tmp/mqlib-c-api-build/solve_qubo- passed./tmp/mqlib-c-api-build/solve_qubo- passed:ALKHAMIS1998 objective 6 solution 1 0 1 runtime 0.010025 secondsHH_BEASLEY1998TS objective 0 solution 0 0 0 runtime 0.507517 secondsMQLIB_UPSTREAM_DIR=/tmp/MQLib-upstream julia +1.12 --project=. test/runtests.jl- passed: QUBODrivers 124/124, C ABI contract 9/9.julia +1.12 --project=. test/runtests.jl- passed: QUBODrivers 124/124, C ABI contract 8/8; upstream source syntax check skipped becauseMQLIB_UPSTREAM_DIRwas not set.Notes
MQLIB_UPSTREAM_DIR=/tmp/MQLib-upstream julia +1.12 --project=. -e 'import Pkg; Pkg.test()'did not execute tests locally because the current manifest fixes localQUBOToolsat 0.12.0 whileProject.tomlrestrictsQUBOToolsto0.10.1, 0.11.Closes #9