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 12.0 migrator. #4400

Merged
merged 19 commits into from
May 31, 2023
Merged

Add CUDA 12.0 migrator. #4400

merged 19 commits into from
May 31, 2023

Conversation

bdice
Copy link
Contributor

@bdice bdice commented May 2, 2023

Checklist

  • Used a personal 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.

This PR adds a migrator for CUDA 12 support. The compiler package name is changing from nvcc to cuda-nvcc, so cuda_compiler must be added to zip_keys. CUDA 12 builds should use gcc/g++ version 12, and they do not need a special CUDA Docker image (the normal Docker image is sufficient).

This is my first time submitting a migrator, so please forgive any egregious mistakes! I'm starting this as a draft and getting some input before opening for review. cc: @jakirkham @leofang @adibbley @vyasr

@conda-forge-webservices
Copy link
Contributor

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.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Bradley! 🙏

Added a few suggestions below 🙂

recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

jakirkham commented May 2, 2023

Went ahead and submitted PR ( #4407 ) to add cuda_compiler to zip_keys. Think we should be able to simplify this with that change

Edit: That PR has now been merged. Have updated the branch here to include those changes

@jakirkham jakirkham mentioned this pull request May 2, 2023
5 tasks
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Included a few suggestions below to simplify things (now that some changes have been moved to conda_build_config.yaml)

recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Added use_local lines to simplify testing. Flagging them as a reminder to remove them

recipe/migrations/cuda120.yaml Outdated Show resolved Hide resolved
recipe/migrations/cuda120_ppc64le_aarch64.yaml Outdated Show resolved Hide resolved
These were pulling in CentOS 6 instead of CentOS 7 images as observed by
Leo. Include his fix for using CentOS 7 images.
@jakirkham jakirkham marked this pull request as ready for review May 31, 2023 08:47
@jakirkham jakirkham requested a review from a team as a code owner May 31, 2023 08:47
@jakirkham
Copy link
Member

Just a note, retested the latest changes here on NCCL, UCX & CuPy. They behaved as expected (equivalent to the current CUDA 12 state for each of those)

Focusing on CUDA 12.0 `x86_64` initially.
@jakirkham
Copy link
Member

Sounds like we are ok with starting this based on discussion in the core meeting. Going to go ahead and merge. Happy to follow up on anything as needed. Thanks all! 🙏

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.

5 participants