Opened 10 months ago

Closed 8 months ago

#26992 closed defect (fixed)

Fix new (lib) gap on py3

Reported by: dimpase Owned by: embray
Priority: major Milestone: sage-8.7
Component: python3 Keywords:
Cc: chapoton, embray, jdemeyer Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 369fade (Commits) Commit: 369fade0a5c2ce0f5a6452d5e07065fd038bf07f
Dependencies: Stopgaps:

Description (last modified by dimpase)

There are py3-specific bugs in the new lib gap, as of #22626. Some of them stem from error handler not being fully string/bytes clean.

3$ ./sage -tp src/sage/libs/gap/util.pyx
Running doctests with ID 2019-01-02-09-11-48-97f4ca0c.
Git branch: HEAD
Using --optional=dochtml,memlimit,mpir,python2,sage
Doctesting 1 file using 8 threads.
sage -t --warn-long 46.7 src/sage/libs/gap/util.pyx
**********************************************************************
File "src/sage/libs/gap/util.pyx", line 389, in sage.libs.gap.util.NULL
Failed example:
    libgap.eval('Complex Field with 53 bits of precision;')
Expected:
    Traceback (most recent call last):
    ...
    ValueError: libGAP: Error, Variable: 'Complex' must have a value
    Syntax error: ; expected in stream:1
    Complex Field with 53 bits of precision;;
     ^^^^^^^^^^^^
    Error, Variable: 'with' must have a value
    Syntax error: ; expected in stream:1
    Complex Field with 53 bits of precision;;
     ^^^^^^^^^^^^^^^^^^^^
    Error, Variable: 'bits' must have a value
    Syntax error: ; expected in stream:1
    Complex Field with 53 bits of precision;;
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    Error, Variable: 'precision' must have a value
Got:
    RuntimeError: Error, Variable: 'Complex' must have a value
    <BLANKLINE>
    The above exception was the direct cause of the following exception:
    <BLANKLINE>
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cec5af8> returned a result with an error set
    Exception ignored in: 'sage.libs.gap.util.error_handler'
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cec5af8> returned a result with an error set
    RuntimeError: Syntax error: ; expected in stream:1
    Complex Field with 53 bits of precision;;
     ^^^^^^^^^^^^^^^^^^^^
    Error, Variable: 'bits' must have a value
    <BLANKLINE>
    The above exception was the direct cause of the following exception:
    <BLANKLINE>
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cc76030> returned a result with an error set
    Exception ignored in: 'sage.libs.gap.util.error_handler'
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cc76030> returned a result with an error set
    Traceback (most recent call last):
      File "/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.libs.gap.util.NULL[4]>", line 1, in <module>
        libgap.eval('Complex Field with 53 bits of precision;')
      File "sage/libs/gap/libgap.pyx", line 410, in sage.libs.gap.libgap.Gap.eval (build/cythonized/sage/libs/gap/libgap.c:4360)
        elem = make_any_gap_element(self, gap_eval(gap_command))
      File "sage/libs/gap/util.pyx", line 442, in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6328)
        raise ValueError('can only evaluate a single statement')
    ValueError: can only evaluate a single statement
**********************************************************************
File "src/sage/libs/gap/util.pyx", line 410, in sage.libs.gap.util.NULL
Failed example:
    libgap.eval('Complex Field with 53 bits of precision;')
Expected:
    Traceback (most recent call last):
    ...
    ValueError: libGAP: Error, Variable: 'Complex' must have a value
    ...
    Error, Variable: 'precision' must have a value
Got:
    RuntimeError: Error, Variable: 'Complex' must have a value
    <BLANKLINE>
    The above exception was the direct cause of the following exception:
    <BLANKLINE>
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cec5bb0> returned a result with an error set
    Exception ignored in: 'sage.libs.gap.util.error_handler'
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cec5bb0> returned a result with an error set
    RuntimeError: Syntax error: ; expected in stream:1
    Complex Field with 53 bits of precision;;
     ^^^^^^^^^^^^^^^^^^^^
    Error, Variable: 'bits' must have a value
    <BLANKLINE>
    The above exception was the direct cause of the following exception:
    <BLANKLINE>
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cc77030> returned a result with an error set
    Exception ignored in: 'sage.libs.gap.util.error_handler'
    Traceback (most recent call last):
      File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
    SystemError: <built-in method replace of str object at 0x7fd29cc77030> returned a result with an error set
    Traceback (most recent call last):
      File "/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.libs.gap.util.NULL[5]>", line 1, in <module>
        libgap.eval('Complex Field with 53 bits of precision;')
      File "sage/libs/gap/libgap.pyx", line 410, in sage.libs.gap.libgap.Gap.eval (build/cythonized/sage/libs/gap/libgap.c:4360)
        elem = make_any_gap_element(self, gap_eval(gap_command))
      File "sage/libs/gap/util.pyx", line 442, in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6328)
        raise ValueError('can only evaluate a single statement')
    ValueError: can only evaluate a single statement
