Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

py3: fix src/sage/misc/nested_class.pyx #27692

Closed
fchapoton opened this issue Apr 17, 2019 · 111 comments
Closed

py3: fix src/sage/misc/nested_class.pyx #27692

fchapoton opened this issue Apr 17, 2019 · 111 comments

Comments

@fchapoton
Copy link
Contributor

traceback:

sage -t --long src/sage/misc/nested_class.pyx
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 39, in sage.misc.nested_class
Failed example:
    A1.A2.A3
Expected:
    <class sage.misc.nested_class.A3 at ...>
Got:
    <class 'sage.misc.nested_class.A1.A2.A3'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 42, in sage.misc.nested_class
Failed example:
    nested_pickle(A1)
Expected:
    <class sage.misc.nested_class.A1 at ...>
Got:
    <class 'sage.misc.nested_class.A1'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 45, in sage.misc.nested_class
Failed example:
    A1.A2
Expected:
    <class sage.misc.nested_class.A1.A2 at ...>
Got:
    <class 'sage.misc.nested_class.A1.A2'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 48, in sage.misc.nested_class
Failed example:
    A1.A2.A3
Expected:
    <class sage.misc.nested_class.A1.A2.A3 at ...>
Got:
    <class 'sage.misc.nested_class.A1.A2.A3'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 131, in sage.misc.nested_class.modify_for_nested_pickle
Failed example:
    getattr(module, 'A.B', 'Not found')
Expected:
    <class '__main__.X.A.B'>
Got:
    <class '__main__.A.B'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 133, in sage.misc.nested_class.modify_for_nested_pickle
Failed example:
    getattr(module, 'X.A.B', 'Not found')
Expected:
    <class '__main__.X.A.B'>
Got:
    <class '__main__.A.B'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 219, in sage.misc.nested_class.nested_pickle
Failed example:
    getattr(module, 'A.B', 'Not found')
Expected:
    <class __main__.A.B at ...>
Got:
    <class '__main__.A.B'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 259, in sage.misc.nested_class.NestedClassMetaclass
Failed example:
    A3.B.__name__
Expected:
    'A3.B'
Got:
    'B'
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 261, in sage.misc.nested_class.NestedClassMetaclass
Failed example:
    getattr(sys.modules['__main__'], 'A3.B', 'Not found')
Expected:
    <class '__main__.A3.B'>
Got:
    'Not found'
**********************************************************************
4 items had failures:
   4 of  14 in sage.misc.nested_class
   2 of   6 in sage.misc.nested_class.NestedClassMetaclass
   2 of  23 in sage.misc.nested_class.modify_for_nested_pickle
   1 of   9 in sage.misc.nested_class.nested_pickle
    [68 tests, 9 failures, 1.77 s]

CC: @embray @jdemeyer @kiwifb

Component: python3

Author: Erik Bray, Kwankyu Lee

Branch/Commit: 562ff1e

Reviewer: Frédéric Chapoton, John Palmieri

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

@fchapoton fchapoton added this to the sage-8.8 milestone Apr 17, 2019
@nbruin
Copy link
Contributor

nbruin commented Apr 17, 2019

comment:1

To get the same repr for a class (without "at ...") in both Py2 and Py3, it may be as simple as defining the classes with class A(object):. It looks like the difference in printing is between old-style and new-style classes, and python3 has only new style classes. So, including the (object) brings the example in Py2 closer to what it does in Py3.

The problem with "name" might need more work.

@embray
Copy link
Contributor

embray commented Apr 19, 2019

comment:2

At some point I definitely fixed or changed something related to this but I'm wracking my brain to remember where.

@embray
Copy link
Contributor

embray commented Apr 19, 2019

comment:3

IIRC one of the main issues here is that I (?) fixed pickling of nested classes such that NestedClassMetaclass is all but unnecessary. But I might have dreamed it; it's all a bit foggy.

@embray
Copy link
Contributor

embray commented Apr 19, 2019

comment:4

Here it is--it turns out that the issue this is trying to work around just isn't relevant on Python 3, so it makes NestedClassMetaclass a no-op to be removed whenever we drop Python 2 support.

diff --git a/src/sage/misc/nested_class.pyx b/src/sage/misc/nested_class.pyx
index 3cd3dfc..cf9c352 100644
--- a/src/sage/misc/nested_class.pyx
+++ b/src/sage/misc/nested_class.pyx
@@ -36,23 +36,27 @@ EXAMPLES::

     sage: A1.A2.A3.__name__
     'A3'
-    sage: A1.A2.A3
+    sage: A1.A2.A3  # py2
     <class sage.misc.nested_class.A3 at ...>

-    sage: nested_pickle(A1)
+    sage: nested_pickle(A1)  # py2
     <class sage.misc.nested_class.A1 at ...>

-    sage: A1.A2
+    sage: A1.A2  # py2
     <class sage.misc.nested_class.A1.A2 at ...>
+    sage: A1.A2  # py3 - note: here we have he desired name for free
+    <class 'sage.misc.nested_class.A1.A2'>

-    sage: A1.A2.A3
+    sage: A1.A2.A3  # py2
     <class sage.misc.nested_class.A1.A2.A3 at ...>
-    sage: A1.A2.A3.__name__
+    sage: A1.A2.A3  # py3
+    <class 'sage.misc.nested_class.A1.A2.A3'>
+    sage: A1.A2.A3.__name__  # py2
     'A1.A2.A3'

-    sage: sage.misc.nested_class.__dict__['A1.A2'] is A1.A2
+    sage: sage.misc.nested_class.__dict__['A1.A2'] is A1.A2  # py2
     True
-    sage: sage.misc.nested_class.__dict__['A1.A2.A3'] is A1.A2.A3
+    sage: sage.misc.nested_class.__dict__['A1.A2.A3'] is A1.A2.A3  # py2
     True

 All of this is not perfect. In the following scenario::
@@ -85,6 +89,8 @@ cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True):
     giving them a mangled name and putting the mangled name in the
     module namespace.

+    On Python 3 this is a no-op and does not perform any mangling.
+
     INPUT:

     - ``cls`` - The class to modify.
@@ -111,26 +117,26 @@ cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True):
         sage: getattr(module, 'A.B', 'Not found')
         'Not found'
         sage: modify_for_nested_pickle(A, 'A', sys.modules['__main__'])
-        sage: A.B.__name__
+        sage: A.B.__name__  # py2
         'A.B'
-        sage: getattr(module, 'A.B', 'Not found')
+        sage: getattr(module, 'A.B', 'Not found')  # py2
         <class '__main__.A.B'>

     Here we demonstrate the effect of the ``first_run`` argument::

         sage: modify_for_nested_pickle(A, 'X', sys.modules['__main__'])
-        sage: A.B.__name__ # nothing changed
+        sage: A.B.__name__ # py2: nothing changed
         'A.B'
         sage: modify_for_nested_pickle(A, 'X', sys.modules['__main__'], first_run=False)
-        sage: A.B.__name__
+        sage: A.B.__name__  # py2
         'X.A.B'

     Note that the class is now found in the module under both its old and
     its new name::

-        sage: getattr(module, 'A.B', 'Not found')
+        sage: getattr(module, 'A.B', 'Not found')  # py2
         <class '__main__.X.A.B'>
-        sage: getattr(module, 'X.A.B', 'Not found')
+        sage: getattr(module, 'X.A.B', 'Not found')  # py2
         <class '__main__.X.A.B'>


@@ -151,17 +157,20 @@ cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True):

     Before :trac:`9107`, the name of ``A1.B1.C1`` would have been wrong::

-        sage: A1.B1.C1.__name__
+        sage: A1.B1.C1.__name__  # py2
         'A1.B1.C1'
-        sage: A1.B2.C2.__name__
+        sage: A1.B2.C2.__name__  # py2
         'A1.B2.C2'
         sage: A_module = sys.modules[A1.__module__]
-        sage: getattr(A_module, 'A1.B1.C1', 'Not found').__name__
+        sage: getattr(A_module, 'A1.B1.C1', 'Not found').__name__  # py2
         'A1.B1.C1'
-        sage: getattr(A_module, 'A1.B2.C2', 'Not found').__name__
+        sage: getattr(A_module, 'A1.B2.C2', 'Not found').__name__  # py2
         'A1.B2.C2'

     """
+    IF PY_MAJOR_VERSION >= 3:
+        return
+
     cdef str name, dotted_name
     cdef str mod_name = module.__name__
     cdef str cls_name = cls.__name__+'.'
@@ -214,9 +223,9 @@ def nested_pickle(cls):
     then the name of class ``"B"`` will be modified to ``"A.B"``, and the ``"A.B"``
     attribute of the module will be set to class ``"B"``::

-        sage: A.B.__name__
+        sage: A.B.__name__  # py2
         'A.B'
-        sage: getattr(module, 'A.B', 'Not found')
+        sage: getattr(module, 'A.B', 'Not found')  # py2
         <class __main__.A.B at ...>

     In Python 2.6, decorators work with classes; then ``@nested_pickle``
@@ -245,6 +254,9 @@ cdef class NestedClassMetaclass(type):
     r"""
     A metaclass for nested pickling.

+    On Python 3 this is just a direct wrapper around the base `type` and does
+    not do anything.
+
     Check that one can use a metaclass to ensure nested_pickle
     is called on any derived subclass::

@@ -256,11 +268,12 @@ cdef class NestedClassMetaclass(type):
         ....:     class B(object):
         ....:         pass
         ...
-        sage: A3.B.__name__
+        sage: A3.B.__name__  # py2
         'A3.B'
-        sage: getattr(sys.modules['__main__'], 'A3.B', 'Not found')
+        sage: getattr(sys.modules['__main__'], 'A3.B', 'Not found')  # py2
         <class '__main__.A3.B'>
     """
+
     def __init__(self, *args):
         r"""
         This invokes the nested_pickle on construction.
@@ -273,7 +286,7 @@ cdef class NestedClassMetaclass(type):
         ...
         sage: A.B
         <class '__main__.A.B'>
-        sage: getattr(sys.modules['__main__'], 'A.B', 'Not found')
+        sage: getattr(sys.modules['__main__'], 'A.B', 'Not found')  # py2
         <class '__main__.A.B'>
         """
         modify_for_nested_pickle(self, self.__name__, sys_modules[self.__module__])
@@ -306,9 +319,9 @@ class MainClass(object, metaclass=NestedClassMetaclass):
                 sage: from sage.misc.nested_class import *
                 sage: loads(dumps(MainClass.NestedClass.NestedSubClass()))
                 <sage.misc.nested_class.MainClass.NestedClass.NestedSubClass object at 0x...>
-                sage: getattr(sage.misc.nested_class, 'MainClass.NestedClass.NestedSubClass')
+                sage: getattr(sage.misc.nested_class, 'MainClass.NestedClass.NestedSubClass')  # py2
                 <class 'sage.misc.nested_class.MainClass.NestedClass.NestedSubClass'>
-                sage: MainClass.NestedClass.NestedSubClass.__name__
+                sage: MainClass.NestedClass.NestedSubClass.__name__  # py2
                 'MainClass.NestedClass.NestedSubClass'
             """
             def dummy(self, x, *args, r=(1,2,3.4), **kwds):
@@ -321,7 +334,7 @@ class MainClass(object, metaclass=NestedClassMetaclass):
                     sage: from sage.misc.nested_class import MainClass
                     sage: print(MainClass.NestedClass.NestedSubClass.dummy.__doc__)
                     NestedSubClass.dummy(self, x, *args, r=(1, 2, 3.4), **kwds)
-                    File: sage/misc/nested_class.pyx (starting at line 314)
+                    File: sage/misc/nested_class.pyx (starting at line 327)
                     <BLANKLINE>
                                     A dummy method to demonstrate the embedding of
                                     method signature for nested classes.

@embray
Copy link
Contributor

embray commented Apr 19, 2019

comment:5

And most of the tests are skipped on Python 3 as they are not relevant.

@fchapoton
Copy link
Contributor Author

comment:6

branch please

@embray
Copy link
Contributor

embray commented Apr 24, 2019

comment:7

Here you go. With this, all the tests for src/sage/misc/nested_class.pyx pass on Python 3 (though many of them are simply skipped as inapplicable).

There might be some other minor failures related to this in miscellaneous modules, but I am not sure. Everything in python3-known-passing.txt still passes.


New commits:

c876038py3: Make NestedClassMetaclass do nothing on Python 3

@embray
Copy link
Contributor

embray commented Apr 24, 2019

Branch: u/embray/python3/ticket-27692

@embray
Copy link
Contributor

embray commented Apr 24, 2019

Commit: c876038

@embray
Copy link
Contributor

embray commented Apr 24, 2019

Author: Erik Bray

@fchapoton
Copy link
Contributor Author

comment:8

one failing doctest in src/sage/misc/sageinspect.py, see patchbot

@fchapoton
Copy link
Contributor Author

Changed branch from u/embray/python3/ticket-27692 to public/python3/ticket-27692

@fchapoton
Copy link
Contributor Author

comment:10

I fixed the doctest


New commits:

a4e9eb3Merge branch 'u/embray/python3/ticket-27692' in 8.8.b3
a902cb2fix doctest in sageinspect

