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 Preferences to toggle precompilation of code that requires GPL dependencies #238

Merged
merged 4 commits into from
Nov 28, 2022
Merged

Use Preferences to toggle precompilation of code that requires GPL dependencies #238

merged 4 commits into from
Nov 28, 2022

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Nov 28, 2022

Fixes #224

We use Preferences. The user can toggle the behavior by specifying the include_sparse preference. If the user does not specify the preference, we default to the value of Base.USE_GPL_LIBS.

@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #238 (fb6fd69) into main (0857619) will decrease coverage by 0.18%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
- Coverage   65.04%   64.86%   -0.19%     
==========================================
  Files          11       12       +1     
  Lines         701      703       +2     
==========================================
  Hits          456      456              
- Misses        245      247       +2     
Impacted Files Coverage Δ
src/LinearSolve.jl 75.00% <ø> (ø)
src/factorization.jl 78.43% <ø> (-0.21%) ⬇️
src/default.jl 46.47% <50.00%> (-1.35%) ⬇️
src/factorization_sparse.jl 100.00% <100.00%> (ø)

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

@andreasnoack
Copy link
Contributor

Ref #224

@DilumAluthge DilumAluthge marked this pull request as draft November 28, 2022 07:40
@DilumAluthge DilumAluthge changed the title Make import LinearSolve work on builds of Julia where Base.USE_GPL_LIBS is false Use Preferences to toggle precompilation of code that requires GPL dependencies Nov 28, 2022
@ChrisRackauckas
Copy link
Member

What about KLUFactorization and UMFPACKFactorization? Won't those need to be avoided? Then won't the default algorithm choice need to pick some other algorithm for sparse? What algorithm would it choose, does Julia have a fallback here?

@DilumAluthge
Copy link
Member Author

In the case where Julia was built with USE_GPL_LIBS=0, I think it's fine for us to say that this package doesn't support any functionality on sparse arrays.

@rayegun
Copy link
Collaborator

rayegun commented Nov 28, 2022

Nothing in SparseArrays for sure.

Only factorizations are GPLd though?

@ChrisRackauckas
Copy link
Member

So then what do we do with the default algorithm choice?

@DilumAluthge
Copy link
Member Author

We could check if LinearSolve.INCLUDE_SPARSE is false, and if it is false, if A is a sparse matrix, then defaultalg(A, ...) throws an error saying that sparse matrices are not supported?

@ChrisRackauckas
Copy link
Member

But then how do we make that dispatch on A?

@DilumAluthge
Copy link
Member Author

The SparseMatrixCSC type still exists in Julia even if Base.USE_GPL_LIBS is false. So we can still dispatch on it.

@ChrisRackauckas
Copy link
Member

And does a generic fallback implementation exist on \?

@ChrisRackauckas
Copy link
Member

@Wimmerer weren't you going to do a pure Julia KLU sometime?

@ChrisRackauckas
Copy link
Member

I guess we can default it to KrylovJL_GMRES

@j-fu
Copy link
Contributor

j-fu commented Nov 28, 2022

And does a generic fallback implementation exist on \?

https://github.com/PetrKryslUCSD/Sparspak.jl

@DilumAluthge
Copy link
Member Author

And does a generic fallback implementation exist on \?

Yeah:

julia> Base.USE_GPL_LIBS
false

julia> using SuiteSparse, SparseArrays, Random, LinearAlgebra

julia> A = sprand(4, 4, 0.3) + I
4×4 SparseMatrixCSC{Float64, Int64} with 6 stored entries:
 1.24017                 
          1.0            
 0.984142      1.0        
              0.824643  1.0

julia> b = rand(4)
4-element Vector{Float64}:
 0.9655062442597853
 0.15175107617153183
 0.004248022435501819
 0.4659030670094594

julia> typeof(A)
SparseMatrixCSC{Float64, Int64}

julia> typeof(b)
Vector{Float64} (alias for Array{Float64, 1})

julia> A\b
4-element Vector{Float64}:
  0.7785278622862858
  0.15175107617153183
 -0.7619340985421428
  1.0942264516593845

@rayegun
Copy link
Collaborator

rayegun commented Nov 28, 2022

It still has to be LGPL, but yeah I'll get it out the door this week @ChrisRackauckas. It just defaults to the dense case as far as I can tell. Once glue deps arrive I can add stuff like Sparspak to LinearSolve? Or I can just go ahead and do that as a direct dep.

@DilumAluthge
Copy link
Member Author

I guess we can default it to KrylovJL_GMRES

Alright, I just pushed b576e40 - if INCLUDE_SPARSE is false and A is a SparseMatrixCSC, we default to KrylovJL_GMRES().

src/default.jl Outdated
Comment on lines 59 to 60
function defaultalg(A, b::GPUArraysCore.AbstractGPUArray, ::OperatorAssumptions{true})
if VERSION >= v"1.8-"
LUFactorization()
else
QRFactorization()
end
KrylovJL_GMRES()
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I think I've reverted that change.

@ChrisRackauckas
Copy link
Member

Once glue deps arrive I can add stuff like Sparspak to LinearSolve? Or I can just go ahead and do that as a direct dep.

I think it's fine to add it as a direct dep

@j-fu
Copy link
Contributor

j-fu commented Nov 28, 2022

Once glue deps arrive I can add stuff like Sparspak to LinearSolve? Or I can just go ahead and do that as a direct dep.

I think it's fine to add it as a direct dep

@PetrKryslUCSD

@ChrisRackauckas ChrisRackauckas merged commit c7f44bb into SciML:main Nov 28, 2022
@DilumAluthge DilumAluthge deleted the dpa/use-gpl-libs branch November 28, 2022 10:18
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.

Avoid (unconditional) precompiling of code that has GPL dependecies
5 participants