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

Tests fail on Mac with M1 chip #447

Closed
abelsiqueira opened this issue Feb 8, 2024 · 6 comments · Fixed by #448
Closed

Tests fail on Mac with M1 chip #447

abelsiqueira opened this issue Feb 8, 2024 · 6 comments · Fixed by #448
Assignees
Labels
Type: bug Something doesn't work correctly Zone: setup/admin Technical stuff that makes it work Zone: testing Related to improving or fixing the tests

Comments

@abelsiqueira
Copy link
Member

Description

Due to Cbc not supporting it: jump-dev/Cbc.jl#193
I recommend we just drop Cbc of the tests. An alternative would be to check conditionally remove it based on Sys.isapple().

Reproduction steps

Just run the tests on a Mac with M1 chip.

Logs

No response

OS

Mac

@abelsiqueira abelsiqueira added the Type: bug Something doesn't work correctly label Feb 8, 2024
@clizbe clizbe added Zone: testing Related to improving or fixing the tests Zone: setup/admin Technical stuff that makes it work labels Feb 9, 2024
@clizbe
Copy link
Member

clizbe commented Feb 9, 2024

Having a specific conditional for this edge case seems messy.

I think as long as we're testing more than one it's fine to remove it.
For now: HIGHS, GLPK
Later add: Xpress

If users run into the problem later hopefully remember this is why.

@clizbe
Copy link
Member

clizbe commented Feb 9, 2024

Maybe we could add a note to the user wherever they choose the solver? 🤔

@abelsiqueira
Copy link
Member Author

This would be just for the test. The code itself remains the same. I would not touch anything on the code or documentation side about Cbc not being supported, since they could fix it eventually, and it creates precedent for informing the user about solver status. I think leaving HiGHS as default is enough, and eventually we can mention that we also use Xpress, if applicable.

Apart from that, I agree with removing it, rather than creating the conditional.

@abelsiqueira
Copy link
Member Author

Well, I spoke too soon. The Cbc case is "necessary" for coverage. To be clearer, we have a case for compute_dual_variables that fails in a specific way for Cbc.
So removing Cbc requires either creating a fake test only to make coverage happy, which is not a good practice. So we either

  • don't remove Cbc and use the conditional instead; or
  • don't implement the check and risk the error appearing in a real situation; or
  • stop having 100% coverage.

I recommend the first.

@clizbe
Copy link
Member

clizbe commented Feb 9, 2024

Ah dang. Weird that there's a test that fails specifically in a way for Cbc...
But I guess that means we're supporting Cbc except for Mac M1, which is good. (Right?)

@abelsiqueira
Copy link
Member Author

Well, Cbc is not in a great place, so it does have these corner cases. Luckily, HiGHS is becoming the de-facto open source MIP solver, so most people won't get into this random failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug Something doesn't work correctly Zone: setup/admin Technical stuff that makes it work Zone: testing Related to improving or fixing the tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants