Fix and simplify libGAP error handling #27155

jdemeyer · 2019-01-28T08:43:15Z

the usage of PyErr_Fetch in error_handler() is a memory leak: the returned objects should be deallocated (Cython does not take care of this since the type is PyObject* as opposed to object).
in various places in the libGAP interface, there is code of the form

        try:
            ...
        except RuntimeError as msg:
            raise ValueError('libGAP: '+str(msg))

It's not clear to me why we need to catch and re-raise an exception here. Instead, we are raising this exception in our error_handler, so we can just use a custom exception class GAPError to make it clear that the error comes from GAP.

while I'm at it, I'm also replacing libGAP -> GAP in various places and removing the obsolete src/sage/libs/gap/test test program.

Depends on #26992

CC: @vbraun @embray

Component: interfaces

Author: Jeroen Demeyer

Branch/Commit: 494ab6d

Reviewer: Erik Bray

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

The text was updated successfully, but these errors were encountered:

jdemeyer · 2019-01-28T10:02:03Z

Branch: u/jdemeyer/simplify_libgap_error_handling

sagetrac-git · 2019-01-28T10:42:26Z

Commit: fa66044

sagetrac-git · 2019-01-28T10:42:26Z

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

`fa66044`	`Simplify libGAP error handling`

jdemeyer · 2019-01-28T10:51:47Z

Dependencies: #26992

sagetrac-git · 2019-01-28T10:57:12Z

Changed commit from fa66044 to 3b7de9a

