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

Fixes for julia 1.0 #184

Merged
merged 20 commits into from
Oct 10, 2018
Merged

Fixes for julia 1.0 #184

merged 20 commits into from
Oct 10, 2018

Conversation

IssamT
Copy link
Contributor

@IssamT IssamT commented Sep 12, 2018

This is a continuation on the nice work started by @matbesancon in #179. Tests are now passing for 0.6 and 1.0. Still some work to do:

  • pass conic tests. I temporarily disabled them since MPB still uses the deprecated speye.
  • fix cpx_ccall_intercept. I temporarily replaced it with cpx_ccall.
  • fix finalizers.

@joaquimg
Copy link
Member

pass conic tests. I temporarily disabled them since MPB still uses the deprecated speye.

Now works, tested in Xpress and Gurobi

src/CPLEX.jl Outdated
if VERSION >= v"0.7.0-DEV.3382"
using SparseArrays
using LinearAlgebra
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also use Compat.XXX

Copy link
Contributor

@matbesancon matbesancon Oct 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given the guideline update on discourse, I think we can remove this all-together and use 1.0-compatible features only
So removing the version check and no Compat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matbesancon, but those are needed for 0.7 and above.

@giadasp
Copy link

giadasp commented Oct 6, 2018

Thank you guys for updating this package!
I've still problems with Julia 1.0, in particular julia doesn't find the function contains and even if I substitute it with occursin it's not able to find the path of CPLEX in my pc.
I also tried by setting ENV["CPLEX_STUDIO_BINARIES"] to the right path but still when I try using CPLEX it says:
CPLEX not properly installed. Please run Pkg.build("CPLEX") Stacktrace: [1] error(::String) at .\error.jl:33 [2] top-level scope at C:\Users\User\.julia\packages\CPLEX\FUtHu\src\CPLEX.jl:17 [3] include at .\boot.jl:317 [inlined] [4] include_relative(::Module, ::String) at .\loading.jl:1041 [5] include(::Module, ::String) at .\sysimg.jl:29 [6] top-level scope at none:2 [7] eval at .\boot.jl:319 [inlined] [8] eval(::Expr) at .\client.jl:389 [9] top-level scope at .\none:3 in expression starting at C:\Users\User\.julia\packages\CPLEX\FUtHu\src\CPLEX.jl:14
What's the problem in your opinion?``

@giadasp
Copy link

giadasp commented Oct 6, 2018

Updated: (sorry, I don't want to add other comments, so I'm updating this one everytime I have news)

Now I can start to optimize a model but still there are problems with CPLEX callbacks for lazy constraints:
MethodError: no method matching cbgetstate(::CPLEX.CplexLazyCallbackData) ,
This is what I've done to make it work, thanks also to your latest corrections:

In CplexSolverInterface.jl

  • change cfunction to @cfunction
  • change contains to occursin
  • change
    cbgetstate(d::CplexCallbackData)=d.state
    to
function MathProgBase.cbgetstate(d::CplexCallbackData) 
 d.state
end

Can you tell me if it's correct?
Thanks :)

@matbesancon
Copy link
Contributor

@giadasp I think this is unrelated to this PR, maybe consider getting help on discourse or slack for this
@IssamT I think we need rebasing here

src/CPLEX.jl Outdated
include("JuMPfunctions.jl")
end
end
# if isdir(Pkg.dir("JuMP")) && VERSION < v"0.6-"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to delete this chunk of code and remove JuMPfunctions.jl.

setbranchcallback!(m::CplexMathProgModel,f) = (m.branchcb = f)
setincumbentcallback!(m::CplexMathProgModel,f) = (m.incumbentcb = f)
setinfocallback!(m::CplexMathProgModel,f) = (m.infocb = f)
MathProgBase.setinfocallback!(m::CplexMathProgModel,f) = (m.infocb = f)

function cbgetmipsolution(d::CplexCallbackData)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The the cb functions also need a MathProgBase. prefix if they're implementing the MPB API.

@@ -615,10 +616,10 @@ function setmathproglazycallback!(model::CplexMathProgModel)
set_param!(model.inner.env, "CPX_PARAM_MIPCBREDLP", 0)
set_param!(model.inner.env, "CPX_PARAM_PRELINEAR", 0)
set_param!(model.inner.env, "CPX_PARAM_REDUCE", CPX_PREREDUCE_PRIMALONLY)
cpxcallback = cfunction(mastercallback, Cint, (Ptr{Void}, Ptr{Void}, Cint, Ptr{Void}, Ptr{Cint}))
cpxcallback = cfunction(mastercallback, Cint, (Ptr{Nothing}, Ptr{Nothing}, Cint, Ptr{Nothing}, Ptr{Cint}))
Copy link
Member

