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

[docs] 🎨 Sphinx Autosummary for generating Python-API documentation #2286

Merged
merged 4 commits into from
Jul 27, 2019
Merged

Conversation

hayesall
Copy link
Contributor

@hayesall hayesall commented Jul 25, 2019

Executive Summary

This replaces autoclass/autofunction with sphinx autosummary to create and paginate the Python API.

Summary of Changes

  • Add docs/.gitignore to not track autosummary stubs
  • Add sphinx.ext.autosummary in docs/conf.py
    • Add 'members' and 'inherited-members' as default parameters
    • Add 'autosummary = True' for setting output with :toctree:
  • Add .. autosummary:: tags to replace .. autoclass:: and .. autofunction::

Pull Request Motivation 📝

Previously the Python-API.rst dumped all of the Python API onto a single page when sphinx-build generated html pages.

This replaces the Python-API documentation with an index listing all modules, and paginates all functions and classes onto separate pages.

In summary, this pull request:

  • Requires less scrolling on the Python-API page
  • Makes it easier to find specific pieces of the full Python documentation

Alternatives

This could also be accomplished by creating individual pages by hand and putting autoclass and autofunction on each. Autosummary simply makes this process more scalable.

Any feedback would be appreciated. Thank you developers!


Screenshots

Before (left) and After (right) of the Python-API.html landing page

before_after

Training API: lightgbm.cv. The lightgbm.cv function has its own page.

Screen Shot 2019-07-25 at 1 49 09 PM

Add `docs/.gitignore` to not track autosummary stubs
Add `sphinx.ext.autosummary` in `docs/conf.py`
  Add 'members' and 'inherited-members' as default parameters
  Add 'autosummary = True' for setting output with `:toctree:`
Add `.. autosummary::` tags to replace `.. autoclass::`

Previously the `Python-API.rst` dumped all of the Python API onto
a single page.

This replaces the Python-API documentation with an index listing
all modules, and paginates all functions and classes onto
separate pages.
@msftclas
Copy link

msftclas commented Jul 25, 2019

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

@hayesall Thanks a lot for your contribution! It looks nice from a first glance! Please let me describe a few drawbacks I see.

  • It breaks previous links to Python API from third-party sites.

  • It generates duplicated entries for class constructors. I guess a workaround can be found by playing around with autoclass_content value:

    autoclass_content = 'both'

    image

    UPD: It can be fixed by setting autoclass_content to 'class'.

  • It removes class inheritance (previously controlled by :show-inheritance: flag).
    image
    UPD: Can be fixed by appending 'show-inheritance' to autodoc_default_flags.

docs/.gitignore Outdated Show resolved Hide resolved
Drop `docs/.gitignore` to use the general `.gitignore`
Add `show-inheritance` to `autodoc_default_flags` in `docs/conf.py`
Fix `both` to `class` in `autoclass_content` in `docs/conf.py`
@hayesall
Copy link
Contributor Author

hayesall commented Jul 25, 2019

✅ Fixed the 'show-inheritance by adding it to the autodoc_default_flags variable:

autodoc_default_flags = ['members', 'inherited-members', 'show-inheritance']

✅ Changing autoclass_content to 'class' removes the duplicate __init__ entries.


❓ I don't have a solution for third-party links, but here is what I have currently:

@StrikerRUS
Copy link
Collaborator

@hayesall

Existing links to a specific portion (e.g. Python-API.html#lightgbm.LGBMModel) will still direct users to the index (in this case: Python-API.html) rather than a 404 error page.

Indeed! Since index page is rather small now, it should be not so hard for users to seek for the specified API member manually.

Fix deprecated `autodoc_default_flags` to `autodoc_default_options`
Add `autodoc_default_flags` with parameters from
  `autodoc_default_options`
@StrikerRUS StrikerRUS changed the title [MRG] 🎨 Sphinx Autosummary for generating Python-API documentation [docs] 🎨 Sphinx Autosummary for generating Python-API documentation Jul 25, 2019
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

LGTM!

@hayesall Thank you for your contribution! Separate pages look much better than previous huge wall of API code.

@StrikerRUS StrikerRUS merged commit 207bb3e into microsoft:master Jul 27, 2019
@hayesall hayesall deleted the pydocs branch July 27, 2019 16:52
@StrikerRUS
Copy link
Collaborator

@hayesall I'm sorry for the late question, but according to the logs it seems that autosummary scans each .rst file to generate a code. Is it possible to help it and specify that we need to scan only Python-API.rst? I guess it will save some time.

D:\Users\nekit\Downloads\LightGBM\docs>make html
Running Sphinx v1.8.5
making output directory...
[autosummary] generating autosummary for: Advanced-Topics.rst, C-API.rst, Development-Guide.rst, Experiments.rst, FAQ.rst, Features.rst, GPU-Performance.rst, GPU-Targets.rst, GPU-Tutorial.rst, GPU-Windows.rst, Installation-Guide.rst, Parallel-Learning-Guide.rst, Parameters-Tuning.rst, Parameters.rst, Python-API.rst, Python-Intro.rst, Quick-Start.rst, README.rst, gcc-Tips.rst, index.rst

...

@hayesall
Copy link
Contributor Author

@StrikerRUS Sphinx's generate.py#L193-L228 module looks like it should support only scanning a specific list of files:

The autosummary_generate variable looks to be where this behavior is accessed, but I would need to experiment a bit to be sure.

I can look later today. If it's an easy fix I'll try to patch it in with #2293. If it's a bit more involved I'll open an issue.

@StrikerRUS
Copy link
Collaborator

@hayesall Thanks a lot for your answer with starting points! It's absolutely not urgent and critical.

Please do not include any fix in #2293, because that PR is about different things. Just post updates here if you find anything valuable or make a new PR if you'd like (and have enough free time 😄 ).

@lock lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants