Code check: Pickling of nested classes

[qed777]

Florent Hivert has recently devised a nice way to use Sphinx to test that nested classes in Sage are picklable. Please see V4 at #7448. From #7448's description:

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

This ticket is about implementing Hivert's idea.

For the record, here a the tickets about bugs caught this way:
#8388, #8475. More have been caught on sage-combinat patch queue, before entering sage.

CC: @hivert @jhpalmieri @nthiery

Component: misc

Author: Florent Hivert, Mitesh Patel

Reviewer: Mitesh Patel, Florent Hivert

Merged: sage-4.3.4.alpha1

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

[qed777]

Attachment: trac_8452-nested_class_check.patch.gz

Add sage -docbuild option --check-nested to check picklability

[qed777]

comment:1

I've attached a patch that adds an option --check-nested to the Sage doc builder. If the flag is present, Sphinx will check nested classes for picklability.

I've adapted sage.misc.nested_class.modify_for_nested_pickle, so that we can put the check in conf.py instead of sage_autodoc.py. We may not need to patch Sphinx. Please test and let me know if this works.

Note: The patch depends on #7448.

[qed777]

Author: Mitesh Patel

[hivert]

comment:2

Replying to @qed777:

I've adapted sage.misc.nested_class.modify_for_nested_pickle, so that we can put the check in conf.py instead of sage_autodoc.py. We may not need to patch Sphinx. Please test and let me know if this works.

Note: The patch depends on #7448.

Hi Mitesh,

As far as I understand, the goal of this ticket is twofold:

• make the check optional;
• put it in a sage plugin of sphinx to avoig patching Sphinx for that.
  So you still relies on sphinx to do this check. As I previously said, a priori, checking for nested class has nothing to do with Sphinx.

Though I definitely don't know where to do it, if possible, it would even be better to do it in sage eg. during the import of the class... Any idea on this way ? Maybe it's not possible without patching type which is exactly what NestedClassMetaclass does, at least conceptually.

By the way, am I allowed to review this patch (there is some code from me in it) ?

[qed777]

Changed author from Mitesh Patel to Florent Hivert, Mitesh Patel

[qed777]

comment:3

The patch really just takes advantage of Sphinx's iteration over the contents of the [documented] modules. Sphinx has a doctest extension, which blurs the "line" between building the documentation and checking the code. If the patch actually works, we could take it as an immediately available but temporary solution while we consider other possibilities. But I don't mind at all if we adopt a different approach.

We could create a new script SAGE_LOCAL/bin/sage-analyze with options to run various checks on library source code. Users would call the script directly or via sage -analyze --option file ....

Instead of starting completely from scratch, we could adapt or extend an existing tool: PyChecker, PyFlakes, Pylint. From my very limited experience, I think Pylint is the most promising of these, as long as we adjust the settings.

I think we could each review the other's contributions and maybe get an assist from a third reviewer.

I just noticed that I didn't put your name in the Author(s) field. I apologize for that.

[hivert]

comment:4

Replying to @qed777:

The patch really just takes advantage of Sphinx's iteration over the contents of the [documented] modules. Sphinx has a doctest extension, which blurs the "line" between building the documentation and checking the code. If the patch actually works, we could take it as an immediately available but temporary solution while we consider other possibilities. But I don't mind at all if we adopt a different approach.

I'm happy to report that the temporary solution is fully working. Running a docbuild
I got

WARNING: Pickling of nested class 'sage.combinat.words.words.Words_all._python_object_alphabet' is probably broken. Please set __metaclass__ of the parent class to sage.misc.nested_class.NestedClassMetaclass.

And indeed:

----------------------------------------------------------------------
| Sage Version 4.3.4.alpha0, Release Date: 2010-03-03                |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
**********************************************************************
*                                                                    *
* Warning: this is a prerelease version, and it may be unstable.     *
*                                                                    *
**********************************************************************
Loading Sage library. Current Mercurial branch is: review
sage: sage: W = Words()
sage: sage: A = W._python_object_alphabet()
sage: sage: loads(dumps(A))
---------------------------------------------------------------------------
PicklingError                             Traceback (most recent call last)
...
PicklingError: Can't pickle <class 'sage.combinat.words.words._python_object_alphabet'>: attribute lookup sage.combinat.words.words._python_object_alphabet failed

I'll report it to the author and create the ticket tomorrow.

We could create a new script SAGE_LOCAL/bin/sage-analyze with options to run various checks on library source code. Users would call the script directly or via sage -analyze --option file ....

Instead of starting completely from scratch, we could adapt or extend an existing tool: PyChecker, PyFlakes, Pylint. From my very limited experience, I think Pylint is the most promising of these, as long as we adjust the settings.

I think this is a very good idea. Nicolas advised me to use PyFlakes. It work for what I need but I've no idea how to extend it.

I think we could each review the other's contributions and maybe get an assist from a third reviewer.

I just noticed that I didn't put your name in the Author(s) field. I apologize for that.

No problem. My work is already acknowledged as the author of #7448. Symmetrically, You've done more than a review work on this ticket and didn't add yourself as author... So please do as you consider fair. I don't feel offended either way.

[hivert]

Description changed:

--- 
+++ 
@@ -3,3 +3,6 @@
    "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."
 
 This ticket is about implementing Hivert's idea.
+
+For the record, here a the tickets about bugs caught this way:
+#8388, #8475. More have been caught on sage-combinat patch queue, before entering sage. 

[hivert]

comment:6

Replying to @qed777:

I think we could each review the other's contributions and maybe get an assist from a third reviewer.

From what's concern me the patch is ready to go. Should we keep the idea of sage -analyze ... for another ticket ?

Florent