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.

Sign up for GitHub

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

Jump to bottom

Add JLArrays, Metal and AMDGPU extensions and S kwarg tests #332

Closed
wants to merge 0 commits into from
Closed

Add JLArrays, Metal and AMDGPU extensions and S kwarg tests #332

wants to merge 0 commits into from

Conversation

nHackel
Copy link
Contributor

@nHackel nHackel commented May 28, 2024 •
edited
Loading

This PR adds a weak extension for JLArrays. These arrays are a CPU-reference-implementation of GPUArrays and allow for easier testing of GPU handling. The weak extension is structured similar to the CUDA extension

We will also add extensions for Metal.jl and AMDGPU.jl. Metal.jl and JLArrays can be tested, AMDGPU.jl would require buildkite with an AMD card.

  • JLArrays + Tests
  • Metal + Tests
  • AMDGPU

@nHackel
Copy link
Contributor Author

nHackel commented May 29, 2024 •
edited
Loading

I've also added some initial tests that check if the S keyword is correctly handled for the basic constructors and the special operators. I've also found a small bug where the S keyword was discarded for the opEye operator.

Per default the tests run on JLArrays, in theory it would also be possible to let the test run on CUDA with the buildkite setup

@nHackel nHackel changed the title Add JLArrays extension Add JLArrays extension and S kwarg tests May 29, 2024
@nHackel nHackel changed the title Add JLArrays extension and S kwarg tests Add JLArrays, Metal and AMDGPU extensions and S kwarg tests Jun 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 5, 2024 •
edited
Loading

Package name latest stable
CaNNOLeS.jl
DCISolver.jl
FletcherPenaltySolver.jl
JSOSolvers.jl
Krylov.jl
NLPModels.jl
NLPModelsModifiers.jl
PROPACK.jl
Percival.jl
QuadraticModels.jl
SolverTools.jl

@tknopp
Copy link
Contributor

tknopp commented Jun 9, 2024

@nHackel: I added Metal support and the Metal tests should only run on the platform arm64. I have not tested though.

I needed to disable the Float64 tests and the SymTridiagonal(Symmetric(mat)) test. The former seems to be simply not supported by Metal. The later I have not investigated. It says: "Compilation to native code failed". Maybe something to be reported at Metal.jl but this can be done later.

@tknopp
Copy link
Contributor

tknopp commented Jun 9, 2024

This is an MWE for the Metal issue:

using Metal, LinearAlgebra

mat = Metal.rand(Float32, 32, 32)
S = Symmetric(mat)
SymTridiagonal(S) # this fails.

@maleadt: Is this a potential Metal issue/limitation and should I file am an issue in Metal.jl?

dpo
dpo reviewed Jun 10, 2024
View reviewed changes
Copy link
Member

@dpo dpo left a comment

Choose a reason for hiding this comment

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

Many thanks for all this! Just a simple comment for now.

test/runtests.jl Outdated
@@ -15,3 +18,4 @@ include("test_deprecated.jl")
include("test_normest.jl")
include("test_diag.jl")
include("test_chainrules.jl")
include("test_S_kwarg.jl")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this file should be under test/gpu for consistency with the Nvidia tests that are already there. I’ll see if I can add a buildkite pipeline for AMD GPUs.

@maleadt
Copy link

maleadt commented Jun 10, 2024

Is this a potential Metal issue/limitation and should I file am an issue in Metal.jl?

That looks like JuliaGPU/Metal.jl#332

@nHackel
Copy link
Contributor Author

nHackel commented Jul 2, 2024 •
edited
Loading

Hello @dpo, I've restructured the GPU related tests into the GPU folder. The normal CI still executes an S keyword test with JLArrays. The metal test is still only run on the appropriate system.

I've also added a test for AMDGPUs as well as a buildkite step. Both AMD and Nvidia test now also run the S keyword test

@nHackel nHackel requested a review from dpo July 16, 2024 12:07
@nHackel nHackel mentioned this pull request Jul 31, 2024
Merged
dpo
dpo reviewed Sep 8, 2024
View reviewed changes
Project.toml Outdated
ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4"
CUDA = "052768ef-5323-5732-b1bb-66c8b64840ba"
LDLFactorizations = "40e66cde-538c-5869-a4ad-c39174c6795b"
JLArrays = "27aeb0d3-9eb9-45fb-866b-73c2ecf80fcb"
Metal = "dde4c033-4e86-420c-a63e-0dd931031962"
Copy link
Member

@dpo dpo Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure how to make this work. Since we added CUDA to [extras], I can't test this package on my Mac any more. I think it's only safe to add JLArrays in here, isn't it?
Nevermind, it was a Manifest issue.

dpo
dpo reviewed Oct 12, 2024
View reviewed changes
Copy link
Member

@dpo dpo left a comment

Choose a reason for hiding this comment

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

So sorry for the long delay. Many thanks for your contribution! Here are two very minor changes that I suggest for consistency.

test/gpu/metal.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@dpo
Copy link
Member

dpo commented Oct 16, 2024

@nHackel I had to rebase your branch and did so in #351.

@nHackel nHackel closed this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpo dpo dpo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nHackel @tknopp @maleadt @dpo