conda-forge / nccl-feedstock

A conda-smithy repository for nccl.
BSD 3-Clause "New" or "Revised" License
4 stars 16 forks source link

[bot-automerge] nccl v2.15.5-1 #80

Closed regro-cf-autotick-bot closed 1 year ago

regro-cf-autotick-bot commented 2 years ago

It is very likely that the current package version for this feedstock is out of date.

Checklist before merging this PR:

Information about this PR:

  1. Feel free to push to the bot's branch to update this PR if needed.
  2. The bot will almost always only open one PR per version.
  3. The bot will stop issuing PRs if more than 3 version bump PRs generated by the bot are open. If you don't want to package a particular version please close the PR.
  4. If you want these PRs to be merged automatically, make an issue with code>@conda-forge-admin,</codeplease add bot automerge in the title and merge the resulting PR. This command will add our bot automerge feature to your feedstock.
  5. If this PR was opened in error or needs to be updated please add the bot-rerun label to this PR. The bot will close this PR and schedule another one. If you do not have permissions to add this label, you can use the phrase code>@<space/conda-forge-admin, please rerun bot in a PR comment to have the conda-forge-admin add it for you.

Closes: #79

Dependency Analysis

We couldn't run dependency analysis due to an internal error in the bot. :/ Help is very welcome!

This PR was created by the regro-cf-autotick-bot. The regro-cf-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. Feel free to drop us a line if there are any issues! This PR was generated by https://github.com/regro/autotick-bot/actions/runs/3323807049, please use this URL for debugging.

conda-forge-linter commented 2 years ago

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.

github-actions[bot] commented 2 years ago

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

Thus the PR was not passing and not merged.

jakirkham commented 2 years ago

@leofang, do you know what the conclusion was with issue ( https://github.com/conda-forge/nccl-feedstock/issues/78 )? See this came up in comment ( https://github.com/conda-forge/nccl-feedstock/pull/79#pullrequestreview-1135150564 )

Note: Stopped CI here so it doesn't automerge yet

leofang commented 2 years ago

Hi @jakirkham I believe @cliffwoolley made a patch that's included since NCCL v2.15.1-1: https://github.com/NVIDIA/nccl/commit/78313a6d216486efebdba96a3c367fe763b26d26. But the lesson here is we should statically link cudart like all other CUDA libraries to avoid issues (sorry, can't find internal guideline for this, but this is standard practice 😅). We need to change CUDARTLIB to cudart_static.

Should we make the change in #79 and let it propagate to newer versions?

jakirkham commented 2 years ago

@kkraus14 or @jaimergp, do you have thoughts on this?

kkraus14 commented 2 years ago

@kkraus14 or @jaimergp, do you have thoughts on this?

IIRC there's some implications for some things related to the CUDA context related to dynamic vs static linking. I don't remember exactly what these implications are, but maybe you could ask around a bit internally @jakirkham?

If there aren't any implications I don't see why we would differ from the standard conda-forge policy regarding static vs dynamic linking unless explicitly necessary.

leofang commented 2 years ago

sorry, can't find internal guideline for this, but this is standard practice

IIRC there's some implications for some things related to the CUDA context related to dynamic vs static linking. I don't remember exactly what these implications are, but maybe you could ask around a bit internally

@kkraus14 @jakirkham Actually there is a public reference for this: https://docs.nvidia.com/deploy/cuda-compatibility/index.html#faq

If we build our CUDA application using CUDA 11.0, can it continue to be used with newer NVIDIA drivers (e.g. CUDA 11.1/R455, 11.x etc.)? Or is it only the other way around? Drivers have always been backwards compatible with CUDA. This means that a CUDA 11.0 application will be compatible with R450 (11.0), R455 (11.1) and beyond. CUDA applications typically statically include all the libraries (for example cudart, CUDA math libraries such as cuBLAS, cuFFT) they need, so they should work on new drivers or CUDA Toolkit installations.

jakirkham commented 2 years ago

Sorry for the slow reply. Got caught up with other things.

Not following why that means we need to statically link. It just seems to suggest there are users out there that choose to (and what happens in that case), which is a different thing.

cliffwoolley commented 2 years ago

You're not required to statically link the CUDA Runtime (libcudart_static.a), but doing so has a number of advantages. Further, static linking to cudart is the default selection when nvcc is used for linking as well as the method used in all of NVIDIA's prebuilt binaries.

jakirkham commented 2 years ago

Static linking in package management has several downsides:

This is covered in more detail in CFEP 18.

As Keith noted above, unless there is a compelling reason (not seeing it yet), we should be using dynamic libraries here.

leofang commented 2 years ago

John, what do you propose to fix #78?

leofang commented 2 years ago

Also, this discussion is very odd to me. We only have this discussion as we choose to build NCCL here, instead of using the pre-built binary as done in all other NVIDIA-related feedstocks. I would prefer to pick one decision quickly and move on. There are a lot of other public challenges to discuss and pan out, and this is not on my list 😅

jakirkham commented 1 year ago

Sorry for the slow reply here

As part of the CUDA 12 bringup work recently, we decided to use cudart statically generally. Relevant discussion starts with comment ( https://github.com/conda-forge/staged-recipes/pull/21723#issuecomment-1404501984 )

Given we are generally preferring linking cudart statically, it seems reasonable to do the same here

conda-forge-webservices[bot] commented 1 year ago

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.

leofang commented 1 year ago

@conda-forge-admin, please rerender

github-actions[bot] commented 1 year ago

Hi! This is the friendly conda-forge automerge bot!

It appears that not all commits to this PR were made by the bot. Thus this PR is not being automatically merged. Please add the automerge label again (or ask a maintainer to do so) if you'd like to enable automerge again!

github-actions[bot] commented 1 year ago

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/nccl-feedstock/actions/runs/4678170947.

leofang commented 1 year ago

Merge and retry on main.

jakirkham commented 1 year ago

Awesome! Thank you Leo for shepherding all of these in ❤️

leofang commented 1 year ago

All 11 packages are up on anaconda.org.