Skip to content

add KA.get_backend(dev) #2779

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

Closed
wants to merge 1 commit into from

Conversation

michel2323
Copy link
Contributor

@vchuravy This is really a small PR. It's the only API that was missing to get this CliMA/Oceananigans.jl#4499 to work. Maybe not worthwhile?

@@ -27,6 +27,7 @@ KA.ones(::CUDABackend, ::Type{T}, dims::Tuple) where T = CUDA.ones(T, dims)

KA.get_backend(::CuArray) = CUDABackend()
KA.get_backend(::CUSPARSE.AbstractCuSparseArray) = CUDABackend()
KA.get_backend(::CuDevice) = CUDABackend()
Copy link
Member

Choose a reason for hiding this comment

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

Eh that is a surprising need. How does one obtain a CuDevice only using KA primitives?
That's why KA.device returns an ordinal and not a complex object.

Copy link
Contributor Author

@michel2323 michel2323 May 12, 2025

Choose a reason for hiding this comment

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

This makes me think about the ordinality of KA.device. Oceananigans has functions that are only called with devices() as an argument or dispatch on the device type. No data, no backend. In addition, they implement this API:

const GPUVar = Union{CuArray, CuContext, CuPtr, Ptr}
@inline UT.sync_device!(::CuDevice)  = UT.sync_device!(CUDABackend())
@inline UT.getdevice(cu::GPUVar, i)     = CUDA.device(cu)
@inline UT.getdevice(cu::GPUVar)        = CUDA..device(cu)
@inline UT.switch_device!(dev::CuDevice)            = CUDA.device!(dev)

Now, the solution would be to add the backend as an argument. However, CUDA provides device(::CuPtr) etc, that they use and might useful. Add a get_backend(::CuPtr) for those? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curiously, CUDA implements CUDA.device(::Ptr).

Copy link
Member

Choose a reason for hiding this comment

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

I am ok, with get_backend(ptr) that extends well to other back ends (which is always the Crux).

device(data) is an odd and increasingly ill formed query. (An array can be spread across multiple devices) So I would look very closely at those usages and how they generalize.

Copy link
Member

Choose a reason for hiding this comment

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

CUDA implements CUDA.device(::Ptr).

FWIW, that (and CUDA.device(::CuPtr)) are fairly specific, almost internal APIs. Not exported from CUDAdrv, but because of historical reasons it aliases with the exported device function that's much more commonly used. I may deprecate it at some point and replace it with a properties dictionary.

All not too relevant here, I just want to say that I wouldn't let it drive any decision.

Copy link

codecov bot commented May 12, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.71%. Comparing base (64f4a23) to head (1d77c90).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/CUDAKernels.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2779   +/-   ##
=======================================
  Coverage   89.71%   89.71%           
=======================================
  Files         153      153           
  Lines       13227    13228    +1     
=======================================
+ Hits        11867    11868    +1     
  Misses       1360     1360           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michel2323
Copy link
Contributor Author

Thanks for your input! Makes sense. I'm closing this. I changed the code in Oceananigans to work with the current API. The Ptr issue is something to keep in mind.

@michel2323 michel2323 closed this May 16, 2025
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.

3 participants