Adding AdiosGlobalComm for global data exchange by hgangwar · Pull Request #54 · SCOREC/redev

hgangwar · 2025-11-13T01:53:38Z

This PR introduces a Global ADIOS communicator

Key Changes

Added a new AdiosGlobalComm::Communicator class. This global communicator is intended for exchanging global data and metadata between coupled applications.
Updated AdiosChannel::CreateComm to route communicator creation through the CommType enum which defines the Ptn and Global communication modes..
Usage example: This creates a bidirectional communicator associated with the channel.

    auto channel =
        rdv.CreateAdiosChannel(name, params, redev::TransportType::BP4);
    auto commPair =
        channel.CreateComm<redev::Real>(name, comm, redev::CommType::Global);

The existing API remains unchanged. If CommType is not provided, channel.CreateComm defaults to CommType::Ptn and behaves as before.
Added a virtual (non-pure) function redev::Communicator::SetCommParams to specify the active variable's name and size.
- The existing redev::Communicator::send() operated on a raw pointer, which prevented communicating the message size without deducing it from the in/out layout.
- A single channel using a Global communicator can now read and write multiple variables.
A testcase to test AdiosGlobalComm.

cwsmith · 2025-11-13T16:58:03Z

Please explain a bit more in the PR description or ideally, in code comments, what 'global' means and an example of how this is used.

Please also undo all the whitespace changes. It looks like redev_comm.h has many lines of just reformatting.

jacobmerson · 2025-12-07T09:05:33Z

        return CommV{impl_.template CreateComm<
            CommunicatorTypeMap<CommunicatorDataType::INT8>::type>(
-            std::move(name), comm)};
+            std::move(name), comm, std::move(ctype))};


No need to move ctype since it's an enum move will have no impact.

jacobmerson · 2026-05-01T16:34:16Z

@hgangwar is this ready for review? If so, can you make it non draft.

hgangwar · 2026-05-01T17:46:29Z

@hgangwar is this ready for review? If so, can you make it non draft.

I would like to test it with the updated PCMS before merging.

jacobmerson · 2026-07-12T17:48:00Z

@cwsmith we need this to finalize some of Harsh's timestepping stuff. Any issues with merging?

Copilot

Pull request overview

This PR adds a new “global” ADIOS2 communicator mode to enable exchanging global data/metadata across coupled applications, while preserving the existing partitioned communicator behavior as the default.

Changes:

Introduces CommType (Partitioned vs Global) and routes channel communicator creation through it.
Adds Communicator::SetCommParams and wires it through BidirectionalComm to support selecting variable name/size dynamically.
Implements AdiosGlobalComm and adds a new MPI test (test_global_comm) plus CMake wiring.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
test_global_comm.cpp	Adds a new integration test exercising the global communicator send/recv path.
redev_types.h	Adds `CommType` enum to select communicator mode.
redev_comm.h	Renames partitioned comm class and adds `AdiosGlobalComm` + `SetCommParams` API.
redev_channel.h	Threads `CommType` through the type-erased Channel communicator factory.
redev_bidirectional_comm.h	Adds a `SetCommParams` helper on the bidirectional communicator wrapper.
redev_adios_channel.h	Creates either partitioned or global communicator implementations based on `CommType`.
CMakeLists.txt	Builds and registers the new `test_global_comm` MPI test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jacobmerson · 2026-07-12T19:02:43Z

+    virtual void SetCommParams(std::string VarName, size_t msgSize ) {
+      throw std::logic_error("Communicator::SetCommParams() called — must be overridden in the derived Comm class");
+    }


This is correct, we should fix this. Is there a reason that SetCommParams cannot just be pure virtual like the other methods?

Looked more in depth. This should definitely be made pure virtual. Also, I think it would make more sense if you put the varName into the constructor which would be consistent with the PartitionedComm. Lastly, consider naming this SetOutSize, or something that is consistent with the partitioned comm.

I can make Communicator::SetCommParams() purely virtual. It provides the flexibility of reusing the bidirectional comm of multiple varibles of different size. That's why I kept VarName separate from comm name used in the constructor, and called it SetCommParams.

cwsmith

I didn't see anything obviously concerning.

If this hasn't been tested on a machine like Frontier across a couple nodes that may be a good thing to do while the code is still 'fresh'.

cwsmith · 2026-07-12T17:59:10Z

+ * across all ranks and partitions.
+ *
+ * It is primarily used for transferring global data and metadata
+ * relevant to coupled applications.


Maybe an example of what is considered 'global data/metadata' would be helpful here.

jacobmerson · 2026-07-13T22:02:13Z

@hgangwar not sure why my other comments didn't go through, but last two things...

move the variable name from SetCommParams to the constructor. This is consistent with the PartitionedComm
Rename SetCommParams to SetOutSize. This will also be more consistent with naming of the PartitionedComm

Let me know if this also seems sensible to you.

Gangwar added 5 commits November 10, 2025 21:34

AdiosComm_to_AdiosPtnComm

74b2678

WIP_global_comm

cd07262

WIP_global_comm

f6896de

WIP_comm_testing

3463800

WIP_ctest_failing

5420fa3

Gangwar added 6 commits November 17, 2025 23:59

WIP_remove_fromatting

f8cf322

WIP_GlobalComm_desc

1edd1c3

WIP_added_SetCommParams

bff8f2c

WIP_msgSize_fix

a8230a5

MPI_warning_fix

8d3689f

handle_no_CommParms

c95c9aa

jacobmerson reviewed Dec 7, 2025

View reviewed changes

jacobmerson marked this pull request as ready for review December 9, 2025 16:39

jacobmerson marked this pull request as draft December 9, 2025 16:39

Merge branch 'SCOREC:main' into separate_comm

956ac18

hgangwar added 2 commits July 12, 2026 12:36

ptn_to_partioned

9511339

rm_ctype_move

d9b7529

hgangwar marked this pull request as ready for review July 12, 2026 17:13

jacobmerson approved these changes Jul 12, 2026

View reviewed changes

jacobmerson requested a review from Copilot July 12, 2026 17:48

Copilot started reviewing on behalf of jacobmerson July 12, 2026 17:48 View session

Copilot AI reviewed Jul 12, 2026

View reviewed changes

cwsmith approved these changes Jul 12, 2026

View reviewed changes

hgangwar added 2 commits July 14, 2026 01:58

fix_deferred_comm

ac2bfb0

fix_documentation

08d6d34

hgangwar force-pushed the separate_comm branch from aa719a5 to 08d6d34 Compare July 14, 2026 06:07

Uh oh!

Conversation

hgangwar commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Key Changes

Uh oh!

cwsmith commented Nov 13, 2025

Uh oh!

Uh oh!

Uh oh!

jacobmerson Dec 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jacobmerson commented May 1, 2026

Uh oh!

hgangwar commented May 1, 2026

Uh oh!

jacobmerson commented Jul 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

jacobmerson Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

jacobmerson Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

hgangwar Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cwsmith left a comment

Choose a reason for hiding this comment

Uh oh!

cwsmith Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

jacobmerson commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hgangwar commented Nov 13, 2025 •

edited

Loading