search

coin-or / pulp

A python Linear Programming API
http://coin-or.github.io/pulp/
Other
2.04k stars 381 forks source link

Add test to expose breaking change #678

Closed aphi closed 11 months ago

aphi commented 1 year ago

There's a breaking change in master for Highs API, but I can't demo it on this repo because all Highs tests are being skipped (from resolving the numpy issue). When adding back the Highs tests (e.g. per this PR), I can demonstrate the below issue.

@siwy @pchtsp From this PR (https://github.com/coin-or/pulp/pull/606), the introduction of setLogCallback seems to be a breaking change for any model using the Highs API solver with msg=True. Running a simple model locally

File "/Users/aphi/repo/pulp/pulp/apis/highs_api.py", line 301, in createAndConfigureSolver
    lp.solverModel.setLogCallback(*callbackTuple)
AttributeError: 'Highs' object has no attribute 'setLogCallback'

Within the Highs source code, I don't think setLogCallback is exposed to Python since it's in Highs.cpp but not highs_bindings.cpp for highspy, so won't be accessible without that modified bindings file.

The CI tests didn't pick it up since this code path requires self.msg to be True, and it's set as always False in BaseSolverTest (line 53 of test_pulp.py).

This PR adds a simple test with msg=True to demonstrate the failing test, and to improve the test coverage.

siwy commented 1 year ago

Thanks for the report and the test! I am pretty sure that this used to work (we're running with msg=True, but also using an older version of highspy=1.5.0.dev0)

setLogCallback used to be part of the python bindings, see: https://github.com/ERGO-Code/HiGHS/commit/d33e8edc7524d872b0db991a043603a74f0e206a

As far as I can tell it was removed in later commits, but the history is a little weird and I can't tell which one it was (and whether this was an intentional change or not)

aphi commented 1 year ago

Ah that makes sense that it used to be part of the bindings in a previous highspy version. I was wondering how else this could've occurred unless you were using a locally amended version. Maybe you can suggest they re-add it.

When highspy tests are reinstated, I'm pretty confident this test will fail since it will use highspy 1.5.3 by default. I got it to fail and expose the issue on my fork previously e.g. https://github.com/aphi/pulp/actions/runs/5795284222/job/15706542792#step:7:746 (but to raise this PR I had to rebase it onto the version that skips highs tests).

If you locally update to highspy 1.5.3 I expect you'll experience the issue locally.

siwy commented 1 year ago

Yep - you're right. The binding got deleted here: https://github.com/ERGO-Code/HiGHS/pull/1183/files

I can file an issue against highs to ask for guidance on how to address this

siwy commented 1 year ago

issue: https://github.com/ERGO-Code/HiGHS/issues/1379

siwy commented 1 year ago

This should be fixed in an upcoming version of HiGHS: https://github.com/ERGO-Code/HiGHS/issues/1379#issuecomment-1669670342