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

Include PRTE RST content in mpirun.1 man page #11820

Merged
merged 4 commits into from
Sep 25, 2023

Conversation

jsquyres
Copy link
Member

@jsquyres jsquyres commented Jul 11, 2023

This PR is in conjunction with openpmix/prrte#1765. The intent is to use RST content from PRTE to fill out Open MPI's mpirun.1 man page (and corresponding HTML rendering).

The exact mechanism and scheme for this has changed a few times over the course of experimenting / development, so I won't comment too much on it here. This PR will likely contain a whole truckload of WIP / ugly commits until we get everything working. At the end, we'll squash and make it pretty / make the git commit messages be accurate / all the things.


As this PR got more mature, it created a lot of new configury cases. This spreadsheet shows all 32 new cases.

@jsquyres
Copy link
Member Author

For those who would like to try this themselves, you will need to update your PRRTE git submodule pointer to point to the branch for the corresponding PRRTE PR openpmix/prrte#1765:

cd root_of_your_git_clone
cd 3rd-party/prrte
git remote add rhc54 [email protected]:rhc54/prrte.git
git fetch --all --prune
git checkout topic/rst

Then you should be able to go back up to the OMPI git clone root and autogen / build from there (ensure that Sphinx is in your path yadda yadda yadda).

docs/Makefile.am Outdated Show resolved Hide resolved
docs/Makefile.am Outdated Show resolved Hide resolved
@jsquyres jsquyres force-pushed the pr/generate-mpirun-man-page branch 4 times, most recently from d15bcde to 67bec85 Compare September 11, 2023 00:23
@jsquyres jsquyres marked this pull request as ready for review September 11, 2023 00:40
@jsquyres
Copy link
Member Author

@bwbarrett This PR is ready for review.

@jsquyres
Copy link
Member Author

As this PR got more mature, it created a lot of new configury cases. This spreadsheet shows all 32 new cases.

@qkoziol
Copy link
Contributor

qkoziol commented Sep 11, 2023

As this PR got more mature, it created a lot of new configury cases. This spreadsheet shows all 32 new cases.

Could you please open access to the spreadsheet?

@jsquyres
Copy link
Member Author

Could you please open access to the spreadsheet?

Done.

@jsquyres
Copy link
Member Author

Only change from the force push a few minutes ago was updating the PRRTE submodule pointer to include Ralph's feedback on openpmix/prrte#1788.

@jsquyres
Copy link
Member Author

  • NVIDIA CI is failing right now due to a UCX issue that is unrelated to this PR.
  • OMPI AWS CI is failing right now due to my submodule update; that will be fixed when schizo/ompi: update RST openpmix/prrte#1788 is merged.

@jsquyres jsquyres marked this pull request as draft September 12, 2023 02:03
@jsquyres
Copy link
Member Author

jsquyres commented Sep 12, 2023

Moving this back to draft 😦

There's at least a few problems that need fixing:

  1. RST content over in PRRTE is still in flux (per schizo/ompi: update RST openpmix/prrte#1788)
  2. OMPI's docs/Makefile.am rules for prrte-rst-content and ompi-schizo-rst-content somehow aren't quite right yet. They fire more than they need to, and Sphinx gets confused about labels on subsequent runs.
  3. The multiple-label problem may actually be a Real Thing (even though it doesn't seem to happen in my runs the first time I build...?) because the RTD CI failed for similar reasons.

This commit introduce a fundamentally new concept: have configure
search PRRTE for RST files to include in Open MPI's documentation
(regardless of whether we're using the internal/bundled PRRTE or an
external PRRTE).  If we're building against an external PRRTE that is
old enough that it doesn't have any RST files installed, we'll make up
some dummy RST files that basically say "you don't get help/content
here because your PRRTE is too old."

To simplify the configury for this scheme, this commit also makes
another change: the pre-built HTML docs and nroff man pages included
in distribution tarballs are now located at docs/html/ and docs/man/,
respectively (vs. the location where we'll build them:
docs/_build/html/ and docs/_build/man/, respectively).  There are two
cases here:

1. If the user has Sphinx available, we'll build the docs under
   docs/_build/, and install those (effectively ignoring the pre-built
   docs).
2. If the user does not have Sphinx available, we'll just install the
   pre-built docs.

This simplified things like "make clean" and "make distcheck".

Including RST content from PRTE required another major change: when we
build the RST docs in a VPATH scenario, we copy the entire docs/
source tree to the build tree.  This allows us to modify the RST
sources a bit (e.g., to include the PRRTE RST files or generate dummy
PRRTE RST files).

mpirun.1.rst is updated to include the RST content from PRRTE about
CLI options.  More work needs to be done here to remove old,
now-redundant content.

Finally, we also amend the advice to implementors to have Sphinx
installed when building their package so that Open MPI's build system
can properly slurp in their PRRTE's RST docs.

Signed-off-by: Jeff Squyres <[email protected]>
Since RTD doesn't run autogen, configure, or make, we now have to
manually copy a few RST files from the embedded PRRTE to the docs/
tree before RTD invokes Sphinx.

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres
Copy link
Member Author

jsquyres commented Sep 24, 2023

All known problems fixed, and intermediate git commits squashed appropriately. Also incorporated OAC_ASSERT_BEFORE feedback from @bwbarrett in Slack. Ready for review. Running all 32 regression test cases, which will take a few hours.

@jsquyres jsquyres marked this pull request as ready for review September 24, 2023 21:29
@jsquyres
Copy link
Member Author

All regression testing passed. Merging.

@jsquyres jsquyres merged commit cdb35c3 into open-mpi:main Sep 25, 2023
8 checks passed
@jsquyres jsquyres deleted the pr/generate-mpirun-man-page branch September 25, 2023 14:28
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.

3 participants