make gather/scatter work with views #546
Conversation
|
In the last commit I stopped testing Enzyme since tests are broken at the moment. The issue with Enzyme should be fixed in a separate PR and tests restored there. cc @wsmoses |
|
Generally LGTM. Do we need to worry about things like non-contiguous views of CuArrays, or are the existing gather/scatter kernels smart enough to work with those? |
|
@CarloLucibello yeah the last patch bump of Enzyme included new functionality that triggered that, but ironically I think I fixed it yesterday here EnzymeAD/Enzyme.jl#1106. You can alternatively version bound Enzyme around it for the tests [its just the latest patch that has it]. We'll try to make another patch shortly. |
|
@CarloLucibello we just released a new patch with that fix (which is seemingly the same as yours)? Mind re-enabling the test and retrying? |
|
I reinstated the enzyme tests, it's working fine now.
Added a test with non-contiguous views. |
test/runtests.jl
Outdated
| # get(ENV, "NNLIB_TEST_AMDGPU", "false") != "true" | ||
| const Test_Enzyme = VERSION <= v"1.10-" && !Sys.iswindows() && | ||
| # TODO Enzyme is not working properly with AMDGPU yet. | ||
| get(ENV, "NNLIB_TEST_AMDGPU", "false") != "true" | ||
| const Test_Enzyme = false |
There was a problem hiding this comment.
don't you need to also remove this variable?
test/runtests.jl
Outdated
| # get(ENV, "NNLIB_TEST_AMDGPU", "false") != "true" | ||
| const Test_Enzyme = VERSION <= v"1.10-" && !Sys.iswindows() && | ||
| # TODO Enzyme is not working properly with AMDGPU yet. | ||
| get(ENV, "NNLIB_TEST_AMDGPU", "false") != "true" | ||
| const Test_Enzyme = false |
|
Ready to go, need approval |
Fix CarloLucibello/GraphNeuralNetworks.jl#349
The time of
using NNlib, after precompilation, doesn't seem affected on julia 1.9