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

Final Sphinx documentation w/ ReadTheDocs #172

Closed
wants to merge 20 commits into from

Conversation

iAdityaEmpire
Copy link
Contributor

@iAdityaEmpire iAdityaEmpire commented Dec 10, 2021

Summary

  • I have read CONTRIBUTING.md to understand how to contribute to this repository :)

<Please summarize what you are trying to achieve, what changes you made, and how they acheive the desired result.>

What This Is

This is comprehensive documentation for AugLy with Sphinx docstring formatting under the hood with a Read the Docs theme to make augmentation parameters, return types, etc more readable and user-friendly.

How It Works

Sphinx is a documentation generator commonly used by the Python community. It also has its own docstring format.

Internal AugLy docstrings utilize tags such as @param or @returns for labeling due to internal Facebook convention. However, Sphinx does not recognize these tags, opting for : in favor of @, among other changes.

Luckily for us, Python docstring format epytext is very similar to AugLy (credit @Cloud9c), meaning that we can claim that we use epytext formatting and then convert it to sphinx when necessary.

Another problem: Sphinx requires explicit types labeled in the form of :type and :rtype to display types once documentation is rendered. However, Python typehints (which AugLy uses generously) are not natively supported. Therefore we use an extension to Sphinx that autodetects typehints and adds them on the fly.

In the end, Sphinx uses the module structure specified in docs/source with rST (filetype similar to Markdown) to generate a table of contents for our final documentation.

How to Build Documentation Locally

  1. Clone repository. git clone github.com/facebookresearch/AugLy
  2. Install all requirements for both the library and documentation-specific dependencies cd docs && pip install -r requirements.txt
  3. Make sure you are in the docs subdirectory. Then run make html to generate documentation. If you want to delete all these later, you can run make clean.
  4. Navigate to docs/build and open index.html

Generating new documentation later

  1. Sphinx can detect new files added to the augly subdirectory and add their .rst files accordingly, but the process of detection needs to be triggered manually. Run sphinx-apidoc -o ./source ../augly in the root directory to do so, and update the toctree in index.rst if necessary.
  2. Edited a docstring and want to see the same changes be reflected in the published documentation? No worries, this will be automatically completed, an overview of which is provided below.

Integration with ReadTheDocs

  1. This documentation uses Sphinx's ReadTheDocs theme to make it easy to publish documentation on RTD's site.
  2. Create a Github webhook to detect pushes made to the repository so documentation can rebuild.
  3. The .readthedocs.yml file specifies the configuration for these builds. By default ffmpeg and libsndfile1 are based on C and are required as prerequisites before requirements in docs/requirements.txt are installed (as RTD uses Ubuntu behind the scenes).
  4. Docstrings aren't stored in the docs subdirectory at all, so all are read from the source folder. Updating the docstrings in augly/<modality> will be sufficient.

@iAdityaEmpire iAdityaEmpire mentioned this pull request Dec 10, 2021
1 task
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 10, 2021
@facebook-github-bot
Copy link
Contributor

@jbitton has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@iAdityaEmpire
Copy link
Contributor Author

image

The reason for the above issue occuring seems due to the way we create TEMPLATE_FILEPATH below:

image

Possible solution: Detecting if "readthedocs.org" is in the filepath, and if so, render the substring after "/latest"

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

17 similar comments
@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jbitton has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@jbitton has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jbitton
Copy link
Contributor

jbitton commented Dec 14, 2021

lil update: @iAdityaEmpire, your fix for version.txt worked! let us know when you've updated this for the rest of the files zoe listed :)

@iAdityaEmpire
Copy link
Contributor Author

@jbitton Haha I think that was Zoe who updated the version.txt file, I was at school during the time. I'll be able to fix the existing issues by the end of day today.

@jbitton
Copy link
Contributor

jbitton commented Dec 14, 2021

ohhh lol i didn't notice. okay, sounds good! no rush :)

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jbitton has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@jbitton jbitton left a comment

Choose a reason for hiding this comment

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

you're the absolute best!!! these are my last super simple comments and then we can land this!!

Comment on lines 4 to 10
Subpackages
-----------

.. toctree::
:maxdepth: 4

augly.image.utils
Copy link
Contributor

Choose a reason for hiding this comment

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

since augly.image.utils doesn't exist anymore as an .rst file, should this be deleted 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.

@jbitton Yep I'll delete that

@@ -0,0 +1,109 @@
augly.text.augmenters package
Copy link
Contributor

Choose a reason for hiding this comment

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

since we deleted the other augmenters .rst files, let's delete this one too for consistency

Comment on lines 4 to 10
Subpackages
-----------

.. toctree::
:maxdepth: 4

augly.text.augmenters
Copy link
Contributor

Choose a reason for hiding this comment

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

and then make sure to remove this as well as necessary

Comment on lines 49 to 53

.. automodule:: augly.text.utils
:members:
:undoc-members:
:show-inheritance:
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to remove this :D

Comment on lines 4 to 10
Subpackages
-----------

.. toctree::
:maxdepth: 4

augly.video.augmenters
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to remove augmenters since it doesn't exist anymore

@zpapakipos
Copy link
Contributor

Hi @iAdityaEmpire, one more thing -- when we import this diff, we get an error that some files contain dos newlines (since you work on a Windows I guess) instead of the expected Unix newlines. This is happening on the following files: all of the READMEs, docs/requirements.txt, & readthedocs.yaml. Can you please try to fix this e.g. using one of these suggestions?

@iAdityaEmpire just flagging that FYI this error is still happening when we import the diff! Did you try the solutions I suggested up here?

@iAdityaEmpire
Copy link
Contributor Author

@zpapakipos Oh that's weird -- I did try using dos2unix and it said it converted the files fine... I'll try using another method.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jbitton has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@iAdityaEmpire has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jbitton has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@jbitton jbitton left a comment

Choose a reason for hiding this comment

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

@iAdityaEmpire this all looks amazing!!!! all the tests on the Meta side have passed successfully!!! thank you so much for all of your extra work to get this done :)

@iAdityaEmpire
Copy link
Contributor Author

Thanks @jbitton! Glad to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants