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 support for CPLEX through the bindings to the C API #43

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

mbiggio
Copy link

@mbiggio mbiggio commented Mar 25, 2024

I added support for the CPLEX solver through the bindings to the C API, which are provided by the cplex-rs crate.

The raw bindings are built at compile time by the cplex-rs-sys crate, checking the CPLEX_PATH env variable to determine the CPLEX installation path, or defaulting to a standard installation directory. cplex-rs provides safe bindings on top of the raw ones.

Calling CPLEX as a library allows to avoid the overhead of going through the IO and parsing for .lp and .sol files generation, and to customize the solver behavior by setting parameters in the Environment object.

I'm not sure however if it is possible to test this feature in the CI, due to the CPLEX proprietary licence.
Would you be interested in considering this pull request? If yes, do you have suggestions on how to integrate it so that the CI is green?

Thanks!

@lovasoa
Copy link
Collaborator

lovasoa commented Mar 25, 2024

would it be possible to test it on CI using the community edition ?

@mbiggio
Copy link
Author

mbiggio commented Mar 25, 2024

would it be possible to test it on CI using the community edition ?

Sure, I can give it a try

@lovasoa lovasoa marked this pull request as draft March 26, 2024 10:47
@lovasoa
Copy link
Collaborator

lovasoa commented Mar 27, 2024

@mbiggio : it looks like you have a syntax error in your yaml

@mbiggio mbiggio force-pushed the wip/mbiggio/cplex-bindings branch 2 times, most recently from 906f79f to 78d2907 Compare March 27, 2024 11:28
@mbiggio
Copy link
Author

mbiggio commented Mar 27, 2024

@mbiggio : it looks like you have a syntax error in your yaml

@lovasoa the yml should be fixed now. The approach I took is to run the cplex tests in a specific job, running on a docker image with CPLEX CE installed, hosted in a private docker repo.

I set up the variable docker_hub_private_repo to identify the docker repo, and the secrets necessary to log in and download the image. If no variable docker_hub_private_repo is present, the cplex tests are skipped.

I took the same approach in the cplex-rs repo, and also added a README.md there on how to setup the docker image with your own CPLEX CE installation.

I did that because in order to get the CPLEX CE installer, an account is needed on the IBM cplex website. Therefore I couldn't just download the installer from the CI.

Since I'm not distributing my own copy of the CPLEX CE, but just using in my CI pipeline, I think this approach should be fine in terms of licensing. However, I could not find the CPLEX CE license to confirm this.

Let me know what you think of this approach :)

@lovasoa
Copy link
Collaborator

lovasoa commented Mar 27, 2024

I'm not a big fan on depending on a private docker repo. This means the CI could break without anything we could do.

Could we have a IBM_LOGIN and IBM_PASSWORD secrets in the repo, and use these directly, keeping everything open-source ?

@mbiggio
Copy link
Author

mbiggio commented Mar 27, 2024

I'm not a big fan on depending on a private docker repo. This means the CI could break without anything we could do.

Could we have a IBM_LOGIN and IBM_PASSWORD secrets in the repo, and use these directly, keeping everything open-source ?

You mean downloading directly the installer from IBM, by logging in with the secret credentials? I agree that would probably be the best solution, but I'm not sure IBM provides a way to programmatically download the installer, at least as far as I could see from their documentation/website

@lovasoa
Copy link
Collaborator

lovasoa commented Mar 27, 2024

Actually, it looks like you don't even need to log in to download the files: https://ibm.ent.box.com/s/9gy0r3eue56ovwrelndqfvtjnxcu5amg

@mbiggio
Copy link
Author

mbiggio commented Mar 27, 2024

Actually, it looks like you don't even need to log in to download the files: https://ibm.ent.box.com/s/9gy0r3eue56ovwrelndqfvtjnxcu5amg

That would be awesome, but in the details section I see:

This shared link will be disabled on Jul 31, 2024.

