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

Set minimum version of NCCL to 2.4.6.1 #309

Merged
merged 2 commits into from
Oct 24, 2019

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Oct 22, 2019

The max_pin of nccl is set to x already in the recipe using this run_exports line. This follows the compatibility guidance we were given in issue ( NVIDIA/nccl#218 ).

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

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

I do have some suggestions for making it better though...

For recipe:

  • license_file entry is missing, but is expected.

@jakirkham
Copy link
Member Author

Thoughts @conda-forge/core?

@@ -492,6 +492,8 @@ mumps_mpi:
- 5.2
mumps_seq:
- 5.2
nccl:
- 2.4.6.1
Copy link
Member

Choose a reason for hiding this comment

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

This must match the pinning x. For example if the pinning was x.x, then this should be 2.4. Since the pinning is x it should be 2. This is so that downstream packages can use newer features introduced in later versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Our goal here is to ensure we build against the oldest version of nccl we care to have supported by all packages using it for maximum compatibility.

If we wanted to bump this later, we can always do a migrator, which would be explicit and preferable to letting this creep forward implicitly due to when we happened to rebuild downstream dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW there are other libraries doing this currently. Please see kealib as one such example, which only requires x.x compatibility, but sets the version used in host to 1.4.10.

@mingwandroid
Copy link
Contributor

Ping @jjhelmus and @msarahan, we have our own recipe of this at the minute, not a fork.

@jakirkham
Copy link
Member Author

Yeah I pointed Jonathan to this work very early on. Here's the feedstock.

ref: https://github.com/conda-forge/nccl-feedstock

@jakirkham
Copy link
Member Author

Friendly nudge @conda-forge/core 😉

@CJ-Wright
Copy link
Member

I'm fine with this, although the "pin to minimum version" seems like a potential policy discussion.

@jakirkham
Copy link
Member Author

On the minimum pin point, please see ( #309 (comment) ) (if you haven't already).

@isuruf
Copy link
Member

isuruf commented Oct 24, 2019

the "pin to minimum version" seems like a potential policy discussion.

I agree that it's a policy discussion. There are advantages and disadvantages.

@jakirkham
Copy link
Member Author

Updated the description to note nccl's compatibility.

@isuruf
Copy link
Member

isuruf commented Oct 24, 2019

As you can see in conda-forge/staged-recipes#9959 (comment) CuPy uses new features of NCCL if compiled against new versions. This means that we have to update the NCCL version in global pinnings every time there is a NCCL pinning to make sure that CuPy has the latest features of NCCL to work with.

@jakirkham
Copy link
Member Author

Actually it is still not clear (at least not to me) what is going on there. It sounds like the concern is being raised that NCCL might be breaking API/ABI, which means we would need a tighter pin than the package has currently. So let's try to understand what is going on there first before jumping to conclusions. 🙂

@scopatz
Copy link
Member

scopatz commented Oct 24, 2019

I trust @jakirkham to keep this updated as needed. I think having a very tight pinning to get started is fine.

@jakirkham jakirkham mentioned this pull request Oct 24, 2019
5 tasks
@isuruf
Copy link
Member

isuruf commented Oct 24, 2019

FYI, this is not a tight pinning. Pin is x in run_exports.

@ocefpaf ocefpaf merged commit f21e68f into conda-forge:master Oct 24, 2019
@jakirkham jakirkham deleted the set_min_nccl branch November 1, 2019 21:12
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.

7 participants