@mlubin mlubin Oct 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cfunction (here and below)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cfunction does not exist in Julia 1.0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was exactly the point of my comment :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Miles, I thought you were making the changes, I didn't understand was only a comment. -.-"
It's sunday, I'm working, please apologize.

@@ -362,7 +363,7 @@ end

function cbgetmipsolution(d::CplexCallbackData, sol::Vector{Cdouble})
@assert d.state == :MIPSol
stat = @cpx_ccall(getcallbacknodex, Cint, (Ptr{Void},Ptr{Void},Cint,Ptr{Cdouble},Cint,Cint),
stat = @cpx_ccall(getcallbacknodex, Cint, (Ptr{Nothing},Ptr{Nothing},Cint,Ptr{Cdouble},Cint,Cint),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Cvoid instead of Nothing when referring to the C type.

end
end

const GChars = Union{Cchar, Char}
const IVec = Vector{Cint}
const FVec = Vector{Cdouble}
const CVec = Vector{Cchar}
const CoeffMat = Union{Matrix{Cdouble}, SparseMatrixCSC{Cdouble}}
const CoeffMat = Union{AbstractArray{Cdouble, 2}, SparseMatrixCSC{Cdouble}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractMatrix would be more concise?

deps/build.jl Outdated
@@ -15,7 +15,8 @@ function write_depsfile(path)
end
end

if is_apple()
if (VERSION >= v"0.7.0-DEV.3382" && Sys.isapple()) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if REQUIRE file specifies julia version at 0.7, we don't need the VERSION >= v"0.7.0-DEV.3382" conditions.
@static can be added in front of the condition too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I thought we wanted to have at least one version of CPLEX.jl that works for both 0.6 and 1.0 (and 0.7) for the transition. No?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, if that's still the case forget what I just wrote :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still can use @static for all these conditions though

deps/build.jl Outdated
@@ -36,7 +39,8 @@ for v in reverse(cpxvers)
end

wincpxvers = ["126","1261","1262","1263","127","1270","1271","128","1280"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const in front of wincpxvers

src/CPLEX.jl Outdated
@@ -6,7 +6,8 @@ module CPLEX
using Libdl
end

if is_apple()
if (VERSION >= v"0.7.0-DEV.3382" && Sys.isapple()) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing here @static and no version check

Copy link
Member

@mlubin mlubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test the callbacks by running the JuMP 0.18 tests with CPLEX.

end
ret
println("some function is called using cpx_ccall_intercept")
# ccall(:jl_exit_on_sigint, Nothing, (Cint,), convert(Cint,0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is blocking the rest of the PR, we can restore it for 0.6 and leave it as a TODO to write a compatible version for 1.0.

@IssamT
Copy link
Contributor Author

IssamT commented Oct 9, 2018

@mlubin, did you mean the file test/callback.jl in JuMP? I fixed some callback errors but when running the file with julia 1.0 I get some errors in that are due to jump I think.

@mlubin
Copy link
Member

mlubin commented Oct 9, 2018

did you mean the file test/callback.jl in JuMP?

Yes.

I get some errors in that are due to jump I think.

What are they? They should be fixed regardless of whether they're in JuMP or CPLEX.jl.

# TODO fix for 0.7 and above (using cpx_call instead temporarily)
if VERSION < v"0.7.0-DEV.3382"
f = "CPX$(func)"
println(f)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover debugging code?

@IssamT
Copy link
Contributor Author

IssamT commented Oct 9, 2018

These are the errors I have now for the test/callback.jl https://justpaste.it/4x67j

@mlubin
Copy link
Member

mlubin commented Oct 9, 2018

@IssamT Could you try JuliaOpt/MathProgBase.jl#227?

@IssamT
Copy link
Contributor Author

IssamT commented Oct 10, 2018

Apparently this doesn't work. from JuMP's callback.jl :

@test macroexpand(:(@lazyconstraint(cb, sum(x) <= 0, badkwarg=true))).head == :error

@IssamT
Copy link
Contributor Author

IssamT commented Oct 10, 2018

All callback tests are passing for 0.6 and 1.0 except the test in line 307 of callback.jl

@test solve(mod, suppress_warnings=true) == :UserLimit

it returns CPXMIP_ABORT_FEAS instead of UserLimit. Is this an error or just a status that was updated ?

@mlubin
Copy link
Member

mlubin commented Oct 10, 2018

CPXMIP_ABORT_FEAS should map to UserLimit under MPB.

@IssamT
Copy link
Contributor Author

IssamT commented Oct 10, 2018

Thanks. Fixed. So the only issue remaining is the test with macroexpand that doesn't run.

@mlubin mlubin changed the title WIP Fixes for julia 1.0 Fixes for julia 1.0 Oct 10, 2018
@mlubin mlubin merged commit 9e60e3d into jump-dev:master Oct 10, 2018
@odow odow mentioned this pull request Oct 11, 2018
@odow odow mentioned this pull request Sep 30, 2020
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.

5 participants