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

Merging NCCL 2.21.5-1 #1271

Conversation

corey-derochie-amd
Copy link
Collaborator

@corey-derochie-amd corey-derochie-amd commented Jul 24, 2024

Details

Do not mention proprietary info or link to internal work items in this PR.

Work item: "Internal", or link to GitHub issue (if applicable).

What were the changes?
Merged NCCL v2.21.5-1 into RCCL develop.

Why were the changes made?
Must remain up-to-date with latest NCCL versions.

How was the outcome achieved?
Performed a git-merge, then resolved merge conflicts, then resolved compiler errors, then resolved logical conflicts due to functional changes in NCCL.

Additional Documentation:
NCCL Documentation:

  • Add support for IB SHARP 1PPN operation with user buffers. Improve support for MNNVL, add NVLS support and multi-clique support.
  • Detect the NVLS clique through NVML
  • Exchange XML between peers in the same NVLS clique and fuse XMLs before creating the topology graph.
  • Rework bootstrap allgather algorithms to allow for large allgather operations intra-node (XML exchange). Net/IB: add support for dynamic GID detection.
  • Automatically select RoCEv2/IPv4 interface by default. Allow to select IPv6 or even the network/mask. Reduce NVLS memory usage.
  • Add stepSize as property of a connection to allow for different sizes on different peers; set it to 128K for NVLink SHARP. Improve tuner loading
  • Look for more paths, be more consistent with the network device plugin.
  • Also search for tuner support inside the net plugin. Improve tuner API
  • Add context to support multi-device per process. Add magic number around comm object to detect comm corruption.
  • Add some basic check around communicators so that we can report a problem when a communicator gets corrupted or a wrong comm pointer is passed to NCCL. Fix net/IB error path. Update some files to enhance robustness. NVIDIA/nccl#1164
  • Fix collnet rail mapping with split comm.
  • Fix packet reordering issue causing bootstrap mismatch
  • Use a different tag in ncclTransportP2pSetup for the connectInfo exchange and the following barrier. Fix hang when crossNic is inconsistent between ranks. Fix minCompCap/maxCompCap computation. all_reduce_perf hangs on DGX-H100 NVIDIA/nccl#1184

Approval Checklist

Do not approve until these items are satisfied.

  • Verify the CHANGELOG has been updated, if
    • there are any NCCL API version changes,
    • any changes impact library users, and/or
    • any changes impact any other ROCm library.

Add support for IB SHARP 1PPN operation with user buffers.
Improve support for MNNVL, add NVLS support and multi-clique support.
 * Detect the NVLS clique through NVML
 * Exchange XML between peers in the same NVLS clique and fuse XMLs
   before creating the topology graph.
 * Rework bootstrap allgather algorithms to allow for large allgather
   operations intra-node (XML exchange).
Net/IB: add support for dynamic GID detection.
 * Automatically select RoCEv2/IPv4 interface by default. Allow to
   select IPv6 or even the network/mask.
Reduce NVLS memory usage.
 * Add stepSize as property of a connection to allow for different
   sizes on different peers; set it to 128K for NVLink SHARP.
Improve tuner loading
 * Look for more paths, be more consistent with the network device
   plugin.
 * Also search for tuner support inside the net plugin.
Improve tuner API
 * Add context to support multi-device per process.
Add magic number around comm object to detect comm corruption.
 * Add some basic check around communicators so that we can report a
   problem when a communicator gets corrupted or a wrong comm pointer
   is passed to NCCL.
Fix net/IB error path. Github PR ROCm#1164
Fix collnet rail mapping with split comm.
Fix packet reordering issue causing bootstrap mismatch
 * Use a different tag in ncclTransportP2pSetup for the connectInfo
   exchange and the following barrier.
Fix hang when crossNic is inconsistent between ranks.
Fix minCompCap/maxCompCap computation. Github issue ROCm#1184
@corey-derochie-amd corey-derochie-amd self-assigned this Jul 24, 2024
@corey-derochie-amd corey-derochie-amd force-pushed the feature/nccl-2.21-sync branch 2 times, most recently from ec54e8f to 059f28c Compare July 24, 2024 15:48
Comment on lines -98 to +97

#define NCCL_TOPO_MAX_LINKS 128
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RCCL expanded this from 32 to 64 for CPX mode, but NCCL expands all the way to 128, so we've taken the larger value.

src/misc/tuner.cc Outdated Show resolved Hide resolved
Comment on lines +202 to +205
int64_t netId;
if (connIndex == NCCL_CONN_IDX_P2P_NET) NCCLCHECK(ncclTopoGetIntraNetDev(comm->topo, myInfo->rank, graph, channelId, 1, &netId, &req.netDev));
if (req.netDev < 0) NCCLCHECK(ncclTopoGetNetDev(comm, myInfo->rank, graph, channelId, peerInfo->rank, &netId, &req.netDev, &proxyRank));
NCCLCHECK(ncclTopoCheckGdr(comm->topo, myInfo->busId, netId, 1, &req.useGdr));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A few places use ncclTopoGetIntraNetDev in RCCL to circumvent ncclTopoGetNetDev, so with the netId changes, this logic had to be updated as well.

@@ -511,16 +538,16 @@ ncclResult_t ncclTopoAddCpu(struct ncclXmlNode* xmlCpu, struct ncclTopoSystem* s
}
for (int s=0; s<xmlCpu->nSubs; s++) {
struct ncclXmlNode* node = xmlCpu->subs[s];
if (strcmp(node->name, "pci") == 0) NCCLCHECK(ncclTopoAddPci(node, system, cpu));
if (strcmp(node->name, "pci") == 0) NCCLCHECK(ncclTopoAddPci(node, system, cpu, systemId));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gilbertlee-amd This looks like a schema change to the XML. Do our models need to be redone? Does the Topology Visualizer need to be updated?

Comment on lines +2007 to +2011
INFO(NCCL_INIT,"ncclCommSplit comm %p rank %d nranks %d cudaDev %d busId %lx parent %p color %d key %d commId 0x%llx - Init START",
comm, comm->rank, comm->nRanks, comm->cudaDev, comm->busId, job->parent, job->color, job->key, (unsigned long long)hashUniqueId(job->commId));
} else {
INFO(NCCL_INIT,"ncclCommInitRank comm %p rank %d nranks %d cudaDev %d busId %lx commId 0x%llx - Init START",
comm, comm->rank, comm->nRanks, comm->cudaDev, comm->busId, (unsigned long long)hashUniqueId(job->commId));
Copy link
Collaborator Author

@corey-derochie-amd corey-derochie-amd Jul 24, 2024

Choose a reason for hiding this comment

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

NCCL uses nvlsDev instead of cudaDev, but RCCL contains this switch so these NCCL changes were modified to match. This pattern appears elsewhere.

Comment on lines +289 to +290
if (xml->maxIndex == xml->maxNodes) {
WARN("Error : too many XML nodes (max %d)", xml->maxNodes);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since MAX_NODES is no longer set in code, it must be added to the XML for CPX mode.

Comment on lines +202 to +203
#define NCCL_TOPO_XML_MAX_NODES 256
#define NCCL_GRAPH_XML_MAX_NODES 4096
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nusislam How do we resolve these against the CPX changes?

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

Successfully merging this pull request may close these issues.

3 participants