feat(core/mpi/nccl) Add an error checking mechanism

[rbourgeois33]

Description

Improves error handling with expected.

Related:

• Error handling, how to do it? #29,
• Ensure all error codes returned by NCCL are checked #184,
• feat(nccl): add checks to all calls #272,
• feat(nccl): Add local metadata checks in nccl communications #273,
• Implement Teuchos MPI operations #10.

Technical description

This PR proposes an error checking mechanism and applies it to send/recv as well as broadcast. I will extend it to other primitives once we converged on technical choices. It is based on expected. I did not wrap Request into expected but rather added a status_ member to Request, so that

• the API does not become cumbersome with .value in the calls (send(...).value().wait()). Crucially, this PR does not breaks the current API.
• There is only one state, contained inRequest object, despite failures that can happen both at the communication creation auto req= KokkosComm::send(..), and the wait() call req.wait().

Blocking call that don't return a Request can now return an expected type (see e.g.mpi/send). Blocking calls are untouched.
Some examples (from the tests):

//Full error checking
auto request = KokkosComm::Experimental::broadcast(comm, v, root);
EXPECT_FALSE(request.has_error());
request.wait();
EXPECT_FALSE(request.has_error());

//No error checking, still possible
KokkosComm::Experimental::broadcast(comm, v, root);

//Failed comm, with KokkosComm error
auto request = KokkosComm::Experimental::broadcast(comm, v, root);
ASSERT_TRUE(request.has_error());
EXPECT_EQ(request.error_code(), KokkosComm::ErrorCode::NotSupported);
//Not a MPI/NCCL error
EXPECT_FALSE(request.backend_error_code().has_value());

//Failed comm, with backend error
#if defined(KOKKOSCOMM_ENABLE_NCCL)
  constexpr int expected_backend = ncclInvalidArgument;
#else
  constexpr int expected_backend = MPI_ERR_RANK;
#endif

//Wrong dst/src
auto request = (rank == src) ? KokkosComm::send(comm, v, 99) : KokkosComm::recv(comm, v, 99);
ASSERT_TRUE(request.has_error());
EXPECT_EQ(request.error_code(), KokkosComm::ErrorCode::BackendError);
EXPECT_EQ(request.backend_error_code(), expected_backend);

Some questions to answer together:

