jump-dev / SCS.jl

A Julia interface for the SCS conic programming solver
https://github.com/cvxgrp/scs
Other
81 stars 27 forks source link

Do we need Requires? #269

Closed kalmarek closed 1 year ago

kalmarek commented 1 year ago

I just did an experiment locally and it turns out we could just drop Requires and remove the whole __init__() hackery:

# Copyright (c) 2014: SCS.jl contributors
#
# Use of this source code is governed by an MIT-style license that can be found
# in the LICENSE.md file or at https://opensource.org/licenses/MIT.

module SCS

import MathOptInterface
import Requires
import SCS_jll
import SparseArrays

global indirect = SCS_jll.libscsindir
global direct = SCS_jll.libscsdir

import SCS_GPU_jll
global gpuindirect = SCS_GPU_jll.libscsgpuindir

if Sys.islinux() && Sys.ARCH == :x86_64
    import SCS_MKL_jll
    import SCS_MKL_jll.MKL_jll
    global mkldirect = SCS_MKL_jll.libscsmkl
end

include("c_wrapper.jl")
include("linear_solvers/direct.jl")
include("linear_solvers/indirect.jl")
include("linear_solvers/gpu_indirect.jl")
include("linear_solvers/mkl_direct.jl")
include("MOI_wrapper/MOI_wrapper.jl")

export scs_solve

end

works just fine. The only difference is that when you try to use e.g. SCS.GpuIndirectSolver without installing CUDA first you'll get (sorry for errors in german ;)

julia> feasible_basic_problems(SCS.GpuIndirectSolver)
ERROR: could not load library "/home/kalmar/.julia/artifacts/2aaf2d3c367d9e1a41ebf36b0bf47a08f363b09b/lib/libscsgpuindir.so"
libcudart.so.11.0: Kann die Shared-Object-Datei nicht öffnen: Datei oder Verzeichnis nicht gefunden
Stacktrace:
 [1] scs_set_default_settings
   @ ~/.julia/dev/SCS/src/linear_solvers/gpu_indirect.jl:14 [inlined]
 [2] ScsSettings
   @ ~/.julia/dev/SCS/src/c_wrapper.jl:43 [inlined]
 [3] scs_solve(linear_solver::Type{SCS.GpuIndirectSolver}, m::Int64, n::Int64, A::Matrix{Float64}, P::SparseMatrixCSC{Float64, Int64}, b::Vector{Float64}, c::Vector{Float64}, z::Int64, l::Int64, bu::Vector{Float64}, bl::Vector{Float64}, q::Vector{Int64}, s::Vector{Int64}, ep::Int64, ed::Int64, p::Vector{Float64}, primal_sol::Vector{Float64}, dual_sol::Vector{Float64}, slack::Vector{Float64}; warm_start::Bool, options::Base.Pairs{Symbol, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ SCS ~/.julia/dev/SCS/src/c_wrapper.jl:319
 [4] scs_solve (repeats 2 times)
   @ ~/.julia/dev/SCS/src/c_wrapper.jl:278 [inlined]
 [5] feasible_basic_problems(solver::Type)
   @ Main ~/.julia/dev/SCS/test/test_problems.jl:701
 [6] top-level scope
   @ REPL[4]:1

If CUDA (e.g. through CUDA_jll) is installed but no GPU is present SCS will throw a standard CUDA error (no CUDA device found or so).


The benefits of doing this:

Drawbacks of dropping Requires

@odow Are we happy with the status quo, should we load SCS_GPU_jll unconditionally, or should we work on a solution with CUDA_Runtime_jll?

odow commented 1 year ago

remove the whole init() hackery

__init__ is required so that someone can overload the binaries.

I'm in favor of keeping Requires for now. It seems to be working, and asking people to explicitly opt into CUDA_jll (or CUDA_Runtime_jll) seems okay.

kalmarek commented 1 year ago

hmm? what do you mean override the binaries? The standard way I do it is to SCS_GPU_jll.dev_jll() and then replace the new library in .julia/dev/SCS_GPU_jll/override/lib; How do you do it with __init__?

odow commented 1 year ago

__init__ needs to load the new path and set the global, otherwise the path gets baked into the precompiled image and the new dev_jll() path isn't used.

kalmarek commented 1 year ago

hmm, maybe I'm missing something: If you're in SCS project and you

pkg"dev SCS_GPU_jll"
import SCS_GPU_jll
SCS_GPU_jll.dev_jll()
using SCS

will precompile the dev-ed SCS_GPU_jll with the override set. The only situation when this goes wrong is when using SCS happens between dev and dev_jll()?

odow commented 1 year ago

See https://github.com/jump-dev/ECOS.jl/pull/134 https://github.com/jump-dev/SCS.jl/pull/248

This can also occur if you use https://jump.dev/JuMP.jl/stable/developers/custom_solver_binaries without the pkg"dev.

kalmarek commented 1 year ago

I see, it's about relocability, thanks!

We will still need to think what to do about CUDA_Runtime_jll