Decorated functions/methods have generic signature in documentation

[johanrosenkilde]

Functions or methods that have been decorated by generic decorators such as sage.misc.decorators.options (moved from sage.misc.plot.options with Trac #9907) degrade documentation by reducing the signature for these callables to the generic "(*args, **kwargs)".

See also the following sage.devel discussion: http://groups.google.com/group/sage-devel/browse_thread/thread/cbd888f0e60130ff/f533792113c45c2f

Let's say that the advanced patch is the most useful and scalable. So ignore the first one and:

Apply

1. attachment: trac_9976_decorated_generic_sigs_alternative.patch
2. attachment: 9976-inspection_of_cython.patch
3. attachment: 9976_doc_fixes_v2.patch
4. attachment: 9976_notebook_doc_fixes.patch

Note that the last patch belongs to a different repository!

CC: @jasongrout @kcrisman @jhpalmieri @novoselt

Component: documentation

Keywords: sphinx, cython inspection

Author: Johan Sebastian Rosenkilde Nielsen, Simon King

Reviewer: Johan Sebastian Rosenkilde Nielsen, Simon King

Merged: sage-4.7.1.alpha0

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

[johanrosenkilde]

Fixes the issue by patching Sage's version of Sphinx to look for a custom attribute in functions and methods.

[johanrosenkilde]

comment:1

Attachment: trac_9976_decorated_generic_sigs.patch.gz

Note that the attached patch assumes Trac #9907 (which in turn assumes #9919). Sorry for the chain of Tracs, but I believe the others are just about finished, so it seems like wasted extra work to first make a version without the dependency and then patch this.

[johanrosenkilde]

comment:2

Now I've finished, cleaned up and tested an alternative patch. It is a slightly more invasive change to the Sphinx autodoc algorithm, and it is a little less intuitive than the other, but it makes it easier for decorators to change the signature of the wrapped function (currently, only @option and @suboption uses this).

In the other patch we used the attribute _sage_decorating which returns the wrapped function to tell Sphinx that it should behave specially, but in the new patch, the decorating callable should now instead define an attribute _sage_getargspec. This should be a function returning an argspec (an argspec is an python.inspect.ArgSpec tuple as returned by python.inspect.getargspec -- basically describing the elements of a function's signature) with the help of an "argspec-retrieving function" taken as argument. This recursive way of doing things is for avoiding the logic within Sphinx of getting the argspec for various types of functions (e.g. Python, Cython or built-in). When Sphinx is asked to format the signature of a function, and that function defines _sage_getargspec, it formats in a standard way whatever _sage_getargspec(getter) returns, where getter is basically itself (that is, the Sphinx function currently being asked to find the argspec of some function).

So for example, @option defines an _sage_getargspec which takes Sphinx's own argspec-finding function -- let's call that getter. _sage_getargspec(getter) then uses getter on the wrapped function, getting the function's original argspec. To this, it adds the options given as arguments to the decorator. This is then returned to Sphinx which formats it and outputs that as the signature for the decorated function.

[johanrosenkilde]

comment:4

I personally prefer the advanced and intrusive patch as it allows more flexibility for the usual developer. I hope that decorators could be more widely used after this patch is introduced, as they are really handy for many things.
So for the everyone -- and in particular the patch buildbot:
Apply trac_9976_decorated_generic_sigs_alternative.patch

[simon-king-jena]

comment:9

I am currently working on a related problem, namely a Cython version of the cached_method decorators - see #11115.

Why is that related?

What one would like to do in this case is to preserve the documentation (and also the source code, file and argspec) of the wrapped function. But if it is Cython, one can not simply override __doc__: Even if __doc__ is defined cdef public, the doc builder would still pick up the the documentation of the decorator, not of the wrapped function.

The obvious solution is to consequently use sage.misc.sageinspect.sage_getdoc and related methods. Like sage_getargspec, it allows for more flexibility.

So, what I would like to do is to edit doc/common/builder.py and replace the calls to inspect by the corresponding calls to sageinspect, and also replace __doc__.

But shall I post that here, since it is related? Or shall I open a new ticket, since, after all, the patch here does not touch builder.py?

Best regards,

Simon

[simon-king-jena]

comment:10

Replying to @simon-king-jena:

So, what I would like to do is to edit doc/common/builder.py and replace the calls to inspect by the corresponding calls to sageinspect, and also replace __doc__.

Oops, me stupid!

I just realised that I was not editing builder.py but sage_autodoc.py. So, we do work on the same file.

I guess that means that I should post my patch here, right?

Best regards,
Simon

[johanrosenkilde]

comment:11

The part about decorators shadowing Cython callable's doc, source code, file, etc. definitely sounds like the same. I guessed that something similar would be wrong with Cython, but I don't write Cython so it was tedious to try and dig up. Good that you came across it. I don't see why this ticket shouldn't encompass both patches for completeness.

I guess that your sage_autodoc patch is not about the cache functionality of your @cached_method decorator -- that is obviously not related to this ticket ;-)

Cheers,
Johan

[simon-king-jena]

comment:12

Replying to @johanrosenkilde:

I guess that your sage_autodoc patch is not about the cache functionality of your @cached_method decorator -- that is obviously not related to this ticket ;-)

Of course! This ticket will eventually become a dependency for #11115.

What I intend to do: Modify the sage_get... functions from sage.misc.sageinspect such that they also work on classes: If you have a class with an attribute _sage_doc_ then this attribute would be called - resulting in a type error that is not caught:

sage: class MyClass:
....:     def _sage_doc_(self):
....:         return "my doc"
....:
sage: O = MyClass()
sage: sage.misc.sageinspect.sage_getdoc(O)
'my doc\n'
sage: sage.misc.sageinspect.sage_getdoc(MyClass)
Traceback (most recent call last):
...
TypeError: unbound method _sage_doc_() must be called with MyClass instance as first argument (got nothing instead)

If that is fixed (similarly in the other sage_get...) then I suggest to replace all calls to inspect.get... and to ...__doc__ by the corresponding function from sage.misc.sageinspect.

Best regards,

Simon

[simon-king-jena]

comment:13

What I would like to have is:

sage: class MyClass:
....:     "Class documentation"
....:     def _sage_doc_(self):
....:         return "Instance documentation"
....:
sage: sage.misc.sageinspect.sage_getdoc(MyClass())
'Instance documention\n'
sage: sage.misc.sageinspect.sage_getdoc(MyClass)
'Class documentation\n'

Then, using sage_getdoc in sage_autodoc.py would mean that "usual" methods of a class would be documented according to their doc string. But if it is a decorated method (at least, the following holds true for @cached_method) then the attribute of the class is in fact a class instance:

sage: class MyClass:
....:     @cached_method
....:     def f(x):
....:         return -x
....:     def g(x):
....:         return -x
....:
sage: MyClass.g.__class__
<type 'instancemethod'>
sage: MyClass.f.__class__
<type 'sage.misc.cachefunc.CachedMethodCaller'>

So, if the decorator has _sage_doc_ and _sage_argspec_ then it can be used when building the reference.

[simon-king-jena]

comment:14

I see this error when building the documentation:

/mnt/local/king/SAGE/broken/devel/sage/doc/en/reference/sage/plot/complex_plot.rst:7: (WARNING/2) error while formatting signature for sage.plot.complex_plot.complex_plot: 'tuple' object has no attribute 'args'

Admittedly this is after I edited sage_autodoc to use more from sageinspect. However, it concerns a part that I did not touch, namely apparently the method format_args(self) in sage_autodoc.

[simon-king-jena]

comment:15

How is the documentation of a class, method etc. actually obtained? There is a method get_doc in sage_autodoc.py, but it was not called when I expected it.

Could it by the the work is done by the method find_attr_docs() of sphinx.pycode.ModuleAnalyzer? That would be bad, because certainly it is ignorant to methods such as _sage_doc_()!

By consequence, if one has a method that is decorated by a cython class (something that I plan for the cached_method decorator) then the documentation will be that of the cython class, not that of the decorated method.

So, could it be that we have to replace sphinx.pycode.ModuleAnalyzer?

[johanrosenkilde]

comment:16

Replying to @simon-king-jena:

I see this error when building the documentation:

/mnt/local/king/SAGE/broken/devel/sage/doc/en/reference/sage/plot/complex_plot.rst:7: (WARNING/2) error while formatting signature for sage.plot.complex_plot.complex_plot: 'tuple' object has no attribute 'args'

Admittedly this is after I edited sage_autodoc to use more from sageinspect. However, it concerns a part that I did not touch, namely apparently the method format_args(self) in sage_autodoc.

I don't know about that error. On my current version of sage 4.6.2 it applies and compiles with no problems; I am currently updating to 4.7.alpha3 to test it there...

[johanrosenkilde]

comment:17

Replying to @simon-king-jena:

How is the documentation of a class, method etc. actually obtained? There is a method get_doc in sage_autodoc.py, but it was not called when I expected it.

That is indeed surprising; I would have thought that was constructing the documentation. You are aware that there are two get_doc functions: one in the Documenter class (which is the father of all documenters), and the overwritten in the ClassLevelDocumenter subclass? So if you were monitoring Documenter's get_doc but were for the documentation of a method or attribute, you would not see the call (I'm compiling, so I can't test myself just now).

Could it by the the work is done by the method find_attr_docs() of sphinx.pycode.ModuleAnalyzer? That would be bad, because certainly it is ignorant to methods such as _sage_doc_()!

By consequence, if one has a method that is decorated by a cython class (something that I plan for the cached_method decorator) then the documentation will be that of the cython class, not that of the decorated method.

So, could it be that we have to replace sphinx.pycode.ModuleAnalyzer?

I hope not. For this ticket, I originally only looked at where the argument string was formatted and that is in sage_autodoc.py. ModuleAnalyzer is (only?) called on line 624 (unpatched) with the following comment: "try to also get a source code analyzer for attribute docs". If what you are suggesting above is true, we should be able to wrap these lines with some exceptions in the cases where a _sage_doc attribute has been set on the object.

[simon-king-jena]

comment:18

Replying to @johanrosenkilde:

Replying to @simon-king-jena:

How is the documentation of a class, method etc. actually obtained? There is a method get_doc in sage_autodoc.py, but it was not called when I expected it.

That is indeed surprising; I would have thought that was constructing the documentation. You are aware that there are two get_doc functions: one in the Documenter class (which is the father of all documenters), and the overwritten in the ClassLevelDocumenter subclass?

I inserted commands into both get_doc methods that I found in sage_autodoc.py that should write some status reports into a file called "simon_logfile" somewhere in my home directory. After deleting one file in doc/output/html/en/reference and doing sage -docbuild reference html`, no file called "simon_logfile" can be found anywhere on the disk (even outside my home directory).

Then, I bumped it up, and inserted a raise RuntimeError into the first line of both methods. Nevertheless, the documentation builds as it did before.

So, I must conclude that neither version of get_doc is used to get documentation.

[simon-king-jena]

comment:19

Replying to @simon-king-jena:

Then, I bumped it up, and inserted a raise RuntimeError into the first line of both methods. Nevertheless, the documentation builds as it did before.

So, I must conclude that neither version of get_doc is used to get documentation.

No, the reason for that behaviour is that apparently the documentation is cached on disk. In other words, I had to delete the html file and to touch the code from which the documentation was taken, and to do "sage -b".

After that preparation, things worked.

My question to you: How can I completely wipe the cache?

I have to leave office now. But I guess a bit later today, or tomorrow, I will be able to provide a patch on top of yours. Its purpose is to allow for Cython objects to provide a documentation that is different from the documentation of their class, and it also provides the possibility to inspect Cython code that is defined during an interactive session.

[simon-king-jena]

comment:20

I added a patch that ought to be applied on top of the previous patch. In sage_autodoc.py, it replaces calls to the inspect module by the corresponding calls to the sage.misc.sageinspect module, and in particular it uses _sage_getdoct_unformatted rather than asking for the __doc__ attribute.

Moreover, it makes sage_getdoc and friends a little more stable. Namely, it catches the type error that resulted when calling sage_getdoc on a class:

sage: class MyClass:
....:     def _sage_doc_(self):
....:         return "some doc"
....:     
sage: sage.misc.sageinspect.sage_getdoc(MyClass())
'some doc\n'
sage: sage.misc.sageinspect.sage_getdoc(MyClass)
''    # This used to give a TypeError

Then, it also allows inspection of Cython code that is defined in an interactive session. Its code is defined in a temporary file, thus, not in the sage library tree. But the old algorithm would only try to find files in the library tree.

   sage: cython('''cpdef test_funct(x,y): return''') 
   sage: sage_getsourcelines(test_funct) 
   (['cpdef test_funct(x,y): return\n'], 6) 

I am building the documentation from scratch. If it works then I can give your patch a positive review. And I hope there will be a reviewer for my patch as well... But careful, my patch modifies something in the sage library, so, doc tests are needed.

For the patchbot:

Apply trac_9976_decorated_generic_sigs_alternative.patch 9976-inspection_of_cython.patch

[simon-king-jena]

Changed keywords from sphinx to sphinx, cython inspection

[simon-king-jena]

Changed author from jsrn to jsrn, Simon King

[simon-king-jena]

Attachment: 9976-inspection_of_cython.patch.gz

Improve inspection on Cython objects, functools.partial and other class instances; let sage_getargspec return an ArgSpec?; improve argument parsing; let a plot raise an AttributeError? if an attribute is missing; include cached functions in references

[simon-king-jena]

comment:170

I have updated my inspection_of_cython patch, by providing two dynamical tests for functools.partial (namely sage_getdoc and sage_getargspec). Such dynamical tests are impossible for sage_getfile and sage_getsource, since interactive Python code won't create a source file to inspect.

I already gave your patches a green light. So, I hope you are happy with my patches as well...

[johanrosenkilde]

comment:171

Great, very nice. Everything applies and works fine here, and I already looked through everything more than once. It's a-go.

[johanrosenkilde]

Changed reviewer from Simon King to jsrn, Simon King

[jdemeyer]

comment:173

Please change the commit message of attachment: trac_9976_decorated_generic_sigs_alternative.patch (use hg qrefresh -e for that).

[jdemeyer]

Changed reviewer from jsrn, Simon King to Johan Sebastian Rosenkilde Nielsen, Simon King

[jdemeyer]

Changed author from jsrn, Simon King to Johan Sebastian Rosenkilde Nielsen, Simon King

[johanrosenkilde]

Attachment: trac_9976_decorated_generic_sigs_alternative.patch.gz

Alternative patch: more intrusive Sphinx patch which makes it easier to let decorators change the signature of the wrapped function.

[johanrosenkilde]

comment:176

Fixed commit message.

[jdemeyer]

Merged: sage-4.7.1.alpha0

[jdemeyer]

Patch rebased to sage-4.7

[jdemeyer]

comment:178

Attachment: 9976_doc_fixes_v2.patch.gz