Skip to content

Update man pages to clarify MPI-4 error handlers #11015

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
merged 1 commit into from
Nov 15, 2022
Merged

Update man pages to clarify MPI-4 error handlers #11015

merged 1 commit into from
Nov 15, 2022

Conversation

drwootton
Copy link
Contributor

This pull request is to update MPI man pages to describe MPI-4 error handling, as described in section 9.3 of the MPI-4 standard, to resolve issue #10783

I'm placing this text in an ERRORS.rst file with the intent that this file be included in any man page where MPI function call error handling needs to be described.

Currently this is just the text I propose, with markup to be added later.

@hppritcha At this point I am looking for comments on this text.

This text might be a bit long. Maybe one way to shorten it is to just name the three predefined error handlers and anyone interested can read the detail in the MPI standard.

Signed-off-by: David Wootton [email protected]

@jjhursey jjhursey requested a review from hppritcha November 1, 2022 20:12
@drwootton drwootton self-assigned this Nov 2, 2022
@jjhursey
Copy link
Member

jjhursey commented Nov 3, 2022

@hppritcha is this what you were looking for from #10783 ?

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

Can you rebase your commits instead of using a merge from main? Using a merge makes this PR look larger than it actually is (i.e., it shows all the changes from the commits that were picked up from main).

@@ -0,0 +1,49 @@
Almost all MPI routines return an error value; C routines as the return result
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to put the "ERRORS" section title in this file, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't think of putting the ERRORS header in this file.

But, as I was going thru the individual manpage files, I noticed some pages had headings included ERRORS in all caps and some as Errors.

If I put the ERRORS heading in this file, then I think it's going to look odd when all the other headings are mixed case on a page.

I can change it if you want it changed anyway.

Copy link
Member

Choose a reason for hiding this comment

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

The man pages were written over a long period of time, by different people. It's possible (likely!) that there are inconsistencies like this (ERRORS vs. Errors).

Let's make new text as consistent as possible (and potentially fix old inconsistencies). I.e., make the Errors subsection follow whatever the convention is for that level of subsection. Are there other titles of the same level of subsection that are inconsistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you're asking to do about the inconsistency regarding ERRORS vs Errors. If I move that into the ERRORS.rst file, then I guess I would use ERRORS there since that is a section heading. But then do I find the other section headings in every man page including the ones I have not yet touched and change all of those to uppercase?

I agree overall formatting consistency is desirable, but I also think consistency within a page is more important than overall consistency.

Should I be changing all section headings in this pull request? That feels a bit out of scope for the intent of this pull request.

Copy link
Member

@jsquyres jsquyres Nov 9, 2022

Choose a reason for hiding this comment

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

Yes, I'm suggesting moving the subsection title into ERRORS.rst. I'm further suggesting make the capitalization of the subsection title in ERRORS.rst be consistent with whatever we do for other subsection titles at the same level in other man pages.

That being said, if we have more than just "ERRORS" vs. "Errors" inconsistencies in capitalization of subsection titles, we should probably make them all consistent. That would satisfy both overall formatting consistency and within-one-man-page consistency. I agree that this should probably be either a separate PR, or a standalone commit on this PR. It probably wouldn't be too hard to hack up a script or two to help automate making the pages' subsection titles be consistent (vs. editing all bazillion of our man pages manually). It might be worth looking at other popular man pages to see what their conventions are -- e.g., some GNU man pages like binutils or coreutils or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsquyres I prefer to handle all the man page section headings changes, which I think can be scripted, as a separate pull request in order to keep that change from being tangled in more changes directly related to this pull request reviews. I also prefer to keep the ERRORS heading in each man page since there are some man pages with text about error handling which is different than what's described in ERRORS.rst. So I think keeping the ERRORS heading in each man page is more consistent and avoids getting anyone confused about where that heading went when ERRORS.rst is included.

Copy link
Member

Choose a reason for hiding this comment

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

Per discussion - this will happen in a separate PR after this PR is merged

@drwootton
Copy link
Contributor Author

@jsquyres I messed up the commit, redid it with @jjhursey help, think it's right now.

@drwootton drwootton marked this pull request as ready for review November 9, 2022 20:38
All man pages use a common text file to describe error handling with additional
text for some MPI_Test*, MPI_Wait, and MPI_Grequest* pages.

Fix review comments

Signed-off-by: David Wootton <[email protected]>
@drwootton drwootton requested a review from jjhursey November 14, 2022 18:33
Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

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

Thanks!

@jjhursey
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jjhursey jjhursey merged commit e358172 into open-mpi:main Nov 15, 2022
@jjhursey
Copy link
Member

@drwootton Please PR to v5. We'll also want to include the section headers PR that you are working on. They can come over to v5 together or separately, just as long as we don't forget. Thanks!

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.

4 participants