-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
RFC: Use compiler('cuda')
#121
RFC: Use compiler('cuda')
#121
Conversation
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 ( |
…nda-forge-pinning 2023.03.28.09.32.54
Can drop `enable_cuda` conditioning of `ucx` as the package works with or without CUDA now.
As `enable_cuda` is no longer used in `meta.yaml`, drop it from `conda_build_config.yaml`.
@conda-forge-admin, please re-render. |
…nda-forge-pinning 2023.03.28.09.32.54
Since there is no `ucx` for `osx`, only require it on `linux`.
@conda-forge-admin, please re-render. |
…nda-forge-pinning 2023.03.28.09.32.54
Transfer `cudatoolkit` constrain to `cuda` compiler. Also pull in associated `zip_keys` so zipping still works with these changes.
Equivalent to the previous behavior where `cuda` builds were always done on `linux`. This just skips non-`cuda` builds to restore that behavior with the `cuda` compiler.
@conda-forge-admin, please re-render. |
…nda-forge-pinning 2023.03.28.09.32.54
Restores the behavior of `cudatoolkit` being optional when using the `cuda` compiler.
@conda-forge-admin, please re-render. |
Hi! This is the friendly automated conda-forge-webservice. I tried to rerender for you, but it looks like there was nothing to do. This message was generated by GitHub actions workflow run https://github.com/conda-forge/openmpi-feedstock/actions/runs/4542296552. |
…nda-forge-pinning 2024.01.05.22.51.09
When `{{ compiler("cuda") }}` is used in a package, a matrix of CUDA versions is considered including a non-CUDA build. As a result, there is always a non-CUDA build. This means that we can just check whether or not the current build uses CUDA and select the packages to include accordingly. In other words there is no need for a separate `enable_cuda` value. So this rewrites the recipe logic to obviate and drop `enable_cuda`. Should make it easier to follow for those familiar with other CUDA recipes.
@conda-forge-admin , please re-render |
…nda-forge-pinning 2024.01.05.22.51.09
This wouldn't be added as `run_exports_from` `{{ compiler('cuda') }}` are ignored. So add it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have commented on a few items of note below to provide any clarifications that might be useful
ignore_run_exports_from: | ||
- {{ compiler('cuda') }} # [cuda_compiler != "None"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we handle our CUDA dependencies via run_constrained
, this disables any exports the compiler would have added
c_compiler: | ||
- gcc | ||
c_compiler_version: | ||
- '12' | ||
- '11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was using GCC 12 before. Am guessing that is just because 12 is the default GCC version used if not otherwise overridden
Whereas CUDA 11.8, is configured with GCC 11
Not sure whether this matters for OpenMPI in practice. So just mentioning for clarity
cdt_name: | ||
- cos6 | ||
- cos7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CUDA 11.8 Docker image is on CentOS 7. So this got bumped from CentOS 6 to 7
Also note that linux_aarch64
and linux_ppc64le
already are using CentOS 7. So this really only affects linux_64
(bringing it inline with the others)
Am guessing the long tail of CentOS 6 is diminishing. Though wouldn't be surprised if there are still some CentOS 6 users out there
Anyways just mentioning it for awareness
cuda_version: | ||
- '11.2' | ||
cuda_compiler: | ||
- nvcc | ||
cuda_compiler_version: | ||
- '11.8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of cuda_version
, we now have cuda_compiler_version
. Also moved to CUDA 11.8 as requested
Though could do 11.2 or another version just as easily. Just need to update the skip
in the recipe
docker_image: # [linux] | ||
- quay.io/condaforge/linux-anvil-cuda:11.2 # [linux64] | ||
- quay.io/condaforge/linux-anvil-ppc64le-cuda:11.2 # [ppc64le] | ||
- quay.io/condaforge/linux-anvil-aarch64-cuda:11.2 # [aarch64] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now handled for us thanks to {{ compiler("cuda") }}
and the global pinnings
cuda_version: # [linux] | ||
- 11.2 # [linux64] | ||
- 11.2 # [ppc64le] | ||
- 11.2 # [aarch64] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is superseded by cuda_compiler_version
, which is set for us
enable_ucx: | ||
- True # [linux] | ||
- False # [not linux] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inlined this as it seems to always be used with Linux. Please let me know if I've missed something here
enable_cuda: | ||
- True # [linux] | ||
- False # [not linux] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When {{ compiler("cuda") }}
is used, there are CUDA and non-CUDA builds. So we can check whether CUDA is used via other variables (like cuda_compiler_version
)
@@ -1,6 +1,7 @@ | |||
{% set version = "5.0.1" %} | |||
{% set major = version.rpartition('.')[0] %} | |||
{% set build = 0 %} | |||
{% set cuda_major = (cuda_compiler_version|default("11.8")).rpartition('.')[0] %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Short-cut determine the CUDA major version. Needed a fallback during conda-build
's rendering passes (hence default
). Though the default
is overridden in the final result
@@ -20,6 +21,8 @@ source: | |||
build: | |||
number: {{ build }} | |||
skip: true # [win] | |||
skip: true # [linux and cuda_compiler_version != "11.8"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures Linux only builds with CUDA and only used CUDA 11.8 (no other versions)
It looks like we are using cross-compilation for openmpi-feedstock/conda-forge.yml Lines 1 to 3 in c577e78
openmpi-feedstock/conda-forge.yml Lines 10 to 12 in c577e78
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, John, LGTM, let's let @dalcinl take a look too.
It is surprising. It seems the migration bot added the cross compiling line above in commit 2783362. Not sure why it'd do that? |
Did a bit more digging, it seems up to the last PR that we merged (#135) we still ran emulation for linux_aarch64. Also, I recalled I didn't quite figure out how to cross-compile Open MPI (#86), so it shouldn't be the default. However, the bot commit was merged 10 months ago, so the only difference that I can think of is |
@conda-forge-admin, please rerender |
…nda-forge-pinning 2024.01.07.10.43.57
Thanks Leo! 🙏 Figured some wires were getting crossed in |
@dalcinl what do you think about this PR? |
Thanks John & Lisandro! Let's merge. |
Thank you both! 🙏 |
Update recipe to use
compiler('cuda')
to configure CUDA builds. Adds content needed toconda-build-config.yaml
as pulled fromconda-forge-pinning
for the specific CUDA & CentOS versions used on the different architectures.Hi! This is the friendly automated conda-forge-webservice.
I've rerendered the recipe as instructed in #120.
Here's a checklist to do before merging.
Fixes #120