JuliaGPU / Adapt.jl

Other
86 stars 24 forks source link

Remove ndims/eltype, and simplify parent type queries. #75

Closed maleadt closed 6 months ago

maleadt commented 6 months ago

Instead of returning the raw typename, provide the full type so that callers can use type variables, if required. Also provide a version that fully unpeels the array wrapper. And remove ndims/eltype, which are queries that Base supports already.

These are breaking changes, but needed for https://github.com/JuliaGPU/CUDA.jl/issues/2191 in order to provide the full parent array's typevars (i.e., the buffer type) to the broadcast implementation.

Also fixes https://github.com/JuliaGPU/Adapt.jl/issues/72

codecov[bot] commented 6 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b50323b) 83.56% compared to head (f9eda78) 93.65%.

Files Patch % Lines
src/wrappers.jl 83.33% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #75 +/- ## =========================================== + Coverage 83.56% 93.65% +10.08% =========================================== Files 6 6 Lines 73 63 -10 =========================================== - Hits 61 59 -2 + Misses 12 4 -8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

maleadt commented 6 months ago

cc @ChrisRackauckas, I remember you being pretty upset with the previous breaking release of Adapt.jl. This time though, it should only really affect GPUArray.jl/CUDA.jl, so I expect CompatHelper to churn through these pretty quickly.

ChrisRackauckas commented 6 months ago

There's no downstream tests here 😅. @avik-pal can you run some checks? I do think this will be breaking somewhere, but it's different from the last case which cause a subset of things to be possible. This allows more information and thus I don't think that effect is possible this time around, and we just need to bugfix around.

maleadt commented 6 months ago

I did a search on JuliaHub and there's (no) public uses of these unexported, undocumented functions (well, except for GPUArrays, but that's why I've marked this a breaking release). So I'd be surprised, but it's always possible of course.

avik-pal commented 6 months ago

It came from cholesky(::Symmetric{.... CuArray}) being broken, let me trigger the NonlinearSolve CI to confirm

maleadt commented 6 months ago

That will fail, because AFAICT you're not using the updated GPUArrays/GPUArraysCore/CUDA branches. As long as you didn't explicitly call into Adapt.(ndims|eltype|parent), it shouldn't introduce failures.

avik-pal commented 6 months ago

Oh ok, yeah there were no direct calls to that, it was via cholesky