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:  sage8.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: 
Description (last modified by )
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
comment:2 Changed 2 years ago by
 Description modified (diff)
comment:3 Changed 2 years ago by
 Branch set to u/klee/26949
 Commit set to 808579c7df14e62eebf550cbb43b913156952a85
comment:4 Changed 2 years ago by
 Component changed from documentation to python3
comment:5 Changed 2 years ago by
 Status changed from new to needs_review
comment:6 Changed 2 years ago by
 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
 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
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 followup: ↓ 12 Changed 2 years ago by
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 followup: ↓ 11 Changed 2 years ago by
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
comment:12 in reply to: ↑ 9 Changed 2 years ago by
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 likemethod()
. 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 saysmethod
.
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/sitepackages/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 excludemembers if self.options.exclude_members: members = [(membername, member) for (membername, member) in members if membername not in self.options.exclude_members] # document nonskipped 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
 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
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
 Commit changed from 808579c7df14e62eebf550cbb43b913156952a85 to fb41ff74528f50996b618bf31de6da6374e44361
Branch pushed to git repo; I updated commit sha1. New commits:
fb41ff7  Have one sage_autodoc for py2 and py3

comment:16 Changed 2 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:17 Changed 2 years ago by
 Commit changed from fb41ff74528f50996b618bf31de6da6374e44361 to 2796bde886b2a5b4ebe3057458b4470c74170730
Branch pushed to git repo; I updated commit sha1. New commits:
2796bde  Remove scaffolds used for debugging

comment:18 Changed 2 years ago by
A cursory look at a python 3 build of the docs looks OK. Now for python 2.
comment:19 in reply to: ↑ description ; followup: ↓ 20 Changed 2 years ago by
comment:20 in reply to: ↑ 19 ; followup: ↓ 27 Changed 2 years ago by
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
 Commit changed from 2796bde886b2a5b4ebe3057458b4470c74170730 to f2cb19948089a96db237ab74cfdec8ea7c0f71d3
Branch pushed to git repo; I updated commit sha1. New commits:
f2cb199  Some pyflakes fixes

comment:22 Changed 2 years ago by
 Commit changed from f2cb19948089a96db237ab74cfdec8ea7c0f71d3 to 16849f30085975888c3a747d1449f5ba78107146
Branch pushed to git repo; I updated commit sha1. New commits:
16849f3  Another pyflakes fix

comment:23 Changed 2 years ago by
Please rebase to 8.5 and squash commits to just one commit.
comment:24 Changed 2 years ago by
 Reviewers set to Jeroen Demeyer
comment:25 Changed 2 years ago by
 Commit changed from 16849f30085975888c3a747d1449f5ba78107146 to bd729605a55d59319d0cf9f5b0cdc9e7ac83c9a9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bd72960  Makes sage autodoc work for both py2 and py3

comment:26 Changed 2 years ago by
 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
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
 Commit changed from bd729605a55d59319d0cf9f5b0cdc9e7ac83c9a9 to 842074c436c5ea4c35ebe8595e509e1332d28988
Branch pushed to git repo; I updated commit sha1. New commits:
842074c  Fix wrongnamed aliases

comment:29 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:30 Changed 2 years ago by
Thanks! I'll have a deeper look at what's going wrong with __qualname__
.
comment:31 Changed 2 years ago by
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 followup: ↓ 35 Changed 2 years ago by
see also #26319
comment:33 followup: ↓ 37 Changed 2 years ago by
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
 Status changed from needs_review to needs_work
comment:35 in reply to: ↑ 32 Changed 2 years ago by
comment:36 Changed 2 years ago by
 Commit changed from 842074c436c5ea4c35ebe8595e509e1332d28988 to 630626bb7183b1b579f57be8d32e7c27014d551e
Branch pushed to git repo; I updated commit sha1. New commits:
630626b  Do not rely on __qualname__ for python2

comment:37 in reply to: ↑ 33 Changed 2 years ago by
Replying to jdemeyer:
I consider the
__qualname__
issue a Cython bug: https://github.com/cython/cython/issues/2772Since 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
 Status changed from needs_work to needs_review
comment:39 Changed 2 years ago by
 Branch changed from u/klee/26949 to u/jdemeyer/26949
comment:40 Changed 2 years ago by
 Commit changed from 630626bb7183b1b579f57be8d32e7c27014d551e to e22f78b3cb8825377170348f0f31ae3d12f14597
