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

Only define methods on SuiteSparse.UMFPACK if GPL libs present #21

Merged
merged 2 commits into from
Jan 16, 2023

Conversation

nickrobinson251
Copy link
Contributor

@nickrobinson251 nickrobinson251 commented Jan 10, 2023

Fix package on Julia versions built without GPL libraries (i.e. with USE_GPL_LIBS=0)

This is so packages depending on SparseMatricesCSR.jl will still compile, even on Julia versions built without GPL libraries. Prompted by dmlc/XGBoost.jl#157

Before we'd get

julia> using SparseMatricesCSR
[ Info: Precompiling SparseMatricesCSR [a0a7dd2c-ebf4-11e9-1f05-cf50bc540ca1]
ERROR: LoadError: UndefVarError: `UMFPACK` not defined
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base.jl:31
 [2] top-level scope
   @ ~/repos/SparseMatricesCSR.jl/src/SparseMatrixCSR.jl:143
 [3] include(mod::Module, _path::String)
   @ Base ./Base.jl:456
 [4] include(x::String)
   @ SparseMatricesCSR ~/repos/SparseMatricesCSR.jl/src/SparseMatricesCSR.jl:1
 [5] top-level scope
   @ ~/repos/SparseMatricesCSR.jl/src/SparseMatricesCSR.jl:17
 [6] include
   @ ./Base.jl:456 [inlined]
 [7] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::Nothing)
   @ Base ./loading.jl:1900
 [8] top-level scope
   @ stdin:2
in expression starting at /Users/nickr/repos/SparseMatricesCSR.jl/src/SparseMatrixCSR.jl:143
in expression starting at /Users/nickr/repos/SparseMatricesCSR.jl/src/SparseMatricesCSR.jl:1
in expression starting at stdin:2
ERROR: Failed to precompile SparseMatricesCSR [a0a7dd2c-ebf4-11e9-1f05-cf50bc540ca1] to "/Users/nickr/.julia/compiled/v1.10/SparseMatricesCSR/jl_iZfEOo".
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] compilecache(pkg::Base.PkgId, path::String, internal_stderr::IO, internal_stdout::IO, keep_loaded_modules::Bool)
   @ Base ./loading.jl:2124
 [3] compilecache
   @ ./loading.jl:2016 [inlined]
 [4] _require(pkg::Base.PkgId, env::String)
   @ Base ./loading.jl:1660
 [5] _require_prelocked(uuidkey::Base.PkgId, env::String)
   @ Base ./loading.jl:1515
 [6] macro expansion
   @ ./loading.jl:1503 [inlined]
 [7] macro expansion
   @ ./lock.jl:267 [inlined]
 [8] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:1466

Now the package compiles as expected

julia> using SparseMatricesCSR
[ Info: Precompiling SparseMatricesCSR [a0a7dd2c-ebf4-11e9-1f05-cf50bc540ca1]

julia>

@nickrobinson251
Copy link
Contributor Author

@amartinhuertas, might you be able to take a look? thanks!

@amartinhuertas
Copy link
Member

Thanks for the PR!

I was not aware of this issue.

The fix that you submitted provenly solves the precompilation error.

However, I wonder whether if- constructs of the type

if Base.USE_GPL_LIBS
....

should also go to other parts of the code to avoid other potential errors. (I agree precompilation is the most annoying).

Whats your view on this?

@nickrobinson251
Copy link
Contributor Author

i couldn't see any other code paths that would error without GPL libraries -- did you have any in mind?

The lu! codepath now behind Base.USE_GPL_LIBS will be an error, which is why the if check in tests too

e.g given

using SparseMatricesCSR, SparseArrays, LinearAlgebra
Bi = 0;
I = [1,1,2,2,2,3,3];
J = [1,2,1,2,3,2,3];
V = [4.0,1.0,-1.0,4.0,1.0,-1.0,4.0];
CSR = sparsecsr(Val(Bi),I,J,V);

in Julia with GPL libs

julia> fact = lu(CSR);

julia> typeof(fact)
Transpose{Float64, SuiteSparse.UMFPACK.UmfpackLU{Float64, Int64}}

julia> lu!(fact, CSR)
Transpose of SuiteSparse.UMFPACK.UmfpackLU{Float64, Int64}
L factor:
3×3 SparseMatrixCSC{Float64, Int64} with 5 stored entries:
  1.0                 
           1.0        
 -0.208333  0.208333  1.0
U factor:
3×3 SparseMatrixCSC{Float64, Int64} with 5 stored entries:
 0.8       0.2
     0.8  -0.2
          0.75

whereas without GPL libs

julia> fact = lu(CSR);

julia> typeof(fact)
Transpose{Float64, LU{Float64, SparseMatrixCSC{Float64, Int64}, Vector{Int64}}}

julia> lu!(fact, CSR)
ERROR: MethodError: no method matching lu!(::Transpose{Float64, LU{Float64, SparseMatrixCSC{Float64, Int64}, Vector{Int64}}}, ::SparseMatrixCSR{0, Float64, Int64})

but at least the package and any dependents are now loadable (i.e. now only this one codepath doesn't work without GPL libs, rather than the whole package)

@amartinhuertas
Copy link
Member

Ok, thanks for the explanation. Once the tests pass, I will accept the PR.

@codecov-commenter
Copy link

Codecov Report

Merging #21 (79a5a6f) into master (dbe71cf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #21   +/-   ##
=======================================
  Coverage   86.17%   86.17%           
=======================================
  Files           3        3           
  Lines         246      246           
=======================================
  Hits          212      212           
  Misses         34       34           
Impacted Files Coverage Δ
src/SparseMatrixCSR.jl 86.47% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@amartinhuertas amartinhuertas merged commit d26c461 into gridap:master Jan 16, 2023
@nickrobinson251 nickrobinson251 deleted the npr/guard-gpl-libs branch January 23, 2023 09:57
@nickrobinson251
Copy link
Contributor Author

Thanks, @amartinhuertas! Would it be possible to get a new patch release of this package which includes this fix please? 🙏

@amartinhuertas
Copy link
Member

Yes. Patch release already triggered.

@nickrobinson251
Copy link
Contributor Author

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.

3 participants