• Are we okay to base error checking on expected ? It seems that this was ~agreed upon (Error handling, how to do it? #29 (comment), Error handling, how to do it? #29 (reply in thread))
• Are we okay with the additional dependency tl::expected, to avoid imposing a c++23 compiler ? (as suggested Error handling, how to do it? #29 (reply in thread))
• The logic of the tests assumesKOKKOSCOMM_ABORT_ON_ERROR=OFF and only works because I set MPI_ERRORS_RETURN by hand. Should we handle all combinations ? In particular if MPI_ERRORS_ARE_FATAL is set, the code can abort even if KOKKOSCOMM_ABORT_ON_ERROR=OFF*. Should there be constraints between the two ?
• Right now, the code can deadlock if one rank fails, and the other don't. I think we do want a KokkosComm::Abort, but I am not sure how to implement it. (Error handling, how to do it? #29 (comment), Implement Teuchos MPI operations #10)
• Should blocking calls ultimately be deleted since they are not used by the core api ?

TODO:

• Separate CUDA errors from nccl errors.

Changes

• Affected areas: core + all backends
• Breaking change(s)? no

N.B. the following list is AI generated

• Add Error, ErrorCode and status_type (tl::expected<void, Error>) in error.hpp
• Add tl-expected dependency (CMake + package config)
• Add KC_MPI_* / KC_NCCL_* / KC_CUDA_CHECK_REQ check macros that return a failed Request instead of aborting
• KOKKOSCOMM_ABORT_ON_ERROR makes all check macros abort instead of returning
• Request: store error status, add failed(), has_error(), error_code(), backend_error_code()
• Request::wait(): record backend errors instead of aborting, skip callbacks on error
• All non-blocking primitives return a failed Request on error:
  • MPI: isend, irecv, ibroadcast, iallgather, iallreduce, ialltoall, ireduce
  • NCCL: send, recv, broadcast, allgather, allreduce, alltoall (incl. pre-2.28 grouped path), reduce
• Report non-contiguous views as NotSupported instead of aborting
• Blocking mpi:: functions, Channel, test, wait_all, wait_any are unchanged (not used by the core API)
• Move fail_if to mpi::deprecated / nccl::deprecated, only kept for the unchanged code above
• Remove the old KC_CUDA_CHECK / KC_NCCL_CHECK library macros
• Use ScopedRegion for profiling regions
• Tests: MPI_ERRORS_RETURN in test_main, error checks in send/recv and broadcast tests
• Tests: add invalid-peer, CUDA-error-during-wait and non-contiguous NotSupported tests (broadcast, allgather, allreduce, alltoall)
• Doxygen for new types and methods

Checklist

• Tests are up-to-date
• Documentation is up-to-date

[cedricchevalier19]

Some questions to answer together:

Are we okay to base error checking on expected ? It seems that this was ~agreed upon (#29 (comment),
#29 (reply in thread))

It is my preference too.

Are we okay with the additional dependency tl::expected, to avoid imposing a c++23 compiler ? (as suggested
#29 (reply in thread))

I am ok, but it will need some thoughts about

The logic of the tests assumes KOKKOSCOMM_ABORT_ON_ERROR=OFF and only works because I set MPI_ERRORS_RETURN by hand. Should we handle all combinations ? In particular if MPI_ERRORS_ARE_FATAL is set, the code can abort even if KOKKOSCOMM_ABORT_ON_ERROR=ON. Should there be constraints between the two ?

Do you mean KOKKOSCOMM_ABORT_ON_ERROR=OFF? I think this should be only for testing. and I do not think we should play with MPI environment variables. Nor intercepting signals etc.

Right now, the code can deadlock if one rank fails, and the other don't. I think we do want a KokkosComm::Abort, but I am not sure how to implement it. (#29 (comment), #10)

This is the really hard thing with distributed computing. I think it is good to focus on local error handling but we have to be contious that we can deadlock or fail.

[rbourgeois33]

#184 (comment)

AFAIK, NCCL/RCCL communication primitives return ncclSuccess when they've been correctly submitted to the stream. Since we only use NCCL communicators in blocking mode (ncclConfig_t.blocking = 1, the default), we should never observe a ncclInProgress when checking the return code of communication primitives.

@dssgabriel, is that enforced somewhere ?

[rbourgeois33]

Added stronger error checking on the nccl backend:

Core idea: MpiError, NcclError and CudaError are now separate cases.

If we catch a cuda error, we return an unexpected CudaError, same for nccl.

In the case where the nccl error is ncclUnhandledCudaErrorwe also peak and print to screen the last CUDA error to help debugging.

In wait(), we now follow the spin-check logic of following https://docs.nvidia.com/deeplearning/nccl/user-guide/docs/usage/communicators.html#asynchronous-errors-and-error-handling.

Which requires to have access to the nccl communicator, now a member of nccl Requests

[rbourgeois33]

By choosing the default move ctor for nccl Request we implicitly rely on NRVO. This bit me in this PR since the new early return path disabled NRVO. I don't think we should.

As a result Request is moved instead of constructed in place, and request_ (a r-value) is copied and the event potentially double-destructed:

  ~Request() noexcept {
    if (request_ != nullptr) {
      KC_CUDA_CHECK(cudaEventDestroy(request_));
    }

To fix it, I rewrote the move ctor:

  Request(Request&& other)
      : request_(std::move(other.request_)),
        callbacks_(std::move(other.callbacks_)),
        status_(std::move(other.status_)),
        comm_(std::move(other.comm_)) {
    other.request_ = nullptr;
  };

[dssgabriel]

By choosing the default move ctor for nccl Request we implicitly rely on NRVO. This bit me in this PR since the new early return path disabled NRVO. I don't think we should.

@rbourgeois33, is this what I reported in #264? I am planning on pushing the code fixing that over the weekend 👍