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

Starting sage with python3: crash handler cannot return the source of the crash #23876

Closed
kiwifb opened this issue Sep 17, 2017 · 33 comments
Closed

Comments

@kiwifb
Copy link
Member

kiwifb commented Sep 17, 2017

because the function that is supposed to provide the information is itself crashing.

When starting sage with python3.6, sage crashes immediately. The crash report ends up with

/usr/lib64/python3.6/site-packages/sage/misc/sageinspect.py in _extract_embedded_signature(docstring=b'\n        Construct a new object of this class...  # indirect doctest\n            True\n        ', name='__classcall__')
    245 
    246     EXAMPLES::
    247 
    248         sage: from sage.misc.sageinspect import _extract_embedded_signature
    249         sage: from sage.misc.nested_class import MainClass
    250         sage: print(_extract_embedded_signature(MainClass.NestedClass.NestedSubClass.dummy.__doc__, 'dummy')[0])
    251         File: sage/misc/nested_class.pyx (starting at line 314)
    252         ...
    253         sage: _extract_embedded_signature(MainClass.NestedClass.NestedSubClass.dummy.__doc__, 'dummy')[1]
    254         ArgSpec(args=['self', 'x', 'r'], varargs='args', keywords='kwds', defaults=((1, 2, 3.4),))
    255         sage: _extract_embedded_signature(range.__call__.__doc__, '__call__')
    256         ('x.__call__(...) <==> x(...)', None)
    257 
    258     """
    259     # If there is an embedded signature, it is in the first line
--> 260     L = docstring.split(os.linesep, 1)
        L = undefined
        docstring.split = <built-in method split of bytes object at 0x7fb5a40c9800>
        global os.linesep = '\n'
    261     firstline = L[0]
    262     # It is possible that the signature is of the form ClassName.method_name,
    263     # and thus we need to do the following:
    264     if name not in firstline:
    265         return docstring, None
    266     signature = firstline.split(name, 1)[-1]
    267     if signature.startswith("(") and signature.endswith(")"):
    268         docstring = L[1] if len(L)>1 else '' # Remove first line, keep the rest
    269         def_string = "def "+name+signature+": pass"
    270         try:
    271             return docstring, inspect.ArgSpec(*_sage_getargspec_cython(def_string))
    272         except SyntaxError:
    273             docstring = os.linesep.join(L)
    274     return docstring, None
    275 

TypeError: a bytes-like object is required, not 'str'

This is not the source of the crash. But the function supposed to provide information on the origin of the crash is itself crashing because docstring is of type str when bytes is required.

The code is in _sage_getdoc_unformatted (docstring is the return value of this function)

    # Check if the __doc__ attribute was actually a string, and
    # not a 'getset_descriptor' or similar.
    if not isinstance(r, string_types):
        return ''
    elif isinstance(r, unicode):
        return r.encode('utf-8', 'ignore')
    else:
        return r

For things to work properly I currently invert the last two returns call. It is probably sub-optimal. But after that operation the crash report works and give us the real reason sage crashed in the first place.

CC: @fchapoton

Component: python3

Author: François Bissey

Branch/Commit: 7076e8d

Reviewer: Jeroen Demeyer

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

@kiwifb kiwifb added this to the sage-8.1 milestone Sep 17, 2017
@kiwifb
Copy link
Member Author

kiwifb commented Sep 17, 2017

Commit: 64b3533

@kiwifb
Copy link
Member Author

kiwifb commented Sep 17, 2017

comment:1

For information once you fix this, the real crash is

/usr/lib64/python3.6/site-packages/sage/categories/finite_enumerated_sets.py in ParentMethods()
    606                 sage: C in EnumeratedSets().Finite()
    607                 True
    608 
    609                 sage: C.random_element.__module__
    610                 'sage.categories.sets_cat'
    611 
    612                 sage: C.cardinality.__module__
    613                 'sage.categories.sets_cat'
    614 
    615                 sage: C.__iter__.__module__
    616                 'sage.categories.sets_cat'
    617             """
    618 
    619             # Ambiguity resolution between methods inherited from
    620             # Sets.CartesianProducts and from EnumeratedSets.Finite.
--> 621             random_element = Sets.CartesianProducts.ParentMethods.random_element.__func__
        global random_element = undefined
        global Sets.CartesianProducts.ParentMethods.random_element.__func__ = undefined
    622             cardinality = Sets.CartesianProducts.ParentMethods.cardinality.__func__
    623             __iter__ = Sets.CartesianProducts.ParentMethods.__iter__.__func__
    624 
    625             def last(self):
    626                 r"""
    627                 Return the last element
    628 
    629                 EXAMPLES::
    630 
    631                     sage: C = cartesian_product([Zmod(42), Partitions(10), IntegerRange(5)])
    632                     sage: C.last()
    633                     (41, [1, 1, 1, 1, 1, 1, 1, 1, 1, 1], 4)
    634                 """
    635                 return self._cartesian_product_of_elements(
    636                         tuple(c.last() for c in self.cartesian_factors()))

AttributeError: 'function' object has no attribute '__func__'

New commits:

64b3533Make the returned value of _sage_getdoc_unformatted a bytes object in python3

@kiwifb
Copy link
Member Author

kiwifb commented Sep 17, 2017

Branch: u/fbissey/python3_crash

@kiwifb
Copy link
Member Author

kiwifb commented Sep 17, 2017

