Sphinx nested class rendering.

[hivert]

This patch fixes rendering of nested class in Sage.

The particular metaclass NestedMetaclass we have to use to work around the nested class pickling bug of python get in the way of Sphinx because it change the __name__ of the nested class.
We have to make Sphinx aware of those particular classes and to update its configuration.

I also took the chance to raise a warning if someone forgot to set the metaclass leading to a unpicklable class. Several bug have been found that way. I'll add a ticket for this. Addendum: See #8452 for this.

CC: @mwhansen @nthiery @jhpalmieri

Component: documentation

Keywords: Sphinx, nested classes

Author: Florent Hivert

Reviewer: Mitesh Patel

Merged: sage-4.3.4.alpha1

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

[mwhansen]

comment:2

See #6664. We need to figure out how to resolve the issues there.

[qed777]

comment:3

Mistaking inner classes for aliased attributes seems likely to be a Sphinx bug that we should report upstream, e.g., to sphinx-dev.

Is there a minimal example we can submit that reproduces the problem?

[hivert]

comment:4

Replying to @qed777:

Mistaking inner classes for aliased attributes seems likely to be a Sphinx bug that we should report upstream, e.g., to sphinx-dev.

Is there a minimal example we can submit that reproduces the problem?

I've made some experiments. Actually it seems that the problem is a bad interaction between sphinx and the particular metaclass NestedMetaclass we have to use to work around the nested class pickling bug of python. If you apply the attached patch, you'll see that TestParent1 is correctly rendered whereas the other parent are not. I think this is a lead which should be followed.

[hivert]

Patch to experiment the interaction Sphinx/NestedMetaclass

[hivert]

comment:5

Attachment: trac_7448-nested_class_sphinx_exper-fh.patch.gz

I've made some experiments. Actually it seems that the problem is a bad interaction between sphinx and the particular metaclass NestedMetaclass we have to use to work around the nested class pickling bug of python. If you apply the attached patch, you'll see that TestParent1 is correctly rendered whereas the other parent are not. I think this is a lead which should be followed.

More info on this: to workaround python's nested class pickle bug we put any class which contain a nested class into NestedMetaclass. The main purpose of this metaclass is to change the class __name__ with the help of the function
modify_for_nested_pickle. As a result sphinx has the impression that the class is an alias. Demonstration: if I comment line 112 of nested_class.py

diff --git a/sage/misc/nested_class.py b/sage/misc/nested_class.py
--- a/sage/misc/nested_class.py
+++ b/sage/misc/nested_class.py
@@ -108,7 +108,7 @@ def modify_for_nested_pickle(cls, name_p
             if v.__name__ == name and v.__module__ == module.__name__ and getattr(module, name, None) is not v:
                 # OK, probably this is a nested class.
                 dotted_name = name_prefix + '.' + name
-                v.__name__ = dotted_name
+                # v.__name__ = dotted_name
                 setattr(module, dotted_name, v)
                 modify_for_nested_pickle(v, dotted_name, module)

Then everything works fine. Any idea to solve this ?

[hivert]

comment:6

Things are progressing with the help of Mike Hansen on irc: The submitted patch to autodoc.py partially solve the problem. Now each nested class is documented twice ! I'm trying to remove one.

[hivert]

Author: Florent Hivert

[hivert]

comment:7

This should solve the problem:
Applying

autodoc.py.patch

to

Sphinx-0.6.3-py2.6.egg/sphinx/ext/autodoc.py

and trac_7448-nested_class_sphinx-fh.patch
should solve the problem.

[hivert]

comment:9

Two remarks:

• Thanks to Mike Hansen for its help on IRC.
• Since I'm patching the sources of sphinx, I probably need something like rebuilding sphinx spkg. Unfortunately, I don't know how to do this. Help welcome !

Florent, off to bed :-)

[jhpalmieri]

comment:11

Replying to @hivert:

Since I'm patching the sources of sphinx, I probably need something like rebuilding sphinx spkg. Unfortunately, I don't know how to do this. Help welcome !

Here's a new spkg:

• http://sage.math.washington.edu/home/palmieri/SPKG/sphinx-0.6.3.p5.spkg

(I haven't tested the patch here, just implemented it and built the spkg. Please check the file autodoc.py to make sure I patched it right.)

[jhpalmieri]

comment:12

To test this, do we need to add sage.misc.nested_class and/or sage.misc.nested_class_test to the reference manual?

[hivert]

comment:13

Hi John,

Replying to @jhpalmieri:

(I haven't tested the patch here, just implemented it and built the spkg. Please check the file autodoc.py to make sure I patched it right.)

Thank for this. Is this normal that the file

./patches/autodoc.py

contains the modifications, whereas

./src/sphinx/ext/autodoc.py

doesn't ? It seems that the answer is yes and that the former replace the later during build. Everthing seems to be ok.

To test this, do we need to add sage.misc.nested_class and/or sage.misc.nested_class_test to the reference manual?

No you don't ! Moreover, sage.misc.nested_class has already been added since #8250 merged in sage-4.3.3.alpha1. If you want to see the result pick a random file with nested class. They are plenty of them in categories, see eg

sage/categories/semigroups.py

which has many thing implemented in nested classes.

Note that now that we do see nested classes, we find out that they are plenty of room for improvement in those docs. It will be the purpose of an (or more probably several) other ticket.

[nthiery]

comment:14

Summary of discussion with Florent over the phone:

• the patch to autodoc could be made more robust and include an analysis of the problem
• we have an example where Sphinx screws up, with plain Python nested classes (without NestedClassMetaclass), and has no chance to do any better without fixing nested class names as does NestedClassMetaclass.

Florent will post an improved patch, but not right away. So if there is an emergency to get this into 4.4, I give a positive review on that particular patch to get in as is.

[jhpalmieri]

comment:15

Replying to @hivert:

Thank for this. Is this normal that the file ./patches/autodoc.py
contains the modifications, whereas
./src/sphinx/ext/autodoc.py
doesn't ? It seems that the answer is yes and that the former replace the later during build. Everthing seems to be ok.

Yes, it's normal: in a typical spkg, the "src" directory is supposed to contain the original unmodified source, and then it gets patched by running spkg-install.

I noticed that the nested classes in categories/coxeter_groups seem to be rendered correctly, so I guess that's good evidence that it's working.

[hivert]

comment:16

Yes, it's normal: in a typical spkg, the "src" directory is supposed to contain the original unmodified source, and then it gets patched by running spkg-install.

So that if I want to update the spkg, I only need to update the file in patch/autodoc.py file, commit it with hg and tar the whole thing, am I right ?

I noticed that the nested classes in categories/coxeter_groups seem to be rendered correctly, so I guess that's good evidence that it's working.

Cool !

[jhpalmieri]

comment:17

Replying to @hivert:

So that if I want to update the spkg, I only need to update the file in patch/autodoc.py file, commit it with hg and tar the whole thing, am I right ?

You should update patches/autodoc.py and also patches/autodoc.patch: this one never gets used, but it's helpful to see how autodoc.py was actually changed. While in the patches directory, I just do something like diff -u ../src/sphinx/ext/autodoc.py autodoc.py > autodoc.patch.

You might also update SPKG.txt, which includes the change log. I put your name down for the new patch to autodoc.py, for example. You also need to decide if you need to bump the patch level up: just keep the directory as it is, sphinx-0.6.3.p5, or rename it to sphinx-0.6.3.p6. I think if you're just tinkering with this patch, keep it as "p5".

Finally, going to the directory containing sphinx-0.6.3.p5, you run "sage -pkg sphinx-0.6.3.p5" to create the spkg file.

[hivert]

Attachment: trac_7448-nested_class_sphinx-fh.patch.gz

[qed777]

comment:21

An off-topic note about

combinat/posets/poset_examples.rst:6: (WARNING/2) error while formatting signature for sage.combinat.posets.poset_examples.random: arg is not a module, class, method, function, traceback, frame, or code object
combinat/posets/posets.rst:6: (WARNING/2) error while formatting signature for sage.combinat.posets.posets.random: arg is not a module, class, method, function, traceback, frame, or code object

I think we can suppress these by using import random and calling random.random().

[qed777]

comment:22

Replying to @qed777:

I think we can suppress these by using import random and calling random.random().

See #8326.

[hivert]

comment:23

After some discussion I've found a better logic for raising warning. I'm working again on it.

[hivert]

Attachment: autodoc.py.gz

Attachment: autodoc.py.2.patch.gz

[hivert]

comment:24

Attachment: trac_7448-nested_class_sphinx-fh.3.patch.gz

Hi there ! I just uploaded what should be the final version of this patch:
autodoc.py and autodoc.py.2.patch and trac_7448-nested_class_sphinx-fh.3.patch. If you wan't to test the rendering you can comment

__all__ = [] # Don't document any parents

at the beginning of nested_class_test.py.

[hivert]

Changed keywords from Sphinx categories. to Sphinx, nested classes

[qed777]

Rebased for queue in comment. Apply only this patch.

[qed777]

comment:26

Attachment: trac_7448-nested_class_sphinx-fh.4.patch.gz

V4 is rebased for the following queue:

trac_8244-slot_wrapper_argspec.patch
trac_8244-conf-autodoc.2.patch
trac_8244-sagedoc.patch
trac_7448-nested_class_sphinx-fh.4.patch

With #8244, we don't need to update the sphinx-*.spkg, so please refresh attachment: trac_7448-nested_class_sphinx-fh.4.patch.

Questions:

• Are there objections to making the unpicklable class check optional? We could add a command-line option.
• Also, is it possible to move the check into a processing function in conf.py? Then we could minimize our changes to autodoc:

diff --git a/doc/common/sage_autodoc.py b/doc/common/sage_autodoc.py
--- a/doc/common/sage_autodoc.py
+++ b/doc/common/sage_autodoc.py
@@ -848,7 +848,9 @@ class ClassDocumenter(ModuleLevelDocumen
         # as data/attribute
         if ret:
             if hasattr(self.object, '__name__'):
-                self.doc_as_attr = (self.objpath[-1] != self.object.__name__)
+                name = self.object.__name__
+                self.doc_as_attr = (self.objpath != name.split('.') and 
+                                    self.check_module())
             else:
                 self.doc_as_attr = True
         return ret

and make it easier to upgrade to newer Sphinx versions.

Note: We have made changes to environment.py and highlighting.py and other changes to autodoc.py. But Georg Brandl has recently committed a change --- requested by Mike Hansen? --- that should allow us to revert to the upstream enviroment.py and autodoc.py, modulo this ticket. Actually, I think we can avoid patching highlighting.py, too, if we subclass Pygments' Python lexer.

[hivert]

comment:27

Replying to @qed777:

V4 is rebased for the following queue:

trac_8244-slot_wrapper_argspec.patch
trac_8244-conf-autodoc.2.patch
trac_8244-sagedoc.patch
trac_7448-nested_class_sphinx-fh.4.patch

Thanks for this !

With #8244, we don't need to update the sphinx-*.spkg, so please refresh attachment: trac_7448-nested_class_sphinx-fh.4.patch.

What do you mean by refresh here ?

Questions:

• Are there objections to making the unpicklable class check optional? We could add a command-line option.

No objection ! Actually I think this has nothing to do with sphinx and it should be tested during the import. I put it here because if a class is detected as unpicklable, there is a very good chance it ends up typeset incorrectly by sphinx.

• Also, is it possible to move the check into a processing function in conf.py? Then we could minimize our changes to autodoc:
  and make it easier to upgrade to newer Sphinx versions.

Sure ! Please do ! I won't have much time working on this so I'll appreciate someone take over those if the architecture is changed.

[qed777]

comment:28

Replying to @hivert:

With #8244, we don't need to update the sphinx-*.spkg, so please refresh attachment: trac_7448-nested_class_sphinx-fh.4.patch.

What do you mean by refresh here ?

Just that I suggest any further changes be updates to that patch, via hg qrefresh, if it's appropriate.

• Are there objections to making the unpicklable class check optional? We could add a command-line option.

No objection ! Actually I think this has nothing to do with sphinx and it should be
tested during the import. I put it here because if a class is detected as unpicklable, there is a very good chance it ends up typeset incorrectly by sphinx.

Since it doesn't seem to belong in the docbuild system, can we put this check elsewhere, e.g., in the unit/doc-testing system? And restrict this ticket to nested class rendering?

• Also, is it possible to move the check into a processing function in conf.py? Then we could minimize our changes to autodoc:
  and make it easier to upgrade to newer Sphinx versions.

Sure ! Please do ! I won't have much time working on this so I'll appreciate someone take over those if the architecture is changed.

I tried doing this and didn't have instant success. But as you suggest, it might be better to do this elsewhere.

I may be overreacting. Thoughts?

[qed777]

Only fix rendering of nested classes.

[qed777]

comment:29

Attachment: trac_7448-nested_class_sphinx-fh.5.patch.gz

Replying to @qed777:

Sure ! Please do ! I won't have much time working on this so I'll appreciate someone take over those if the architecture is changed.

I tried doing this and didn't have instant success. But as you suggest, it might be better to do this elsewhere.

I've attached V5, which should "just" fix Sphinx's rendering of nested classes. To the extent it counts, my review is positive. Could someone please review V5 for this ticket? I've opened #8452 for the nested class pickling check and will make a patch for that.

[qed777]

Reviewer: Mitesh Patel

[hivert]

comment:31

Replying to @qed777:

Replying to @qed777:

Sure ! Please do ! I won't have much time working on this so I'll appreciate someone take over those if the architecture is changed.

I tried doing this and didn't have instant success. But as you suggest, it might be better to do this elsewhere.

I've attached V5, which should "just" fix Sphinx's rendering of nested classes. To the extent it counts, my review is positive. Could someone please review V5 for this ticket? I've opened #8452 for the nested class pickling check and will make a patch for that.

Hi Mitesh,

Thanks for taking care of this ! I'm Ok with the change you made to my patches so that I suppose I'm allowed to put a positive review on this ticket.

[mwhansen]

Merged: sage-4.3.4.alpha1

[mwhansen]

comment:32

Merged just trac_7448-nested_class_sphinx-fh.5.patch