NCCL2 support #105
NCCL2 support #105
Conversation
chainermn/communicators/__init__.py
Outdated
| =============== === === ======== ======================================= | ||
|
|
||
| Args: | ||
| communicator_name: The name of communicator (``naive``, ``flat``, | ||
| ``hierarchical``, ``two_dimensional``, or ``single_node``) | ||
| ``hierarchical``, ``two_dimensional``, ``nccl``, or |
There was a problem hiding this comment.
How should we treat hierarchical communicator?
It is no longer necessary if we have NCCL2...
There was a problem hiding this comment.
I think we do not need hierarchical communicator if we use NCCL 2. But it is still needed because chainermn supports NCCL 1.
| intra_size, nccl_comm_id, intra_rank) | ||
| return intra_mpi_comm, inter_mpi_comm, intra_nccl_comm | ||
| intra_size, intra_nccl_comm_id, intra_rank) | ||
| if nccl.get_version() >= 2000: |
There was a problem hiding this comment.
What happens if the user specified 'nccl' communicator but only have NCCL 1 ?
Does it detect it and raise and error appropriately?
There was a problem hiding this comment.
If 'nccl' is used with NCCL 1, an exception error occurs in the constructor in NcclCommunicator. So it is able to detect.
chainermn/communicators/__init__.py
Outdated
| @@ -17,11 +17,13 @@ def create_communicator( | |||
| two_dimensional OK Required Each node has multiple NICs or HCAs | |||
| single_node OK Required Single node with multiple GPUs | |||
| flat OK N/A | |||
| nccl OK Required N/A | |||
There was a problem hiding this comment.
Why is nccl's recommended use cases "N/A" ?
There was a problem hiding this comment.
Update: nccl is recommended when NCCL2 is available in the environment, but it's still experimental support.
chainermn/nccl/chainermn_nccl.h
Outdated
| @@ -0,0 +1,98 @@ | |||
| // This file is a stub header file of nccl for Read the Docs. | |||
There was a problem hiding this comment.
What does this line mean? I think we don't compile NCCL-related code at ReadTheDocs, so I think this line is not true.
chainermn/communicators/__init__.py
Outdated
| +---------------+---+---+--------+--------------------------------------+ | ||
| |flat | |OK | |N/A | | ||
| +---------------+---+---+--------+--------------------------------------+ | ||
| |nccl | |OK |Required|``nccl`` is recommended when NCCL2 is | |
There was a problem hiding this comment.
How about adding version specification? (e.g., Required (>= v2))
| self.gpu_buffer_b.ptr(), n_elems_total, | ||
| nccl.NCCL_FLOAT, nccl.NCCL_SUM, | ||
| stream.ptr) | ||
| stream.synchronize() |
There was a problem hiding this comment.
I think this synchronization is not necessary, as everything (including NCCL communication and following array manipulation) is done in the null stream.
| from chainermn import nccl | ||
|
|
||
|
|
||
| class NcclCommunicator(_base.CommunicatorBase): |
There was a problem hiding this comment.
We already have chainermn.nccl.NcclCommunicator. This adds new NcclCommunicator (chainermn.communicators.NcclCommunicator). I think it is quite confusing. Is it possible to change the name of either one?
|
LGTM!!!!!! |
No description provided.