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.

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

add CUDA extension #2268

Merged
merged 6 commits into from
Jul 9, 2023
Merged

add CUDA extension #2268

merged 6 commits into from
Jul 9, 2023

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented Jun 16, 2023

Building on FluxML/NNlib.jl#492, this removes CUDA as a dependency and implements option (1) in #2265, meaning that the CUDA functionality is now unlocked with the explicit double import using CUDA, cuDNN in users' code.

Other notes:

  • Minimum supported julia version becomes 1.9
  • This is a breaking change
  • Moving to option (2) in how to make CUDA functionalities an extension #2265 requires minimal changes to this PR and the creation of a FluxCUDA package.
  • I didn't add/remove/change any test, just moved them around
  • I'm closely following the structure of the AMDGPU extension

@CarloLucibello CarloLucibello added this to the v0.14 milestone Jun 16, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2023

Codecov Report

Patch coverage: 18.00% and project coverage change: -1.86 ⚠️

Comparison is base (24b1eb2) 80.87% compared to head (dc9e252) 79.02%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2268      +/-   ##
==========================================
- Coverage   80.87%   79.02%   -1.86%     
==========================================
  Files          29       31       +2     
  Lines        1710     1721      +11     
==========================================
- Hits         1383     1360      -23     
- Misses        327      361      +34     
Impacted Files Coverage Δ
ext/FluxAMDGPUExt/FluxAMDGPUExt.jl 0.00% <0.00%> (ø)
ext/FluxAMDGPUExt/batchnorm.jl 0.00% <ø> (ø)
ext/FluxAMDGPUExt/conv.jl 0.00% <ø> (ø)
ext/FluxAMDGPUExt/functor.jl 0.00% <0.00%> (ø)
ext/FluxCUDAExt/functor.jl 0.00% <0.00%> (ø)
ext/FluxCUDAExt/utils.jl 0.00% <0.00%> (ø)
ext/FluxCUDAcuDNNExt/FluxCUDAcuDNNExt.jl 0.00% <0.00%> (ø)
src/Flux.jl 66.66% <ø> (ø)
src/losses/Losses.jl 100.00% <ø> (ø)
src/utils.jl 97.31% <ø> (-0.02%) ⬇️
... and 2 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ToucheSir
Copy link
Member

I'm pretty ambivalent on whether 1) or 2) would be better, but 2) might let us keep compat with older Julia versions if we made it load NNlibCUDA and/or the Flux CUDA code on <1.9.

@CarloLucibello
Copy link
Member Author

I'm pretty ambivalent on whether 1) or 2) would be better, but 2) might let us keep compat with older Julia versions if we made it load NNlibCUDA and/or the Flux CUDA code on <1.9.

let's discuss this in #2265

@CarloLucibello
Copy link
Member Author

CarloLucibello commented Jul 8, 2023

@ToucheSir your suggestion works!

NEWS.md Show resolved Hide resolved
@CarloLucibello
Copy link
Member Author

This should be ready to go. After merging this I will wait a couple of days, do some local testing, then tag the v0.14 release. Cc @ToucheSir @mcabbott @darsnack

.buildkite/pipeline.yml Show resolved Hide resolved
try
Base.require(Flux, :cuDNN)
catch
@warn """Package cuDNN not found in current path.
Copy link
Member

@ToucheSir ToucheSir Jul 8, 2023

Choose a reason for hiding this comment

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

I'm seeing this warning on the GPU CI despite all the tests passing. Do you know what's going on with that?

Edit: if I print out the error from this, it's still the same not found in dependencies one :(

Copy link
Member

Choose a reason for hiding this comment

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

Ok, tried some ideas locally and this appears to work:

try
    Base.require(Main, :cuDNN)

Not sure how general this is, but I haven't encountered any issues yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

This works!

@CarloLucibello
Copy link
Member Author

@ToucheSir everything seems fine now. Should we pull the trigger?

@darsnack
Copy link
Member

darsnack commented Jul 9, 2023

No complaints from me

@CarloLucibello CarloLucibello merged commit d3a083c into master Jul 9, 2023
@IanButterworth
Copy link
Contributor

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants