pmix3x: use PMIX_VALUE_LOAD() and PMIX_INFO_LOAD() macros

[ggouaillardet]

Refs. #10416

bot:notacherrypick

[github-actions]

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

381aabf: pmix3x: use PMIX_VALUE_LOAD() and PMIX_INFO_LOAD()...

• check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

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

[github-actions]

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

2b97485: pmix3x: use PMIX_VALUE_LOAD() and PMIX_INFO_LOAD()...

• check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

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

[github-actions]

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

51acc18: pmix3x: use PMIX_VALUE_LOAD() and PMIX_INFO_LOAD()...

• check_cherry_pick: does not include a cherry pick message (did you need to bot:notacherrypick?)

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

[jsquyres]

@ggouaillardet What's the status of this PR? It's still in "Draft" mode.

[ggouaillardet]

I did not have much time to work on it. It is not fully completed, but i think it can be used. FWIW, it avoids an immediate problem with the latest PMIx (4.2.3 IIRC) that will be fixed in 4.2.4.

…

On Wed, Mar 15, 2023, 00:00 Jeff Squyres ***@***.***> wrote: @ggouaillardet <https://github.com/ggouaillardet> What's the status of this PR? It's still in "Draft" mode. — Reply to this email directly, view it on GitHub <#11472 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABXF524I44TMIOZGX5OLAJLW4CBYJANCNFSM6AAAAAAVTGOHZI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rhc54]

Well, it won't be "fixed" in 4.2.4 as it isn't really a "problem" in 4.2.3 😄 - it has to do with the OMPI code not sticking to the official public APIs and instead using internal PMIx library functions (albeit publicly visible symbols). All I've done for 4.2.4 is to "typedef" the old symbols to their new standardized versions to help alleviate breakage.

I'm not sure I understand why all the changes were made to switch to "info_load" instead of "value_load" as the latter remains a perfectly fine, standardized way to load values. You just need to use the official macros instead of the internal functions. You are welcome to make the change if that's what you want - but it certainly isn't required.

[dvzrv]

Well, it won't be "fixed" in 4.2.4 as it isn't really a "problem" in 4.2.3 smile - it has to do with the OMPI code not sticking to the official public APIs and instead using internal PMIx library functions (albeit publicly visible symbols). All I've done for 4.2.4 is to "typedef" the old symbols to their new standardized versions to help alleviate breakage.

I'm not sure I understand why all the changes were made to switch to "info_load" instead of "value_load" as the latter remains a perfectly fine, standardized way to load values. You just need to use the official macros instead of the internal functions. You are welcome to make the change if that's what you want - but it certainly isn't required.

@rhc54 could you point out which commits you are referring to on https://github.com/openpmix/openpmix/tree/v4.2 so that we can potentially cherry-pick from that branch?

[rhc54]

@rhc54 could you point out which commits you are referring to on https://github.com/openpmix/openpmix/tree/v4.2 so that we can potentially cherry-pick from that branch?

Looks like it was 8e70e37e2

[lahwaacz]

@rhc54 I actually tested these two approaches:

1. I built openmpi-4.1.5 with the patch from this pull request applied
2. I built openpmix-4.2.4rc1 and then rebuilt openmpi-4.1.5 without the patch

But only the first one fixes the issues with MPI_Comm_split_type (see the issue #10416). In the second approach, MPI_Comm_split_type has the same issues (in my case it just deadlocks). This was on Arch Linux before @dvzrv applied the patch distro-wide.

[rhc54]

I built openpmix-4.2.4rc1 and then rebuilt openmpi-4.1.5 without the patch

Quite possible that the fix post-dates the rc1 tag. Might be worth trying it with the head of the PMIx v4.2 branch.

[lahwaacz]

Quite possible that the fix post-dates the rc1 tag. Might be worth trying it with the head of the PMIx v4.2 branch.

The pinpointed commit openpmix/openpmix@8e70e37 is older than the tag - it's 3rd line below:

$ git log --format=oneline v4.2.4rc1 | head
8e71cf0e6753192b075edffc46fe09de597dc7c1 (tag: refs/tags/v4.2.4rc1) Update branch headers for rc1
a00074c2fcf448d2bc6c61453c727f7ac94ed96d Fix fetch of globally unique keys
8e70e37e2ab973b50251bd2704214029ee7d38e4 Cleanup code style, fix bit checks, and fix backward compatibility
b3bc7ec76523a1266b2bb7407dbab02420925d34 Correct cbdata type in pmix_show_help callback function
ec2e2f8776283de5188d3f924eb43c7ec6807800 Fix some Python bindings errors
b3d578dd6c3c3eea081f5ebffa9394e0ce443de4 Complete PMIX_*_FREE() conversion.
f51d343142de3bcdd127edcf8d5fadf94b126b1c Silence couple of nit warnings
d2ea5c67cbd70926b00f6e8467dbc55e748b0a7e Improve PMIX_*_RELEASE() consistency.
22a64fe8d312d6192452e9a31df7b40126af247d Convert the majority of PMIX_*_FREE() macros.
e6328142f66fb5f2c3f8d8ff342c057d830e4f42 Plug memory leaks.

[rhc54]

Could be that there are other commits required - I honestly don't know. Try with the head of the v4.2 branch and see if the problem still exists.

[lahwaacz]

Could be that there are other commits required - I honestly don't know. Try with the head of the v4.2 branch and see if the problem still exists.

Ok, I just did that and the current head of the v4.2 branch (openpmix/openpmix@cd813ef) behaves the same as the v4.2.4rc1 tag (I get a deadlock).

[rhc54]

Then I guess you should use this patch - I have no other advice as this is an OMPI integration issue.

[jsquyres]

@ggouaillardet It looks like this patch is necessary to both fix #11729 and make Open MPI v4.1.x compatible with any PMIx >= v4.2.3.

You still have this patch marked as "Draft". What's the current status?

[rhc54]

I believe it is draft solely because I questioned whether all the changes are truly necessary - however, there is no harm done by the larger patch. This patch should be fine as-is.

[jsquyres]

I believe it is draft solely because I questioned whether all the changes are truly necessary - however, there is no harm done by the larger patch. This patch should be fine as-is.

Ok. I see @ggouaillardet's comment from 14 Mar:

I did not have much time to work on it.

It is not fully completed, but i think it can be used.

Is there more to be done here?

[jsquyres]

We have a report from #11729 (comment) that it makes his usage of Open MPI v4.1.x work with PMIx 4.2.3.

@ggouaillardet Is this PR sufficient as-is?

[jsquyres]

We had some off-PR discussion about this:

• @ggouaillardet mentioned that he wanted to take another pass through this PR and see if he missed anything.
• The PR has been shown in at least 2 cases to enable people to use OpenPMIX v4.2.3 with Open MPI v4.1.x.
• Meaning: this PR seems to fix at least some -- if not all -- of the problem. It seems safe to merge as-is.
• That being said, it might not be a bad thing for someone to take a pass through this PR and see if we missed anything.

[rhc54]

I looked it over and think it is okay. There are additional data types supported by PMIx that are not covered here, but they also are not data types used by OMPI - so no point in worrying about something you'll never see.

[jsquyres]

Thanks @rhc54!

[jsquyres]

#11472 has been merged, so I'm closing this issue. Thank you for the report!