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

RFC: Use compiler('cuda') #121

Merged
merged 38 commits into from
Jan 10, 2024

Conversation

conda-forge-admin
Copy link
Contributor

@conda-forge-admin conda-forge-admin commented Mar 28, 2023

Update recipe to use compiler('cuda') to configure CUDA builds. Adds content needed to conda-build-config.yaml as pulled from conda-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.

  • Bump the build number if needed.

Fixes #120

@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.

conda-forge-webservices[bot] and others added 3 commits March 28, 2023 10:17
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`.
@jakirkham
Copy link
Member

@conda-forge-admin, please re-render.

conda-forge-webservices[bot] and others added 2 commits March 28, 2023 10:24
Since there is no `ucx` for `osx`, only require it on `linux`.
@jakirkham jakirkham marked this pull request as draft March 28, 2023 10:29
@jakirkham
Copy link
Member

@conda-forge-admin, please re-render.

conda-forge-webservices[bot] and others added 3 commits March 28, 2023 10:35
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.
@jakirkham
Copy link
Member

@conda-forge-admin, please re-render.

conda-forge-webservices[bot] and others added 3 commits March 28, 2023 10:56
Restores the behavior of `cudatoolkit` being optional when using the
`cuda` compiler.
@jakirkham
Copy link
Member

@conda-forge-admin, please re-render.

@github-actions
Copy link
Contributor

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.

conda-forge-webservices[bot] and others added 2 commits January 6, 2024 07:30
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.
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

conda-forge-webservices[bot] and others added 3 commits January 6, 2024 07:41
This wouldn't be added as `run_exports_from` `{{ compiler('cuda') }}`
are ignored. So add it manually.
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.

Have commented on a few items of note below to provide any clarifications that might be useful

Comment on lines +36 to +37
ignore_run_exports_from:
- {{ compiler('cuda') }} # [cuda_compiler != "None"]
Copy link
Member

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

Comment on lines 1 to +4
c_compiler:
- gcc
c_compiler_version:
- '12'
- '11'
Copy link
Member

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

Comment on lines 5 to +6
cdt_name:
- cos6
- cos7
Copy link
Member

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

Comment on lines -11 to +14
cuda_version:
- '11.2'
cuda_compiler:
- nvcc
cuda_compiler_version:
- '11.8'
Copy link
Member

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

Comment on lines -4 to -7
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]
Copy link
Member

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

Comment on lines -14 to -17
cuda_version: # [linux]
- 11.2 # [linux64]
- 11.2 # [ppc64le]
- 11.2 # [aarch64]
Copy link
Member

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

Comment on lines -8 to -10
enable_ucx:
- True # [linux]
- False # [not linux]
Copy link
Member

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

Comment on lines -11 to -13
enable_cuda:
- True # [linux]
- False # [not linux]
Copy link
Member

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] %}
Copy link
Member

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"]
Copy link
Member

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)

@jakirkham jakirkham changed the title WIP, RFC: Use compiler('cuda') RFC: Use compiler('cuda') Jan 6, 2024
@jakirkham jakirkham marked this pull request as ready for review January 6, 2024 08:18
@jakirkham
Copy link
Member

It looks like we are using cross-compilation for linux_aarch64 and emulation for linux_ppc64le. Is that expected?

build_platform:
linux_aarch64: linux_64
osx_arm64: osx_64

provider:
linux_aarch64: default
linux_ppc64le: azure

Copy link
Member

@leofang leofang left a 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.

recipe/meta.yaml Outdated Show resolved Hide resolved
@leofang
Copy link
Member

leofang commented Jan 8, 2024

It looks like we are using cross-compilation for linux_aarch64 and emulation for linux_ppc64le. Is that expected?

build_platform:
linux_aarch64: linux_64
osx_arm64: osx_64

It is surprising. It seems the migration bot added the cross compiling line above in commit 2783362. Not sure why it'd do that?

@leofang
Copy link
Member

leofang commented Jan 8, 2024

It looks like we are using cross-compilation for linux_aarch64 and emulation for linux_ppc64le. Is that expected?

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 someone somehow switching to the CUDA compiler triggers this issue (that should have surfaced earlier). Let me force using QEMU and retry.

@leofang
Copy link
Member

leofang commented Jan 8, 2024

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits January 8, 2024 00:40
@jakirkham
Copy link
Member

Thanks Leo! 🙏

Figured some wires were getting crossed in linux_aarch64. Just wasn't sure which way to untangle them. Thanks for sorting that out 🙂

@jakirkham
Copy link
Member

@dalcinl what do you think about this PR?

@leofang
Copy link
Member

leofang commented Jan 10, 2024

Thanks John & Lisandro! Let's merge.

@leofang leofang merged commit bea25a2 into conda-forge:main Jan 10, 2024
9 checks passed
@jakirkham
Copy link
Member

Thank you both! 🙏

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.

@conda-forge-admin , please re-render
4 participants