Opened 10 years ago

Closed 10 years ago

#8244 closed defect (fixed)

Annoying warnings when building the HTML reference manual

Reported by: mpatel Owned by: mvngu
Priority: major Milestone: sage-4.3.4
Component: documentation Keywords:
Cc: mvngu, jhpalmieri, mhansen, craigcitro, robertwb Merged in: sage-4.3.4.alpha0
Authors: John Palmieri, Mitesh Patel Reviewers: Mitesh Patel, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Let's fix or suppress these, so that it's easier to identify new problems.

See the attachment.

Attachments (5)

refbuild.log (24.8 KB) - added by mpatel 10 years ago.
HTML reference manual docbuild warnings for 4.3.3.alpha0. Not a patch.
trac_8244-slot_wrapper_argspec.patch (957 bytes) - added by mpatel 10 years ago.
Handle *.next methods. sage repo.
trac_8244-conf-autodoc.patch (11.6 KB) - added by jhpalmieri 10 years ago.
apply on top of other patch
trac_8244-sagedoc.patch (1.9 KB) - added by jhpalmieri 10 years ago.
apply on top of other patches
trac_8244-conf-autodoc.2.patch (47.1 KB) - added by mpatel 10 years ago.
Replaces "conf_autodoc" patch.

Download all attachments as: .zip

Change History (25)

Changed 10 years ago by mpatel

HTML reference manual docbuild warnings for 4.3.3.alpha0. Not a patch.

comment:1 Changed 10 years ago by mpatel

Should we follow #6419 for the nested classes? We could use that approach for

sagenb.notebook.twist.UserToplevel.userchild_download_worksheets.zip

comment:2 Changed 10 years ago by jhpalmieri

  • Cc mhansen craigcitro added

I've looked at these a bit, but I have no idea how to fix them. Oh, except for

plot/plot3d/base.rst:6: (WARNING/2) error while formatting signature for sage.plot.plot3d.base.Graphics3d.export_jmol: Could not parse cython argspec

This is because _sage_getargspec_cython in sage.misc.sageinspect is a little broken, and fixing it might be lot of work: right now it parses arguments to Cython function by doing basic string and regular expression manipulations, in particular separating arguments by splitting the line on commas. The function export_jmol has argspec

    def export_jmol(self, filename='jmol_shape.jmol', force_reload=False,
                    zoom=100, spin=False, background=(1,1,1), stereo=False,
                    mesh=False, dots=False,
                    perspective_depth = True,
                    orientation = (-764,-346,-545,76.39), **ignored_kwds):

and so splitting at commas doesn't work. To do this right, we either need to write a good parser (which deals with nested lists, tuples, etc.), or perhaps change the whole way we deal with argspecs for Cython functions; for example, just read off the entire string of all the arguments and pass that to the appropriate function, instead of trying to break it into arguments, default values, etc. This is a pretty rare problem and it looks annoying to deal with, so I haven't felt like putting any time into it.

mhansen and craigcitro: any comments or ideas?

comment:3 Changed 10 years ago by mvngu

Ticket #8243 is a duplicate of this one.

comment:4 Changed 10 years ago by mhansen

  • Cc robertwb added

Well, Cython itself has to parse such a thing, but I don't know how easy it is go get the data we need out of Cython. Maybe Robert might have an idea?

comment:5 Changed 10 years ago by mpatel

I'm not very familiar with ASTs (or ast), but with

import ast
import inspect
import sage.misc.sageinspect as sms

class SageVisitor(ast.NodeVisitor):
    def visit_Name(self, node):
        what = node.id
        if what == 'None':
            return None
        elif what == 'True':
            return True
        elif what == 'False':
            return False
        return node.id

    def visit_Num(self, node):
        return node.n

    def visit_Str(self, node):
        return node.s

    def visit_List(self, node):
        t = []
        for n in node.elts:
            t.append(self.visit(n))
        return t

    def visit_Tuple(self, node):
        t = []
        for n in node.elts:
            t.append(self.visit(n))
        return tuple(t)

    def visit_Dict(self, node):
        d = {}
        for k, v in zip(node.keys, node.values):
            d[self.visit(k)] = self.visit(v)
        return d


def getargspec_via_ast(source):
    if not isinstance(source, basestring):
        source = sms.sage_getsource(source)

    ast_args = ast.parse(source.lstrip()).body[0].args

    args = []
    defaults = []

    for a in ast_args.args:
        args.append(SageVisitor().visit(a))

    for d in ast_args.defaults:
        defaults.append(SageVisitor().visit(d))

    return inspect.ArgSpec(args, ast_args.vararg, ast_args.kwarg,
                           tuple(defaults) if defaults else None)

I get

sage: inspect.getargspec(factor) == getargspec_via_ast(factor)
True
sage: getargspec_via_ast(sage.plot.plot3d.base.Graphics3d.export_jmol)
ArgSpec(args=['self', 'filename', 'force_reload', 'zoom', 'spin', 'background', 'stereo', 'mesh', 'dots', 'perspective_depth', 'orientation'], varargs=None, keywords='ignored_kwds', defaults=('jmol_shape.jmol', False, 100, False, (1, 1, 1), False, False, False, True, (-764, -346, -545, 76.390000000000001)))

Maybe we can use this as a last resort? I think we'd first need to remove Cython-specific constructs.

comment:6 follow-up: Changed 10 years ago by mpatel

It seems that nearly all of the warnings are from Cython files and that we really need a more robust analogue of inspect.getargspec for Cython source.

For the ElementMethods, ParentMethods, userchild_download_worksheets.zip warnings, we can expand the autodoc skip method handler.

The other warnings seem to be reST formatting problems, but I'm not sure about

<autodoc>:0: (ERROR/3) Unexpected indentation.
<autodoc>:0: (ERROR/3) Unexpected indentation.

comment:7 Changed 10 years ago by mpatel

comment:8 in reply to: ↑ 6 Changed 10 years ago by jhpalmieri

Replying to mpatel:

It seems that nearly all of the warnings are from Cython files and that we really need a more robust analogue of inspect.getargspec for Cython source.

Or for now, a way to suppress all of the error message.

I'm not sure about the <autodoc>... errors, either.

comment:9 Changed 10 years ago by mpatel

The following seems to remove the "arg is not a Python function" warnings:

  • autodoc.py

    old new class MethodDocumenter(ClassLevelDocumen 
    10051005            else:
    10061006                return None
    10071007        else:
    1008             argspec = inspect.getargspec(self.object)
     1008            # The check above misses ordinary Python methods in Cython
     1009            # files.
     1010            try:
     1011                argspec = inspect.getargspec(self.object)
     1012            except TypeError:
     1013                if (inspect.ismethod(self.object) and
     1014                    self.env.config.autodoc_builtin_argspec):
     1015                    argspec = self.env.config.autodoc_builtin_argspec(self.object.im_func)
     1016                else:
     1017                    return None
    10091018        if argspec[0] and argspec[0][0] in ('cls', 'self'):
    10101019            del argspec[0][0]
    10111020        return inspect.formatargspec(*argspec)

Should we copy autodoc.py to SAGE_DOC/common/sage_autodoc.py, make all of our changes in the latter, and add the custom extension in SAGE_DOC/common/conf.py?

Changed 10 years ago by mpatel

Handle *.next methods. sage repo.

comment:10 Changed 10 years ago by mpatel

The attached patch should remove the warnings that end in

.next: arg is not a module, class, method, function, traceback, frame, or code object

comment:11 Changed 10 years ago by mpatel

With the patches, I get about 10 non-reST warnings. I think we can skip

sagenb.notebook.twist.UserToplevel.userchild_download_worksheets.zip

in conf.py. We should coordinate the spkg updates.

comment:12 Changed 10 years ago by jhpalmieri

This is great progress. By the way, I know how to fix the warnings for sage.misc.sagedoc.process_dollars: we shouldn't run process_dollars on this function. That is, change the first line of process_dollars in conf.py from

    if len(docstringlines) > 0:

to

    if len(docstringlines) > 0 and name.find("process_dollars") == -1:

Similarly for process_mathtt. I've included this change in my patch.

Should we copy autodoc.py to SAGE_DOC/common/sage_autodoc.py, make all of our changes in the latter, and add the custom extension in SAGE_DOC/common/conf.py?

This is an interesting idea since we're patching it so much. Then it would be under Sage revision control. We would just need to keep an eye on it whenever we upgrade Sphinx. I'm posting a patch which adds it. Actually, it doesn't add the whole thing, since I got an error when I tried that. Instead, it does

from sphinx.ext.autodoc import *

and then it defines FunctionDocumenter, MethodDocumenter, setup, and ClassDocumenter (this one isn't patched right now, but it looks like it will be in #7448).

comment:13 Changed 10 years ago by jhpalmieri

My patch also skips sagenb.notebook.twist.UserToplevel.userchild_download_worksheets.zip.

Changed 10 years ago by jhpalmieri

apply on top of other patch

comment:14 Changed 10 years ago by jhpalmieri

Here's one more patch: this fixes one last warning message about sage.misc.process_dollars.

Changed 10 years ago by jhpalmieri

apply on top of other patches

comment:15 Changed 10 years ago by mpatel

  • Authors set to John Palmieri, Mitesh Patel
  • Reviewers set to Mitesh Patel

Thanks! Thanks also for making the extension patch. To the extent it counts, my review is positive.

I'll rebase #7448.

To the release manager: Please apply

trac_8244-slot_wrapper_argspec.patch
trac_8244-conf-autodoc.patch
trac_8244-sagedoc.patch

comment:16 Changed 10 years ago by jhpalmieri

  • Reviewers changed from Mitesh Patel to Mitesh Patel, John Palmieri
  • Status changed from new to needs_review

comment:17 Changed 10 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

Changed 10 years ago by mpatel

Replaces "conf_autodoc" patch.

comment:18 follow-up: Changed 10 years ago by mpatel

I've attached a replacement for the "conf-autodoc" patch that adds all of autodoc (as sage_autodoc) and, with some redefinition, avoids the ExtensionError.

A self-contained sage_autodoc should make it less likely that sage_autodoc stops working, when we upgrade or test new versions of Sphinx. (I was just hit by this during experiments with a development version).

What do you think?

comment:19 in reply to: ↑ 18 Changed 10 years ago by jhpalmieri

Replying to mpatel:

I've attached a replacement for the "conf-autodoc" patch that adds all of autodoc (as sage_autodoc) and, with some redefinition, avoids the ExtensionError.

A self-contained sage_autodoc should make it less likely that sage_autodoc stops working, when we upgrade or test new versions of Sphinx. (I was just hit by this during experiments with a development version).

What do you think?

Looks good to me. Still a positive review.

comment:20 Changed 10 years ago by mvngu

  • Merged in set to sage-4.3.4.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.