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

Replace autodoc with autoapi #482

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Replace autodoc with autoapi #482

merged 2 commits into from
Apr 14, 2022

Conversation

ascillitoe
Copy link
Contributor

@ascillitoe ascillitoe commented Apr 12, 2022

Replaces sphinx autodoc (and sphinx-apidoc) with sphinx-autoapi. As discussed by @jklaise in SeldonIO/alibi#471, sphinx-autoapi avoids the need to import (or mock) modules at docs compilation time. This is particularly relevant for #469, since I've been unable to reliably generate api docs for the pydantic models when typings contain mocked modules.

Summary of changes

Main changes involve:

  • A few tweaks to docs/source/conf.py.
  • Minor changes to requirements/docs.txt.
  • Changing references to api docs in the main docs e.g. [source](../../api/alibi_detect.ad.adversarialae.rst) is changed to [source](../../api/alibi_detect/ad/adversarialae/index.rst).

Implications

There are a number of implications:

  • Issues with importing and/or mocking modules are avoided (the new docs in Config driven detectors - part 3 #469 now compile!).

  • Stylistically the api docs are changed a little (i.e. the way subpackages and submodules are arranged). sphinx-autoapi offers some configuration options. For the most part I've tried to match our original autodoc settings, but there is room to tweak in the future.

  • Currently, the autoapi_options setting isn't as powerful as equivalent autodoc_default_options (it is simply a list of strings, rather than a dict of lists/bools/strings etc). A consequence of this is that if we wanted to address Sphinx API and inheritance #163 by turning on inherited-members, we could currently only set inherited-members to True. With autodoc we could instead set it as a list, which means we can globally display inherited-members, but disable this for certain parent classes such as pydantic's BaseModel. Hopefully this can be changed in the future, even if it means us submitting a PR to sphinx-autoapi.

  • sphinx-autoapi arguably has more powerful/flexible templates (https://github.com/readthedocs/sphinx-autoapi/tree/master/autoapi/templates/python), which can be customised to customise our api docs. We could explore this in the future.

  • One other downside is that sphinx-autoapi doesn't automatically work with autodoc-pydantic. This is a nice package which makes the api docs of pydantic models look far nicer (I also couldn't get it to work with our original autodoc docs since we need to mock tensorflow, numpy, transformers etc.). We should be able to create a custom template to do something similar, but it is a shame it doesn't work out-of-box.

  • This change does introduce 2 new warnings during the build process, but neither impact the docs too badly:

    1. WARNING: Cannot resolve import of alibi_detect.utils._types.Literal in alibi_detect.od.sr WARNING: Cannot resolve import of unknown module alibi_detect.utils.statstest in alibi_detect.cd.base - It appears sphinx-autoapi doesn't like this try/except pattern (similar to Cannot resolve import on re-imported class from module. readthedocs/sphinx-autoapi#285). It doesn't affect the actual docs too much so I'm tempted to open a post-PR issue. It will no longer be a problem once we drop 3.7 too.
    2. doc/source/examples/cd_context_20newsgroup.ipynb:15: ERROR: Content block expected for the "raw" directive; none found. - Also don't understand, but again the docs page looks fine.
  • Slightly annoying, submodule/subpackage level attributes such as logger are now documented. The easiest way to do this would be to turn off the 'undoc-members' setting, so that only members with docstrings are documented. IMO we should probably do this anyway, as I don't think we should have undocumented public objects (such as the 'UAE' class).

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ascillitoe ascillitoe mentioned this pull request Apr 12, 2022
5 tasks
Comment on lines +277 to +278
detector by packaging it as a callable function using {func}`~alibi_detect.cd.pytorch.preprocess.preprocess_drift`
and {func}`~functools.partial`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these directives are for stylistic rendering? Perhaps we should add these rules to CONTRIBUTING.md (sort of separate from docstring conventions, which, for the time being, are on alibi).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep just stylistic, the old refs to api docs pages still work. These directives are just a bit more inline with what we use elsewhere, e.g. in docstrings and also the api pages themselves. If you're happy with these I'll add a note to CONTRIBUTING.md.

Copy link
Contributor

@jklaise jklaise left a comment

Choose a reason for hiding this comment

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

LGTM although now I'm interested in seeing what the docs look like for pydantic models with this and with autodoc-pydantic (but if we can't get it working then the point is moot).

Agree with undoc-members so stuff without docstrings doesn't get documented, but probably requires a follow-up PR as there are a few things we have to document but haven't (like the UAE).

I think we can give this a go and switching back to autodoc if needed shouldn't be too complex (although at the cost of having to figure out those pydantic models again...).

@ascillitoe
Copy link
Contributor Author

Having a go makes sense, it should be easy to change back (excluding the pydantic issue!).

The current pydantic model api page looks like this (page here):

image

So pydantic models are just shown as normal classes, with the fields shown as attributes. This is generated with the usual docstring below each field/attribute approach to adding attribute docstrings, e.g.

    name: str
    "Name of the detector e.g. `MMDDrift`.

The type info isn't rendered properly because of [this issue](tox-dev/sphinx-autodoc-typehints#44 with sphinx-autodoc-typehints). If we instead describe the fields in the Attributes section of the main class docstring, like this:

class DetectorConfig(CustomBaseModel):
    """
    Base detector config schema. Only fields universal across all detectors are defined here.

    Attributes
    ----------
    name : str
        Name of the detector e.g. `MMDDrift`.
    backend : Literal['tensorflow', 'pytorch', 'sklearn']
        The detector backend.
    meta : Optional[MetaData]
        Config metadata. Should not be edited.
    """
    name: str
    backend: Literal['tensorflow', 'pytorch', 'sklearn'] = 'tensorflow'
    meta: Optional[MetaData] = MetaData()

we get this:

image

The types are now rendered properly, but until we turn off undoc-members the fields are documented twice.

For an example of what the autodoc-pydantic package gives us, see here. I think we can get most of this behavior with a custom sphinx-autoapi template, but it would be preferable to get autodoc-pydantic working with sphinx-autoapi instead.

@ascillitoe
Copy link
Contributor Author

p.s. @jklaise I'm documented additional issues that will need opening after #469 in the OP of that PR. I can add points about improving the pydantic api and adding missing docstrings there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants