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

Upgrade to gap_packages 4.10 and remove database_gap #26856

Closed
dimpase opened this issue Dec 8, 2018 · 187 comments
Closed

Upgrade to gap_packages 4.10 and remove database_gap #26856

dimpase opened this issue Dec 8, 2018 · 187 comments

Comments

@dimpase
Copy link
Member

dimpase commented Dec 8, 2018

After the upgrade to GAP 4.10 in #22626, we revisit what
GAP packages are available in Sage via the GAP standard
SPKG and via extra optional SPGKs.

All GAP packages formerly in our "database_gap"
optional SPKG, except tomlib, now get installed
with GAP as part of our "gap" standard SPKG.

This ticket adds tomlib to the "gap" standard SPKG
and removes the "database_gap" optional SPKG.

(All packages previously in "database_gap" have
GPL-compatible licenses and can therefore be
distributed as part of the "gap" standard SPKG
without license worries.)

We also update the list of extra GAP packages
in gap_packages, which remains a separate and
optional SPKG.

Once the present ticket is merged, GAP packages
shipped with the "gap" standard SPKG and with
the "gap_packages" optional SPKG are as follows.

'''GAP packages now shipped with "gap"
(standard SageMath package)'''

A basic package used by most GAP packages
for their documentation

  • GAPDoc

Five database packages formerly in "database_gap"

  • PrimGrp
  • SmallGrp
  • TransGrp
  • TomLib
  • AtlasRep

Twelve more packages

  • AutPGrp
  • Alnuth
  • CRISP
  • !CTblLib
  • FactInt
  • FGA
  • IRREDSOL
  • LAGUNA
  • Polenta
  • Polycyclic
  • ResClasses
  • Sophus

'''GAP packages now shipped with "gap_packages"
(optional SageMath package)'''

Twenty-five "pure GAP" packages
(not requiring compilation)

  • AClib
  • AutoDoc
  • CoReLG
  • CRIME
  • Cryst
  • CrystCat
  • DESIGN
  • GBNP
  • HAP
  • HAPcryst
  • hecke
  • LieAlgDB
  • LiePRing
  • LieRing
  • loops
  • MapClass
  • polymaking
  • QPA
  • QuaGroup
  • RadiRoot
  • Repsn
  • SLA
  • SONATA
  • Toric
  • utils

Four compiled GAP packages

  • cohomolo
  • GRAPE
  • GUAVA
  • nq

No longer distributed

Depends on #22626
Depends on #26889
Depends on #26965

CC: @alex-konovalov @antonio-rojas @dimpase @embray @kiwifb @mantepse @miguelmarco @slel @tscrim @vbraun

Component: packages: optional

Keywords: GAP

Author: Dima Pasechnik, Erik Bray

Branch: 8bf1044

Reviewer: Erik Bray, Dima Pasechnik

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

@dimpase dimpase added this to the sage-8.5 milestone Dec 8, 2018
@dimpase
Copy link
Member Author

dimpase commented Dec 8, 2018

Commit: 1ed2d00

@dimpase
Copy link
Member Author

dimpase commented Dec 8, 2018

Author: Dima Pasechnik

@dimpase
Copy link
Member Author

dimpase commented Dec 8, 2018

Last 10 new commits:

8b53d4fthis file is no longer needed since I'm patching GAP with this functionality instead
94f09a6speed up this code up a little bit
f571399use ViewObj instead of ViewString
b106ff9use more GAP_Enter() and GAP_Leave() where it seems appropriate
acc75fefix some tests whose output changed since
e588e7afix needed due to changes in record keys
e11e438minor test fixes and cleanup
b686acfuse changesignal to restore SIGCHLD and SIGINT handling
09d229fremove this debug function for now
1ed2d00fix doctests changed due to GAP formatting etc changes

@dimpase
Copy link
Member Author

dimpase commented Dec 8, 2018

Branch: u/dimpase/GQ

@dimpase
Copy link
Member Author

dimpase commented Dec 8, 2018

Dependencies: #22626

@embray
Copy link
Contributor

embray commented Dec 10, 2018

comment:3

Is there a particular reason to merge database_gap into gap_packages?

@dimpase
Copy link
Member Author

dimpase commented Dec 10, 2018

comment:4

historically, there were several reasons to have separate gap, database_gap and gap_packages, and do not install all the possible GAP packages

  • largish sizes of downloads
  • licensing issues---stuff in database_gap is not GPL-compatible before GAP 4.9 or 4.10
  • non-working with libgap GAP kernel extensions

the 1st one, if we decide not to re-package original GAP tarball (and I am very much for not re-packaging), would be gone.

the 2nd one is (completely, from the point of view of GAP's LICENCE file) fixed since GAP 4.10 (or even 4.9).

the 3rd one is fixed by #22626, although this has to be verified.

@embray
Copy link
Contributor

embray commented Dec 10, 2018

comment:5

Replying to @dimpase:

the 3rd one is fixed by #22626, although this has to be verified.

Last I checked this is broken actually, but it's mostly a problem with my install script I think. I need to tinker with that a bit more.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2018

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

5d3fce1fixed noisy "#I MakeReadWriteGlobal" things

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 10, 2018

Changed commit from 1ed2d00 to 5d3fce1

@dimpase
Copy link
Member Author

dimpase commented Dec 11, 2018

comment:7

Note that most of database_gap is already in our new gap (4.10) package, with exception of tomlib.

I'll add tomlib to gap 4.10, and get rid of database_gap.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2018

comment:8

I am checking whether database_gap is already in the branch on #22626, running

./sage -tp  --optional=dochtml,memlimit,mpir,python2,database_gap,sage src/sage/groups/perm_gps/permgroup.py 

(that is, adding database_gap to the optional list) and get the following weird error:

Using --optional=database_gap,dochtml,memlimit,mpir,python2,sage
Doctesting 1 file using 4 threads.
sage -t src/sage/groups/perm_gps/permgroup.py
**********************************************************************
File "src/sage/groups/perm_gps/permgroup.py", line 733, in sage.groups.perm_gps.permgroup.PermutationGroup_generic._coerce_map_from_
Failed example:
    p = f(mg); p
Exception raised:
    Traceback (most recent call last):
      File "/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.groups.perm_gps.permgroup.PermutationGroup_generic._coerce_map_from_[19]>", line 1, in <module>
        p = f(mg); p
    TypeError: 'NoneType' object is not callable
**********************************************************************
File "src/sage/groups/perm_gps/permgroup.py", line 735, in sage.groups.perm_gps.permgroup.PermutationGroup_generic._coerce_map_from_
Failed example:
    PG(p._gap_()) == p
Exception raised:
    Traceback (most recent call last):
      File "/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/dimpase/Sage/sagetrac-mirror/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.groups.perm_gps.permgroup.PermutationGroup_generic._coerce_map_from_[20]>", line 1, in <module>
        PG(p._gap_()) == p
    NameError: name 'p' is not defined
**********************************************************************
1 item had failures:
   2 of  27 in sage.groups.perm_gps.permgroup.PermutationGroup_generic._coerce_map_from_
    [862 tests, 2 failures, 18.36 s]
----------------------------------------------------------------------
sage -t src/sage/groups/perm_gps/permgroup.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 18.6 seconds

Without database_gap in optional, everything passes. The trouble is that the failing tests are not tagged as optional, i.e. it should not matter whether database_gap is passed or not.

Needless to say, these tests work at the Sage prompt just fine.

My hunch is that it might be related to the recent #25706, which unfortunately mixes libgap and gap. (Even if this were true, it would not take away the general flakiness of Sage's doctesting framework...)

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2018

Changed commit from 5d3fce1 to none

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2018

Changed branch from u/dimpase/GQ to none

@jdemeyer
Copy link

comment:9

Replying to @dimpase:

it would not take away the general flakiness of Sage's doctesting framework...

The "doctest framework" is not flaky. Code in Sage or its tests is flaky.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2018

comment:10

One way or another, I have no idea where to even start looking, and what kind of side effects might cause this, and whether it's side-effects in Sage alone, or in Sage interacting with the doctest framework---in the latter case this means it's not possible to reproduce on "standalone" Sage.

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2018

Commit: ef2230a

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2018

comment:11

The issue in comment 8 disappears (I dare not call this a fix, cause the only proof I have is that tests pass!) with the latest commit on this branch,
which reduces insanity in src/sage/groups/matrix_gps/finitely_generated.py, see #26889,
where it should be posted as an independent thing.

At least I can say that my hunch seems to have been right...


Last 10 new commits:

60d7eb6Allow Gap.__getattr__ to return GAP objects other than just functions
0cf2047updated GAP_Enter patch that does not require gapstate.h in libgap-api.h
2de776dMerge branch 'u/embray/spkgs/gap-410' of trac.sagemath.org:sage into HEAD
eefc0fcfix warning from cython
9292d47add sig_on/off() around GAP_Initialize, just in case unhandled segfaults or the like occur
132a7c3copy the current environment before passing it to GAP
29d4c84Merge branch 'u/embray/spkgs/gap-410' of trac.sagemath.org:sage into HEAD
d7c85cdWhen running doctests, use the current_randstate()'s seed for Interface.rand_seed()
e6a7397Merge branch 'u/embray/doctests/interfaces-rand-seed' of trac.sagemath.org:sage into HEAD
ef2230amore libGAP in as_permutation(), less pexpect mess

@dimpase
Copy link
Member Author

dimpase commented Dec 13, 2018

Branch: u/dimpase/GQ

@jdemeyer
Copy link

comment:12

Replying to @dimpase:

One way or another, I have no idea where to even start looking, and what kind of side effects might cause this

In many cases, the mysterious failure of doctests is due to some changing interaction with the Python garbage collector. For example, adding an extra test might change the time when the garbage collector kicks in, causing these strange failures. This is especially true when weak references are involved. And the failing test involves the coercion model, which is using weak references.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 14, 2018

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

9266fedadjusting to the new bigger GAP database, better messages

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 14, 2018

Changed commit from ef2230a to 9266fed

@dimpase
Copy link
Member Author

dimpase commented Dec 14, 2018

comment:14

All that is needed to have the doctests pass with database_gap in --optional is the latest commit.

We still need to make sure that tables of marks (GAP 4.8 tomlib package) are there, and then remove #optional : database_gap tags and database_gap package.

@dimpase
Copy link
Member Author

dimpase commented Dec 14, 2018

comment:15

OK, I'll make our default GAP, without gap_packages installed, to be the same as vanilla GAP 4.10, which is, package-wise, as follows:

$ ./gap
 ┌───────┐   GAP 4.10.0 of 01-Nov-2018
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-default64
 Configuration:  gmp 6.1.2, readline
 Loading the library and packages ...
 Packages:   AClib 1.3.1, Alnuth 3.1.0, AtlasRep 1.5.1, AutoDoc 2018.09.20, AutPGrp 1.10, CRISP 1.4.4, Cryst 4.1.18, CrystCat 1.1.8, CTblLib 1.2.2, 
             FactInt 1.6.2, FGA 1.4.0, GAPDoc 1.6.2, IRREDSOL 1.4, LAGUNA 3.9.0, Polenta 1.3.8, Polycyclic 2.14, PrimGrp 3.3.2, RadiRoot 2.8, 
             ResClasses 4.7.1, SmallGrp 1.3, Sophus 1.24, SpinSym 1.5, TomLib 1.2.7, TransGrp 2.0.4, utils 0.59 

This covers everything that is in database_gap, and more - so we'll move some things out of gap_packages into the main gap package.

@dimpase
Copy link
Member Author

dimpase commented Dec 14, 2018

comment:16

I thought that we should remove build/pkgs/gap/patches/nodefaultpackages.patch, but this breaks pexpect interface to GAP. Note that database_gap and gap_packages do not not attempt to change the GAP package autoloading settings, so we should leave them as they are (it does not seem to be easy to fix).

It does not really matter. We can modify stripts starting gap_console() to enable autoloading (or not)...

@dimpase
Copy link
Member Author

dimpase commented Dec 28, 2018

comment:126

comment in the commit 293e79a is a bit off, as altlasrep has a cache of files downloaded from the net, not generated.

@dimpase
Copy link
Member Author

dimpase commented Dec 28, 2018

comment:127

I am getting timeouts in these iffy parts of libgap:

sage -t --warn-long 109.1 src/sage/libs/gap/all_documented_functions.py
    Timed out
sage -t --warn-long 109.1 src/sage/libs/gap/assigned_names.py
    Timed out

something these should be tagged # long time ?

@embray
Copy link
Contributor

embray commented Dec 28, 2018

comment:128

Wherever the files come from I just mean that they are created by the package at runtime and are not "installed".

@embray
Copy link
Contributor

embray commented Dec 28, 2018

comment:129

Replying to @dimpase:

I am getting timeouts in these iffy parts of libgap:

sage -t --warn-long 109.1 src/sage/libs/gap/all_documented_functions.py
    Timed out
sage -t --warn-long 109.1 src/sage/libs/gap/assigned_names.py
    Timed out

something these should be tagged # long time ?

I think they are. I originally meant, as part of #22626 to deprecate these modules and disable their tests, but I forgot to add that to the work items.

While we're at it I'll go ahead and mark the tests to be skipped, and will handle the deprecation later, perhaps as part of #26959.

@dimpase
Copy link
Member Author

dimpase commented Dec 28, 2018

comment:130

OK, tests are running, I'll have a look in the morning (I'm on UTC+8 now, sorry :-))

@embray
Copy link
Contributor

embray commented Dec 28, 2018

comment:131

I did a fresh test run earlier today and everything looked good, but it will be good to get a second opinion. I'm about to push one more change to disable skip those problematic tests.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2018

Changed commit from 9854c3a to 8bf1044

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 28, 2018

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

8bf1044disable doctesting of these modules

@mantepse
Copy link
Collaborator

comment:133

seems to work here! (same timeouts, because I didn't have the final commit yet)

Thank you!

@dimpase
Copy link
Member Author

dimpase commented Dec 29, 2018

comment:134

OK, great, all works.


New commits:

8bf1044disable doctesting of these modules

@dimpase
Copy link
Member Author

dimpase commented Dec 29, 2018

Reviewer: Erik Bray, Dima Pasechnik

@dimpase
Copy link
Member Author

dimpase commented Dec 29, 2018

comment:136

Replying to @slel:

Does this also fix #23054?

yes it does, thanks for pointing this out.

@vbraun
Copy link
Member

vbraun commented Dec 31, 2018

Changed branch from public/ticket-26856 to 8bf1044

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Jul 7, 2021

Changed work issues from work out where/how to build GAP packages to none

@slel
Copy link
Member

slel commented Jul 7, 2021

Changed commit from 8bf1044 to none

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

6 participants