**********************************************************************
1 item had failures:
   2 of   8 in sage.libs.gap.util.NULL
    [21 tests, 2 failures, 1.75 s]
----------------------------------------------------------------------
sage -t --warn-long 46.7 src/sage/libs/gap/util.pyx  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 1.8 seconds
    cpu time: 1.8 seconds
    cumulative wall time: 1.8 seconds

Change History (19)

comment:1 Changed 9 months ago by dimpase

  • Description modified (diff)

comment:2 Changed 9 months ago by dimpase

reducing the number of words in that string:

sage:  libgap.eval('Complex Field with;')
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
RuntimeError: Error, Variable: 'Complex' must have a value

The above exception was the direct cause of the following exception:

SystemError                               Traceback (most recent call last)
/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)()
    508     if msg != NULL:
    509         msg_py = char_to_str(msg)
--> 510         msg_py = msg_py.replace('For debugging hints type ?Recovery from '
    511                                 'NoMethodFound\n', '').strip()
    512     else:

SystemError: <built-in method replace of str object at 0x7fe880330618> returned a result with an error set
Exception ignored in: 'sage.libs.gap.util.error_handler'
Traceback (most recent call last):
  File "sage/libs/gap/util.pyx", line 510, in sage.libs.gap.util.extract_libgap_errout (build/cythonized/sage/libs/gap/util.c:6662)
SystemError: <built-in method replace of str object at 0x7fe880330618> returned a result with an error set
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-1-b7d3bc318a36> in <module>()
----> 1 libgap.eval('Complex Field with;')

/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/libgap.pyx in sage.libs.gap.libgap.Gap.eval (build/cythonized/sage/libs/gap/libgap.c:4360)()
    408             gap_command = str(gap_command._gap_init_())
    409 
--> 410         elem = make_any_gap_element(self, gap_eval(gap_command))
    411 
    412         # If the element is NULL just return None instead

/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6328)()
    440         if nresults > 1:  # to mimick the old libGAP
    441             # TODO: Get rid of this restriction eventually?
--> 442             raise ValueError('can only evaluate a single statement')
    443 
    444         # Get the result of the first statement

ValueError: can only evaluate a single statement

still the same

sage:  libgap.eval('Complex Field;')
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6295)()
    430         GAP_Enter()
--> 431         result = GAP_EvalString(cmd)
    432         # We can assume that the result object is a GAP PList (plain list)

RuntimeError: Error, Variable: 'Complex' must have a value

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
<ipython-input-2-124a0085564b> in <module>()
----> 1 libgap.eval('Complex Field;')

/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/libgap.pyx in sage.libs.gap.libgap.Gap.eval (build/cythonized/sage/libs/gap/libgap.c:4360)()
    408             gap_command = str(gap_command._gap_init_())
    409 
--> 410         elem = make_any_gap_element(self, gap_eval(gap_command))
    411 
    412         # If the element is NULL just return None instead

/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6432)()
    459         return ELM0_LIST(result, 2)
    460     except RuntimeError as msg:
--> 461         raise ValueError(f'libGAP: {msg}')
    462     finally:
    463         GAP_Leave()

ValueError: libGAP: Error, Variable: 'Complex' must have a value

this is different, more sane - this is treated as a single statement, at least.

sage:  libgap.eval('Complex;')
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6295)()
    430         GAP_Enter()
--> 431         result = GAP_EvalString(cmd)
    432         # We can assume that the result object is a GAP PList (plain list)

RuntimeError: Error, Variable: 'Complex' must have a value

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
<ipython-input-4-4f8aa29a844d> in <module>()
----> 1 libgap.eval('Complex;')

/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/libgap.pyx in sage.libs.gap.libgap.Gap.eval (build/cythonized/sage/libs/gap/libgap.c:4360)()
    408             gap_command = str(gap_command._gap_init_())
    409 
--> 410         elem = make_any_gap_element(self, gap_eval(gap_command))
    411 
    412         # If the element is NULL just return None instead

/home/dimpase/sagepy3/local/lib/python3.6/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6432)()
    459         return ELM0_LIST(result, 2)
    460     except RuntimeError as msg:
--> 461         raise ValueError(f'libGAP: {msg}')
    462     finally:
    463         GAP_Leave()

ValueError: libGAP: Error, Variable: 'Complex' must have a value

looks OK.

comment:3 Changed 9 months ago by dimpase

And with python2 one gets the intended (roughly, what GAP reports):

sage:  libgap.eval('Complex Field with 53 bits of precision;')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-1-8255f9147a12> in <module>()
----> 1 libgap.eval('Complex Field with 53 bits of precision;')

/home/dimpase/sage/local/lib/python2.7/site-packages/sage/libs/gap/libgap.pyx in sage.libs.gap.libgap.Gap.eval (build/cythonized/sage/libs/gap/libgap.c:4360)()
    408             gap_command = str(gap_command._gap_init_())
    409 
--> 410         elem = make_any_gap_element(self, gap_eval(gap_command))
    411 
    412         # If the element is NULL just return None instead

/home/dimpase/sage/local/lib/python2.7/site-packages/sage/libs/gap/util.pyx in sage.libs.gap.util.gap_eval (build/cythonized/sage/libs/gap/util.c:6563)()
    473         return ELM0_LIST(result, 2)
    474     except RuntimeError as msg:
--> 475         raise ValueError(f'libGAP: {msg}')
    476     finally:
    477         GAP_Leave()

ValueError: libGAP: Error, Variable: 'Complex' must have a value
Syntax error: ; expected in stream:1
Complex Field with 53 bits of precision;;
 ^^^^^^^^^^^^
Error, Variable: 'with' must have a value
Syntax error: ; expected in stream:1
Complex Field with 53 bits of precision;;
 ^^^^^^^^^^^^^^^^^^^^
Error, Variable: 'bits' must have a value
Syntax error: ; expected in stream:1
Complex Field with 53 bits of precision;;
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error, Variable: 'precision' must have a value

comment:4 Changed 9 months ago by dimpase

The error comes from the new-style-formatted string in

--> 475    raise ValueError(f'libGAP: {msg}')

And indeed, this looks very dangerous coding style, as f' formatting allows Python expressions inside, and so a carefully constructed GAP error message may do really fun things...

comment:5 Changed 9 months ago by dimpase

  • Status changed from new to needs_info

No, actually, it's merely GAP producing a string that needs extra sanitation (in Python3). Specifically, if I do the following

--- a/src/sage/libs/gap/util.pyx
+++ b/src/sage/libs/gap/util.pyx
@@ -507,8 +507,8 @@ cdef object extract_libgap_errout():
     msg = CSTR_STRING(r)
     if msg != NULL:
         msg_py = char_to_str(msg)
-        msg_py = msg_py.replace('For debugging hints type ?Recovery from '
-                                'NoMethodFound\n', '').strip()
+        #msg_py = msg_py.replace('For debugging hints type ?Recovery from '
+        #                        'NoMethodFound\n', '').strip()
     else:
         # Shouldn't happen but just in case...
         msg_py = ""

then all the tests on this file pass.

So, somehow, Python3 cannot correctly do search/replace/strip here, while Python2 can. Any idea what to do?

comment:6 Changed 9 months ago by embray

  • Owner changed from (none) to embray

comment:7 Changed 9 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-26992
  • Commit set to cbd12284c4403c52b01f2befae9436101ba6b2b4
  • Status changed from needs_info to needs_review

The main issue here (which I've encountered before in other contexts) is that in Python 3 a lot of the interpreter and core types and modules are much stricter about checking whether or not an exception is already pending before trying to go down various code paths (without this you can end up in strange situations where built-in methods get called even after an exception has been raised).

In this case, the code in the error handler which manipulates the exception message (using Python) could break if the error handler was entered recursively, so it had to be reorganized slightly to account for this possibility.

I also added with gil to both the error handler callback and the gasman callback: Although this isn't strictly necessary at the moment (it seems we don't release the GIL when entering libgap code) this was my first suspicion, and it's a good thing to do anyways just in case.


New commits:

3a99bafrearrange this code so that PyErr_Fetch is called before extract_libgap_errout()
e5454edensure the GIL is held when entering these callback functions
cbd1228trivial python3 test fix

comment:8 Changed 9 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good to me - on py3 docbuilding (that was the place I saw this bug 1st) still breaks somewhere later, but this (a problem with facade-sets label) is another story for another ticket.

comment:9 Changed 9 months ago by embray

  • Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

comment:10 Changed 9 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:11 Changed 9 months ago by embray

A mystery merge conflict.

comment:12 Changed 9 months ago by chapoton

Well, the pull request here is stalled..

https://github.com/sagemath/git-trac-command/pull/30

comment:13 Changed 9 months ago by chapoton

  • Branch changed from u/embray/ticket-26992 to public/ticket/26992
  • Commit changed from cbd12284c4403c52b01f2befae9436101ba6b2b4 to 369fade0a5c2ce0f5a6452d5e07065fd038bf07f
  • Status changed from needs_work to positive_review

trivial conflict fixed


New commits:

369fadeMerge branch 'u/embray/ticket-26992' in 8.7.b1

comment:14 Changed 9 months ago by jdemeyer

Follow-up ticket: #27155

comment:15 Changed 9 months ago by jdemeyer

Note that this usage of PyErr_Fetch() is a memory leak: the objects which are returned by PyErr_Fetch should be deallocated explicitly (if you are dealing with PyObject* instead of object, Cython doesn't take care of refcounts). I'll fix that on #27155.

comment:16 Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work

I'm getting a quite consistent error (with or without 27155)

**********************************************************************
File "src/sage/groups/matrix_gps/coxeter_group.py", line 421, in sage.groups.matrix_gps.coxeter_group.CoxeterMatrixGroup.is_finite
Failed example:
    [l for l in range(2, 9) if
     CoxeterGroup([[1,3,2,2,2], [3,1,3,3,2], [2,3,1,2,2],
                   [2,3,2,1,l], [2,2,2,l,1]]).is_finite()]
Exception raised:
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.groups.matrix_gps.coxeter_group.CoxeterMatrixGroup.is_finite[2]>", line 3, in <module>
        [Integer(2),Integer(3),Integer(2),Integer(1),l], [Integer(2),Integer(2),Integer(2),l,Integer(1)]]).is_finite()]
      File "sage/misc/lazy_import.pyx", line 354, in sage.misc.lazy_import.LazyImport.__call__ (build/cythonized/sage/misc/lazy_import.c:3683)
        return self.get_object()(*args, **kwds)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/root_system/coxeter_group.py", line 132, in CoxeterGroup
        return CoxeterMatrixGroup(data, base_ring, index_set)
      File "sage/misc/classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1700)
        return cls.classcall(cls, *args, **kwds)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/groups/matrix_gps/coxeter_group.py", line 233, in __classcall_private__
        data, base_ring, data.index_set())
      File "sage/misc/cachefunc.pyx", line 1005, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:6067)
        w = self.f(*args, **kwds)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/structure/unique_representation.py", line 1027, in __classcall__
        instance = typecall(cls, *args, **options)
      File "sage/misc/classcall_metaclass.pyx", line 497, in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:2150)
        return (<PyTypeObject*>type).tp_call(cls, args, kwds)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/groups/matrix_gps/coxeter_group.py", line 303, in __init__
        for i in range(n)]
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/groups/matrix_gps/coxeter_group.py", line 284, in val
        return E(2 * x) + ~E(2 * x)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/rings/universal_cyclotomic_field.py", line 1258, in gen
        return self.element_class(self, libgap.E(n)**k)
      File "sage/libs/gap/element.pyx", line 1062, in sage.libs.gap.element.GapElement.__pow__ (build/cythonized/sage/libs/gap/element.c:10893)
        sig_on()
    SystemError: calling remove_from_pari_stack() inside sig_on()
**********************************************************************
1 item had failures:
   1 of   6 in sage.groups.matrix_gps.coxeter_group.CoxeterMatrixGroup.is_finite
    [136 tests, 1 failure, 4.76 s]
----------------------------------------------------------------------
sage -t --long src/sage/groups/matrix_gps/coxeter_group.py  # 1 doctest failed
----------------------------------------------------------------------

comment:17 Changed 8 months ago by jdemeyer

  • Milestone changed from sage-8.7 to sage-pending
  • Status changed from needs_work to positive_review

Yes, this #27140. I was hoping to fix that error on top of #26992 and #27155.

comment:18 Changed 8 months ago by jdemeyer

  • Milestone changed from sage-pending to sage-8.7

To the release manager: tickets #26992, #27155 and #27140 should be merged together to avoid test failures.

comment:19 Changed 8 months ago by vbraun

  • Branch changed from public/ticket/26992 to 369fade0a5c2ce0f5a6452d5e07065fd038bf07f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.