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

ompi/schizo: Expose "--mca" when parsing command line. #1311

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

awlauria
Copy link
Contributor

Signed-off-by: Austen Lauria awlauria@us.ibm.com

@ggouaillardet
Copy link
Contributor

Thanks @awlauria!

That looks good to me, but based on the previous exchanges on that topic, I do not think I am qualified to review this PR.

@rhc54
Copy link
Contributor

rhc54 commented Mar 29, 2022

Hmmm...no, that's not quite correct. This will only handle the generic "mca" option. It should check for the "--omca" option as well. We have already converted the PRRTE and PMIx generic options prior to passing the cmd line to the schizo component, so those should be okay.

I'm not convinced this is the correct fix, but it might be part of it. The looping described by @ggouaillardet is simply incorrect and may be contributing to the problem. I'd prefer to wait until that gets unraveled so we understand how this all fits together. Otherwise, we may chase our tails on this one.

Signed-off-by: Austen Lauria <awlauria@us.ibm.com>
@awlauria
Copy link
Contributor Author

Thanks, added --omca.

At worst this makes the --mca handling consistent with the others, which in general is a good thing. If there's a refactor that makes this code moot later that is fine, but this going in should be fine for ompi and is a relatively safe change for ompi v5.

@rhc54
Copy link
Contributor

rhc54 commented Mar 29, 2022

Okay - we can deal with any issues as they arise. We need to resolve this loop logic issue prior to release, but it might be another couple of weeks before I can get to it.

@rhc54
Copy link
Contributor

rhc54 commented Mar 29, 2022

FWIW: here is what should be happening. PRRTE should call "setup_application" with a directive that we only harvest envars, and specifying the programming model ("ompi" or whatever). This is done by the user-facing launch tool (e.g., prun). It then takes the results (which will contain an array of PMIX_SET_ENVAR infos) and creates an env for the application.

We then process that using the command line, altering the env array as required. This is the env that is included in the pmix_app_t. This is why I have concerns about pushing things into the environment - we don't want OMPI params polluting the environment of the PRRTE tools, just as we don't want PRRTE params polluting the environment of OMPI apps. There is no need for OMPI params to be in the environment - they only need to be in the env array of the pmix_app_t.

When PRRTE goes to launch, it again calls "setup_application", but this time it passes in the job map and directives to assign fabric values like endpts. This is done by the DVM master, and it specifically does not ask to harvest envars as the DVM master is not guaranteed local to the user. This information is added to the launch message sent to the backend daemons.

We need to review the current logic to ensure this gets done correctly when the launch tool and DVM master are one and the same (i.e., prterun). It sounds like that isn't happening and there is an extra call being made that screws things up. Also need to check the split between PMIx and PRRTE to ensure we don't have overlap in their roles as that would likewise cause problems.

@rhc54
Copy link
Contributor

rhc54 commented Mar 29, 2022

Yeah, something is wrong with dmodex. It is failing in only 4 seconds, yet the timeout is being set to 10 seconds. It looks like it is not correctly performing the dmodex operation, but instead just looking for the data and reporting back that it isn't found. My best guess is that we record that someone asked for data about a given proc, and then someone else comes along and asks for data (perhaps a different piece of data) from the same proc. We only record that we asked about the proc - we don't record what info we wanted.

It is therefore possible that we could get an answer back from the remote end that contains the data for the first request, but not for the second. This would cause the second request to return a "not found" error. I'm not sure if that is what is happening here, but it is a "hole" in the operation.

We'll need to investigate it, but that should be as a separate issue. For now, I'd recommend disabling it.

@rhc54 rhc54 merged commit 4dfcdd3 into openpmix:master Mar 29, 2022
@ggouaillardet
Copy link
Contributor

@rhc54 is your latest comment related to this issue?

@rhc54
Copy link
Contributor

rhc54 commented Mar 29, 2022

No - I was commenting about the fact that the dmodex test in the IBM/PGI CI on this PR failed. We continue to see intermittent failures that require retest and then pass. I think the problem lies in a race condition in the PRRTE/PMIx support for the dmodex operation and should be investigated separately - it has nothing to do with this PR and shouldn't hold it up.

@awlauria
Copy link
Contributor Author

@rhc54 thanks for the explanation. I agree that the different mca's shouldn't pollute different environments, but I don't think it is a blocker for release ompi. Maybe @janjust and @gpaulsen feel differently though.

Re the modex test - it is now moved to the last test run. So there's nothing really gained by disabling it - but feel free to do so if it bothers. There is something going on there still unfortunately. :(

@awlauria awlauria deleted the expose_mca_ompi_schizo branch March 29, 2022 02:46
@rhc54
Copy link
Contributor

rhc54 commented Mar 29, 2022

@rhc54 thanks for the explanation. I agree that the different mca's shouldn't pollute different environments, but I don't think it is a blocker for release ompi. Maybe @janjust and @gpaulsen feel differently though.

I cannot speak for OMPI's release schedule, but PRRTE won't release until we get the sequence correct as it impacts more than just MCA params and a broader scope than just OMPI. We need to get it reviewed and fixed.

Re the modex test - it is now moved to the last test run. So there's nothing really gained by disabling it - but feel free to do so if it bothers. There is something going on there still unfortunately. :(

Yeah, I just don't want people wasting time retesting it every time it fails. We know we have a problem, and it is very unlikely that someone's PR is affecting it. We have an issue for it, though I think by now we are painfully aware enough that the issue is hardly required to remind us.

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.

3 participants