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

Potentially change lib check to a warning #163

Closed
ttonelli opened this issue Mar 21, 2022 · 11 comments
Closed

Potentially change lib check to a warning #163

ttonelli opened this issue Mar 21, 2022 · 11 comments
Labels

Comments

@ttonelli
Copy link

Thanks for maintaining this package, it is super helpful!

So, the default build process currently raises an error if XPRESSDIR does not point to a valid Xpress installation. Is there any chance we could change the error call to be a @warning instead?

I understand we can set the XPRESS_JL_SKIP_LIB_CHECK env variable to skip that check. But I am working on a package that depends on Xpress.jl (and other solvers), and I don't want to force every user of my package to have to set this variable. If the default is to skip the check, then they can install my package and will only have an issue if they tried to actually use the Xpress integration, which should be fine if they don't have it installed.

Thanks for considering this!

@ttonelli
Copy link
Author

@joaquimg sorry for pinging you, but I was wondering if you have any opinion on this. Are there specific reasons for enforcing the license at installation time?

@joaquimg
Copy link
Member

you can do
ENV["XPRESS_JL_SKIP_LIB_CHECK "] = "1"
in your julia code before loading xpress, does not need to be in the environment.

Another option is not to have Xpress as a dependency but allow it as a solver parameter to be passed. Like JuMP does. This seems reasonable since you are allowing users to choose.

@joaquimg
Copy link
Member

The reason for enforcing is that most users just use defaults that should be very easy to handle: default install + quick check if
everything should work.

@kpamnany
Copy link

If I may expand on @ttonelli's request... if XPRESS_JL_SKIP_LIB_CHECK is not set, the error is thrown and the package is left unbuilt (deps.jl is not written). The user then has to fix the problem and rebuild. When the package is being installed as a dependency of some other package, the error terminates the build of that package as well (even if Xpress.jl is an optional dependency (a concept Pkg doesn't currently understand)).

If the error is changed to a warning, nothing changes for Xpress.jl -- it is still left unbuilt. However, for a package that has Xpress.jl as a dependency, building can proceed. Hence the request that the error be changed to a warning display instead.

@joaquimg
Copy link
Member

I don't want to diverge from
Gurobi: https://github.com/jump-dev/Gurobi.jl/blob/4a37a9ec16ab7e92148ebc61e6642ea7f34eac4b/deps/build.jl#L170
CPLEX: https://github.com/jump-dev/CPLEX.jl/blob/e962188bf3c769d12fb93794cebc4a5013f06aec/deps/build.jl#L132

@odow invited here. If we end up changing the standard in the jump-dev org I would accept PR's.

Meanwhile,
Can you expand on your use case?

If this is about a julia package intended for julia users, you pass the Optimizer as a function argument, this would allow user to go for whatever solver the have: Xpress, CPLEX, Gurobi, HiGHS etc, no reason for locking in Xpress.

If you want to deploy something like a compiled binary, you can set that env var right before calling julia, either on a shell(like) script or enve in the c-caller program.

@odow
Copy link
Member

odow commented Mar 31, 2022

I'm not going to change. We've had a few requests for this over the years.

Pros

  • Packages can include Xpress/Gurobi/CPLEX as a dependency and install in CI without a license, but not run the solver

Cons

  • Users will install a package, see that it works, and then try to run something and have it break. Given the frequency with which we have people encounter problems, things are already difficult, and I don't intend to make it worse.

Alternatives

If you have PkgA that wants to depend on Xpress, my first answer is don't. Depend on JuMP or MOI and problem solved. People can use any solver they like.

If you have PkgA and it needs solver-specific functionality in Xpress, you have two options:

  • Force people to always have an Xpress installation
  • Develop PkgAXpressCompat which contains the necessary shim between PkgA and Xpress. If people want to use the Xpress component, they should do using PkgAXpressCompat.

As an example of the second option:

Don't use Requires.jl, it just leads to trouble:

Note that you can have multiple Julia packages in the same Git repository:

@joaquimg
Copy link
Member

joaquimg commented Apr 4, 2022

I will close this as wontfix, as its the current standard in the ecosystem.
Feel free to keep talking here!

@kpamnany
Copy link

kpamnany commented Apr 4, 2022

  • Users will install a package, see that it works, and then try to run something and have it break. Given the frequency with which we have people encounter problems, things are already difficult, and I don't intend to make it worse.

Just to make sure that I understand this comment -- if the error() is changed to @warn or similar, the only effect should be on packages that are installing this as a dependency; there should be no effect on Xpress itself correct?

@joaquimg
Copy link
Member

joaquimg commented Apr 5, 2022

Yes. If you fork and do that change, you should be fine. You users might have some difficulty make sure everything is ok, just like jump users used to...

@odow
Copy link
Member

odow commented Apr 5, 2022

the only effect should be on packages that are installing this as a dependency; there should be no effect on Xpress itself correct?

The choice is whether to error on Pkg.add or using. At the moment, if you don't have Xpress installed and configured,

import Pkg; Pkg.add("Xpress")  # Errors

Changing that to a warn could mean

import Pkg; Pkg.add("Xpress")  # Works
using Xpress  # Errors

Or even

import Pkg; Pkg.add("Xpress")  # Works
using Xpress  # Works
Xpress.Optimizer() # Fails

@kpamnany
Copy link

kpamnany commented Apr 5, 2022

Right, this is where I'm not seeing the distinction:

The choice is whether to error on Pkg.add or using. At the moment, if you don't have Xpress installed and configured,

import Pkg; Pkg.add("Xpress")  # Errors

At this level, the Pkg.add("Xpress") simply outputs a message; whether that's a println() or an error() doesn't matter. If you ignore the message (irrespective of whether it was a warning or an error) and try using Xpress next, you will get an error because deps.jl was not created.

The error does not uninstall the package -- Pkg.status() will show Xpress v0.15.1 as it remains added to the environment's Project.toml.

Changing that to a warn could mean

import Pkg; Pkg.add("Xpress")  # Works
using Xpress  # Errors

No, the Pkg.add() would output the warning (could even output the stack trace just as though it was an error()) and then leave Xpress in the exact same state as it was in the case above, i.e. using Xpress will produce an error because deps.jl was not created.

This is why it seems to me that the only difference is in dependency-based addition of the package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants