Opened 12 years ago

Closed 11 years ago

#7448 closed defect (fixed)

Sphinx nested class rendering.

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-4.3.4
Component: documentation Keywords: Sphinx, nested classes
Cc: mhansen, nthiery, jhpalmieri Merged in: sage-4.3.4.alpha1
Authors: Florent Hivert Reviewers: Mitesh Patel
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mpatel)

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.

Attachments (9)

trac_7448-nested_class_sphinx_exper-fh.patch (2.9 KB) - added by hivert 11 years ago.
Patch to experiment the interaction Sphinx/NestedMetaclass?
trac_7448-nested_class_sphinx-fh.patch (4.2 KB) - added by hivert 11 years ago.
autodoc.py.patch (3.9 KB) - added by hivert 11 years ago.
patch to sphinx autodoc.py
trac_7448-nested_class_sphinx-fh.2.patch (9.3 KB) - added by mpatel 11 years ago.
Combined patch rebased vs. #8244. Apply only this patch. sage repo.
autodoc.py (46.0 KB) - added by hivert 11 years ago.
autodoc.py.2.patch (3.9 KB) - added by hivert 11 years ago.
trac_7448-nested_class_sphinx-fh.3.patch (8.3 KB) - added by hivert 11 years ago.
trac_7448-nested_class_sphinx-fh.4.patch (13.3 KB) - added by mpatel 11 years ago.
Rebased for queue in comment. Apply only this patch.
trac_7448-nested_class_sphinx-fh.5.patch (11.8 KB) - added by mpatel 11 years ago.
Only fix rendering of nested classes.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 12 years ago by hivert

  • Cc nthiery added
  • Status changed from new to needs_info

comment:2 Changed 12 years ago by mhansen

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

comment:3 follow-up: Changed 11 years ago by mpatel

  • Report Upstream set to N/A

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?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 11 years ago by hivert

Replying to mpatel:

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.

Changed 11 years ago by hivert

Patch to experiment the interaction Sphinx/NestedMetaclass?

comment:5 in reply to: ↑ 4 Changed 11 years ago by hivert

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 ?

comment:6 Changed 11 years ago by hivert

  • Status changed from needs_info to needs_work

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.

comment:7 Changed 11 years ago by hivert

  • Authors set to Florent Hivert
  • Summary changed from Improve sphinx rendering of categories in reference manual. to Sphinx nested class rendering.

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.

comment:8 Changed 11 years ago by hivert

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

comment:9 follow-up: Changed 11 years ago by hivert

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

comment:10 Changed 11 years ago by jhpalmieri

  • Cc jhpalmieri added

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

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:

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

comment:12 follow-up: Changed 11 years ago by jhpalmieri

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

comment:13 in reply to: ↑ 12 ; follow-up: Changed 11 years ago by hivert

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.

comment:14 Changed 11 years ago by nthiery

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.

comment:15 in reply to: ↑ 13 ; follow-up: Changed 11 years ago by jhpalmieri

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.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 11 years ago by hivert

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 !

comment:17 in reply to: ↑ 16 Changed 11 years ago by jhpalmieri

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.

Changed 11 years ago by hivert

Changed 11 years ago by hivert

patch to sphinx autodoc.py

comment:18 Changed 11 years ago by hivert

  • Description modified (diff)

I should have reached a final state for this ticket. I've added a lot of comment explaining what's happening. Please review.

I uploaded a new patch for sphinx and for autodoc.py. Do *not* apply trac_7448-nested_class_sphinx_exper-fh.patch which was used for experiments.

mpatel told on #8258 he will take care or building the spkg. Many thanks to him.

Changed 11 years ago by mpatel

Combined patch rebased vs. #8244. Apply only this patch. sage repo.

comment:19 Changed 11 years ago by mpatel

I've attached a combined patch that's rebased against #8244. That ticket adds a custom sage_autodoc extension to the sage repository. We don't need to update the Sphinx spkg here.

Please check that I did this correctly. Thanks!

comment:20 Changed 11 years ago by mpatel

After this ticket is reviewed, I'll rebase #7549.

comment:21 follow-up: Changed 11 years ago by mpatel

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

comment:22 in reply to: ↑ 21 Changed 11 years ago by mpatel

Replying to mpatel:

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

See #8326.

comment:23 Changed 11 years ago by hivert

  • Status changed from needs_review to needs_work

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

Changed 11 years ago by hivert

Changed 11 years ago by hivert

Changed 11 years ago by hivert

comment:24 Changed 11 years ago by hivert

  • Keywords nested classes added; categories. removed

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.

comment:25 Changed 11 years ago by hivert

  • Status changed from needs_work to needs_review

Changed 11 years ago by mpatel

Rebased for queue in comment. Apply only this patch.

comment:26 follow-up: Changed 11 years ago by mpatel

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 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:
    • doc/common/sage_autodoc.py

      diff --git a/doc/common/sage_autodoc.py b/doc/common/sage_autodoc.py
      a b class ClassDocumenter(ModuleLevelDocumen 
      848848        # as data/attribute
      849849        if ret:
      850850            if hasattr(self.object, '__name__'):
      851                 self.doc_as_attr = (self.objpath[-1] != self.object.__name__)
       851                name = self.object.__name__
       852                self.doc_as_attr = (self.objpath != name.split('.') and
       853                                    self.check_module())
      852854            else:
      853855                self.doc_as_attr = True
      854856        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.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 11 years ago by hivert

Replying to mpatel:

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

comment:28 in reply to: ↑ 27 ; follow-up: Changed 11 years ago by mpatel

Replying to hivert:

With #8244, we don't need to update the sphinx-*.spkg, so please refresh 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?

Changed 11 years ago by mpatel

Only fix rendering of nested classes.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 11 years ago by mpatel

  • Reviewers set to Mitesh Patel

Replying to mpatel:

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.

comment:30 Changed 11 years ago by mpatel

  • Description modified (diff)

comment:31 in reply to: ↑ 29 Changed 11 years ago by hivert

  • Milestone set to sage-4.3.4
  • Status changed from needs_review to positive_review

Replying to mpatel:

Replying to mpatel:

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.

comment:32 Changed 11 years ago by mhansen

  • Merged in set to sage-4.3.4.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged just trac_7448-nested_class_sphinx-fh.5.patch

Note: See TracTickets for help on using tickets.