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

Fix Numba decorated function docstrings #173

Merged
merged 10 commits into from
May 22, 2020
Merged

Fix Numba decorated function docstrings #173

merged 10 commits into from
May 22, 2020

Conversation

thisac
Copy link
Contributor

@thisac thisac commented May 21, 2020

Context:
The generated docstrings for functions/methods decorated with Numba's @jit decorator didn't render properly since:

  1. The decorator obscured the signature to Sphinx.

  2. numba and numba.jit were mocked out inconf.py causing magicmock to provide its docstring instead of numba (which actually provides the correct, original docstring).

Description of the Change:

  1. An autodoc-signature-process function was added to conf.py that checks whether the function is a Numba object, and in that case retrieves the original signature and forwards it to Sphinx.

  2. The mocked out functions/modules numba and numba.jit are removed from conf.py, letting the correct docstrings be rendered.

  3. The mocked out functions/modules numpy and numpy.dtype are removed from conf.py, letting the correct dtype be rendered inside the signatures.

Also fixed a few docstrings that weren't rendered correctly due to missing line-breaks.

Benefits:
The docstrings and signatures now render as they should.

Possible Drawbacks:
I'm not sure exactly why numpy and numba were mocked out in conf.py (nor the other funtions/method), so I might have broken something on the way. The documentation rendered nicely locally though.

Related GitHub Issues:
N/A

@thisac thisac self-assigned this May 21, 2020
@thisac thisac requested review from josh146 and nquesada May 21, 2020 01:43
@thisac thisac added bug Something isn't working review-ready labels May 21, 2020
@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #173 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #173   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1002      1002           
=========================================
  Hits          1002      1002           
Impacted Files Coverage Δ
thewalrus/csamples.py 100.00% <ø> (ø)
thewalrus/fock_gradients.py 100.00% <ø> (ø)
thewalrus/samples.py 100.00% <ø> (ø)
thewalrus/symplectic.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf7a19e...8d203f7. Read the comment docs.

Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks for fixing this @thisac!

Since this requires that Numba not be mocked out, one thing we'll have to double check is if Numba is installable on readthedocs. I've created a hidden build of this branch here: https://hafnian.readthedocs.io/en/numba_fix_decorator, and we can track builds here: https://readthedocs.org/projects/hafnian/builds/

docs/conf.py Outdated
Comment on lines 40 to 51
MOCK_MODULES = [
'numpy',
'numpy.dtype',
# 'numpy',
# 'numpy.dtype',
'scipy',
'scipy.special',
'scipy.optimize',
'scipy.stats',
'cython',
'thewalrus.libwalrus',
'numba',
'numba.jit'
# 'numba',
# 'numba.jit'
]
Copy link
Member

Choose a reason for hiding this comment

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

There were originally several reasons why these modules are mocked out:

  • There is a bug in Sphinx (it might be fixed now) where Sphinx-autodoc would mistakenly include NumPy function documentation in projects that simply import it. This happened in Strawberry Fields, hence why it is probably mocked out here. I think it's okay to un-mock it now.

  • Mocking allows Sphinx to import the project and inspect docstrings without requiring the dependencies be installed. This is quite important for 'thewalrus.libwalrus', as we don't want to have to build the C++ code on the RTDs environment.

Copy link
Member

Choose a reason for hiding this comment

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

I've noticed that scipy and numpy are now in the doc/requirements.txt, so are being installed on read the docs. So maybe SciPy doesn't have to be mocked any more either.

docs/conf.py Outdated
Comment on lines 344 to 360
def process_numba_signature(app, what, name, obj, options, signature, return_annotation):
if isinstance(obj, numba.targets.registry.CPUDispatcher):
original = obj.py_func
orig_sig = inspect.signature(original)

if (orig_sig.return_annotation) is inspect._empty:
ret_ann = None
else:
ret_ann = orig_sig.return_annotation.__name__

return (str(orig_sig), ret_ann)

return (signature, return_annotation)


def setup(app):
app.connect('autodoc-process-signature', process_numba_signature)
Copy link
Member

Choose a reason for hiding this comment

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

Awesome that this works!

@josh146
Copy link
Member

josh146 commented May 21, 2020

https://github.com/numba/llvmlite/blob/c4f5a085c9f9c7a192579b02a11347bfc2ac768b/setup.py#L25

Looks like llvmlite disables installation on readthedocs environments :(

@josh146
Copy link
Member

josh146 commented May 21, 2020

@thisac: I updated the .readthedocs.yml file to use Python 3.6, and this avoids llvmlite/setup.py from running, since the binary wheel is available.

But RTDs then runs into a different error:

  File "/home/docs/checkouts/readthedocs.org/user_builds/hafnian/checkouts/fix_numba_decorator/docs/conf.py", line 345, in process_numba_signature
    if isinstance(obj, numba.targets.registry.CPUDispatcher):
AttributeError: module 'numba' has no attribute 'targets'

See the full build log here: https://readthedocs.org/projects/hafnian/builds/11086294/

Edit: could something have changed in the latest Numba release? What version of Numba do you have installed locally?

@thisac
Copy link
Contributor Author

thisac commented May 21, 2020

@thisac: I updated the .readthedocs.yml file to use Python 3.6, and this avoids llvmlite/setup.py from running, since the binary wheel is available.

But RTDs then runs into a different error:

  File "/home/docs/checkouts/readthedocs.org/user_builds/hafnian/checkouts/fix_numba_decorator/docs/conf.py", line 345, in process_numba_signature
    if isinstance(obj, numba.targets.registry.CPUDispatcher):
AttributeError: module 'numba' has no attribute 'targets'

See the full build log here: https://readthedocs.org/projects/hafnian/builds/11086294/

Edit: could something have changed in the latest Numba release? What version of Numba do you have installed locally?

Thanks @josh146. I've fixed the AttributeError: module 'numba' has no attribute 'targets' by updating the requirements for Numba to numba>=0.49.1. I believe I could fix the mocking issue, although, as you mentioned, it might not be necessary, so I've left it as it is at the moment.

@nquesada
Copy link
Collaborator

hey @thisac : This is looking pretty good. There is one minor possibly related issue and is that the modules quantum and samples are no longer listed in The Walrus API:

image

@thisac thisac requested a review from josh146 May 22, 2020 14:06
Copy link
Member

@josh146 josh146 left a comment

Choose a reason for hiding this comment

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

Looks good @thisac, and nice fix! Note that you will need to add scipy to the list of documentation requirements so that it will build on read the docs (you can check the build console here: https://readthedocs.org/projects/hafnian/builds/).

Once the fix_numba_decorator is passing, feel free to merge this in 💯

docs/conf.py Outdated
Comment on lines 41 to 46
# 'numpy',
# 'numpy.dtype',
# 'scipy',
# 'scipy.special',
# 'scipy.optimize',
# 'scipy.stats',
Copy link
Member

Choose a reason for hiding this comment

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

Safe to delete all of these :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

docs/requirements.txt Show resolved Hide resolved
thisac and others added 2 commits May 22, 2020 10:25
Co-authored-by: Josh Izaac <josh146@gmail.com>
@thisac thisac merged commit e1db968 into master May 22, 2020
@thisac thisac deleted the fix_numba_decorator branch May 22, 2020 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working review-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants