Repository navigation
Add support for NCCL - #195
Merged
Merged
Conversation
msimberg
commented
Dec 22, 2025
msimberg
commented
Dec 22, 2025
msimberg
commented
Dec 22, 2025
msimberg
commented
Dec 22, 2025
msimberg
commented
Dec 22, 2025
msimberg
commented
Dec 22, 2025
msimberg
commented
Dec 22, 2025
msimberg
commented
Jun 29, 2026
msimberg
commented
Jun 29, 2026
…cture - Split pack() into pack() + post_sends() for communication_object_ipr; exchange() now does pack; start_group; post_recvs; post_sends; end_group (single path, no backend dispatch, no unpack since ipr receives in-place) - communication_object: remove debugging-leftover dual-path dispatch; all exchange methods use the single pack/group/post/unpack path; delete pack_and_send() methods - Re-enable the in_place_receive test (pre-existing segfault fixed by corrected pack/send ordering relative to the group)
Wrap the in_place_receive test in a try/catch block matching the pattern used by other tests (data_descriptor, etc.) to handle the expected 'NCCL not supported with thread_safe = true' exception on the NCCL backend. Without this, test_parallel_2_nccl fails because test_in_place_receive_threads runs with thread_safe=true on NCCL.
Pre-existing UCX failure on CSCS CI GPU nodes: UCX CUDA IPC triggers cuDeviceGet failure on no-GPU builds, causing 'recv message truncated'. Passes locally with MPI and UCX, and on CSCS with MPI/NCCL. The 2-rank path (test_in_place_receive_threads) remains enabled to validate the NCCL thread_safe exception handling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
include/ghex/device/cuda/event.hpp:35
m_recordedis read byis_ready()but never initialized in any constructor, which is undefined behavior (it can randomly appear "recorded"). Initialize it tofalsewhen creating the event sois_ready()is reliable before the firstrecord()call.
cuda_event()
: cuda_event(cudaEventDisableTiming)
{
}
explicit cuda_event(unsigned int flags) {
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
msimberg
changed the title
communication_object
msimberg
marked this pull request as ready for review
July 1, 2026 13:32
Collaborator
Author
|
This caveat applies here as well: ghex-org/oomph#55 (comment). I will nevertheless merge this as "experimental support". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This requires ghex-org/oomph#55.
Updates
communication_object{,_ipr}to use thestart_group/end_groupfunctionality from oomph/NCCL, as well as takingis_stream_awareinto account.Also does a minor refactoring of packer and communication object helper functions so that the different stages are a bit easier to follow: