Skip to content

implement MPI_Info_create_env #10727

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 21, 2022
Merged

implement MPI_Info_create_env #10727

merged 1 commit into from
Sep 21, 2022

Conversation

hppritcha
Copy link
Member

relatd to #7993

Signed-off-by: Howard Pritchard [email protected]

@hppritcha hppritcha force-pushed the info_create_env branch 2 times, most recently from 7e95bc2 to ce3e06e Compare August 30, 2022 14:36
@hppritcha hppritcha requested a review from devreal August 30, 2022 15:30
@hppritcha
Copy link
Member Author

@devreal could you review this PR when you have a chance?

@hppritcha hppritcha requested review from jjhursey and removed request for devreal September 6, 2022 15:25
@jjhursey jjhursey requested a review from devreal September 6, 2022 18:12
@hppritcha hppritcha removed the request for review from devreal September 7, 2022 14:21
@hppritcha
Copy link
Member Author

@jjhursey i think @devreal is out for a while.

devreal
devreal requested changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

Apologies, seems like my email filter alerting me to review requests failed to alert me. Just two minor points from my side.

jjhursey
jjhursey previously requested changes Sep 7, 2022
View reviewed changes
@dalcinl
Copy link
Contributor

dalcinl commented Sep 8, 2022

@hppritcha Looks like this PR breaks singleton MPI init 😞 [logs].

@hppritcha
Copy link
Member Author

@dalcinl could you point me to with mpi4py test is failing? I can't tell from the logs.

@dalcinl
Copy link
Contributor

dalcinl commented Sep 8, 2022

@dalcinl could you point me to with mpi4py test is failing? I can't tell from the logs.

Sorry, I was leaving office in a hurry and I was not clear enough. The tests had no chance to run, the failure happens way before, at MPI_Init_thread time, when Python executes the from mpi4py import MPI statement, which in turns by default calls MPI_Init_thread(NULL, NULL, required,&provided).

The following command line should be enough to reproduce:

$ python -c "from mpi4py import MPI" # note I'm not using mpiexec

but I would expect any other C program initializing MPI and run without mpiexec to fail in a similar way.

dalcinl
dalcinl reviewed Sep 8, 2022
View reviewed changes
dalcinl
dalcinl reviewed Sep 8, 2022
View reviewed changes
@dalcinl
Copy link
Contributor

dalcinl commented Sep 8, 2022

@hppritcha mpi4py tests failed again, although this time it happened in the specific test for the MPI.Info.Create_env() method. Note that this failure still happened in the singleton MPI init mode.

dalcinl
dalcinl reviewed Sep 8, 2022
View reviewed changes
dalcinl
dalcinl reviewed Sep 8, 2022
View reviewed changes
@dalcinl
Copy link
Contributor

dalcinl commented Sep 8, 2022

@hppritcha The specific test now passes.
Note: the full testsuite did do not run because your branch is missing your recent fixes related to MPI_ERR_SESSION.

@dalcinl
Copy link
Contributor

dalcinl commented Sep 8, 2022

Should this PR be labeled with "v.5.0.x"?

@hppritcha
Copy link
Member Author

no we do funny label convention. for issues we label with all branches that should get a fix, but for a pr its one to one with the target branch.

@hppritcha
Copy link
Member Author

i just pushed a rebase on top of current main

@dalcinl
Copy link
Contributor

dalcinl commented Sep 8, 2022

i just pushed a rebase on top of current main

Hooray! All green now!

jsquyres
jsquyres reviewed Sep 8, 2022
View reviewed changes
@hppritcha
Copy link
Member Author

opened an issue #10783 to better track discussion of ERRORS sections of 307 other open mpi man3 man pages in addition to the one in this PR.

@dalcinl
Copy link
Contributor

dalcinl commented Sep 14, 2022

@jsquyres Is there is anything left to be addressed from your review?

@dalcinl
Copy link
Contributor

dalcinl commented Sep 20, 2022

Gentle reminder and request to push this PR forward.

@jsquyres jsquyres dismissed jjhursey’s stale review September 20, 2022 20:25

Josh's concerns have been addressed, and he's OOO for a while. Taking the liberty of dismissing his negative review.

@jsquyres
Copy link
Member

jsquyres commented Sep 20, 2022 •
edited
Loading

@jsquyres Is there is anything left to be addressed from your review?

Just the one comment of mine that @hppritcha may have missed...? (#10727 (comment)) I've pinged him on Slack to see if we can resolve. I also pinged @devreal to re-review.

devreal
devreal approved these changes Sep 20, 2022
View reviewed changes
@github-actions
Copy link

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

00b2357: Update docs/man-openmpi/man3/MPI_Info_create_env.3...

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

@hppritcha
implement MPI_Info_create_env
7589a5b 
related to open-mpi#7993

Signed-off-by: Howard Pritchard <[email protected]>
@hppritcha hppritcha removed the request for review from jjhursey September 21, 2022 14:36
@hppritcha hppritcha merged commit 14bc2ed into open-mpi:main Sep 21, 2022
@jjhursey jjhursey mentioned this pull request Nov 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalcinl dalcinl dalcinl approved these changes

@jsquyres jsquyres jsquyres approved these changes

@devreal devreal devreal approved these changes

@jjhursey jjhursey jjhursey left review comments

Assignees
No one assigned
Labels
MPI-4.0 Target: main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hppritcha @dalcinl @jsquyres @jjhursey @devreal