Opened 2 years ago
Closed 22 months ago
#26992 closed defect (fixed)
Fix new (lib) gap on py3
Reported by:  dimpase  Owned by:  embray 

Priority:  major  Milestone:  sage8.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 )
There are py3specific 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 2019010209114897f4ca0c. Git branch: HEAD Using optional=dochtml,memlimit,mpir,python2,sage Doctesting 1 file using 8 threads. sage t warnlong 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: <builtin 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: <builtin 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: <builtin 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: <builtin 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/sitepackages/sage/doctest/forker.py", line 671, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/dimpase/sagepy3/local/lib/python3.6/sitepackages/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: <builtin 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: <builtin 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: <builtin 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: <builtin 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/sitepackages/sage/doctest/forker.py", line 671, in _run self.compile_and_execute(example, compiler, test.globs) File "/home/dimpase/sagepy3/local/lib/python3.6/sitepackages/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 warnlong 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 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
comment:3 Changed 2 years ago by
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) <ipythoninput18255f9147a12> in <module>() > 1 libgap.eval('Complex Field with 53 bits of precision;') /home/dimpase/sage/local/lib/python2.7/sitepackages/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/sitepackages/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 2 years ago by
The error comes from the newstyleformatted 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 2 years ago by
 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 2 years ago by
 Owner changed from (none) to embray
comment:7 Changed 2 years ago by
 Branch set to u/embray/ticket26992
 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 builtin 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:
3a99baf  rearrange this code so that PyErr_Fetch is called before extract_libgap_errout()

e5454ed  ensure the GIL is held when entering these callback functions

cbd1228  trivial python3 test fix

comment:8 Changed 2 years ago by
 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 facadesets
label) is another story for another ticket.
comment:9 Changed 23 months ago by
 Milestone changed from sage8.6 to sage8.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 sagepending or sagewishlist.
comment:10 Changed 23 months ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:11 Changed 23 months ago by
A mystery merge conflict.
comment:12 Changed 23 months ago by
Well, the pull request here is stalled..
comment:13 Changed 22 months ago by
 Branch changed from u/embray/ticket26992 to public/ticket/26992
 Commit changed from cbd12284c4403c52b01f2befae9436101ba6b2b4 to 369fade0a5c2ce0f5a6452d5e07065fd038bf07f
 Status changed from needs_work to positive_review
comment:14 Changed 22 months ago by
Followup ticket: #27155
comment:15 Changed 22 months ago by
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 22 months ago by
 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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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 22 months ago by
 Milestone changed from sage8.7 to sagepending
 Status changed from needs_work to positive_review
comment:18 Changed 22 months ago by
 Milestone changed from sagepending to sage8.7
comment:19 Changed 22 months ago by
 Branch changed from public/ticket/26992 to 369fade0a5c2ce0f5a6452d5e07065fd038bf07f
 Resolution set to fixed
 Status changed from positive_review to closed
reducing the number of words in that string:
still the same
this is different, more sane  this is treated as a single statement, at least.
looks OK.