@lovasoa
Copy link
Collaborator

lovasoa commented Mar 27, 2024

Ok, I just uploaded it, I'll take the responsibility if anything happens: https://github.com/rust-or/good_lp/releases/download/cplex/cplex.bin

@mbiggio
Copy link
Author

mbiggio commented Mar 27, 2024

Ok, I just uploaded it, I'll take the responsibility if anything happens: https://github.com/rust-or/good_lp/releases/download/cplex/cplex.bin

Ok, thanks, I will update the PR tomorrow

Add CPLEX tests to the CI. Note that 'lpsolve' and 'cplex-rs' features
are incompatible, as both imply linking statically to blas/lapack
libraries and therefore lead to problems with duplicate symbols
definition at compile time. That's why a compile-time check was added to
ensure these two features are not present at the same time
@mbiggio
Copy link
Author

mbiggio commented Mar 28, 2024

@lovasoa : I added the installation of CPLEX from the link you provided in the last commit.

There is however one further fix I needed to do.
The cplex-rs and lpsolve fetures are going to be incompatible, since both crates link statically to C-libraries defining some common symbols, and therefore generate some linker errors:

  = note: /usr/bin/ld: /opt/ibm/ILOG/CPLEX_Studio_Community2211/cplex/lib/x86-64_linux/static_pic/libcplex.a(_daxpy_lp64.o): in function `daxpy':
          _daxpy.c:(.text+0x0): multiple definition of `daxpy'; /home/runner/work/good_lp/good_lp/target/debug/deps/liblpsolve_sys-319d5ea6682c179f.rlib(myblas.o):/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lpsolve-sys-5.5.0/lp_solve_5.5/shared/myblas.c:169: first defined here
          collect2: error: ld returned 1 exit status

In case the two features are simultaneously activated, I'am now generating a compile-time error message instead of a linker error, which is probably a little nicer, but this anyway means that compilation will --all-features will not work anymore.

There is no other solution I could foresee, lemme know whether this is acceptable or not

@lovasoa
Copy link
Collaborator

lovasoa commented Mar 30, 2024

I think that's okay if it's well documented. An explicit note about --all-features in the readme, maybe...

@lovasoa
Copy link
Collaborator

lovasoa commented Mar 30, 2024

We need to add information about cplex to the readme together with other solvers too.

@lovasoa lovasoa marked this pull request as ready for review April 2, 2024 08:18
@lovasoa
Copy link
Collaborator

lovasoa commented Apr 2, 2024

Great ! Thank you @mbiggio !

@mbiggio
Copy link
Author

mbiggio commented Apr 2, 2024

Great ! Thank you @mbiggio !

You are welcome!

@lovasoa lovasoa merged commit f40b7b7 into rust-or:main Apr 2, 2024
1 check passed
@lovasoa
Copy link
Collaborator

lovasoa commented Apr 5, 2024

@mbiggio : looking at the code after the fact, I have a suggestion.

You may want to create a CplexSolver(CplexEnvironment) struct, in order to be able to use a custom environment and still use the nice default maximize(objective).using(CplexSolver(my_env)) syntax.

You can have a look at how it's done in the LpSolvers solver, which also takes a configuration option: https://github.com/rust-or/good_lp/blob/main/src/solvers/lp_solvers.rs#L21-L23

@mbiggio
Copy link
Author

mbiggio commented Apr 5, 2024

@mbiggio : looking at the code after the fact, I have a suggestion.

You may want to create a CplexSolver(CplexEnvironment) struct, in order to be able to use a custom environment and still use the nice default maximize(objective).using(CplexSolver(my_env)) syntax.

You can have a look at how it's done in the LpSolvers solver, which also takes a configuration option: https://github.com/rust-or/good_lp/blob/main/src/solvers/lp_solvers.rs#L21-L23

@lovasoa yes, makes sense. Will do the change, thanks

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.

2 participants