Opened 2 years ago

Closed 2 years ago

#26949 closed defect (fixed)

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

Reported by: klee Owned by:
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: Merged in:
Authors: Kwankyu Lee Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 16d81eb (Commits, GitHub, GitLab) Commit: 16d81eb2f945aa75a3aefccd71c5e455dba0a44d
Dependencies: Stopgaps:

Status badges

Description (last modified by klee)

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.

Change History (80)

comment:1 Changed 2 years ago by klee

Last edited 2 years ago by klee (previous) (diff)

comment:2 Changed 2 years ago by klee

  • Description modified (diff)

comment:3 Changed 2 years ago by klee

  • Authors set to Kwankyu Lee
  • Branch set to u/klee/26949
  • Commit set to 808579c7df14e62eebf550cbb43b913156952a85

New commits:

808579cMake sage autodoc work for both py2 and py3

comment:4 Changed 2 years ago by klee

  • Component changed from documentation to python3

comment:5 Changed 2 years ago by klee

  • Status changed from new to needs_review

comment:6 Changed 2 years ago by klee

  • Summary changed from Make sage autodoc extension work for both python2 and python3 to py3: Make sage autodoc extension work for both python2 and python3

comment:7 Changed 2 years ago by klee

  • Summary changed from py3: Make sage autodoc extension work for both python2 and python3 to py3: make sage autodoc extension work for both python2 and python3

comment:8 Changed 2 years ago by strogdon

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.

comment:9 follow-up: Changed 2 years ago by 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.

comment:10 follow-up: Changed 2 years ago by 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).

comment:11 in reply to: ↑ 10 Changed 2 years ago by jhpalmieri

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.

comment:12 in reply to: ↑ 9 Changed 2 years ago by klee

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))

        ...

comment:13 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

comment:14 Changed 2 years ago by jdemeyer

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.

comment:15 Changed 2 years ago by git

  • Commit changed from 808579c7df14e62eebf550cbb43b913156952a85 to fb41ff74528f50996b618bf31de6da6374e44361

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

fb41ff7Have one sage_autodoc for py2 and py3

comment:16 Changed 2 years ago by klee

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:17 Changed 2 years ago by git

  • Commit changed from fb41ff74528f50996b618bf31de6da6374e44361 to 2796bde886b2a5b4ebe3057458b4470c74170730

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

2796bdeRemove scaffolds used for debugging

comment:18 Changed 2 years ago by strogdon

A cursory look at a python 3 build of the docs looks OK. Now for python 2.

comment:19 in reply to: ↑ description ; follow-up: Changed 2 years ago by jdemeyer

Replying to klee:

trimmed a lot to be in sync well with Sphinx' orthodox autodoc.

