Skip to content

v5.0.x: mpi4py: Run accept/connect tests unconditionally #12486

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

Merged

Conversation

jsquyres
Copy link
Member

Signed-off-by: Lisandro Dalcin dalcinl@gmail.com
(cherry picked from commit 1d29af4)

This is a v5.0.x PR corresponding to main PR #12477

FYI @dalcinl

Signed-off-by: Lisandro Dalcin <dalcinl@gmail.com>
(cherry picked from commit 1d29af4)
@jsquyres jsquyres requested review from hppritcha and wenduwan April 22, 2024 18:56
@jsquyres jsquyres changed the title mpi4py: Run accept/connect tests unconditionally v5.0.x: mpi4py: Run accept/connect tests unconditionally Apr 22, 2024
@github-actions github-actions bot added this to the v5.0.4 milestone Apr 22, 2024
@wenduwan
Copy link
Contributor

Thanks @jsquyres But could you remind me which change fixed this test on v5.0.x?

@dalcinl
Copy link
Contributor

dalcinl commented Apr 22, 2024

@wenduwan I think there is no matching change in v5.0.x, simply because the issue never hit that branch? @hppritcha is this correct? I'm talking about #12465.

@wenduwan
Copy link
Contributor

I think the root cause is also present on v5.0.x but not revealed. Perhaps we should backport the fix more than else.

@dalcinl
Copy link
Contributor

dalcinl commented Apr 22, 2024

Just In case, I'm manually running this mpi4py branch against current ompi at v5.0.x.
https://github.com/mpi4py/mpi4py-testing/actions/runs/8790349763/job/24122172405
If all goes well, I will merge the mpi4py branch. Then you can re-run the mpi4py workflow here.

@dalcinl
Copy link
Contributor

dalcinl commented Apr 22, 2024

I think the root cause is also present on v5.0.x but not revealed. Perhaps we should backport the fix more than else.

You may be definitely right. @hppritcha, your input is much needed here.

@hppritcha
Copy link
Member

@dalcinl are you referring to the cid fix in #12465 ?

I could cherry-pick that (and #12474 ) back to v5.0.x if you think it helps. I thought it wasn't necessary yet as the commit which revealed this problem - 23df181 - hasn't been cherry-picked back to v5.0.x.

@dalcinl
Copy link
Contributor

dalcinl commented Apr 23, 2024

@dalcinl are you referring to the cid fix in #12465 ?

Yes.

I thought it wasn't necessary yet as the commit which revealed this problem - 23df181 - hasn't been cherry-picked back to v5.0.x.

IMHO, uses of stack variables like the one fixed by 23df181 are really hideous bugs that stay dormant until they trigger a disaster (hard to debug memory corruption). Therefore, that fix should already have gone to v5.0.x! Then your work from #12465 would be needed. Would it be to hard to backport everything related to v5.0.x in one shot?

@hppritcha
Copy link
Member

I'll open an omnibus pr that cherry-picks these commits over in one step.

@hppritcha
Copy link
Member

well this is kind of a mess. looks like there were a number of preceeding commits that touched ompi/communicator/comm_cid.c which haven't been ported back to 5.0.x yet.

@wenduwan
Copy link
Contributor

@hppritcha Good point. I don't think those commits need backport - is it possible to make the change without cherrypick?

@hppritcha
Copy link
Member

if we don't need 23df181 then i can manually patch for #12465.
the former is tied up with a series of commits to support disjoint proc feature.

@wenduwan
Copy link
Contributor

That's right we don't need the disjoint flags. Thank you!

@janjust janjust merged commit 4d2a635 into open-mpi:v5.0.x Apr 24, 2024
@jsquyres jsquyres deleted the pr/v5.0.x/mpi4py-remove-some-restrictions branch September 2, 2024 19:22
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.

None yet

5 participants