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

Adding cuda-cccl recipe #21953

Merged
merged 24 commits into from
Apr 7, 2023
Merged

Adding cuda-cccl recipe #21953

merged 24 commits into from
Apr 7, 2023

Conversation

adibbley
Copy link
Contributor

@adibbley adibbley commented Feb 3, 2023

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-webservices
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 (recipes/cuda-cccl) and found it was in an excellent condition.

@adibbley adibbley mentioned this pull request Feb 3, 2023
49 tasks
recipes/cuda-cccl/build.sh Outdated Show resolved Hide resolved
@robertmaynard robertmaynard mentioned this pull request Feb 14, 2023
10 tasks
@adibbley adibbley marked this pull request as ready for review February 14, 2023 16:46
@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda-cccl) and found some lint.

Here's what I've got...

For recipes/cuda-cccl:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-webservices
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 (recipes/cuda-cccl) and found it was in an excellent condition.

@jakirkham jakirkham requested review from robertmaynard and kkraus14 and removed request for robertmaynard February 15, 2023 19:19
@jakirkham
Copy link
Member

@conda-forge/core @conda-forge/staged-recipes, could you please review?

As noted in comment ( #21723 (comment) ), this is needed for cuda-cudart, which is needed for cuda-nvcc

@jakirkham
Copy link
Member

cc @leofang

@kkraus14
Copy link
Contributor

Are we not splitting this up into cub, thrust, libcu++, etc. packages?

Also, what is the strategy with regards to this and the public GitHub repos for these libraries?

@jakirkham
Copy link
Member

cc @robertmaynard (who was thinking about similar questions recently)

@leofang
Copy link
Member

leofang commented Feb 15, 2023

Are we not splitting this up into cub, thrust, libcu++, etc. packages?

From what I learned from Jake we should not do this. From the package maintenance viewpoint it's also more work. We should just retire {thrust,cub}-feedstocks going forward.

@isuruf
Copy link
Member

isuruf commented Feb 15, 2023

Preferring closed source builds over open source builds is not something we want to ever encourage.

@kkraus14
Copy link
Contributor

Preferring closed source builds over open source builds is not something we want to ever encourage.

I don't think that's what Leo is saying. Jake = @jrhemstad who leads the cccl team at NVIDIA.

I believe he's saying that we shouldn't package Thrust, Cub, libcudf++, and anything else contained in cccl separately, presumably because there's plans that blur the boundaries between these libraries more than they're already blurred.

That was my first question. The question about how we're handling using the open source repo(s) vs this prepackaged release from NVIDIA is a separate question.

@jrhemstad
Copy link

jrhemstad commented Feb 16, 2023

I believe he's saying that we shouldn't package Thrust, Cub, libcudf++, and anything else contained in cccl separately, presumably because there's plans that blur the boundaries between these libraries more than they're already blurred.

Correct. This hasn't been formally announced yet, but the Thrust/CUB/libcu++ repos will be merging into a single CCCL monorepo. Our vision for the future of these libraries is that they are parts of a unified whole and shouldn't be separated.

The question about how we're handling using the open source repo(s) vs this prepackaged release from NVIDIA is a separate question.

I'm ignorant when it comes to conda things. What are the pros/cons of pulling CCCL components from the prepackaged release vs. just pulling them from GitHub? GitHub is our source of truth, so I'd be more inclined to think the conda package should come from GH, but y'all know better than me.

@leofang
Copy link
Member

leofang commented Feb 16, 2023

Thanks, Keith/Jake. Indeed, after NVIDIA/CCCL GH repo is set up we can definitely switch to use the open sourced tarballs, but right now we're not there yet, so what Alex has in this PR is just a temporary solution.

I have made no comment on the choice of close vs open sources. Seems to me like a misinterpretation?

@isuruf
Copy link
Member

isuruf commented Feb 16, 2023

Thanks for the clarification. Until NVIDIA/CCCL repo is setup, I think we should depend on thrust, cub conda packages in this package. Otherwise they will clobber each other.

@leofang
Copy link
Member

leofang commented Feb 16, 2023

Clobbering is an important concern, but I think we can set up run_constrained to avoid this. I can't comment on if it's possible (or how much work is needed) to set up Thrust/CUB/... and make an equivalent cuda-cccl package, though, so I'd leave this to others.

@jakirkham
Copy link
Member

Would suggest that we not mess with how the compiler has been packaged by pulling in components not tested or used with it. This seems to introduce unnecessary reliability risk in a key component of the cuda-nvcc toolchain

@kkraus14
Copy link
Contributor

Would suggest that we not mess with how the compiler has been packaged by pulling in components not tested or used with it. This seems to introduce unnecessary reliability risk in a key component of the cuda-nvcc toolchain

The challenge here is that in a normal system install, cccl lands in /usr/local/cuda and then if someone was to install Thrust themselves it would typically land in /usr/local, so the two wouldn't clobber.

For us in a conda environment, both cccl and Thrust land in $CONDA_PREFIX and do clobber. And then historically, Thrust, Cub, and libcu++ have had a release cycle that isn't necessarily aligned to the release cycle of CUDA and projects like RAPIDS have taken advantage of new versions that don't coincide with a CUDA release version. On the other hand, I believe cccl is tied to the CUDA release schedule, at least as of now?

Has anyone inspected the cccl package to see if it's essentially just a metapackage of Thrust, Cub, and libcu++?

Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda-cccl, recipes/cuda-cccl-impl) and found some lint.

Here's what I've got...

For recipes/cuda-cccl-impl:

  • requirements: run_constrained: cub < 0.0.0a0 should not contain a space between relational operator and the version, i.e. cub <0.0.0a0
  • requirements: run_constrained: thrust < 0.0.0a0 should not contain a space between relational operator and the version, i.e. thrust <0.0.0a0

@conda-forge-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/cuda-cccl, recipes/cuda-cccl-impl) and found some lint.

Here's what I've got...

For recipes/cuda-cccl-impl:

  • requirements: run_constrained: thrust< 0.0.0a0 must contain a space between the name and the pin, i.e. thrust <0.0.0a0

@conda-forge-webservices
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 (recipes/cuda-cccl, recipes/cuda-cccl-impl) and found it was in an excellent condition.

@kkraus14
Copy link
Contributor

kkraus14 commented Apr 5, 2023

This looks good to go from my perspective. @adibbley @bdice @leofang @jakirkham @isuruf anything else that should be addressed before we merge this?

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

@jakirkham Asked me to take a look here. My only comment is that it'd be nice to include a comment in each recipe explaining how it relates to the other recipes. This doesn't have to be detailed, but it will help us debug things in the future. Otherwise, I don't see anything obviously wrong with the recipes.

Co-authored-by: Bradley Dice <bdice@bradleydice.com>
@bdice
Copy link
Contributor

bdice commented Apr 5, 2023

@adibbley Can you copy this package description to the cuda-cccl meta.yaml? #21953 (comment)

Then let’s merge? 😉

@leofang
Copy link
Member

leofang commented Apr 5, 2023

I would suggest we merge as-is, and add the docs to conda-forge.github.io feedstock. Hiding this in a recipe comment is not very useful for conda-forge package maintainers. I can help this later.

@adibbley
Copy link
Contributor Author

adibbley commented Apr 5, 2023

I already copied it. Can't hurt to have it in the docs as well once this is merged.

@leofang
Copy link
Member

leofang commented Apr 5, 2023

Even better, thanks!

@jakirkham
Copy link
Member

Planning on merging EOD tomorrow if no comments

@jakirkham jakirkham merged commit 35caaad into conda-forge:main Apr 7, 2023
@jakirkham
Copy link
Member

Thanks all! 🙏

Let's follow up on anything else on the feedstocks

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

Successfully merging this pull request may close these issues.

9 participants