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

Use Xpress_jll for binaries #220

Merged
merged 9 commits into from
Apr 2, 2024
Merged

Use Xpress_jll for binaries #220

merged 9 commits into from
Apr 2, 2024

Conversation

odow
Copy link
Member

@odow odow commented Mar 1, 2024

Alternative to #218

  • Wait for FICO to okay

If we like this, and once the kinks are sorted, I'll move https://github.com/odow/Xpress_jll.jl to JuMP.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 67.91%. Comparing base (82fe624) to head (0c7664b).

Files Patch % Lines
src/Xpress.jl 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
- Coverage   68.13%   67.91%   -0.22%     
==========================================
  Files           6        6              
  Lines        3295     3301       +6     
==========================================
- Hits         2245     2242       -3     
- Misses       1050     1059       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odow
Copy link
Member Author

odow commented Mar 4, 2024

Making progress with Gurobi too: jump-dev/Gurobi.jl#545

@odow odow changed the title Use Xpress_jll for binaries DNMY: use Xpress_jll for binaries Mar 7, 2024
@odow
Copy link
Member Author

odow commented Mar 7, 2024

I'm talking to FICO privately about this.

@odow
Copy link
Member Author

odow commented Mar 11, 2024

So I wonder about

import Xpress_jll
ENV["XPRESS_JL_LIBRARY"] = Xpress_jll.libxprs
using Xpress

Then we don't automatically install the binaries for people who already have one, and we can clearly document the license conditions that come with Xpress_jll.

@odow
Copy link
Member Author

odow commented Mar 18, 2024

Xpress are okay with this.

image

I'll register Xpress_jll, and then we can tweak this with documentation, etc.

JuliaRegistries/General#103121

@joaquimg
Copy link
Member

This broke some of our code.
We will have to double check what happened.

@odow
Copy link
Member Author

odow commented Mar 21, 2024

On the current commit? I guess I need to refine the check for "is this Xpress.jl's CI."

deps/build.jl Outdated Show resolved Hide resolved
@odow odow changed the title DNMY: use Xpress_jll for binaries Use Xpress_jll for binaries Mar 24, 2024
@odow
Copy link
Member Author

odow commented Mar 24, 2024

There will be a conflict with #251, so we should merge that one first.

@joaquimg
Copy link
Member

I think this PR would enjoy some comments on the readme about:

  • Default xpress (its limitations: community lic is limited)
  • how to use other versions from jll
  • how to use local versions

@odow odow force-pushed the od/xpress-jll branch 2 times, most recently from 2396980 to 2e40dd5 Compare March 25, 2024 02:42
@odow
Copy link
Member Author

odow commented Mar 28, 2024

So new new problem. The community license for 8.13.4 has expired

@joaquimg
Copy link
Member

Can you add the latest community lic to the 8.13 jll?

@odow
Copy link
Member Author

odow commented Mar 29, 2024

No, we can't modify the JLLs. I think it's fairly reasonable from FICO's point of view: they want people using older versions to upgrade. And if they're using the community license for more than one year...then pay for support.

For "our" license, I've just added the most recent, and we can manually update the secret.

@joaquimg
Copy link
Member

I am not sure this is exactly on purpose or just lazyness from FICO perspective.
I wonder if we should have two differente artifacts, one for license and one for the rest.
This would also match well with the fact that the user need their own license but dont need to get into the trouble of getting binaries.

@odow
Copy link
Member Author

odow commented Mar 31, 2024

I think it's purposeful on FICO's behalf. And we won't be modifying their artifacts, or providing a separate mechanism for users to obtain licenses.

Our options are:

  1. Store a community license in a secret that we periodically update
  2. Rely on the latest version of Xpress_jll having a valid license that we grab from on install

This PR currently does (2), but I also tried (1). I'm ambivalent, but (2) is simpler for now. It'll also force me to keep Xpress_jll updated.

@joaquimg
Copy link
Member

I think we can ask FICO to check.

If a separate artifact for licenses is really not doable, then I think we could go for 2 and document in the readme how a user can do it...

@odow
Copy link
Member Author

odow commented Apr 1, 2024

then I think we could go for 2 and document in the readme how a user can do it...

We document how to use Xpress_jll here:
https://github.com/jump-dev/Xpress.jl/tree/od/xpress-jll?tab=readme-ov-file#use-with-xpress_jll

I am not going to document how people can get a community license to use 8.x.y by grabbing one from a newer JLL. We can do it in the GitHub action, but it won't be documented.

We can, at most, document what is done for Python: https://anaconda.org/fico-xpress/xpress

@joaquimg
Copy link
Member

joaquimg commented Apr 1, 2024

We document how to use Xpress_jll here: https://github.com/jump-dev/Xpress.jl/tree/od/xpress-jll?tab=readme-ov-file#use-with-xpress_jll

This looks good.

I am not going to document how people can get a community license to use 8.x.y by grabbing one from a newer JLL. We can do it in the GitHub action, but it won't be documented.

We can, at most, document what is done for Python: https://anaconda.org/fico-xpress/xpress

Forcing people to use always the latest version is a source of non-reproducibility.
They can't even go back to compare at some point.
Unless they dig down on how things are automatically downloaded and test newer lics with old libs.

The python thing bad with such lic problem.

But we can keep complexity low and just warn the user that community lic expires and they might be fine grabbing a newer one and using with an old binary.

README.md Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Apr 1, 2024

They can't even go back to compare at some point.
The python thing bad with such lic problem.

Oh, I'm well aware, and I agree.

But FICO are saying that they have released a community license where people get to use vX.Y for a period of 12 months. That seems like a reasonable position, even if it means that someone cannot go back and reproduce old code. (They of course can, provided they have a license.)

Co-authored-by: Joaquim Dias Garcia <joaquimdgarcia@gmail.com>
@joaquimg
Copy link
Member

joaquimg commented Apr 1, 2024

I still think the lic thing is a lazyness/"keep it simple" procedure than any particular enforcement.

However, lets leave this for the future. This PR is great as is.

@odow odow merged commit 5eb8119 into master Apr 2, 2024
14 of 16 checks passed
@odow odow deleted the od/xpress-jll branch April 2, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants