Skip to content

Implement Connectivity.java example using Java bindings #13201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vdhyasani17
Copy link

@vdhyasani17 vdhyasani17 commented Apr 18, 2025

[WIP] This adds a Java version of the connectivity example using Open MPI's Java bindings. It demonstrates basic point-to-point communication and serves as a reference for Java-based MPI applications. The functionality will be the same as connectivity_c.c.

Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

a46c371: Create Connectivity.java and be able to compile/ru...

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

2 similar comments
Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

a46c371: Create Connectivity.java and be able to compile/ru...

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

Copy link

Hello! The Git Commit Checker CI bot found a few problems with this PR:

a46c371: Create Connectivity.java and be able to compile/ru...

  • check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

@vdhyasani17 vdhyasani17 force-pushed the java-connectivity-example branch from dad9cfe to 448388b Compare April 19, 2025 00:02
@jsquyres jsquyres marked this pull request as draft April 21, 2025 12:41
@vdhyasani17 vdhyasani17 marked this pull request as ready for review April 30, 2025 15:57
@vdhyasani17
Copy link
Author

Hi @jsquyres, can you review my PR when you have the chance? Thank you!

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure how the extra mpi.h.in / python changes came into this PR. Can you rebase to the current tip of main and avoid including Howard's commit here?

For more information see MPI-3 section 14.3.2.
* ``MPI_T_BIND_MPI_SESSION``: MPI session

For more information see MPI-4 section 14.3.2.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MPI tools chapter moved in MPI-4.

Suggested change
For more information see MPI-4 section 14.3.2.
For more information see MPI-4.1 section 15.3.2.

@@ -0,0 +1,66 @@
/*
* Test the connectivity between all processes
* WIP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove WIP.

MPI.Init(args);

/*
* MPI_COMM_WORLD is the communicator provided when MPI is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* MPI_COMM_WORLD is the communicator provided when MPI is
* MPI.COMM_WORLD is the communicator provided when MPI is

int myRank = MPI.COMM_WORLD.getRank();
int numProcesses = MPI.COMM_WORLD.getSize();
Status status;
int verbose = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a boolean instead of an integer?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Not sure why the C example uses an integer. Is that standard C convention?

String processorName = MPI.getProcessorName();

for (int i = 0; i < numProcesses; i++) {
if (myRank == i) { /* Find current process */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the Java ecosystem at all -- is it more common for single-line comments to be // instead of /* ... */?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other Java programs here that I've seen use both, but mostly use /*.

System.out.printf("Checking connection between rank %d on %s and rank %d\n", i, processorName,
j);

int[] dataBuffer = { myRank }; /*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's mixing of styles here: end-of-line comments and before-the-line comments. We should probably be consistent.

In the majority of the rest of the Open MPI code base (and in examples/connectivity.c), we use before-the-line comments. That might be a good choice here, for consistency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will fix that to strictly use before-the-line comments.

* Data buffer to be sent to next process (in this case, only send
* rank num)
*/
int[] recBuffer = new int[1]; /* Data buffer that current process receives from previous process */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In connectivity_c.c, the variables are named rank and peer. If this example is meant to be the Java version of the C example, it might be good to make them similar in spirit -- e.g., have the variable names and meanings agree.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I will rename them to rank and peer.


/* Process must send message first then wait to receive to avoid deadlock */
MPI.COMM_WORLD.send(dataBuffer, 1, MPI.INT, j, myRank);
MPI.COMM_WORLD.recv(recBuffer, 1, MPI.INT, j, j);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh. I guess we hadn't really looked closely at connectivity_c.c before, because technically this is a deadlock -- using a blocking send to send to yourself might block and might not. In this case, it (most likely) won't because the message is so short -- but it definitely could.

This example -- and the C example -- should likely be fixed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, interesting. Should I use non-blocking iSend and iRecv instead to fix this?

}
} else if (myRank > i) {
int[] dataBuffer = { myRank };
int[] recBuffer = new int[1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use a fixed array for dataBuffer and an alloc'ed buffer for recBuffer? I.e., why the difference?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a fixed array for data buffer because I thought it would be fine prepopulating the array with myRank since I knew I would be sending that, but I will keep it consistent in this next push. Also, I will be using IntBuffer instead of int[] for iSend and iRecv methods.

Add comments to explain the functionality

Signed-off-by: vdhyasani17 <[email protected]>
Fix comment semantics and structure

Add -v verbose check

Signed-off-by: vdhyasani17 <[email protected]>
@vdhyasani17 vdhyasani17 force-pushed the java-connectivity-example branch from af303de to 64badb5 Compare May 1, 2025 16:11
@vdhyasani17
Copy link
Author

Sorry, I had some issues with rebasing, so all of my commits are now underneath the comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants