py3: make sage autodoc extension work for both python2 and python3

[kwankyu]

Sage using python3 presently fails to build the sage documentation. This is because the current sage autodoc extension does not properly work with python3. This ticket provides new sage_autodoc that works for both python2 and python3.

The new sage_autodoc is based on the existing sage_autodoc but trimmed a lot to be in sync well with Sphinx' orthodox autodoc. This is to make it more maintainable by clarifying modifications made by Sage to Sphinx autodoc extension.

Component: python3

Author: Kwankyu Lee

Branch/Commit: 16d81eb

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/26949

[kwankyu]

Branch: u/klee/26949

[kwankyu]

New commits:

808579c

Make sage autodoc work for both py2 and py3

[kwankyu]

Author: Kwankyu Lee

[kwankyu]

Commit: 808579c

[strogdon]

comment:8

I had always thought the issue was with sage_autodoc.py but I didn't have the python 3 background to attempt a change. I'll give it a try.

[jhpalmieri]

comment:9

This is good progress. Methods in the Python 3 documentation look like method(self):, while in the Python 2 version, they look like method(). I don't object to that, but do we know why this change has happened? More significantly, in the Python 3 version, it seems that if a method has a decorator like @cached_method or @abstract_method, then the signature is gone completely and it just says method.

[strogdon]

comment:10

Also under Sets
an_element() with Python 2 appears as an_element with Python 3. This may be due to confusing py:method (Python 2) with py:attribute (Python 3).

[jhpalmieri]

comment:11

Replying to @strogdon:

Also under Sets
an_element() with Python 2 appears as an_element with Python 3. This may be due to confusing py:method (Python 2) with py:attribute (Python 3).

I was guessing that this was because an_element is marked as @cached_method.

[kwankyu]

comment:12

Replying to @jhpalmieri:

This is good progress. Methods in the Python 3 documentation look like method(self):, while in the Python 2 version, they look like method(). I don't object to that, but do we know why this change has happened? More significantly, in the Python 3 version, it seems that if a method has a decorator like @cached_method or @abstract_method, then the signature is gone completely and it just says method.

I expected these subtle differences. I cannot explain the cause of the differences, but know where to look. You can start with

vimdiff src/sage_setup/docbuild/ext/sage_autodoc3.py local/lib/python2.7/site-packages/sphinx/ext/autodoc/__init__.py

and perhaps also with comparing sage_autodoc3 with the original sage_autodoc.

I would welcome commits to fix these unpleasant changes, after switching to a public branch.

To understand how an attribute (in the general sense, of a module or of a class) is rendered by Sphinx, note that a suitable Documenter is selected for each attribute in the last lines of the following (in sage_autodoc):

    def document_members(self, all_members=False):
        """Generate reST for member documentation.

        If *all_members* is True, do all members, else those given by
        *self.options.members*.
        """
        # set current namespace for finding members
        self.env.temp_data['autodoc:module'] = self.modname
        if self.objpath:
            self.env.temp_data['autodoc:class'] = self.objpath[0]

        want_all = all_members or self.options.inherited_members or \
            self.options.members is ALL
        # find out which members are documentable
        members_check_module, members = self.get_object_members(want_all)

        # remove members given by exclude-members
        if self.options.exclude_members:
            members = [(membername, member) for (membername, member) in members
                       if membername not in self.options.exclude_members]

        # document non-skipped members
        memberdocumenters = []
        for (mname, member, isattr) in self.filter_members(members, want_all): <------------------------
            classes = [cls for cls in itervalues(self.documenters)
                       if cls.can_document_member(member, mname, isattr, self)]  <----------------------
            if not classes:
                # don't know how to document this member
                continue
            # prefer the documenter with the highest priority
            classes.sort(key=lambda cls: cls.priority)                 <----------------------
            # give explicitly separated module name, so that members
            # of inner classes can be documented
            full_mname = self.modname + '::' + \
                '.'.join(self.objpath + [mname])
            documenter = classes[-1](self.directive, full_mname, self.indent)               <---------- look here

            memberdocumenters.append((documenter, isattr))

        ...

[jdemeyer]

comment:13

While making things compatible with Python 3 is a good goal, having two completely independent implementations of it is a horrible solution. Then we would have two autodoc extensions to maintain instead of one.

[jdemeyer]

comment:14

Also, I would personally prefer to wait with this until after #26451. But it's partially my fault for dragging that, so consider this just a suggestion.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

fb41ff7

Have one sage_autodoc for py2 and py3

[sagetrac-git]

Changed commit from 808579c to fb41ff7

[sagetrac-git]

Changed commit from fb41ff7 to 2796bde

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

2796bde

Remove scaffolds used for debugging

[jdemeyer]

comment:59

OK, do you plan further changes? I'll just need to know when I can review the final version of this branch.

[kwankyu]

comment:60

Replying to @jdemeyer:

OK, do you plan further changes? I'll just need to know when I can review the final version of this branch.

No. Please proceed.

[embray]

comment:61

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

[fchapoton]

comment:62

any progress here ? Will this fix make doc for python3 ?

[kwankyu]

comment:63

Replying to @fchapoton:

any progress here ? Will this fix make doc for python3 ?

Yes. I am waiting for a final review.

[fchapoton]

comment:64

could you please check these pyflakes warnings (maybe not pertinent)

+src/sage_setup/docbuild/ext/sage_autodoc.py:33: 'warnings' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:36: 'six.iteritems' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:39: 'sphinx.errors.ExtensionError' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:41: 'sphinx.ext.autodoc.inspector.format_annotation' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:47: 'sphinx.util.inspect.Signature' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:50: 'sphinx.util.inspect.getargspec' imported but unused

[jdemeyer]

comment:65

Sorry, I forgot about this ticket. I will review it. Please do not make further changes.

[kwankyu]

comment:66

Replying to @fchapoton:

could you please check these pyflakes warnings (maybe not pertinent)

+src/sage_setup/docbuild/ext/sage_autodoc.py:33: 'warnings' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:36: 'six.iteritems' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:39: 'sphinx.errors.ExtensionError' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:41: 'sphinx.ext.autodoc.inspector.format_annotation' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:47: 'sphinx.util.inspect.Signature' imported but unused
+src/sage_setup/docbuild/ext/sage_autodoc.py:50: 'sphinx.util.inspect.getargspec' imported but unused

Fixed all pyflakes warnings, except one aboutsphinx.util.inspect.Signature. Signature is imported in the original Sphinx autodoc, and I think is very likely to be used in future, perhaps in #26451 or in #26254.

[sagetrac-git]

Changed commit from bdb5063 to 4cece14

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

4cece14

Remove unused imports

[jdemeyer]

comment:68

I explicitly asked not to make further changes, so I will revert that last commit.

[kwankyu]

comment:69

Replying to @jdemeyer:

I explicitly asked not to make further changes, so I will revert that last commit.

Ok with that. But I am curious why the small, rather insignificant, change like the last commit is unacceptable to you. It seems not a big deal to me... Is there a technical reason that I miss?

[jdemeyer]

comment:70

I just don't want to review a moving target. The change itself may or may not be fine but at some point the branch needs to be fixed. For example, I would need to review the fact that the change is as innocent as you claim it to be.

[kwankyu]

comment:71

Replying to @jdemeyer:

I just don't want to review a moving target. The change itself may or may not be fine but at some point the branch needs to be fixed. For example, I would need to review the fact that the change is as innocent as you claim it to be.

I agree in general. Ok.

[jhpalmieri]

comment:72

I know that everyone is in agreement, but if you want actual evidence: in #16298, an import was apparently unused anywhere, so removing it was recommended by pyflakes and looked innocent to all parties involved, but removing it actually broke things. So (a) pyflakes can be wrong, and (b) "innocent" changes may not actually be innocent.

[jdemeyer]

Changed branch from u/klee/26949 to u/jdemeyer/26949

[sagetrac-git]

Changed commit from 4cece14 to 16d81eb

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

16d81eb

Make sage autodoc work for both py2 and py3

[jdemeyer]

comment:75

Reverted last commit and squashed the other commits.

[kwankyu]

comment:77

Thank you. I wish that now with this patch, make builds the documentation also on python3!

[embray]

comment:78

Replying to @jdemeyer:

I explicitly asked not to make further changes, so I will revert that last commit.

I understand your reasoning, but given that the commit was made and you hadn't actually (as far as anyone can tell) begun the review the change, why not just incorporate it into your review? It does look pretty harmless to me and, while it should still have been checked, really doesn't add significant burden to review.

[jdemeyer]

comment:79

Replying to @embray:

you hadn't actually (as far as anyone can tell) begun the review the change,

I thought that "I will review it. Please do not make further changes." was clear enough. So yes, I was already building the documentation when that additional commit was pushed.

Can we please not make further fuss about this? This ticket has positive review now, let's keep it that way. It's not perfect and that's fine. And the pyflakes warnings are the least of my worries with the Sage docbuilder.

[vbraun]

Changed branch from u/jdemeyer/26949 to 16d81eb