Skip to content
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

Sync with PMIx master #4469

Merged
merged 1 commit into from
Nov 8, 2017
Merged

Sync with PMIx master #4469

merged 1 commit into from
Nov 8, 2017

Conversation

rhc54
Copy link
Contributor

@rhc54 rhc54 commented Nov 8, 2017

Implement direct modex protection to turn off PMIx dstore when direct modex scenario is detected

Signed-off-by: Ralph Castain rhc@open-mpi.org

Implement direct modex protection to turn off PMIx dstore when direct modex scenario is detected

Signed-off-by: Ralph Castain <rhc@open-mpi.org>
@ompiteam-bot
Copy link

All Tests Passed!

@rhc54 rhc54 merged commit 4b5be96 into open-mpi:master Nov 8, 2017
@rhc54 rhc54 deleted the topic/pmup branch November 8, 2017 03:46
@@ -99,6 +99,11 @@ int pmix3x_client_init(opal_list_t *ilist)
ninfo = 0;
}

/* check for direct modex use-case */
if (opal_pmix_base_async_modex && !opal_pmix_collect_all_data) {
opal_setenv("PMIX_MCA_gds", "hash", true, &environ);
Copy link
Contributor

Choose a reason for hiding this comment

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

@rhc54
This line disables disables dstore. We accidentally found that.
Can you remind me why this was done?
Also I think this kind of change should come in a separate PR as it significantly changes the behavior yet quite small to find it between other 400+ changed lines.

@jladd-mlnx FYI

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhc54
I see that this PR was filed on Nov, 7.
And we had a dix for the dstore and local commits on
Dec, 20:
openpmix/openpmix#613

Am I right that it supposed to fix this issue and we can remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhc54 FYI we discovered the issue with openpmix/openpmix#613.
@karasevb is going to PR the fix today

Copy link
Member

Choose a reason for hiding this comment

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

@rhc54
Copy link
Contributor Author

rhc54 commented Mar 14, 2018

You are commenting on something committed 5 months ago? IIRC, and it has been a long time, the change you cite above was done after general consultation as a workaround to the issue you cite that was not fixed until today. I believe the change required some corresponding changes in PMIx, which is why it was brought in together.

I have no issue with removing it now that the fix is available, once that fix has transitioned into OMPI.

@artpol84
Copy link
Contributor

You are commenting on something committed 5 months ago?
Yeah, this change was made here, so I decided to have a discussion in the context.

I have no issue with removing it now that the fix is available, once that fix has transitioned into OMPI.
Thanks!

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

Successfully merging this pull request may close these issues.

4 participants