Author: François Bissey

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:2

OK, the patch makes building the documentation fail with python2.7 so it will have to be more subtle than that.

@jhpalmieri
Copy link
Member

comment:3

To get the Sage library to build at all, I need to make this change:

diff --git a/src/sage/misc/package.py b/src/sage/misc/package.py
index a3c3e923f6..fd58ddab29 100644
--- a/src/sage/misc/package.py
+++ b/src/sage/misc/package.py
@@ -144,7 +144,7 @@ def pip_installed_packages():
         '...'
     """
     proc = subprocess.Popen(["pip", "list", "--no-index", "--format", "json"], stdout=subprocess.PIPE)
-    stdout = str(proc.communicate()[0])
+    stdout = proc.communicate()[0]
     return {package['name'].lower():package['version'] for package in json.loads(stdout)}
 
 def list_packages(*pkg_types, **opts):

What else do I need to do to get to your stage? Do you have an experimental Python 3 branch?

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:4

I don't understand half the problems you have building with python3. sage-on-gentoo just builds out of the box with python3.6. That being said your problem is with package.py which in sage-on-gentoo I replace with a shorter version https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sage/files/sage-7.3-package.py this is because from the point of view of sage-on-gentoo the sage "library" has no business in package management and what's left is solely to make patching minimal in other areas of the sage code.

I do not mess anything just to get python3 to build (apart from pynac for side by side python install), if there is something helping that's just a side effect.

I guess you have the same kind of issue python3 must expect stdout to be bytes when in python2 it was made a nice string.

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:5

I am just finishing a build of sage-on-gentoo with both python2.7 and 3.6 - documentation build with 2.7 using this patch instead of what's in the branch

diff --git a/src/sage/misc/sageinspect.py b/src/sage/misc/sageinspect.py
index f9dffc1ce3..d6013851fc 100644
--- a/src/sage/misc/sageinspect.py
+++ b/src/sage/misc/sageinspect.py
@@ -1636,8 +1636,6 @@ def _sage_getdoc_unformatted(obj):
     # not a 'getset_descriptor' or similar.
     if not isinstance(r, string_types):
         return ''
-    elif isinstance(r, text_type):  # unicode (py2) = str (py3)
-        return r.encode('utf-8', 'ignore')
     else:
         return r
 

I'll see if it survives doctesting in a couple of minutes.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Changed commit from 64b3533 to b75a0ec

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

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

b75a0ecsimplify patch and the result allow documentation to be built

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:7

Survived doctesting with python2.7 so I pushed. I am fairly sure that's the only place text_type is used in the file so further edits should probably be done.

@jdemeyer
Copy link

comment:8

I would prefer to replace

elif isinstance(r, text_type):

by

elif not isinstance(r, str):

I think that would work on Python 3 and the functionality would remain exactly the same on Python 2 since we already know at that point that it's either str or unicode.

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:10

Ok Jeroen, it does work at runtime with python3. Doing a full build to see if work as intended with python2 including building documentation.

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:11

Looks like it does the job for both python2 and python3 so I made a clean branch.


New commits:

0886c6bChange test in _sage_getdoc_unformatted so that a bytes object is returned with python3

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

Changed commit from b75a0ec to 0886c6b

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

Changed branch from u/fbissey/python3_crash to u/fbissey/23876v2

@jdemeyer
Copy link

comment:12

The comment # unicode (py2) = str (py3) no longer makes sense. Maybe write # On Python 2, we want str, not unicode.

If the comment is fixed and make doc-clean && make ptestlong passes, this can be set to positive_review.

@jdemeyer
Copy link

comment:13

Although... maybe the code could be restructured to be more clear:

    if isinstance(r, str):
        return r
    elif isinstance(r, text_type):
        # On Python 2, we want str, not unicode
        return r.encode('utf-8', 'ignore')
    else:
        # Not a string of any kind
        return ''

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:14

I'll give that a go in the morning, I agree it looks a bit better.

@jhpalmieri
Copy link
Member

Changed branch from u/fbissey/23876v2 to u/fbissey/python3_crash

@jhpalmieri
Copy link
Member

comment:15

My problem in comment:3 is being taken care of in #23822.

@jhpalmieri
Copy link
Member

Changed commit from 0886c6b to b75a0ec

@jhpalmieri
Copy link
Member

Changed reviewer from Jeroen Demeyer to none

@jhpalmieri
Copy link
Member

Changed commit from b75a0ec to 0886c6b

@jhpalmieri
Copy link
Member

Changed branch from u/fbissey/python3_crash to u/fbissey/23876v2

@jhpalmieri
Copy link
Member

Reviewer: Jeroen Demeyer

@jhpalmieri
Copy link
Member

comment:17

(Sorry, when I added my last comment, I messed up various other fields for this ticket: "Reviewer", "Commit", "Branch". I've tried to fix them.)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Changed commit from 0886c6b to 7076e8d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

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

7076e8dUse Jeroen latest suggestion which is way clearer

@kiwifb
Copy link
Member Author

kiwifb commented Sep 18, 2017

comment:19

Ok that works with python3 and pass make doc-clean && make ptestlong with python2. So this is ready to go.

@jdemeyer
Copy link

comment:20

+1

@vbraun
Copy link
Member

vbraun commented Sep 20, 2017

Changed branch from u/fbissey/23876v2 to 7076e8d

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

4 participants