Can you justify this? In particular, which "orthodox autodoc" are you talking about? Given that Sage is not using the latest Sphinx (see #26451 for that), this is a relevant question.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 2 years ago by klee

Replying to jdemeyer:

Replying to klee:

trimmed a lot to be in sync well with Sphinx' orthodox autodoc.

Can you justify this? In particular, which "orthodox autodoc" are you talking about? Given that Sage is not using the latest Sphinx (see #26451 for that), this is a relevant question.

I just meant the one currently shipped with Sage. So not the latest.

You know what I mean by "sync well" if you "vimdiff" old sage_autodoc and then the new sage_autodoc, against Sphinx' ext/autodoc/__init__py.

comment:21 Changed 2 years ago by git

  • Commit changed from 2796bde886b2a5b4ebe3057458b4470c74170730 to f2cb19948089a96db237ab74cfdec8ea7c0f71d3

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

f2cb199Some pyflakes fixes

comment:22 Changed 2 years ago by git

  • Commit changed from f2cb19948089a96db237ab74cfdec8ea7c0f71d3 to 16849f30085975888c3a747d1449f5ba78107146

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

16849f3Another pyflakes fix

comment:23 Changed 2 years ago by jdemeyer

Please rebase to 8.5 and squash commits to just one commit.

comment:24 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:25 Changed 2 years ago by git

  • Commit changed from 16849f30085975888c3a747d1449f5ba78107146 to bd729605a55d59319d0cf9f5b0cdc9e7ac83c9a9

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

bd72960Makes sage autodoc work for both py2 and py3

comment:26 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

sage.combinat.diagram_algebras.AbstractPartitionDiagrams.Element is now documented as "alias of InheritComparisonClasscallMetaclass" which is not very informative.

This used to be "alias of AbstractPartitionDiagram".

comment:27 in reply to: ↑ 20 Changed 2 years ago by jdemeyer

Replying to klee:

I just meant the one currently shipped with Sage. So not the latest.

The most reasonable comparison is probably with Sphinx 1.7.9, the lastest bugfix of the version currently in Sage.

comment:28 Changed 2 years ago by git

  • Commit changed from bd729605a55d59319d0cf9f5b0cdc9e7ac83c9a9 to 842074c436c5ea4c35ebe8595e509e1332d28988

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

842074cFix wrong-named aliases

comment:29 Changed 2 years ago by klee

  • Status changed from needs_work to needs_review

comment:30 Changed 2 years ago by jdemeyer

Thanks! I'll have a deeper look at what's going wrong with __qualname__.

comment:31 Changed 2 years ago by jdemeyer

The "problem" seems to be that Cython defines an ordinary attribute __qualname__ on the metaclass. I'm not entirely convinced that your fix is the right thing to do, I'll need to think about it.

comment:32 follow-up: Changed 2 years ago by chapoton

see also #26319

comment:33 follow-up: Changed 2 years ago by jdemeyer

I consider the __qualname__ issue a Cython bug: https://github.com/cython/cython/issues/2772

Since the issue is almost certainly not limited to InheritComparisonMetaclass, it's pointless to work around it only there.

Instead, I suggest to simply not use __qualname__ in autodoc until this Cython bug is fixed.

comment:34 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:35 in reply to: ↑ 32 Changed 2 years ago by jdemeyer

Replying to chapoton:

see also #26319

That is a different unrelated problem with __qualname__

comment:36 Changed 2 years ago by git

  • Commit changed from 842074c436c5ea4c35ebe8595e509e1332d28988 to 630626bb7183b1b579f57be8d32e7c27014d551e

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

630626bDo not rely on __qualname__ for python2

comment:37 in reply to: ↑ 33 Changed 2 years ago by klee

Replying to jdemeyer:

I consider the __qualname__ issue a Cython bug: https://github.com/cython/cython/issues/2772

Since the issue is almost certainly not limited to InheritComparisonMetaclass, it's pointless to work around it only there.

Right.

Instead, I suggest to simply not use __qualname__ in autodoc until this Cython bug is fixed.

Done. Thanks.

comment:38 Changed 2 years ago by klee

  • Status changed from needs_work to needs_review

comment:39 Changed 2 years ago by jdemeyer

  • Branch changed from u/klee/26949 to u/jdemeyer/26949

comment:40 Changed 2 years ago by jdemeyer

  • Commit changed from 630626bb7183b1b579f57be8d32e7c27014d551e to e22f78b3cb8825377170348f0f31ae3d12f14597

Rebased to 8.6.beta0 and squashed.


New commits:

e22f78bMakes sage autodoc work for both py2 and py3

comment:41 Changed 2 years ago by jdemeyer

Building the docs now...

comment:42 Changed 2 years ago by jdemeyer

Since the change involving __qualname__ is a functional change, I decided to deal with that in a separate ticket: #27002

comment:43 Changed 2 years ago by git

  • Commit changed from e22f78b3cb8825377170348f0f31ae3d12f14597 to 1b4d0f474cc28d4cf7bf25f8f3b13aca46122d1b

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

1b4d0f4Makes sage autodoc work for both py2 and py3

comment:44 follow-up: Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Some things are no longer documented. For example sage.schemes.elliptic_curves.padics.padic_lseries which is a CachedMethod.

comment:45 in reply to: ↑ 44 ; follow-up: Changed 2 years ago by klee

Replying to jdemeyer:

Some things are no longer documented. For example sage.schemes.elliptic_curves.padics.padic_lseries which is a CachedMethod.

You might have noticed that these cached functions have wrong documentations already with the current sage. With the present patch, they now have no documentation. We are just seeing different manifestations of an issue with cached functions. Fixing this issue with cached functions seems to be out of the scope of the present ticket. I may open a ticket to tackle this issue if you agree.

comment:46 in reply to: ↑ 45 Changed 2 years ago by klee

Replying to klee:

Replying to jdemeyer:

Some things are no longer documented. For example sage.schemes.elliptic_curves.padics.padic_lseries which is a CachedMethod.

You might have noticed that these cached functions have wrong documentations already with the current sage. With the present patch, they now have no documentation.

This is right.

We are just seeing different manifestations of an issue with cached functions.

This is wrong. There is no issue with cached functions! All functions defined in sage.schemes.elliptic_curves.padics are imported and embedded into the class EllipticCurve_rational_field defined in sage.schemes.elliptic_curves.padics.ell_rational_field. Thus the functions are well documented as methods in the documentation of the class. Hence there is no need to (should not) document the functions in padics.py in duplicate!

Fixing this issue with cached functions seems to be out of the scope of the present ticket. I may open a ticket to tackle this issue if you agree.

What should be done is to remove the article Miscellaneous p-adic functions from the Sage documentation. I may open a ticket for this, if you agree.

comment:47 Changed 2 years ago by klee

  • Status changed from needs_work to needs_review

comment:48 follow-up: Changed 2 years ago by klee

  • Branch changed from u/jdemeyer/26949 to u/klee/26949
  • Commit changed from 1b4d0f474cc28d4cf7bf25f8f3b13aca46122d1b to fe42da1310513ea1f1f79147b4ba1ed9525ef4ce

Rebased on sage 8.6.rc0 and removed a few unuseful comments like # type: ...


New commits:

fe42da1Make sage autodoc work for both py2 and py3

comment:49 Changed 2 years ago by jdemeyer

Fair enough regarding padic_lseries.

Still, there is more documentation which is removed. For example sage.combinat.finite_state_machine.FiniteStateMachine.output_alphabet

In the sources, there is

    output_alphabet = None
    """
    A list of letters representing the output alphabet of the finite
    state machine.

    It can be set by the parameter ``output_alphabet`` when initializing
    a finite state machine, see :class:`FiniteStateMachine`.

    It can also be set by the method :meth:`determine_alphabets`.

    .. SEEALSO::

        :class:`FiniteStateMachine`,
        :meth:`determine_alphabets`,
        :attr:`input_alphabet`.
    """

and apparently Sphinx parses this correctly before this patch but not after this patch.

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:50 in reply to: ↑ 48 ; follow-up: Changed 2 years ago by jdemeyer

Replying to klee:

removed a few unuseful comments like # type: ...

If those comments appear in the original, it's more maintainable to keep them.

comment:51 in reply to: ↑ 50 ; follow-up: Changed 2 years ago by klee

Replying to jdemeyer:

Replying to klee:

removed a few unuseful comments like # type: ...

If those comments appear in the original, it's more maintainable to keep them.

The old sage_autodoc didn't keep those comments. It seems to be a convention of Sphinx, but absolutely not of Sage. Anyway I don't care. Do you prefer to follow Sphinx?

comment:52 Changed 2 years ago by git

  • Commit changed from fe42da1310513ea1f1f79147b4ba1ed9525ef4ce to 3aefb2b297d8ccf04f4f30691a778765ccd7a6b5

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

3aefb2bRecover missing attributes

comment:53 in reply to: ↑ 51 Changed 2 years ago by jdemeyer

Replying to klee:

It seems to be a convention of Sphinx, but absolutely not of Sage.

It's not just a Sphinx thing: https://www.python.org/dev/peps/pep-0484/#suggested-syntax-for-python-2-7-and-straddling-code

Do you prefer to follow Sphinx?

Yes. More generally, we should stay as close to upstream Sphinx as possible.

comment:54 follow-up: Changed 2 years ago by klee

I am not sure if my last commit will finally make everything right or break something somewhere. This business is going to be close to blind trials...

Anyway personally I could not afford more trials if anything breaks, for some weeks. Sorry.

comment:55 in reply to: ↑ 54 Changed 2 years ago by jdemeyer

Replying to klee:

Anyway personally I could not afford more trials if anything breaks, for some weeks. Sorry.

No need to apologize. You already did very good work!

Good to know that you'll be inactive. I'll try to fix any issues myself.

comment:56 Changed 2 years ago by jdemeyer

I'm happy enough with the generated documentation now.

comment:57 Changed 2 years ago by git

  • Commit changed from 3aefb2b297d8ccf04f4f30691a778765ccd7a6b5 to bdb5063567a9e5cffd416d64d94a0d0b204c95e7

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

bdb5063Increase sync rate with Sphinx autodoc of ver 1.7.6

comment:58 Changed 2 years ago by klee

The last commit should not affect documentation. Tried a tiny bit to prepare for Sphinx 1.8.

comment:59 follow-up: Changed 2 years ago by jdemeyer

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

comment:60 in reply to: ↑ 59 Changed 2 years ago by klee

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.

comment:61 Changed 2 years ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

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.

comment:62 follow-up: Changed 2 years ago by chapoton

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

comment:63 in reply to: ↑ 62 Changed 2 years ago by klee

Replying to chapoton:

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

Yes. I am waiting for a final review.

comment:64 follow-up: Changed 2 years ago by chapoton

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

comment:65 Changed 2 years ago by jdemeyer

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

comment:66 in reply to: ↑ 64 Changed 2 years ago by klee

Replying to chapoton:

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.

comment:67 Changed 2 years ago by git

  • Commit changed from bdb5063567a9e5cffd416d64d94a0d0b204c95e7 to 4cece146efb583fa775b8ba20565af62996f770c

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

4cece14Remove unused imports

comment:68 follow-ups: Changed 2 years ago by jdemeyer

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

comment:69 in reply to: ↑ 68 Changed 2 years ago by klee

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?

comment:70 follow-up: Changed 2 years ago by 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.

comment:71 in reply to: ↑ 70 Changed 2 years ago by klee

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.

comment:72 Changed 2 years ago by jhpalmieri

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.

comment:73 Changed 2 years ago by jdemeyer

  • Branch changed from u/klee/26949 to u/jdemeyer/26949

comment:74 Changed 2 years ago by git

  • Commit changed from 4cece146efb583fa775b8ba20565af62996f770c to 16d81eb2f945aa75a3aefccd71c5e455dba0a44d

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

16d81ebMake sage autodoc work for both py2 and py3

comment:75 Changed 2 years ago by jdemeyer

Reverted last commit and squashed the other commits.

comment:76 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:77 Changed 2 years ago by klee

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

comment:78 in reply to: ↑ 68 ; follow-up: Changed 2 years ago by embray

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.

comment:79 in reply to: ↑ 78 Changed 2 years ago by jdemeyer

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.

comment:80 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/26949 to 16d81eb2f945aa75a3aefccd71c5e455dba0a44d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.