GPU Attempt 2 #472

willtebbutt · 2022-08-31T17:41:37Z

Summary

Supercedes #470 . I messed up some of my gitconfig stuff and had commits with the wrong username etc. Easiest thing was to start again

Proposed changes

...

What alternatives have you considered?

Breaking changes

edit:

TODO:

sort out codecov on buildkite branch
sort out formatting

src/TestUtils.jl

test/cuda.jl

codecov · 2022-08-31T18:02:24Z

Codecov Report

Base: 90.00% // Head: 86.53% // Decreases project coverage by -3.47% ⚠️

Coverage data is based on head (12fe669) compared to base (dede8a0).
Patch coverage: 18.57% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #472      +/-   ##
==========================================
- Coverage   90.00%   86.53%   -3.48%     
==========================================
  Files          52       53       +1     
  Lines        1351     1478     +127     
==========================================
+ Hits         1216     1279      +63     
- Misses        135      199      +64

Impacted Files	Coverage Δ
src/KernelFunctions.jl	`100.00% <ø> (ø)`
src/distances/cuda.jl	`0.00% <0.00%> (ø)`
src/TestUtils.jl	`47.36% <6.55%> (-47.37%)`	⬇️
src/basekernels/fbm.jl	`96.07% <77.77%> (-3.93%)`	⬇️
src/basekernels/exponential.jl	`100.00% <100.00%> (ø)`
src/basekernels/matern.jl	`100.00% <100.00%> (ø)`
src/basekernels/wiener.jl	`92.85% <100.00%> (ø)`
src/transform/scaletransform.jl	`100.00% <0.00%> (+13.33%)`	⬆️
src/transform/periodic_transform.jl	`50.00% <0.00%> (+20.00%)`	⬆️
src/transform/transform.jl	`100.00% <0.00%> (+40.00%)`	⬆️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/TestUtils.jl

src/basekernels/matern.jl

test/cuda.jl

willtebbutt · 2022-08-31T21:28:44Z

@theogf there are a couple of minor things left to do, but I think this is defintely ready for review

theogf

Wow that's an amazing load of good work. My only concern is that a lot of the code consist in getting pairwise done right and in a better world this should be done in an external package. Especially because of the new CUDA dependency...

theogf · 2022-09-01T19:29:02Z

src/TestUtils.jl

@@ -176,27 +190,108 @@ function test_interface(k::Kernel, T::Type{<:Real}=Float64; kwargs...)
    return test_interface(Random.GLOBAL_RNG, k, T; kwargs...)
 end

+const FloatType = Union{Float32, Float64}


Why not AbstractFloat?

theogf · 2022-09-01T19:30:15Z

src/KernelFunctions.jl

@@ -47,6 +47,7 @@ export IndependentMOKernel,
 export tensor, ⊗, compose

 using Compat
+using CUDA


Unless things changed, that's quite a massive dependency no? I saw CuArrays.jl got deprecated, does this mean that CUDA is light now?

As we already depend on Requires, maybe it could be hidden behind a @require?

Alternatively, maybe the lightweight GPUArraysCore (https://github.com/JuliaGPU/GPUArrays.jl/tree/master/lib/GPUArraysCore) is sufficient?

theogf · 2022-09-01T19:30:33Z

.buildkite/pipeline.yml

@@ -0,0 +1,14 @@
+steps:


theogf · 2022-09-01T19:32:29Z

src/TestUtils.jl

+end
+
+_to_cpu(x::CuArray{<:Real}) = Array(x)
+_to_cpu(x::ColVecs{<:Real, <:CuMatrix}) = ColVecs(_to_cpu(x.X))


Can't we just overload cpu on our type? (or cpu is part of Flux? I don't remember...)

Could we use/define Adapt.adapt? And let users pass the types they want to adapt it to?
According to the README of Adapt, we probably would want to define

Adapt.adapt_structure(to, x::ColVecs{<:Real}) = ColVecs(adapt(to, x.X)) Adapt.adapt_structure(to, x::RowVecs{<:Real}) = RowVecs(adapt(to, x.X))

And then one could use e.g. adapt(CuArray, x) or adapt(Array, x) where x could be a ColVecs{<:Real, Matrix{<:Real}}/RowVecs{<:Real,Matrix{<:Real}} or a ColVecs{<:Real,<:CuMatrix}/RowVecs{<:Real,<:CuMatrix} (for the other direction). If the testing function allows to specify the desired type (such as Array or CuArray) users/developers could pass it and we don't have to deal with it in KernelFunctions here, maybe?

theogf · 2022-09-01T19:33:40Z

src/basekernels/matern.jl

@@ -77,7 +77,7 @@ end

 Matern32Kernel(; metric=Euclidean()) = Matern32Kernel(metric)

-kappa(::Matern32Kernel, d::Real) = (1 + sqrt(3) * d) * exp(-sqrt(3) * d)
+kappa(::Matern32Kernel, d::T) where {T<:Real} = (1 + T(sqrt(3)) * d) * exp(-T(sqrt(3)) * d)


Can't T(sqrt(3)), or at least sqrt(3) be precalculated?

One could use

Suggested change

kappa(::Matern32Kernel, d::T) where {T<:Real} = (1 + T(sqrt(3)) * d) * exp(-T(sqrt(3)) * d)

function kappa(::Matern32Kernel, d::Real)

x = sqrt3 * d

return (1 + x) * exp(-x)

end

and load IrrationalConstants.sqrt3 in src/KernelFunctions.jl

theogf · 2022-09-01T19:33:56Z

src/basekernels/matern.jl

@@ -108,7 +108,7 @@ end

 Matern52Kernel(; metric=Euclidean()) = Matern52Kernel(metric)

-kappa(::Matern52Kernel, d::Real) = (1 + sqrt(5) * d + 5 * d^2 / 3) * exp(-sqrt(5) * d)
+kappa(::Matern52Kernel, d::T) where {T<:Real} = (1 + T(sqrt(5)) * d + 5 * d^2 / 3) * exp(-T(sqrt(5)) * d)


Same here with sqrt(5)

Unfortunately, IrrationalConstants doesn't define sqrt5 (maybe should be added?) but one could still improve it a bit:

Suggested change

kappa(::Matern52Kernel, d::T) where {T<:Real} = (1 + T(sqrt(5)) * d + 5 * d^2 / 3) * exp(-T(sqrt(5)) * d)

function kappa(::Matern52Kernel, d::Real)

x = sqrt(oftype(d, 5))

return (1 + x + 5 * d^2 / 3) * exp(-x)

end

This would also be a bit safer as it also works if d is not a floating point number.

theogf · 2022-09-01T19:34:09Z

src/basekernels/wiener.jl

@@ -62,22 +62,22 @@ function (::WienerKernel{1})(x, y)
    X = sqrt(sum(abs2, x))
    Y = sqrt(sum(abs2, y))
    minXY = min(X, Y)
-    return 1 / 3 * minXY^3 + 1 / 2 * minXY^2 * euclidean(x, y)
+    return minXY^3 / 3 + minXY^2 * euclidean(x, y) / 2


Nice catches here!

devmotion · 2022-09-01T20:38:02Z

src/basekernels/fbm.jl

+_ones(el_type, x::AbstractVector, sz...) = ones(el_type, sz...)
+function _ones(
+    el_type,
+    x::Union{CuVector{<:Real}, ColVecs{<:Real, <:CuMatrix}, RowVecs{<:Real, <:CuMatrix}},
+    sz...,
+)
+    return CUDA.ones(el_type, sz...)
+end


Could we use adapt here?

devmotion · 2022-09-01T20:40:29Z

src/distances/cuda.jl

