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

Copied comment-docs to torch.distributions wrappers from corresponding sources. #3243

Closed
wants to merge 2 commits into from

Conversation

rtviii
Copy link
Contributor

@rtviii rtviii commented Jul 19, 2023

Was surprised not to find a little handy annotations, had to drill down to source make sure i'm using the parameter correctly.

Copy link
Member

@fritzo fritzo left a comment

Choose a reason for hiding this comment

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

Good point that it would be helpful to remove that layer of indirection between Pyro wrappers and their PyTorch distributions. However I have a couple concerns with the proposed change:

  1. We'd prefer to programmatically copy docs directly torch via @copy_docs_from rather than manually embed those docs once. The programmatic approach would allow Pyro's wrappers to receive upstream updates from the docs and would reduce our team's maintenance burden.
  2. Pyro uses sphinx-style docstrings whereas PyTorch uses NumPy/Google style docstrings. This unfortunate design choice prevents Pyro's doc building system from directly embedding PyTorch docstrings in Pyro wrappers (and I suspect this PR's docs build will fail even after we make lint to fix black errors).

I think we can address both concerns with little effort by:

  1. Updating the loop at the end of this file to programmatically insert the torch docstrings into our wrappers,
  2. Adding napoleaon to our sphinx dependencies to start supporting NumPy/Google style docstrings as in PyTorch.

@rtviii would you like to try this alternative approach, or would you like me to do it? Either way, thanks for the nudge to fix our redirected docstrings 🙂

@rtviii
Copy link
Contributor Author

rtviii commented Jul 19, 2023

Certainly, i can try. Took a brief look..

  • soo that loop at bottom of torch.py ought to grab the _Dist's __doc__ and set the _PyroDist's __doc__ attr to the same value? Is that how you see it?

  • napoleon is easy to add to sphinx's conf.py.. never used it though, do i have to invoke it explicitly in the make routines?

@fritzo
Copy link
Member

fritzo commented Jul 19, 2023

  • in the loop I think I'd keep the existing docstring and append the new docstring to it. Our current docstring points to the original, and while I agree with you it's helpful to have the original docstring, it's also useful to keep a link to the base distribution.
  • I believe we can just add napoleon to our dev requirements in setup.py and docs/requirements.txt. Let's see 🙂
  • Also you can format and lint our python code via make format and make lint respectively. That should help avoid these CI linting errors

@rtviii
Copy link
Contributor Author

rtviii commented Jul 20, 2023

Hey, Fritz.

A few things.. This fails(asking for nonidented lines) seemingly after sphinx.ext.napoleon is enabled in conf.py. That's the way napoleon site recommends it be used by the way, it comes prepackaged with sphinx now, they say.

So even thought that suggests to me napoleion works, i'm still getting the error (below) unforutantely, which seems to have do with the identation characteristic of the numpy/google docstrings i'm pulling from pytorch. I expected napoleon to take care of that. Maybe you know what's going on there?

Furthermore, i had to disable the doctest ( see doctest_disable in 1 below) for the build to succeed for now (not sure if that can be a part of the problem). Perhaps you guys would want to go about it otherwise

1. Updated docbuilding loop in `torch.py`
def doctest_disable(docstring):
    _ = ""
    for line in docstring.splitlines():
        _ += line + "#doctest: +DISABLE"
    return _


# Programmatically load all distributions from PyTorch.
__all__ = []
for _name, _Dist in torch.distributions.__dict__.items():
    if not isinstance(_Dist, type):
        continue
    if not issubclass(_Dist, torch.distributions.Distribution):
        continue
    if _Dist is torch.distributions.Distribution:
        continue

    try:
        _PyroDist = locals()[_name]
        torchDistDocstring = _Dist.__doc__

    except KeyError:
        _PyroDist = type(_name, (_Dist, TorchDistributionMixin), {})
        _PyroDist.__module__ = __name__
        locals()[_name] = _PyroDist
        torchDistDocstring = None

    _PyroDist.__doc__ = """
    Wraps :class:`{}.{}` with
    :class:`~pyro.distributions.torch_distribution.TorchDistributionMixin`.

    """.format(
        _Dist.__module__, _Dist.__name__
    ) + (
        "\n\n" + doctest_disable(torchDistDocstring)
        if torchDistDocstring is not None
        else ""
    )
    __all__.append(_name)


# Create sphinx documentation.
__doc__ = "\n\n".join(
    [
        """
    {0}
    ----------------------------------------------------------------
    .. autoclass:: pyro.distributions.{0}
    """.format(
            _name
        )
        for _name in sorted(__all__)
    ]
)
2. Error from building with pytorch docstrings (`napoleon` enabled)
ᢹ saeta.rtviii[ ~/pyro ]  make format                                                                                                                                                                             [dev]
python scripts/update_headers.py
ruff check --fix .
black *.py pyro examples tests scripts profiler
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install "black[jupyter]"``
reformatted /Users/rtviii/pyro/pyro/contrib/forecast/forecaster.py

All done! ✨ 🍰 ✨
1 file reformatted, 616 files left unchanged.


ᢹ saeta.rtviii[ ~/pyro ]  make test                                                                                                                                                                               [dev]
ruff check .
black --check *.py pyro examples tests scripts profiler
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install "black[jupyter]"``
All done! ✨ 🍰 ✨
617 files would be left unchanged.
python scripts/update_headers.py --check
mypy --install-types --non-interactive pyro scripts
Success: no issues found in 319 source files
/Library/Developer/CommandLineTools/usr/bin/make -C docs html
Running Sphinx v6.2.1
loading intersphinx inventory from https://docs.python.org/3/objects.inv...
loading intersphinx inventory from https://pytorch.org/docs/master/objects.inv...
loading intersphinx inventory from http://funsor.pyro.ai/en/stable/objects.inv...
loading intersphinx inventory from https://optimized-einsum.readthedocs.io/en/stable/objects.inv...
loading intersphinx inventory from https://docs.scipy.org/doc/scipy/reference/objects.inv...
loading intersphinx inventory from https://biopython.org/docs/latest/api/objects.inv...
loading intersphinx inventory from https://horovod.readthedocs.io/en/stable/objects.inv...
loading intersphinx inventory from https://graphviz.readthedocs.io/en/stable/objects.inv...
intersphinx inventory has moved: http://funsor.pyro.ai/en/stable/objects.inv -> https://funsor.pyro.ai/en/stable/objects.inv
intersphinx inventory has moved: https://docs.scipy.org/doc/scipy/reference/objects.inv -> https://docs.scipy.org/doc/scipy/objects.inv
building [mo]: targets for 0 po files that are out of date
writing output...
building [html]: targets for 32 source files that are out of date
updating environment: [new config] 32 added, 0 changed, 0 removed
reading sources... [100%] testing

Warning, treated as error:
/Users/rtviii/pyro/pyro/distributions/torch.py:docstring of pyro.distributions.torch.Categorical:6:Inline interpreted text or phrase reference start-string without end-string.
make[1]: *** [html] Error 2
make: *** [docs] Error 2

@fritzo
Copy link
Member

fritzo commented Jul 24, 2023

Hey @rtviii i worked out the errors you discovered and I think things are working in #3246. Feel free to merge those changes into this PR or we can close this PR and merge that, either way.

@rtviii
Copy link
Contributor Author

rtviii commented Jul 25, 2023

Closing in favor of #3246

@rtviii rtviii closed this Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants