make_any_gap_element() should not be called inside sig_on(), part 2

[simonbrandhorst]

Fix further make_any_gap_element() calls in sage.libs.element similarly to #27140.

---------------------------------------------------------------------
SystemError                         Traceback (most recent call last)
<ipython-input-6-7b242311279b> in <module>()
----> 1 classify_ord_pe(L,Integer(2),Integer(2),"results/order4.txt","w")

/home/simon/.sage/temp/k3/8598/K3_aut_classification.sageqIg9CK.py in classify_ord_pe(L, p, e, file_name, rw)
    252         print(" ")
    253         cofix = cofixed[k].twist(-_sage_const_1 )
--> 254         for Aa in k3_prime_power(fix.genus(), p, e):
    255             A, a, Oa = Aa
    256             actsg = MaximalK3surfaceAut(A, cofix, a, Oa)

/home/simon/.sage/temp/k3/8598/prime_power.sage5OzLUx.py in k3_prime_power(genus, prime, e)
    138             signatures += [[ranks_E[k]]*(weights[k]//_sage_const_2 ) for k in range(_sage_const_1 ,n)]
    139         signatures[-_sage_const_1 ][_sage_const_0 ] -= _sage_const_1
--> 140         for act in prime_power_actions(genus,prime,ranks,signatures):
    141             yield act
    142

/home/simon/.sage/temp/k3/8598/prime_power.sage5OzLUx.py in prime_power_actions(genus, p, ranks, signatures)
    276                     # recurse
    277                     # print(R)
--> 278                     for N in prime_power_actions(R, p, ranks[:-_sage_const_1 ], R_signatures):
    279                         N, fN, GN = N
    280                         ext = extensions(M, fM, N, fN, GM, GN, glue_order, p)

/home/simon/.sage/temp/k3/8598/prime_power.sage5OzLUx.py in prime_power_actions(genus, p, ranks, signatures)
    240             if G.is_even() and not M.is_even():
    241                 continue
--> 242             GM = Oq_equiv(M, fM, p**e)
    243             DM = M.discriminant_group()
    244             M_max_glue_group = (M.span((fM**(p**(e-_sage_const_1 )) - fM**_sage_const_0 ).inverse())

/home/simon/.sage/temp/k3/8598/prime_power.sage5OzLUx.py in Oq_equiv(M, fM, order)
    514     """
    515     if order==_sage_const_1  or order==_sage_const_2 :
--> 516         return M.image_in_Oq()
    517     elif prod(M.signature_pair()) == _sage_const_0 :
    518         #

This looks like a bug to me but I am not enough of an expert to decide.
/home/simon/sage/local/lib/python2.7/site-packages/sage/modules/free_quadratic_module_integer_symmetric.pyc in image_in_Oq(self)
   1443             generated by 5 elements
   1444         """
-> 1445         Oq = self.discriminant_group().orthogonal_group()
   1446         sig = self.signature_pair()
   1447         if sig[0]*sig[1]==0 or self.rank()==2:

/home/simon/sage/local/lib/python2.7/site-packages/sage/misc/cachefunc.pyx in sage.misc.cachefunc.CachedMethodCaller.__call__ (build/cythonized/sage/misc/cachefunc.c:10232)()
   1948                 return cache[k]
   1949         except KeyError:
-> 1950             w = self._instance_call(*args, **kwds)
   1951             cache[k] = w
   1952             return w

/home/simon/sage/local/lib/python2.7/site-packages/sage/misc/cachefunc.pyx in sage.misc.cachefunc.CachedMethodCaller._instance_call (build/cythonized/sage/misc/cachefunc.c:9717)()
   1824             True
   1825         """
-> 1826         return self.f(self._instance, *args, **kwds)
   1827
   1828     cdef fix_args_kwds(self, tuple args, dict kwds):

/home/simon/sage/local/lib/python2.7/site-packages/sage/modules/torsion_quadratic_module.pyc in orthogonal_group(self, gens, check)
   1196             gens = [matrix(g) for g in _isom_fqf(self)]
   1197         ambient = AbelianGroupGap(self.invariants()).aut()
-> 1198         gens = [ambient(g) for g in gens]
   1199         gens = tuple(g for g in gens if g != ambient.one())
   1200         return FqfOrthogonalGroup(ambient, gens, self, check=check)

/home/simon/sage/local/lib/python2.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9171)()
    897         if mor is not None:
    898             if no_extra_args:
--> 899                 return mor._call_(x)
    900             else:
    901                 return mor._call_with_args(x, args, kwds)

/home/simon/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4547)()
    160                 print(type(C), C)
    161                 print(type(C._element_constructor), C._element_constructor)
--> 162             raise
    163
    164     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/home/simon/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4439)()
    155         cdef Parent C = self._codomain
    156         try:
--> 157             return C._element_constructor(x)
    158         except Exception:
    159             if print_warnings:

/home/simon/sage/local/lib/python2.7/site-packages/sage/groups/abelian_gps/abelian_aut.pyc in _element_constructor_(self, x, check)
    289         if x in self._covering_matrix_ring:
    290             dom = self._domain
--> 291             images = [dom(row).gap() for row in x.rows()]
    292             x = dom.gap().GroupHomomorphismByImages(dom.gap(), images)
    293         from sage.modules.fg_pid.fgp_morphism import FGP_Morphism

/home/simon/sage/local/lib/python2.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9171)()
    897         if mor is not None:
    898             if no_extra_args:
--> 899                 return mor._call_(x)
    900             else:
    901                 return mor._call_with_args(x, args, kwds)

/home/simon/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4547)()
    160                 print(type(C), C)
    161                 print(type(C._element_constructor), C._element_constructor)
--> 162             raise
    163
    164     cpdef Element _call_with_args(self, x, args=(), kwds={}):

/home/simon/sage/local/lib/python2.7/site-packages/sage/structure/coerce_maps.pyx in sage.structure.coerce_maps.DefaultConvertMap_unique._call_ (build/cythonized/sage/structure/coerce_maps.c:4439)()
    155         cdef Parent C = self._codomain
    156         try:
--> 157             return C._element_constructor(x)
    158         except Exception:
    159             if print_warnings:

/home/simon/sage/local/lib/python2.7/site-packages/sage/groups/abelian_gps/abelian_group_gap.pyc in _element_constructor_(self, x, check)
    357             x = gens_gap[0]**0
    358             for i in range(len(exp)):
--> 359                 x *= gens_gap[i]**exp[i]
    360             x = x.gap()
    361         return self.element_class(self, x, check=check)

/home/simon/sage/local/lib/python2.7/site-packages/sage/groups/libgap_wrapper.pyx in sage.groups.libgap_wrapper.ElementLibGAP.__pow__ (build/cythonized/sage/groups/libgap_wrapper.c:7268)()
    659             raise TypeError("exponent must be an integer")
    660         P = self.parent()
--> 661         return P.element_class(P, self.gap() ** n)
    662
    663     def __invert__(self):

/home/simon/sage/local/lib/python2.7/site-packages/sage/groups/abelian_gps/abelian_group_gap.pyc in __init__(self, parent, x, check)
     71             sage: TestSuite(g).run()
     72         """
---> 73         if check and x not in parent.gap():
     74             raise ValueError("%s is not in the group %s" % (x, parent))
     75         ElementLibGAP.__init__(self, parent, x)

/home/simon/sage/local/lib/python2.7/site-packages/sage/libs/gap/element.pyx in sage.libs.gap.element.GapElement.__contains__ (build/cythonized/sage/libs/gap/element.c:7087)()
    581         from sage.libs.gap.libgap import libgap
    582         GAP_IN = libgap.eval(r'\in')
--> 583         return GAP_IN(other, self).sage()
    584
    585     cpdef _type_number(self):

/home/simon/sage/local/lib/python2.7/site-packages/sage/libs/gap/element.pyx in sage.libs.gap.element.GapElement_Function.__call__ (build/cythonized/sage/libs/gap/element.c:18667)()
   2408             a = [x if isinstance(x, GapElement) else libgap(x) for x in args]
   2409
-> 2410         sig_on()
   2411         try:
   2412             GAP_Enter()

SystemError: calling remove_from_pari_stack() inside sig_on()

CC: @embray

Component: cython

Author: Simon Brandhorst

Branch: 46557d6

Reviewer: Jeroen Demeyer

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

[embray]

comment:4

I also saw a different instance of this on one of the patchbots recently:

sage -t --long src/sage/rings/number_field/number_field.py
**********************************************************************
File "src/sage/rings/number_field/number_field.py", line 1288, in sage.rings.number_field.number_field.NumberField_generic
Failed example:
    for chi in G:
        D = ModularSymbols(chi, 2, -1).cuspidal_subspace().new_subspace().decomposition()
        for f in D:
            elt = f.q_eigenform(10, 'alpha')[3]
            assert elt.is_integral()
Exception raised:
    Traceback (most recent call last):
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.number_field.number_field.NumberField_generic[47]>", line 4, in <module>
        elt = f.q_eigenform(Integer(10), 'alpha')[Integer(3)]
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/modular/modsym/space.py", line 1263, in q_eigenform
        ext = [self.eigenvalue(n, names) for n in range(f.prec(),prec)]
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/modular/hecke/module.py", line 1304, in eigenvalue
        Tn_e = self._eigen_nonzero_element(n)
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/modular/hecke/module.py", line 635, in _eigen_nonzero_element
        return A._hecke_image_of_ith_basis_vector(n, i)
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/modular/hecke/module.py", line 650, in _hecke_image_of_ith_basis_vector
        return T.apply_sparse(self.gen(i))
      File "/home/vdelecro/sage_patchbot/local/lib/python2.7/site-packages/sage/modular/modsym/hecke_operator.py", line 72, in apply_sparse
        return M(v.parent()((W * R).row(0)))
      File "sage/structure/element.pyx", line 3681, in sage.structure.element.Matrix.__mul__ (build/cythonized/sage/structure/element.c:22564)
        return coercion_model.bin_op(left, right, mul)
      File "sage/structure/coerce.pyx", line 1158, in sage.structure.coerce.CoercionModel.bin_op (build/cythonized/sage/structure/coerce.c:9985)
        return (<Action>action)._act_(x, y)
      File "sage/matrix/action.pyx", line 260, in sage.matrix.action.MatrixMatrixAction._act_ (build/cythonized/sage/matrix/action.c:4606)
        prod = A._matrix_times_matrix_(B)
      File "sage/matrix/matrix0.pyx", line 5213, in sage.matrix.matrix0.Matrix._matrix_times_matrix_ (build/cythonized/sage/matrix/matrix0.c:35087)
        return self._multiply_classical(right)
      File "sage/matrix/matrix_sparse.pyx", line 193, in sage.matrix.matrix_sparse.Matrix_sparse._multiply_classical (build/cythonized/sage/matrix/matrix_sparse.c:3861)
        sig_on()
    SystemError: calling remove_from_pari_stack() inside sig_on()
**********************************************************************
1 item had failures:
   1 of  49 in sage.rings.number_field.number_field.NumberField_generic
    [2044 tests, 1 failure, 62.94 s]

It seems a bit random, since this didn't occur on other patchbots that tested the same branch.

[simonbrandhorst]

Branch: u/sbrandhorst/make_any_gap_element___should_not_be_called_inside_sig_on____part_2

[simonbrandhorst]

New commits:

46557d6

Following #27140 make_any_gap_element() is not called

[simonbrandhorst]

Commit: 46557d6

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:7

A nice improvement, although still not perfect (make_gap_list could still break thing). But we can leave that for another ticket.

I'm running make ptestlong now, positive review if that passes.

Don't forget to add your name as author.

[simonbrandhorst]

Author: Simon Brandhorst

[jdemeyer]

comment:9

Replying to @embray:

I also saw a different instance of this on one of the patchbots recently:

I opened #27477 for that.

[embray]

comment:11

Seems fine to me. Though I would like to clarify that I originally had the sig_on() outside the try, and the sig_off() inside the finally: based on this example from the cysignals docs:

from cysignals.signals cimport sig_on, sig_off
def try_finally_example():
    sig_on()  # This must be OUTSIDE the try
    try:
        # (some long computation, potentially raising exceptions)
        return something
    finally:
        sig_off()

Perhaps Jeroen could clarify why in this case putting sig_on/off inside the try: block is better, because it's honestly not obvious to me. Also, what happens if an exception (say, from a SIGSEGV) is raised during execution of the GAP code? Since sig_off() is outside the finally: doesn't that mean it won't be called?

[jdemeyer]

comment:12

Replying to @embray:

Perhaps Jeroen could clarify why in this case putting sig_on/off inside the try: block is better

The try/except in this ticket is meant for GAP_Leave(), not for sig_on().

Also, what happens if an exception (say, from a SIGSEGV) is raised during execution of the GAP code? Since sig_off() is outside the finally: doesn't that mean it won't be called?

The new macro sig_GAP_Enter takes care of that: whenever GAP_Enter returns an error, we call sig_error() which reuses the cysignals machinery to deal with exceptions.

[jdemeyer]

comment:13

Replying to @embray:

what happens if an exception (say, from a SIGSEGV) is raised during execution of the GAP code?

This SIGSEGV will be seen by sig_on(), which will raise an exception. Then the finally block executes GAP_Leave().

[vbraun]

Changed branch from u/sbrandhorst/make_any_gap_element___should_not_be_called_inside_sig_on____part_2 to 46557d6

[embray]

Changed commit from 46557d6 to none

[embray]

comment:15

Replying to @jdemeyer:

Replying to @embray:

what happens if an exception (say, from a SIGSEGV) is raised during execution of the GAP code?

This SIGSEGV will be seen by sig_on(), which will raise an exception. Then the finally block executes GAP_Leave().

But what's not clear to me is why you don't need to call sig_off() then?

[jdemeyer]

comment:16

Replying to @embray:

But what's not clear to me is why you don't need to call sig_off() then?

If sig_on() raises an exception (from a signal or from sig_error()), there must not be a sig_off(). That's why the usual code structure is

sig_on()
try:
    do_something()
finally:
    sig_off()

The try/finally block in this branch serves an unrelated purpose: it's meant for dealing with GAP_Leave().