Rebased to 8.6.beta0 and squashed.
New commits:
e22f78b  Makes sage autodoc work for both py2 and py3

comment:41 Changed 2 years ago by
Building the docs now...
comment:42 Changed 2 years ago by
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
 Commit changed from e22f78b3cb8825377170348f0f31ae3d12f14597 to 1b4d0f474cc28d4cf7bf25f8f3b13aca46122d1b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1b4d0f4  Makes sage autodoc work for both py2 and py3

comment:44 followup: ↓ 45 Changed 2 years ago by
 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 ; followup: ↓ 46 Changed 2 years ago by
Replying to jdemeyer:
Some things are no longer documented. For example
sage.schemes.elliptic_curves.padics.padic_lseries
which is aCachedMethod
.
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
Replying to klee:
Replying to jdemeyer:
Some things are no longer documented. For example
sage.schemes.elliptic_curves.padics.padic_lseries
which is aCachedMethod
.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 padic functions
from the Sage documentation. I may open a ticket for this, if you agree.
comment:47 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:48 followup: ↓ 50 Changed 2 years ago by
 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:
fe42da1  Make sage autodoc work for both py2 and py3

comment:49 Changed 2 years ago by
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.
comment:50 in reply to: ↑ 48 ; followup: ↓ 51 Changed 2 years ago by
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 ; followup: ↓ 53 Changed 2 years ago by
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
 Commit changed from fe42da1310513ea1f1f79147b4ba1ed9525ef4ce to 3aefb2b297d8ccf04f4f30691a778765ccd7a6b5
Branch pushed to git repo; I updated commit sha1. New commits:
3aefb2b  Recover missing attributes

comment:53 in reply to: ↑ 51 Changed 2 years ago by
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/pep0484/#suggestedsyntaxforpython27andstraddlingcode
Do you prefer to follow Sphinx?
Yes. More generally, we should stay as close to upstream Sphinx as possible.
comment:54 followup: ↓ 55 Changed 2 years ago by
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
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
I'm happy enough with the generated documentation now.
comment:57 Changed 2 years ago by
 Commit changed from 3aefb2b297d8ccf04f4f30691a778765ccd7a6b5 to bdb5063567a9e5cffd416d64d94a0d0b204c95e7
Branch pushed to git repo; I updated commit sha1. New commits:
bdb5063  Increase sync rate with Sphinx autodoc of ver 1.7.6

comment:58 Changed 2 years ago by
The last commit should not affect documentation. Tried a tiny bit to prepare for Sphinx 1.8.
comment:59 followup: ↓ 60 Changed 2 years ago by
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
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
 Milestone changed from sage8.6 to sage8.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 sagepending or sagewishlist.
comment:62 followup: ↓ 63 Changed 2 years ago by
any progress here ? Will this fix make doc
for python3 ?
comment:63 in reply to: ↑ 62 Changed 2 years ago by
Replying to chapoton:
any progress here ? Will this fix
make doc
for python3 ?
Yes. I am waiting for a final review.
comment:64 followup: ↓ 66 Changed 2 years ago by
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
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
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
 Commit changed from bdb5063567a9e5cffd416d64d94a0d0b204c95e7 to 4cece146efb583fa775b8ba20565af62996f770c
Branch pushed to git repo; I updated commit sha1. New commits:
4cece14  Remove unused imports

comment:68 followups: ↓ 69 ↓ 78 Changed 2 years ago by
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
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 followup: ↓ 71 Changed 2 years ago by
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
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
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
 Branch changed from u/klee/26949 to u/jdemeyer/26949
comment:74 Changed 2 years ago by
 Commit changed from 4cece146efb583fa775b8ba20565af62996f770c to 16d81eb2f945aa75a3aefccd71c5e455dba0a44d
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

comment:75 Changed 2 years ago by
Reverted last commit and squashed the other commits.
comment:76 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:77 Changed 2 years ago by
Thank you. I wish that now with this patch, make
builds the documentation also on python3!
comment:78 in reply to: ↑ 68 ; followup: ↓ 79 Changed 2 years ago by
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
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
 Branch changed from u/jdemeyer/26949 to 16d81eb2f945aa75a3aefccd71c5e455dba0a44d
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Make sage autodoc work for both py2 and py3