+const CuSubArray{T} = SubArray{T, <:Any, <:CuArray}
+const CuColVecs{T} = ColVecs{T, <:Union{CuMatrix{T}, CuSubArray{T}}}
+const CuRowVecs{T} = RowVecs{T, <:Union{CuMatrix{T}, CuSubArray{T}}}
+
+const _CuVector{T} = Union{CuVector{T}, SubArray{T, 1, <:CuArray}}


Do we need to restrict it to these types? Could we just define our pairwise (IIRC we own it?) in this way for generic AbstractVector/ColVecs/RowVecs, and only forward the Vector{<:Real}/ColVecs{<:Real,Matrix{<:Real}}/... to Distances?

devmotion

@willtebbutt Do you think it will take some time to sort out all remaining comments and issues in this PR? If so, we could add only the buildkite configuration as a first step to fix CI errors and add GPU-specific tests in a follow-up PR, similar to JuliaGaussianProcesses/AbstractGPs.jl#335.

devmotion · 2022-09-26T11:15:09Z

.buildkite/pipeline.yml

+    plugins:
+      - JuliaCI/julia#v1:
+          version: "1"
+      - JuliaCI/julia-test#v1: ~


Suggested change

- JuliaCI/julia-test#v1: ~

- JuliaCI/julia-test#v1: ~

- JuliaCI/julia-coverage#v1:

codecov: true

devmotion · 2022-09-26T11:15:29Z

.buildkite/pipeline.yml

+    timeout_in_minutes: 60
+
+env:
+  GROUP: CUDA


Suggested change

GROUP: CUDA

GROUP: CUDA

SECRET_CODECOV_TOKEN: "t5UlmhN6IORkngUHsuzYWkMJoIcAu+appM1fZJxZitCmw8gn8EA4jukHSaps23tMpVI19qgHr/F6XeS5M/SG1+lrSa/cG2e2kAD03E34vhK0+P6Fx8CZxL3RsIc1XDSX9qEs1/BGUNHP8t0B/vQumJRqPH5F+IGXhR5yRolhqquJZ5OUHwMGo2+FWY12YWehGjXOTCy/y0f0vYAysLU+TPF2Xa8xpDEJtCQlFDFVSPwtBFqB+8XD9bmtGQkDFfZOw0/5dHCWKMmr/E3Z9xXHgIk74mN91PtVJZDQjEVZOrPLOMOIheTtQFSErVXoKXBXElorcAY96oJQVuvqK61H/A==;U2FsdGVkX18ne7l2AH/iweW0yXOQV+yawNU6Ax21o+vkqTztlWAelcsyPaaMFSqpsYT0+uHAHQTPfqzuKJH7QA=="

willtebbutt · 2022-09-26T11:29:44Z

@willtebbutt Do you think it will take some time to sort out all remaining comments and issues in this PR? If so, we could add only the buildkite configuration as a first step to fix CI errors and add GPU-specific tests in a follow-up PR, similar to JuliaGaussianProcesses/AbstractGPs.jl#335.

That would be ideal. I thought I was going to have time to follow up on this immediately, but it's actually going to be a little while. If you could open a PR like the one you made to AbstractGPs, that would be ideal.

Will Tebbutt added 5 commits August 31, 2022 08:48

Initial work

35f4d97

Just use CUDA

935e3aa

Some tweaks

f09ae92

Make kernels work on the GPU

3c6ed73

Change default group

42cfdf0

github-actions bot reviewed Aug 31, 2022

View reviewed changes

willtebbutt changed the title ~~Attempt 2~~ GPU Attempt 2 Aug 31, 2022

Will Tebbutt added 3 commits August 31, 2022 17:46

Remove redundant comment

d40301d

Remove InteractiveUtils

457609a

Change buildkite group variable

8372ed4

willtebbutt mentioned this pull request Aug 31, 2022

Initial work #471

Closed

Will Tebbutt added 4 commits August 31, 2022 18:32

Fix typo

608b174

Bump patch version

7f21ec2

Tweak comment

11a5b20

Add some more tests

12fe669

github-actions bot reviewed Aug 31, 2022

View reviewed changes

src/TestUtils.jl Show resolved Hide resolved

src/basekernels/matern.jl Show resolved Hide resolved

test/cuda.jl Show resolved Hide resolved

test/cuda.jl Show resolved Hide resolved

test/cuda.jl Show resolved Hide resolved

willtebbutt requested a review from theogf August 31, 2022 21:28

theogf reviewed Sep 1, 2022

View reviewed changes

devmotion reviewed Sep 1, 2022

View reviewed changes

devmotion reviewed Sep 26, 2022

View reviewed changes

devmotion mentioned this pull request Sep 26, 2022

Add buildkite configuration #477

Merged

willtebbutt marked this pull request as draft September 15, 2023 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GPU Attempt 2 #472

GPU Attempt 2 #472

willtebbutt commented Aug 31, 2022 •

edited

Loading

codecov bot commented Aug 31, 2022 •

edited

Loading

willtebbutt commented Aug 31, 2022

theogf left a comment

theogf Sep 1, 2022

theogf Sep 1, 2022

devmotion Sep 1, 2022

devmotion Sep 1, 2022

theogf Sep 1, 2022

theogf Sep 1, 2022

devmotion Sep 1, 2022

theogf Sep 1, 2022

devmotion Sep 1, 2022

theogf Sep 1, 2022

devmotion Sep 1, 2022 •

edited

Loading

theogf Sep 1, 2022

devmotion Sep 1, 2022

devmotion Sep 1, 2022

devmotion left a comment

devmotion Sep 26, 2022

devmotion Sep 26, 2022

willtebbutt commented Sep 26, 2022

-kappa(::Matern32Kernel, d::T) where {T<:Real} = (1 + T(sqrt(3)) * d) * exp(-T(sqrt(3)) * d)
+function kappa(::Matern32Kernel, d::Real)
+    x = sqrt3 * d
+    return (1 + x) * exp(-x)
+end

-kappa(::Matern52Kernel, d::T) where {T<:Real} = (1 + T(sqrt(5)) * d + 5 * d^2 / 3) * exp(-T(sqrt(5)) * d)
+function kappa(::Matern52Kernel, d::Real)
+    x = sqrt(oftype(d, 5))
+    return (1 + x + 5 * d^2 / 3) * exp(-x)
+end

	GROUP: CUDA
	GROUP: CUDA
	SECRET_CODECOV_TOKEN: "t5UlmhN6IORkngUHsuzYWkMJoIcAu+appM1fZJxZitCmw8gn8EA4jukHSaps23tMpVI19qgHr/F6XeS5M/SG1+lrSa/cG2e2kAD03E34vhK0+P6Fx8CZxL3RsIc1XDSX9qEs1/BGUNHP8t0B/vQumJRqPH5F+IGXhR5yRolhqquJZ5OUHwMGo2+FWY12YWehGjXOTCy/y0f0vYAysLU+TPF2Xa8xpDEJtCQlFDFVSPwtBFqB+8XD9bmtGQkDFfZOw0/5dHCWKMmr/E3Z9xXHgIk74mN91PtVJZDQjEVZOrPLOMOIheTtQFSErVXoKXBXElorcAY96oJQVuvqK61H/A==;U2FsdGVkX18ne7l2AH/iweW0yXOQV+yawNU6Ax21o+vkqTztlWAelcsyPaaMFSqpsYT0+uHAHQTPfqzuKJH7QA=="

GPU Attempt 2 #472

Are you sure you want to change the base?

GPU Attempt 2 #472

Conversation

willtebbutt commented Aug 31, 2022 • edited Loading

codecov bot commented Aug 31, 2022 • edited Loading

Codecov Report

willtebbutt commented Aug 31, 2022

theogf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devmotion Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devmotion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willtebbutt commented Sep 26, 2022

willtebbutt commented Aug 31, 2022 •

edited

Loading

codecov bot commented Aug 31, 2022 •

edited

Loading

devmotion Sep 1, 2022 •

edited

Loading