Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test to expose breaking change #678

Merged
merged 2 commits into from
Sep 24, 2023
Merged

Add test to expose breaking change #678

merged 2 commits into from
Sep 24, 2023

Conversation

aphi
Copy link
Contributor

@aphi aphi commented Aug 8, 2023

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 (#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
Copy link
Contributor

siwy commented Aug 8, 2023

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: ERGO-Code/HiGHS@d33e8ed

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
Copy link
Contributor Author

aphi commented Aug 8, 2023

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
Copy link
Contributor

siwy commented Aug 8, 2023

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
Copy link
Contributor

siwy commented Aug 8, 2023

issue: ERGO-Code/HiGHS#1379

@siwy
Copy link
Contributor

siwy commented Aug 8, 2023

This should be fixed in an upcoming version of HiGHS: ERGO-Code/HiGHS#1379 (comment)

@pchtsp pchtsp merged commit 25f5a59 into coin-or:master Sep 24, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants