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.

Sign up for GitHub

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

Jump to bottom

Add the CUDA compiler #285

Merged
merged 2 commits into from
Oct 11, 2019
Merged

Add the CUDA compiler #285

merged 2 commits into from
Oct 11, 2019

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Sep 27, 2019 •
edited
Loading

Note: This is still a work-in-progress and still needs some changes.

This adds the CUDA compiler to conda-forge. It works by allowing users to require the CUDA compiler during their build. When this happens in a feedstock, we leveraging the conda-forge Docker images (based off of NVIDIA's Docker images), which contain the NVCC toolchain. These Docker images are selected with a version matching that of CUDA compiler requested by the user. We set the supported CUDA versions (and Docker images that match) much like we do with Python. Additionally users can also have a CPU only build, which is useful for packages that have CPU and GPU builds.

On testing this works fine for things like the ucx build, but is still a little odd for non-GPU builds. So likely needs some work to preserve the same behavior for existing feedstocks.

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Fixes #237

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • license_file entry is missing, but is expected.

isuruf
isuruf reviewed Sep 27, 2019
View reviewed changes
recipe/conda_build_config.yaml Outdated
@@ -110,6 +119,9 @@ channel_targets:

docker_image: # [linux]
- condaforge/linux-anvil-comp7 # [linux64]
- condaforge/linux-anvil-cuda:9.2 # [linux64]
- condaforge/linux-anvil-cuda:10.0 # [linux64]
- condaforge/linux-anvil-cuda:10.1 # [linux64]
Copy link
Member

Choose a reason for hiding this comment

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

Which one of these is used when cuda is not used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be condaforge/linux-anvil-comp7. Though probably more tweaking is needed to get that to work correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Can you try rerendering an existing recipe without cuda to see what happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have previously and recall there were issues, but I don't recall what they were. Are you interested in knowing exactly how it fails? If so, I can put together an example.

Copy link
Member

Choose a reason for hiding this comment

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

Are you interested in knowing exactly how it fails?

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Should add something is sorting docker_image's values. I'm not sure if it is conda-smithy or conda-build.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's https://github.com/conda-forge/conda-smithy/blob/6b31b680b3d35b5b164a8ea619459782d004c126/conda_smithy/configure_feedstock.py#L114

Sorting was added there to have stable rerenders.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's what I figured. Was playing with that.

Maybe we can enforce an explicit sorting order here? Though am also ok with the conda-smithy PR above (if you are too).

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with the conda-smithy PR, but does that work? From your previous comment, I thought it didn't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does work. We might want to come up with something more robust in the long run, but it's ok for now.

@jakirkham jakirkham force-pushed the add_cuda_compiler branch 4 times, most recently from 0481f34 to 38628da Compare October 10, 2019 17:50
@jakirkham jakirkham mentioned this pull request Oct 10, 2019
Closed
5 tasks
@jakirkham jakirkham mentioned this pull request Oct 10, 2019
Merged
1 task
@jakirkham jakirkham marked this pull request as ready for review October 11, 2019 04:07
@jakirkham
Copy link
Member Author

Now that PR ( conda-forge/conda-smithy#1176 ) has landed, am marking this as ready for review. Though it is still waiting for a conda-smithy release ( conda-forge/conda-smithy#1177 ). We also need to bump the version here.

@CJ-Wright
Copy link
Member

Do you want to do a migration for this? If so how would you like to do it?

@jakirkham
Copy link
Member Author

There’s not much stuff that would need to change. So it’s probably fine to do it manually.

@jakirkham jakirkham changed the title WIP: Add the CUDA compiler Add the CUDA compiler Oct 11, 2019
@jakirkham
Copy link
Member Author

@conda-forge/core, this is ready to go! 😄

isuruf
isuruf reviewed Oct 11, 2019
View reviewed changes
recipe/conda_build_config.yaml Outdated
@@ -109,6 +121,9 @@ zip_keys:
- vc # [win]
- c_compiler # [win]
- cxx_compiler # [win]
-
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the selector here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Yep have added it. Thanks.

@jakirkham
Copy link
Member Author

Anything else? Or is this good to go?

@ocefpaf ocefpaf merged commit 885cccb into conda-forge:master Oct 11, 2019
@jakirkham jakirkham deleted the add_cuda_compiler branch October 11, 2019 18:19
This was referenced Oct 11, 2019
Merged
Merged
@jakirkham
Copy link
Member Author

Thanks all! 😄

@CJ-Wright
Copy link
Member

This needs to get a blog post and some social media things!

@jakirkham
Copy link
Member Author

Agreed! Let me run it by my team. I'm guessing they will want to get involved in review of the blogpost and amplification.

This was referenced Oct 11, 2019
Closed
Closed
@jakirkham jakirkham mentioned this pull request Oct 24, 2020
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isuruf isuruf isuruf left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Determining CUDA Docker image/compiler version
5 participants
@jakirkham @conda-forge-linter @CJ-Wright @isuruf @ocefpaf