Skip to content

Allow any backing array type and add Adapt support #9

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

Merged
merged 4 commits into from
Mar 4, 2022

Conversation

darsnack
Copy link
Contributor

@darsnack darsnack commented Mar 2, 2022

Closes #8. Currently, I didn't add tests since the repo doesn't have a build kite pipeline. Does JuliaReinforcementLearning have the app installed? I'm happy to help coordinate this with JuliaGPU if needed.

@darsnack darsnack marked this pull request as draft March 2, 2022 18:11
@darsnack
Copy link
Contributor Author

darsnack commented Mar 2, 2022

With some testing on a machine with a GPU, I see that this isn't going to just work. Let me play around with it some more.

@findmyway
Copy link
Member

I think push!, pushfirst!, pop! or append! should just work. The problem is with views and element-wise operations. Could you share your desired usage? I guess a convert function to convert a view into a CuArray would meet most cases?

@darsnack darsnack marked this pull request as ready for review March 3, 2022 18:15
@darsnack
Copy link
Contributor Author

darsnack commented Mar 3, 2022

I had to make some minor adjustments to the code to get push! etc. to work. I also added tests that replicate the full test suite for CuArrays. This will not be able to run on GHA, so tests will fail right now. I can verify that they pass locally on my machine with a GPU.

For element-wise operations, broadcasting is the recommended solution for GPUs. In this case, we will need to take on a CUDA.jl dep to specify the broadcasting style. The code is a one-liner, but I'll leave it to you whether the dep is worth it. I don't think this would be a common use case, and a user can always index the full array.

For views, it becomes an issue because we have a SubArray of a CircularArrayBuffer which CUDA.jl will treat as non-isbits. We could return a view of the underlying buffer instead which would only be an issue if the indices are non-contiguous (triggering scalar indexing). This is an outstanding issue for plain CuArrays as well.

My use-case is that I have data streaming in as CuArrays and I need to store these in a multi-dimensional circular buffer for use later. I am taking a GPU-compatible view of the multi-dimensional circular buffer later in the code and doing some computation with it. Not having a CuArray as the underlying buffer means copying data back and forth from the GPU, and my reason for using this package over DataStuctures is that it minimizes the allocations/copies in my use case (both for CPU and GPU).

@findmyway
Copy link
Member

For element-wise operations, broadcasting is the recommended solution for GPUs. In this case, we will need to take on a CUDA.jl dep to specify the broadcasting style. The code is a one-liner, but I'll leave it to you whether the dep is worth it. I don't think this would be a common use case, and a user can always index the full array.

👍

@findmyway findmyway merged commit 3622bde into JuliaReinforcementLearning:master Mar 4, 2022
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.

Allow GPU support?
2 participants