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

Improve documentation build and reference #1529

Merged
merged 4 commits into from Nov 25, 2019

Conversation

@EnricoMi
Copy link
Contributor

EnricoMi commented Nov 21, 2019

  • Adds documentation on how to build the docs
  • Links the online documentation from prominent location in README.rst
  • Links to the Contributors Guide from new section Compiling
  • Makes imports robust against possible doc build failure:
autodoc: failed to import module 'tensorflow' from module 'horovod'; the following exception was raised:
Traceback (most recent call last):
  ...
  File ".../horovod/common/basics.py", line 19, in <module>
    import horovod.common.util as util
AttributeError: module 'horovod.common' has no attribute 'util'
EnricoMi added 3 commits Nov 21, 2019
Signed-off-by: Enrico Minack <github@enrico.minack.dev>
The original import broke `make html` in my setup:
  File ".../horovod/common/basics.py", line 19, in <module>
    import horovod.common.util as util
  AttributeError: module 'horovod.common' has no attribute 'util'

Signed-off-by: Enrico Minack <github@enrico.minack.dev>
Signed-off-by: Enrico Minack <github@enrico.minack.dev>
@tgaddair tgaddair requested a review from sblotner Nov 23, 2019
README.rst Outdated
@@ -35,7 +35,7 @@ Horovod
|

Horovod is a distributed training framework for TensorFlow, Keras, PyTorch, and MXNet. The goal of Horovod is to make
distributed Deep Learning fast and easy to use.
distributed Deep Learning fast and easy to use. Full documentation can be found at https://horovod.readthedocs.io/en/latest.

This comment has been minimized.

Copy link
@tgaddair

tgaddair Nov 23, 2019

Collaborator

Instead of putting this here, can we move this below the "Contents" and say something like:

"Full documentation and an API reference can be found here <https://horovod.readthedocs.io/en/latest>_."

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

I agree with this suggestion. "See the full documentation and an API reference at https://horovod.readthedocs.io/en/latest."

README.rst Outdated
@@ -117,6 +117,13 @@ If you're installing Horovod on a server with GPUs, read the `Horovod on GPU <do
If you want to use Docker, read the `Horovod in Docker <docs/docker.rst>`_ page.


Compiling

This comment has been minimized.

Copy link
@tgaddair

tgaddair Nov 23, 2019

Collaborator

Let's merge this in the the "Install" section above, and remove references to the read-the-docs page. So just say "To compile Horovod from source, follow the industrutions in the Contributor Guide <docs/contributors.rst>_.

Then, we can add something similar in https://github.com/horovod/horovod/blob/master/docs/summary.rst, which will then link to the correct GitHub or read-the-docs page depending on where the users is viewing the docs from.

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

I agree with this edit

Copy link
Collaborator

sblotner left a comment

I agree with Travis's comments. Added a few editorial suggestions as well

README.rst Outdated
@@ -35,7 +35,7 @@ Horovod
|

Horovod is a distributed training framework for TensorFlow, Keras, PyTorch, and MXNet. The goal of Horovod is to make
distributed Deep Learning fast and easy to use.
distributed Deep Learning fast and easy to use. Full documentation can be found at https://horovod.readthedocs.io/en/latest.

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

I agree with this suggestion. "See the full documentation and an API reference at https://horovod.readthedocs.io/en/latest."

README.rst Outdated
@@ -117,6 +117,13 @@ If you're installing Horovod on a server with GPUs, read the `Horovod on GPU <do
If you want to use Docker, read the `Horovod in Docker <docs/docker.rst>`_ page.


Compiling

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

I agree with this edit

The Horovod documentation is available at https://horovod.readthedocs.io/.

Those html pages can be rendered from ``.rst`` files located in the `docs` directory.
You need to setup Sphinx before you compile the documentation the first time:

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

setup --> set up

Documentation
-------------

The Horovod documentation is available at https://horovod.readthedocs.io/.

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

is available at --> is published to


The Horovod documentation is available at https://horovod.readthedocs.io/.

Those html pages can be rendered from ``.rst`` files located in the `docs` directory.

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

HTML

$ pip install -r requirements.txt
$ make clean
Then you can build the html pages and open ``docs/_build/html/index.html``:

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

HTML

$ make html
$ open _build/html/index.html
Sphinx can render the documentation in many other formats, just type ``make`` to get a list of available formats.

This comment has been minimized.

Copy link
@sblotner

sblotner Nov 23, 2019

Collaborator

, just type --> . Type

Signed-off-by: Enrico Minack <github@enrico.minack.dev>
@EnricoMi

This comment has been minimized.

Copy link
Contributor Author

EnricoMi commented Nov 24, 2019

I have applied all suggestions. Thanks for the feedback!

@EnricoMi

This comment has been minimized.

Copy link
Contributor Author

EnricoMi commented Nov 24, 2019

The README.rst and docs/summary.rst look very similar. They differ in some places. Some pieces of content surely belong to one or the other place, but most differences look like they went out of sync: diff README.rst docs/summary.rst.

Should they be brought back in sync again? Should be done outside this PR though. What do you think?

Copy link
Collaborator

tgaddair left a comment

Looks great, thanks!

@tgaddair

This comment has been minimized.

Copy link
Collaborator

tgaddair commented Nov 25, 2019

The README.rst and docs/summary.rst look very similar. They differ in some places. Some pieces of content surely belong to one or the other place, but most differences look like they went out of sync: diff README.rst docs/summary.rst.

Should they be brought back in sync again? Should be done outside this PR though. What do you think?

Yes, I agree. They should definitely be brought back in sync in a separate PR.

@tgaddair tgaddair merged commit 3237ccc into horovod:master Nov 25, 2019
2 checks passed
2 checks passed
DCO DCO
Details
buildkite/horovod/pr Build #1417 passed (47 minutes, 24 seconds)
Details
@EnricoMi EnricoMi deleted the G-Research:branch-make-doc branch Nov 25, 2019
jeffdaily added a commit to ROCmSoftwarePlatform/horovod that referenced this pull request Nov 27, 2019
Signed-off-by: Enrico Minack <github@enrico.minack.dev>
@EnricoMi

This comment has been minimized.

Copy link
Contributor Author

EnricoMi commented Nov 28, 2019

PR #1538 syncs README.rst and docs/summary.rst.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.