sagetrac-git · 2019-01-28T10:57:12Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. 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`
`369fade`	`Merge branch 'u/embray/ticket-26992' in 8.7.b1`
`3b7de9a`	`Simplify libGAP error handling`

sagetrac-git · 2019-01-28T11:47:00Z

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

`73c0437`	`Simplify libGAP error handling`

sagetrac-git · 2019-01-28T11:47:00Z

Changed commit from 3b7de9a to 73c0437

sagetrac-git · 2019-01-28T11:59:00Z

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

`494ab6d`	`Simplify libGAP error handling`

sagetrac-git · 2019-01-28T11:59:00Z

Changed commit from 73c0437 to 494ab6d

embray · 2019-01-28T13:01:43Z

Reviewer: Erik Bray

embray · 2019-01-28T13:01:43Z

comment:10

Thanks for tracking that down--I didn't realize that about PyErr_Fetch; I was careless there about reference counts and just assume for some reason that it returned borrowed references. Except the documentation is quite clear about this:

If it is set, it will be cleared and you own a reference to each object retrieved.

The GapError stuff looks good too--at one point I think I considered doing something similar, but at the time was trying to avoid too many changes like this one where there would be hundreds of little find/replaces obscuring the more important changes.

jdemeyer · 2019-02-05T15:18:42Z

comment:11

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

vbraun · 2019-02-05T23:10:47Z

comment:12

I got this with all three....

**********************************************************************
File "src/sage/modular/abvar/torsion_subgroup.py", line 46, in sage.modular.abvar.torsion_subgroup
Failed example:
    for N in range(1,38):
       for A in J0(N).new_subvariety().decomposition():
           T = A.rational_torsion_subgroup()
           print('%-5s%-5s%-5s%-5s'%(N, A.dimension(), T.divisor_of_order(), T.multiple_of_order()))
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.modular.abvar.torsion_subgroup[9]>", line 2, in <module>
        for A in J0(N).new_subvariety().decomposition():
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/abvar/abvar.py", line 4441, in new_subvariety
        A = self.modular_symbols()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/abvar/abvar.py", line 4198, in modular_symbols
        M = self._modular_symbols().modular_symbols_of_sign(sign)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/abvar/abvar_ambient_jacobian.py", line 111, in _modular_symbols
        self.__modsym = ModularSymbols(self.__group, weight=2).cuspidal_submodule()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/modsym.py", line 358, in ModularSymbols
        group.level(),sign, base_ring, custom_init=custom_init)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/ambient.py", line 2772, in __init__
        custom_init=custom_init, category=category)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/ambient.py", line 2495, in __init__
        custom_init=custom_init, category=category)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/ambient.py", line 189, in __init__
        rank = self.rank()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/ambient.py", line 1559, in rank
        self.__rank = len(self.manin_basis())
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/ambient.py", line 279, in manin_basis
        self.compute_presentation()
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/ambient.py", line 313, in compute_presentation
        self.base_ring())
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/relation_matrix.py", line 452, in compute_presentation
        B, basis = gens_to_basis_matrix(syms, R, mod, field, sparse)
      File "/var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/modular/modsym/relation_matrix.py", line 313, in gens_to_basis_matrix
        height_guess=1)
      File "sage/matrix/matrix_rational_sparse.pyx", line 545, in sage.matrix.matrix_rational_sparse.Matrix_rational_sparse.echelon_form (build/cythonized/sage/matrix/matrix_rational_sparse.c:7025)
        E, pivots = self._echelon_form_multimodular(height_guess, proof=proof)
      File "sage/matrix/matrix_rational_sparse.pyx", line 572, in sage.matrix.matrix_rational_sparse.Matrix_rational_sparse._echelon_form_multimodular (build/cythonized/sage/matrix/matrix_rational_sparse.c:7517)
        E, pivots = matrix_rational_echelon_form_multimodular(self,
      File "sage/matrix/misc.pyx", line 419, in sage.matrix.misc.matrix_rational_echelon_form_multimodular (build/cythonized/sage/matrix/misc.c:7659)
        E = L.rational_reconstruction(prod)
      File "sage/matrix/matrix_integer_sparse.pyx", line 358, in sage.matrix.matrix_integer_sparse.Matrix_integer_sparse.rational_reconstruction (build/cythonized/sage/matrix/matrix_integer_sparse.cpp:6987)
        return matrix_integer_sparse_rational_reconstruction(self, N)
      File "sage/matrix/misc.pyx", line 182, in sage.matrix.misc.matrix_integer_sparse_rational_reconstruction (build/cythonized/sage/matrix/misc.c:4929)
        sig_on()
    SystemError: calling remove_from_pari_stack() inside sig_on()
**********************************************************************
1 item had failures:
   1 of  13 in sage.modular.abvar.torsion_subgroup
    [86 tests, 1 failure, 4.33 s]
----------------------------------------------------------------------
sage -t --long src/sage/modular/abvar/torsion_subgroup.py  # 1 doctest failed
----------------------------------------------------------------------

For my sanity I would prefer to do any further work on a separate ticket after the three are merged, though.

jdemeyer · 2019-02-06T06:14:00Z

comment:13

Replying to @vbraun:

I got this with all three....

That's #27224.

vbraun · 2019-02-06T10:39:17Z

Changed branch from u/jdemeyer/simplify_libgap_error_handling to 494ab6d

jdemeyer added this to the sage-8.7 milestone Jan 28, 2019

jdemeyer added c: interfaces labels Jan 28, 2019

This comment has been minimized.

Sign in to view

jdemeyer added the s: needs review label Jan 28, 2019

This comment has been minimized.

Sign in to view

jdemeyer changed the title ~~Simplify libGAP error handling~~ Fix and simplify libGAP error handling Jan 28, 2019

This comment has been minimized.

Sign in to view

embray added s: positive review and removed s: needs review labels Jan 28, 2019

vbraun removed the s: positive review label Feb 6, 2019

vbraun closed this as completed in 700570d Feb 6, 2019

vbraun mentioned this issue Feb 6, 2019

make_any_gap_element() should not be called inside sig_on() #27140

Closed

kiwifb mentioned this issue May 26, 2022

Remove sage/libs/gap/test from MANIFEST.in #33859

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix and simplify libGAP error handling #27155

Fix and simplify libGAP error handling #27155

jdemeyer commented Jan 28, 2019

jdemeyer commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

This comment has been minimized.

jdemeyer commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

This comment has been minimized.

This comment has been minimized.

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

embray commented Jan 28, 2019

embray commented Jan 28, 2019

jdemeyer commented Feb 5, 2019

vbraun commented Feb 5, 2019

jdemeyer commented Feb 6, 2019

vbraun commented Feb 6, 2019

Fix and simplify libGAP error handling #27155

Fix and simplify libGAP error handling #27155

Comments

jdemeyer commented Jan 28, 2019

jdemeyer commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

This comment has been minimized.

jdemeyer commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

This comment has been minimized.

This comment has been minimized.

sagetrac-git mannequin commented Jan 28, 2019

sagetrac-git mannequin commented Jan 28, 2019

embray commented Jan 28, 2019

embray commented Jan 28, 2019

jdemeyer commented Feb 5, 2019

vbraun commented Feb 5, 2019

jdemeyer commented Feb 6, 2019

vbraun commented Feb 6, 2019