Conversation
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
JuliaFormatter
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 263 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 267 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 271 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 275 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 276 to 278 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 280 to 290 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 292 to 311 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 313 to 342 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 344 to 346 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 348 to 349 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 351 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 353 to 361 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 363 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 365 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 366 to 368 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 370 to 390 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 392 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Line 395 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Reactant.jl/test/integration/mpi.jl
Lines 396 to 398 in d6e4ba0
[JuliaFormatter] reported by reviewdog 🐶
Line 143 in d6e4ba0
There was a problem hiding this comment.
XLA:CUDA already ships with NCCL, why do we need to load these via an extension?
There was a problem hiding this comment.
Hm yeah I suppose we could do it that way. Would that be better?
There was a problem hiding this comment.
Yeah actually on reflection that is definitely better. I will refactor this.
There was a problem hiding this comment.
Note also that in the future, we'll probably move nccl communicator creation to enzymejax, but setting it up somewhat clunkily in reactant was what @wsmoses and I decided on for a first pass.
There was a problem hiding this comment.
@avik-pal we agreed to get this done like @romanlee to get some MPI runs working soon. I agree we should better use the XLA-built NCCL (and since we do that, we can maybe write our own custom calls directly in Enzyme-JAX), but we can fix it and improve it incrementally. That's what also the new comm dialect is about.
@romanlee I feel like this should better live in its own NCCL extension. You're using a lot of MPI to set it up, but the user is gonna know if it's going to execute on a GPU cluster with NCCL. Ultimately, the NCCL backend for MPI should be used for converting user-code, but the set up should be done with NCCL and not MPI. It's true that we are gonna use XLA-built NCCL, which can lead to some conflicts, and would like to know your opinion.
EDIT: I see that you had your code in a NCCL extension first, and I think we should discuss it.
| ) | ||
| withenv( | ||
| "XLA_REACTANT_GPU_MEM_FRACTION" => 1 / (nranks + 0.1), | ||
| "XLA_REACTANT_GPU_PREALLOCATE" => false, |
There was a problem hiding this comment.
just curious, is preallocation giving problems? also, if you don't preallocate, then does XLA_REACTANT_GPU_MEM_FRACTION does anything?
There was a problem hiding this comment.
Glad you pointed this out. This shouldn't be necessary so I'll remove it, was a hydra thing.
Incidentally, I assume MEM_FRACTION sets the budget regardless of whether it's preallocated or allocated on demand? That's what other usage would suggest, eg just above in the same file
| if ( | ||
| # MPI is only supported on CPU | ||
| (BACKEND == "cpu" || BACKEND == "auto") && ( | ||
| BACKEND in ("auto", "cpu", "cuda", "gpu") && ( |
There was a problem hiding this comment.
mmm gpu is CUDA but also ROCm, which can right now gives problems because we don't yet support RCCL (which will be trivial to support once we merge this or other features)
There was a problem hiding this comment.
Wait, thinking a bit more about this, are you sure this is right? Doesn't "gpu" sometimes mean "cuda"? How do we make sure to catch that case properly?
Also, as written I have "auto" routing to set_default_backend("cpu"), which probably isn't right either. Because "auto" defaults to "cuda" or "rocm" with higher priority than "cpu", which we probably want to respect.
If you could clarify the intended usage of "REACTANT_BACKEND_GROUP" here that would be very helpful.
There was a problem hiding this comment.
Alright, I sorted this out. See 14a4e76.
Feel free to double check my logic, but it should be all good
| function default_nccl_comm_handle() | ||
| ext = Base.get_extension(@__MODULE__, :ReactantMPIExt) | ||
| ext === nothing && error("ReactantMPIExt is not loaded; load MPI first") | ||
| return ext.default_comm_handle() | ||
| end | ||
|
|
||
| function set_nccl_device!(::Integer) | ||
| return error( | ||
| "GPU MPI requires CUDA.jl; load CUDA before initializing the NCCL communicator" | ||
| ) | ||
| end | ||
|
|
There was a problem hiding this comment.
So... you shouldn't call Base.get_extension inside regular lib code. Instead, you should declare it but left undefined, and define it in the appropriate extension.
But since whole ReactantMPIExt/Nccl.jl file doesn't depend on anything inside the ReactantMPIExt module, maybe better move to its own file inside the src/ folder?
There was a problem hiding this comment.
Ah thanks, this didn't feel quite right to me either. See 54a04fc. Re set_nccl_device!, I think it's appropriate as is. Let me know if you disagree.
And re moving Nccl.jl to src/, I think it makes sense to leave it in the ReactantMPIExt module even if it doesn't strictly need to be there, since it is always used together with the mpi extension. But if that's not the right style/you feel strongly I'm happy to move it
| if op in integer_bool_ops && !(T <: Integer || T <: Bool) | ||
| continue | ||
| end | ||
| try |
There was a problem hiding this comment.
maybe moving the try/finally to the runtests.jl file would make it more legible? wdyt?
There was a problem hiding this comment.
I don't think that's really possible bc we can't access the stuff in the finally block in runtests.jl? Anyway, that feels like it would be slightly less readable to me, so I'd prefer to keep it where it is.
That said, I did make some minor changes to make it more readable here a0ebd04
| const DEFAULT_COMM = Ref{ncclComm_t}(C_NULL) | ||
| const DEFAULT_COMM_HANDLE = Ref{UInt}(0) | ||
| const DEFAULT_XLA_DEVICE = Ref{Union{Nothing,Reactant.XLA.AbstractDevice}}(nothing) |
There was a problem hiding this comment.
mind prefixing sth like REACTANT_NCCL_ here? the reasoning being that there is overlap with MPI and both are on the same module
There was a problem hiding this comment.
if you move to its own module, that would be another solution.
There was a problem hiding this comment.
Good idea. I renamed to DEFAULT_COMM_* to NCCL_COMM_*. And DEFAULT_XLA_DEVICE got separately refactored away.
Co-authored-by: Sergio Sánchez Ramírez <mofeing+github@gmail.com>
@mofeing I actually think it's better to not have a NCCL extension for a few reasons: (1) we avoid incompatibility issues between nccl.jl's and xla's packaged nccl libs, (2) since what we're doing with nccl is relatively straightfoward (just setting up a communicator) it's relatively easy to just create the wrappers ourselves and that allows us to avoid another dependency, and (3) ultimately we'll move communicator creation out of Reactant anyway, at which point we won't need the extension, so it's fine to do something lightweight for now. Curious what @avik-pal's opinion is, but I assume based on his original comment that he was also thinking better to avoid an extension unless necessary. Re using a lot of MPI to set it up, that's just how you set up NCCL. See https://docs.nvidia.com/deeplearning/nccl/user-guide/docs/usage/communicators.html#creating-a-communicator. |
|
After thinking more about it, I decided to refactor the way the NCCL is initialized. See 15662fc. Specifically, previously we selected the NCCL device after XLA client creation. This was fragile because it left all devices addressable. This refactor adds a function |
Add the foundations for GPU MPI support (via NCCL) to ReactantMPIExt. Mainly, this amounts to adding logic and state to properly set up NCCL. This logic is concentrated in
ext/ReactantMPIExt/Nccl.jl, and specifically in the functionReactantMPIExt.initialize!(MPI.COMM_WORLD)which:We also add backend gpu tests for MPI Allreduce, which will be supported when EnzymeAD/Enzyme-JAX#2699 lands
NOTE: Ultimately we might want to move NCCL communicator creation to EnzymeJAX, but this is what we agreed on for a first pass.