Skip to content

Conversation

@boi4
Copy link

@boi4 boi4 commented Mar 17, 2023

Hello,

I wanted to write some Fortran code that uses MPI Sessions and realized that MPI_SESSION_NULL is not available in Fortran. As a consequence, I added support myself by adding the constant to the Fortran header generator + implementing comparison functions for type(MPI_Session) and by making MPI_Session_finalize return an MPI_SESSION_NULL handle on return.

Here are is a code pieces that wouldn't compile (because MPI_SESSION_NULL was not defined), but works now:

program main
  use mpi
  implicit none

  integer :: ierror
  integer :: session

  call mpi_session_init(MPI_INFO_NULL, MPI_ERRORS_ARE_FATAL, session, ierror)
  if (ierror /=0) stop 'mpi session init fail'

  call mpi_session_finalize(session, ierror)
  if (ierror /=0) stop 'mpi session finalize fail'

  if (session /= MPI_SESSION_NULL) then
    print *, "Session handle was not set to MPI_SESSION_NULL!"
  end if

end program main

@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@github-actions
Copy link

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

c3bbab7: Add MPI_SESSION_NULL to fortran

  • 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!

@boi4 boi4 force-pushed the fix-missing-session-null branch from c3bbab7 to 3c30018 Compare March 17, 2023 00:43
@boi4
Copy link
Author

boi4 commented Mar 17, 2023

Just added the missing "signed off by" via force push

@hppritcha
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hppritcha
Copy link
Member

looks like something's borked up with mellanox ci system:

[1679155353.295017] [swx-jupiter04:24518:0]     ucp_context.c:1176 UCX  WARN  network device 'mlx5_0:1' is not available, please use one or more of: 'eth0'(tcp), 'lo'(tcp)

@yosefe
Copy link
Contributor

yosefe commented Mar 19, 2023

looks like something's borked up with mellanox ci system:

[1679155353.295017] [swx-jupiter04:24518:0]     ucp_context.c:1176 UCX  WARN  network device 'mlx5_0:1' is not available, please use one or more of: 'eth0'(tcp), 'lo'(tcp)

@artemry-nv

@hppritcha
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@artemry-nv
Copy link

Still investigating the Mellanox CI issue, looks like a CI setup misconfiguration issue after recent lab shutdown.

@artemry-nv
Copy link

We run Mellanox CI for main (failed) and v5.0.x (passed) - looks there may be a regression on main. Potentially this one may relate #11499 (CI passed)

@hppritcha
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hppritcha hppritcha self-requested a review March 21, 2023 15:37
@boi4
Copy link
Author

boi4 commented Mar 21, 2023

Should I rebase my commits onto main once the regression is fixed?

@hppritcha
Copy link
Member

yes please do.

@jsquyres
Copy link
Member

@boi4 The regression was fixed a few days ago; can you rebase this PR? Thanks!

Comment on lines 84 to 89
/* This value comes from the MPI_SESSION_NULL value in mpif.h. Do not
change without consulting mpif.h! */

if (MPI_SUCCESS == c_ierr) {
*session = 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the indenting here? We use 4-space tabs in Open MPI. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I don't know how that happened. It's fixed now, thanks for noticing!

@github-actions
Copy link

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

8f82e84: Fix invalid indentation in session_finalize_f.c

  • 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!

@boi4 boi4 force-pushed the fix-missing-session-null branch from 8f82e84 to c296496 Compare March 23, 2023 17:55
@jsquyres
Copy link
Member

Somehow you ended up with a bunch of extra commits on this PR. Can you reduce it back down to just the one commit?

@boi4 boi4 force-pushed the fix-missing-session-null branch from c296496 to a692e78 Compare March 23, 2023 18:28
@boi4
Copy link
Author

boi4 commented Mar 23, 2023

Sorry, I am not sure how to do this. I just moved the two commits (feature+whitespace fix) to the end of my branch, hoping that github will detect that the other commits align with the main branch.
I will try to find a fix for this.

@boi4 boi4 force-pushed the fix-missing-session-null branch 2 times, most recently from 67c46fd to 5ea77f8 Compare March 23, 2023 18:44
@boi4
Copy link
Author

boi4 commented Mar 23, 2023

Ok. I believe I have fixed it now. Sorry for the confusion.

@boi4 boi4 force-pushed the fix-missing-session-null branch from 5ea77f8 to 0841305 Compare March 23, 2023 18:46
@jsquyres
Copy link
Member

@boi4 Thank you! Sorry to be picky, but could you squash those 2 commits down into 1 commit?

Signed-off-by: Jan Fecht <mail@fecht.cc>
@boi4 boi4 force-pushed the fix-missing-session-null branch from 0841305 to 7314f4e Compare March 24, 2023 00:04
@boi4
Copy link
Author

boi4 commented Mar 24, 2023

@boi4 Thank you! Sorry to be picky, but could you squash those 2 commits down into 1 commit?

Done

@jsquyres
Copy link
Member

Done

Awesome; thanks so much.

@jsquyres jsquyres merged commit 241181b into open-mpi:main Mar 25, 2023
@jsquyres
Copy link
Member

@boi4 Could you cherry pick this to the v5.0.x branch?

@boi4
Copy link
Author

boi4 commented Mar 25, 2023

Sure, I have opened a new PR: #11533

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.

6 participants