@fchapoton
Copy link
Contributor Author

Changed commit from c876038 to a902cb2

@embray
Copy link
Contributor

embray commented Apr 25, 2019

comment:11

Right...there's always one of those...

@embray
Copy link
Contributor

embray commented Apr 25, 2019

Changed commit from a902cb2 to 174e895

@embray
Copy link
Contributor

embray commented Apr 25, 2019

New commits:

174e895py3: Make NestedClassMetaclass do nothing on Python 3

@embray
Copy link
Contributor

embray commented Apr 25, 2019

Changed branch from public/python3/ticket-27692 to u/embray/python3/ticket-27692

@fchapoton
Copy link
Contributor Author

comment:13

ok, great. Thx

@fchapoton
Copy link
Contributor Author

Reviewer: Frédéric Chapoton

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

a1e3a4cMerge branch 'develop' into t/27692/public/27692
86df126trac 27692: fix one citation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 23, 2019

Changed commit from 040265b to 86df126

@jhpalmieri
Copy link
Member

comment:79

This should fix the problem.

@vbraun
Copy link
Member

vbraun commented Jul 27, 2019

comment:80

Now the pdf docs are broken

[docpdf] Underfull \hbox (badness 10000) in paragraph at lines 4766--4767
[docpdf] \T1/ptm/m/n/10 Bases: [][]\T1/pcr/m/n/10 sage.categories.category_with_axiom.
[docpdf] 
[docpdf] ! LaTeX Error: Too deeply nested.
[docpdf] 
[docpdf] See the LaTeX manual or LaTeX Companion for explanation.
[docpdf] Type  H <return>  for immediate help.
[docpdf]  ...                                              
[docpdf]                                                   
[docpdf] l.4779 \begin{itemize}
[docpdf]                       
[docpdf] ? 
[docpdf] ! Emergency stop.
[docpdf]  ...                                              

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2019

Changed commit from 86df126 to 2f3fc35

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 28, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

13eb46fMerge branch 'develop'
fb30abbMerge branch 'public/27692'
fefe75eFix class documenter in sage_autodoc
2f3fc35Recover some original docstrings

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 28, 2019

comment:82

Seems to have fixed pdf docs.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

Branch pushed to git repo; I updated commit sha1. New commits:

d8cc214Make aliases documented

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

Changed commit from 2f3fc35 to d8cc214

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 29, 2019

comment:85

The last commit makes aliases of nested classes documented.

It looks all good to me. But the nested class stuff of Sage is very subtle, and these changes might have unexpected effects at unexpected places in the documentation.

Please check the generated docs carefully, before we move on.

@kiwifb
Copy link
Member

kiwifb commented Jul 29, 2019

comment:86

Needs rebasing on top of 8.9.beta4.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

562ff1eFix nested_class for python 3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 29, 2019

Changed commit from d8cc214 to 562ff1e

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 29, 2019

Changed reviewer from Frédéric Chapoton, John Palmieri, Kwankyu Lee to Frédéric Chapoton, John Palmieri

@jhpalmieri
Copy link
Member

comment:89

With the new version, attributes appear in the reference manual. This is a change, but it is fine with me.

@jhpalmieri
Copy link
Member

comment:90

That's the only real difference, as far as I can tell. There may be some small differences in the indices (files like http://doc.sagemath.org/html/en/reference/genindex-A.html), as well, but very minor ones. I am happy with this.

(Edit: I built docs with both Python 2 and Python 3, with and without this branch, and ran diff -r ... on the resulting doc directories, so I could see all of the differences. I only checked the html and latex output.)

@kwankyu
Copy link
Collaborator

kwankyu commented Jul 30, 2019

comment:91

I am also happy with more documentation. Just for the record, more members of a class, like attributes and aliases, now appear in the documentation because of a change in skip_member function defined in sage/docs/conf.py, which used to skip them for documentation:

     objname = getattr(obj, "__name__", None)
     if objname is not None:
-        if objname.find('.') != -1 and objname.split('.')[-1] != name:
-            return True
+        # check if name was inserted to the module by NestedClassMetaclass
+        if name.find('.') != -1 and objname.find('.') != -1:
+            if objname.split('.')[-1] == name.split('.')[-1]:
+                return True
 
     if name.find("userchild_download_worksheets.zip") != -1:
         return True

@vbraun
Copy link
Member

vbraun commented Aug 1, 2019

Changed branch from public/27692 to 562ff1e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants