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

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

Merged
merged 2 commits into from
Apr 12, 2023

Conversation

regro-cf-autotick-bot
Copy link
Contributor

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

Checklist before merging this PR:

  • Dependencies have been updated if changed: see upstream
  • Tests have passed
  • Updated license if changed and license_file is packaged

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 @conda-forge-admin,please 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 @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
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 (recipe) and found it was in an excellent condition.

@github-actions
Copy link
Contributor

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

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: failed

Thus the PR was not passing and not merged.

@jakirkham
Copy link
Member

@leofang, do you know what the conclusion was with issue ( #78 )? See this came up in comment ( #79 (review) )

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

@leofang
Copy link
Member

leofang commented Oct 25, 2022

Hi @jakirkham I believe @cliffwoolley made a patch that's included since NCCL v2.15.1-1: NVIDIA/nccl@78313a6. 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
Copy link
Member

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

@kkraus14
Copy link
Contributor

@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
Copy link
Member

leofang commented Oct 28, 2022

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

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
Copy link

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

Static linking in package management has several downsides:

  • Larger binaries with duplicated material
  • Less visibility as to what is included in those binaries
  • More cumbersome process of getting bug fixes/features
  • Potential complications around licensing

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

leofang commented Nov 10, 2022

John, what do you propose to fix #78?

@leofang
Copy link
Member

leofang commented Nov 10, 2022

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

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 ( conda-forge/staged-recipes#21723 (comment) )

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

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

@leofang
Copy link
Member

leofang commented Apr 12, 2023

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

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

@leofang
Copy link
Member

leofang commented Apr 12, 2023

Merge and retry on main.

@leofang leofang merged commit 8c3a6ad into conda-forge:main Apr 12, 2023
@regro-cf-autotick-bot regro-cf-autotick-bot deleted the 2.15.5-1_h13bbec branch April 12, 2023 20:11
@jakirkham
Copy link
Member

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

@leofang
Copy link
Member

leofang commented Apr 14, 2023

All 11 packages are up on anaconda.org.

@leofang leofang mentioned this pull request Oct 18, 2024
5 tasks
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.

6 participants