Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[WIP] Improving Python Docs API #16392

Merged
merged 13 commits into from Oct 10, 2019

Conversation

sojiadeshina
Copy link
Contributor

Description

Reorganizes python API docs in the new website. We now have coverage on all the mxnet python submodules, building on #16377

@ThomasDelteil @aaronmarkham @eric-haibin-lin

Changed the API front page to provide quick links for frequently used packages, this is also reflected on the top level of the toc-tree in the left hand side.

Screen Shot 2019-10-07 at 5 55 23 PM

There's also a new page that allows you to easily navigate to all the submodules.

Screen Shot 2019-10-07 at 5 44 46 PM

Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

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

Looks like you might need to rebase to get past the sanity check on CI.
Also, I have a bunch of questions.
This is a big change. Let's get it published on the staging server.

setTimeout(auto_index, 500);
}
}
$(document).ready(auto_index);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the no new line going to be a problem? Seems like I've failed some linter because of that before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linter didn't yell at me for this so I guess it's fine


/* Customizations to the Sphinx auto module plugin output */
function auto_index() {
var targets = $("dl.class>dt,dl.function>dt");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function be described?
We ran into issues with the last site because the Sphinx customizations and js were undocumented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
Essentially what's going on is it's going through all the classes/methods on the page and truncating the name to just the last two parts i.e from something like mxnet.gluon.nn.contrib to just nn.contrib and then adding that to a list that can be toggled so that the sidebar has information and anchors to specific methods on the page even when the sphinx toc-tree doesn't.

@ThomasDelteil wrote this part so he can correct me if i missed anything or give a more detailed description of what's going on.

@@ -36,3 +36,4 @@ dependencies:
- breathe
- mock
- awscli
- autodocsumm
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's where it's from.

@@ -47,7 +46,7 @@ build/%: %
html: $(OBJ)
mkdir -p build
cp Makefile_sphinx build/Makefile
sphinx-autogen build/api/*.rst build/api/*/*.rst -t build/_templates/
sphinx-autogen build/api/*.rst build/api/**/*.rst -t build/_templates/
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that ** some recursive notation for file paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the idea but actually I think the effect is the same with a single * .


.. automodule:: mxnet.gluon.model_zoo.vision
:noindex:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason on when you use this approach:

.. automodule:: mxnet.gluon.model_zoo.vision
   :noindex:

...versus this:

.. automodule:: mxnet.gluon.loss
    :members:
    :imported-members:
    :autosummary:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The difference is this.

For model_zoo.vision, I wanted to keep the documentation organized by the different model architectures: resnet, vgg etcs. it would have been super cluttered if it was just done all automatically. There are a ton of pretrained models in the model zoo so it's worth it to have them organized by sections.

The second approach in mxnet.gluon.loss automatically generates the documentation for all classes in the package and :autosummary: allows you to create a summary table at top automatically. Since there are not that many loss functions the autogenerated table summary is sufficient for this class.

@@ -146,27 +147,21 @@ API Reference

.. automodule:: mxnet.gluon.contrib
:members:
:imported-members:
Copy link
Contributor

Choose a reason for hiding this comment

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

When do you decide to not include :imported-members:?

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 take out imported members when the package level module also incidentally imports stuff from elsewhere that's already documented. For example here I think this also imports mxnet.ndarray which leads to a ton of warnings during the sphinx build.

Copy link
Contributor

Choose a reason for hiding this comment

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

I take out imported members when the package level module also incidentally imports stuff from elsewhere that's already documented. For example here I think this also imports mxnet.ndarray which leads to a ton of warnings during the sphinx build.

Ok that's good then, because we need to get warnings back down to zero.



Sequential containers
---------------------


.. autosummary::
:toctree: _autogen
:nosignatures:
Copy link
Contributor

Choose a reason for hiding this comment

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

This was questioned in another issue. What are some set with :nosignatures: and others are not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an aesthetic/readability choice and the idea here is similar to the comment in the other issue. Here the doc is listing different classes under the different groupings so there's no need to have the method signatures.

At the bottom when autosummary generates an alphabetical listing of the classes, the method signatures are included.

=================

.. automodule:: mxnet.initializer
:members:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no inherited-members?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean :imported-members:? if so then it's the same as : #16392 (comment)

if you meant :inherited-members: I think that only works for .. autoclass::

Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean :imported-members:? if so then it's the same as : #16392 (comment)

if you meant :inherited-members: I think that only works for .. autoclass::

I noticed that sometimes it's imported-members and sometimes it's inherited-members. I would like to better understand the distinction and when one should be used, both should be used, none should be used.

@aaronmarkham
Copy link
Contributor

@sojiadeshina Once this PR goes in you can publish to staging. Plus, I'm having the same sanity test issue even after rebasing, so you might want track that. #16411

@aaronmarkham aaronmarkham mentioned this pull request Oct 10, 2019
@ThomasDelteil
Copy link
Contributor

@ChaiBapchya please restart CI

@ChaiBapchya
Copy link
Contributor

Retriggered the windows-gpu, windows-cpu

@ChaiBapchya
Copy link
Contributor

@ThomasDelteil
@aaronmarkham @eric-haibin-lin Ready to review/merge

Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

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

🚀

@aaronmarkham aaronmarkham merged commit 243ade9 into apache:master Oct 10, 2019
aaronmarkham pushed a commit to aaronmarkham/incubator-mxnet that referenced this pull request Oct 16, 2019
* doc fixes for gluon api sub modules only

* update to the docs

* adding the autodoc js plugin

* adding autodoc javascript, fixing some warnings

* fixing model zoo

* python api docs major refactor -> ndarray, symbol and other mxnet libraries

* restructure top level toctree navs and bubble more submodules to the top

* modify import to reference defined classes and functions directly

* fix some broken links

* add extra classes imported from c++

* fix .htaccess redirects since api links have changed again

* included license on files missing license

* Update monitor.py
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.

None yet

4 participants