Opened 3 years ago

Closed 3 years ago

# Upgrade to gap_packages 4.10 and remove database_gap

Reported by: Owned by: dimpase critical sage-8.6 packages: optional GAP alexk, arojas, dimpase, embray, fbissey, mantepse, mmarco, slelievre, tscrim, vbraun Dima Pasechnik, Erik Bray Erik Bray, Dima Pasechnik N/A 8bf1044 #22626, #26889, #26965

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
• Repsn
• SLA
• SONATA
• Toric
• utils

Four compiled GAP packages

• cohomolo
• GRAPE
• GUAVA
• nq

No longer distributed

### comment:1 Changed 3 years ago by dimpase

• Authors set to Dima Pasechnik
• Branch set to u/dimpase/GQ
• Commit set to 1ed2d00b452155e803c79040460edcd55c120320

Last 10 new commits:

 ​8b53d4f this file is no longer needed since I'm patching GAP with this functionality instead ​94f09a6 speed up this code up a little bit ​f571399 use ViewObj instead of ViewString ​b106ff9 use more GAP_Enter() and GAP_Leave() where it seems appropriate ​acc75fe fix some tests whose output changed since ​e588e7a fix needed due to changes in record keys ​e11e438 minor test fixes and cleanup ​b686acf use changesignal to restore SIGCHLD and SIGINT handling ​09d229f remove this debug function for now ​1ed2d00 fix doctests changed due to GAP formatting etc changes

### comment:2 Changed 3 years ago by dimpase

• Dependencies set to #22626

### comment:3 Changed 3 years ago by embray

Is there a particular reason to merge database_gap into gap_packages?

### comment:4 follow-up: ↓ 5 Changed 3 years ago by dimpase

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

• 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.

### comment:5 in reply to: ↑ 4 Changed 3 years ago by embray

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.

### comment:6 Changed 3 years ago by git

• Commit changed from 1ed2d00b452155e803c79040460edcd55c120320 to 5d3fce1000f67cfea8be984df8959933c403e83c

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

 ​5d3fce1 fixed noisy "#I MakeReadWriteGlobal" things

### comment:7 Changed 3 years ago by dimpase

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.

### comment:8 follow-ups: ↓ 9 ↓ 89 Changed 3 years ago by dimpase

• Branch u/dimpase/GQ deleted
• Commit 5d3fce1000f67cfea8be984df8959933c403e83c deleted

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
**********************************************************************
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...)

Last edited 3 years ago by dimpase (previous) (diff)

### comment:9 in reply to: ↑ 8 Changed 3 years ago by jdemeyer

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.

### comment:10 follow-up: ↓ 12 Changed 3 years ago by dimpase

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.

Last edited 3 years ago by dimpase (previous) (diff)

### comment:11 Changed 3 years ago by dimpase

• Branch set to u/dimpase/GQ
• Commit set to ef2230a3c98cb6d8b500e5ba6b925199c30a5647
• Status changed from new to needs_review

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:

 ​60d7eb6 Allow Gap.__getattr__ to return GAP objects other than just functions ​0cf2047 updated GAP_Enter patch that does not require gapstate.h in libgap-api.h ​2de776d Merge branch 'u/embray/spkgs/gap-410' of trac.sagemath.org:sage into HEAD ​eefc0fc fix warning from cython ​9292d47 add sig_on/off() around GAP_Initialize, just in case unhandled segfaults or the like occur ​132a7c3 copy the current environment before passing it to GAP ​29d4c84 Merge branch 'u/embray/spkgs/gap-410' of trac.sagemath.org:sage into HEAD ​d7c85cd When running doctests, use the current_randstate()'s seed for Interface.rand_seed() ​e6a7397 Merge branch 'u/embray/doctests/interfaces-rand-seed' of trac.sagemath.org:sage into HEAD ​ef2230a more libGAP in as_permutation(), less pexpect mess
Last edited 3 years ago by dimpase (previous) (diff)

### comment:12 in reply to: ↑ 10 Changed 3 years ago by jdemeyer

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.

### comment:13 Changed 3 years ago by git

• Commit changed from ef2230a3c98cb6d8b500e5ba6b925199c30a5647 to 9266fed2e972f870d0c4ba891993bbfbb13f2100

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

 ​9266fed adjusting to the new bigger GAP database, better messages

### comment:14 Changed 3 years ago by dimpase

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.

### comment:15 Changed 3 years ago by dimpase

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
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.

Last edited 3 years ago by dimpase (previous) (diff)

### comment:16 Changed 3 years ago by dimpase

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)...

### comment:17 Changed 3 years ago by git

• Commit changed from 9266fed2e972f870d0c4ba891993bbfbb13f2100 to 01665fa57aa56458b2d43765fd17b88944186f23

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

 ​01665fa rm #optional - database_gap tags, doc updates, few GAP->libGAP changes

### comment:18 Changed 3 years ago by git

• Commit changed from 01665fa57aa56458b2d43765fd17b88944186f23 to 90f2fea00aa8aa088c619ac128faf2e5ec5cecbd

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

 ​a0915b5 another doctest with different format ​2908fc4 trimming backspaces, in part due to previous mass edit ​9a81e52 removing last traces of database_gap ​90f2fea remaining places dealing with database_gap

### comment:19 Changed 3 years ago by dimpase

done getting rid of database_gap

Now on to gap_packages...

### comment:20 Changed 3 years ago by dimpase

• Dependencies changed from #22626 to #22626, #26889

### comment:21 Changed 3 years ago by dimpase

opened #26901 to deal with changes in the only (apart from gap_packages) package of Sage dependent on database_gap.

Last edited 3 years ago by dimpase (previous) (diff)

### comment:22 Changed 3 years ago by git

• Commit changed from 90f2fea00aa8aa088c619ac128faf2e5ec5cecbd to 9407fba12c12b60c77eb47d11046964be2dd2c74

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

 ​9407fba reviewer's example and fix

### comment:23 Changed 3 years ago by git

• Commit changed from 9407fba12c12b60c77eb47d11046964be2dd2c74 to ca7f6ff6bf514e3b640ab39d8f51760eacc75a88

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

 ​68a50fc Merge branch 'u/embray/doctests/interfaces-rand-seed' of trac.sagemath.org:sage into g410 ​38e687b install only those packages that are required by GAP to run ​8b0d0ad Implement GAPElement_List.__bool__ to work like a Python list ​6a5d34a fix the test for available operations on an object ​72df280 clean up gap_root() a bit more; remove obsolete warning message ​4e6eb3e Merge branch 'u/embray/spkgs/gap-410' of trac.sagemath.org:sage into g410 ​ca7f6ff Merge branch 'u/dimpase/GQ' of trac.sagemath.org:sage into g410

### comment:24 Changed 3 years ago by git

• Commit changed from ca7f6ff6bf514e3b640ab39d8f51760eacc75a88 to b63db23cde4ed4800a93caec12edec29d4c24aab

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

 ​b63db23 allow GAP default packages and install them

### comment:25 Changed 3 years ago by git

• Commit changed from b63db23cde4ed4800a93caec12edec29d4c24aab to d305273fcdd2c247a0c50b8432e2939bc9c6a0f6

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

 ​d305273 install atlasrep, as it is a dependency of tomlib

### comment:26 Changed 3 years ago by git

• Commit changed from d305273fcdd2c247a0c50b8432e2939bc9c6a0f6 to 1b7109983cb030067c6a7e471da076edf8fe6f64

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

 ​3a7b9f7 less libgap.eval usage ​1b71099 adjust doctests to accommodate more GAP methods

### comment:27 Changed 3 years ago by git

• Commit changed from 1b7109983cb030067c6a7e471da076edf8fe6f64 to d879657c584c866152fe795888ee7d02c8668f12

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

 ​5daf576 get rid of explicit GAP package list ​d879657 attempt to replicate GAP packages from 4.8.6

### comment:28 Changed 3 years ago by dimpase

The latest bit does not work, as I don't know how to build GAP packages in the current setup. We have built them in place before, so this needs some work.

### comment:29 Changed 3 years ago by dimpase

• Status changed from needs_review to needs_work
• Work issues set to work out where/how to build GAP packages

### comment:30 Changed 3 years ago by dimpase

Certainly, we can keep the present arrangement, where binaries of GAP packages are installed in place, into package's bin/ subdirectories.

### comment:31 Changed 3 years ago by git

• Commit changed from d879657c584c866152fe795888ee7d02c8668f12 to 785b9dedc352d33a74f8f5d54f6c91c436b225d8

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

 ​5e61d7b pass -A option to GAP to disable autoloading of default packages for both GAP interfaces ​18f2f43 instead of disabling autoloading of default packages, just install all default packages as a standard part of the GAP SPKG ​da909ef more fixes to the GAP SPKG installation ​d1c98f3 also include the atlasrep package, which is a dependency of tomlib ​785b9de Merge branch 'u/embray/spkgs/gap-410' of trac.sagemath.org:sage into gap410pkg

### comment:32 Changed 3 years ago by git

• Commit changed from 785b9dedc352d33a74f8f5d54f6c91c436b225d8 to 67ca32109f47388f820d95f4171f01bc050e32c8

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

 ​67ca321 fixed gap_packages, also package compiling/installing

### comment:33 Changed 3 years ago by git

• Commit changed from 67ca32109f47388f820d95f4171f01bc050e32c8 to a7ef9e015984e48785b80d95ff422bcd3f6ae232

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

 ​a7ef9e0 backport guava patch to allow no gcc

### comment:34 Changed 3 years ago by dimpase

It turns out that most of the changes so far here should go to #22626. Once this is done I'll update the branch accordingly.

### comment:35 follow-up: ↓ 37 Changed 3 years ago by embray

Is there any reason the IO package isn't installed here? It seems to be mentioned quite often, but we don't provide a means to install it.

### comment:36 Changed 3 years ago by git

• Commit changed from a7ef9e015984e48785b80d95ff422bcd3f6ae232 to 01d85a7b1ebe5b95cb8a84d273dafd13c62f4a65

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

 ​53f07cc disable parallel make for gap ​b446ebb adjust doctests to accommodate more GAP methods ​01d85a7 Merge branch 'u/embray/spkgs/gap-410' of trac.sagemath.org:sage into gap410pkg

### comment:37 in reply to: ↑ 35 Changed 3 years ago by dimpase

Is there any reason the IO package isn't installed here? It seems to be mentioned quite often, but we don't provide a means to install it.

I'm currently going for the minimal thing: get gap_packages spkg working the way it was. We never had io package there, as it was breaking libgap in a bad way. (there are open tickets about io package).

Does this sound like a good plan to you? We can postpone io package, it's not needed to be ready by the Debian freeze deadline.

### comment:38 Changed 3 years ago by dimpase

I need to adjust formatting in a dozen of tests, after that should be ready for review.

### comment:39 Changed 3 years ago by embray

OK regarding the IO package. If we weren't installing it in the first place I'm fine just sticking to the minimum set of packages that we were always installing in the first place.

I am also making some updates to this: In particular I'm not happy with the way the compiled packages are installed. Let me know when you're done your changes (set needs_review) and I'll add my changes on top of that.

### comment:40 follow-up: ↓ 43 Changed 3 years ago by dimpase

I'm getting an unpleasant surprise from the doctest framework here - it mangles long lines in this file, e.g. here:

File "src/sage/algebras/quantum_groups/quantum_group_gap.py", line 757, in sage.algebras.quantum_groups.quantum_group_gap.QuantumGroup.coproduct
Failed example:
[Q.coproduct(f, 2)._latex_() for f in Q.F_simple()]  # optional - gap_packages
Expected:
['1 {(1<x>1<x>F_{\\alpha_{1}})}+-q^{2}+q^{-2} {(1<x>F_{\\alpha_{1}}<x>[ K_{1} ; 1 ])}+1 {(1<x>F_{\\alpha_{1}}<x>K_{1})}+q^{4}-2+q^{-4} {(F_{\\alpha_{1}}<x>[ K_{1} ; 1 ]<x>[ K_{1} ; 1 ])}+-q^{2}+q^{-2} {(F_{\\alpha_{1}}<x>[ K_{1} ; 1 ]<x>K_{1})}+-q^{2}+q^{-2} {(F_{\\alpha_{1}}<x>K_{1}<x>[ K_{1} ; 1 ])}+1 {(F_{\\alpha_{1}}<x>K_{1}<x>K_{1})}',
'1 {(1<x>1<x>F_{\\alpha_{2}})}+-q+q^{-1} {(1<x>F_{\\alpha_{2}}<x>[ K_{2} ; 1 ])}+1 {(1<x>F_{\\alpha_{2}}<x>K_{2})}+q^{2}-2+q^{-2} {(F_{\\alpha_{2}}<x>[ K_{2} ; 1 ]<x>[ K_{2} ; 1 ])}+-q+q^{-1} {(F_{\\alpha_{2}}<x>[ K_{2} ; 1 ]<x>K_{2})}+-q+q^{-1} {(F_{\\alpha_{2}}<x>K_{2}<x>[ K_{2} ; 1 ])}+1 {(F_{\\alpha_{2}}<x>K_{2}<x>K_{2})}']
Got:
['1 {(1<x>1<x>F_{\\alpha_{1}})}+-q^{2}+q^{-2} {(1<x>F_{\\alpha_{1}}<x>[ K_{1} ; 1 ])}+1 {(1<x>F_{\\alpha_{1}}<x>K_{1})}+q^{4}-2+q^{-4} {(F\n1<x>[ K_{1} ; 1 ]<x>[ K_{1} ; 1 ])}+-q^{2}+q^{-2} {(F_{\\alpha_{1}}<x>[ K_{1} ; 1 ]<x>K_{1})}+-q^{2}+q^{-2} {(F\n1<x>K_{1}<x>[ K_{1} ; 1 ])}+1 {(F_{\\alpha_{1}}<x>K_{1}<x>K_{1})}',
'1 {(1<x>1<x>F_{\\alpha_{2}})}+-q+q^{-1} {(1<x>F_{\\alpha_{2}}<x>[ K_{2} ; 1 ])}+1 {(1<x>F_{\\alpha_{2}}<x>K_{2})}+q^{2}-2+q^{-2} {(F\n4<x>[ K_{2} ; 1 ]<x>[ K_{2} ; 1 ])}+-q+q^{-1} {(F_{\\alpha_{2}}<x>[ K_{2} ; 1 ]<x>K_{2})}+-q+q^{-1} {(F_{\\alpha_{2}}<x>K\n2<x>[ K_{2} ; 1 ])}+1 {(F_{\\alpha_{2}}<x>K_{2}<x>K_{2})}']


and instead of F_{\\alpha_{1}} at few places it outputs F\n1 - probably some unfortunate combination of escape sequences, dunno... (needless to say, the output at the sage prompt is fine).

I'm inclined to mark these as # known bug and deal with them later...

Last edited 3 years ago by dimpase (previous) (diff)

### comment:41 Changed 3 years ago by embray

In the case of the "grape" package, ISTM the only actual "compiled" part is not technically part of the package at all, but rather it builds its own copy of nauty. It ought to be able to use the nauty that's already included in Sage.

I probably won't stress about it too much in the short time that we have, but this should probably be fixed later.

### comment:42 Changed 3 years ago by dimpase

I agree about nauty/grape - see #26923

### comment:43 in reply to: ↑ 40 ; follow-up: ↓ 44 Changed 3 years ago by embray

I'm getting an unpleasant surprise from the doctest framework here - it mangles long lines in this file, e.g. here: and instead of F_{\\alpha_{1}} at few places it outputs F\n1 - probably some unfortunate combination of escape sequences, dunno... (needless to say, the output at the sage prompt is fine).

I'm not so sure about that. I don't think the doctest framework is doing that, but I'll look into it later.

### comment:44 in reply to: ↑ 43 Changed 3 years ago by dimpase

I'm getting an unpleasant surprise from the doctest framework here - it mangles long lines in this file, e.g. here: and instead of F_{\\alpha_{1}} at few places it outputs F\n1 - probably some unfortunate combination of escape sequences, dunno... (needless to say, the output at the sage prompt is fine).

I'm not so sure about that. I don't think the doctest framework is doing that, but I'll look into it later.

Might be a "noisy line" thing - but why only while doctesting? Anyhow I have opened #26924 to deal with it.

### comment:45 Changed 3 years ago by embray

I'm running the tests for some of these packages. A bunch of tests fail for guava: I'm not sure whether or not this is expected.

### comment:46 follow-up: ↓ 48 Changed 3 years ago by embray

A new failure I'm getting is:

File "src/sage/graphs/strongly_regular_db.pyx", line 2445, in sage.graphs.strongly_regular_db.SRG_416_100_36_20
Failed example:
g = SRG_416_100_36_20()                # optional - gap_packages # long time
Expected nothing
Got:
#I  io package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
#I  io package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
**********************************************************************
1 of   4 in sage.graphs.strongly_regular_db.SRG_416_100_36_20
[334 tests, 1 failure, 61.16 s]


So perhaps one of these packages at least tries to load the IO package if available, though doesn't strictly need it. In that case we should at least be silencing this message.

### comment:47 follow-ups: ↓ 49 ↓ 57 Changed 3 years ago by embray

Another one that's a bit troubling is:

sage -t --long src/sage/tests/gap_packages.py
**********************************************************************
File "src/sage/tests/gap_packages.py", line 8, in sage.tests.gap_packages
Failed example:
test_packages(pkgs, only_failures=True)    # optional - gap_packages
Expected:
Status   Package   GAP Output
+--------+---------+------------+
Got:
#I  HAP warning: Set POLYMAKE_PATH manually if needed.
#I  HAP warning: Set BROWSER_PATH manually if needed.
#I  HAP warning: Set ASY_PATH manually if needed.
#I  You may wish to install the xgap package
#I  and enjoy the graphic capabilities of SONATA.
#I  qpa-version package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
Status    Package       GAP Output
+---------+-------------+------------+
Failure   polenta       fail
Failure   qpa-version   fail
Failure   resclasses    fail
**********************************************************************
1 of   5 in sage.tests.gap_packages
[10 tests, 1 failure, 3.98 s]


I get these warnings when loading the relevant GAP packages in a vanilla GAP source tree, so I suppose they're normal and/or expected, or at least not indicative of any specific bug on our part. But we probably need to at least temporarily silence them for the purposes of the code and tests they affect.

The one exception being the error about "qpa-version". The name of the package is just "qpa", but its source directory is unusually named "qpa-version-<x.y>", so there's some code that's making an invalid assumption about GAP package names :sigh:

Last edited 3 years ago by embray (previous) (diff)

### comment:48 in reply to: ↑ 46 Changed 3 years ago by dimpase

A new failure I'm getting is:

File "src/sage/graphs/strongly_regular_db.pyx", line 2445, in sage.graphs.strongly_regular_db.SRG_416_100_36_20
Failed example:
g = SRG_416_100_36_20()                # optional - gap_packages # long time
Expected nothing
Got:
#I  io package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
#I  io package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
**********************************************************************
1 of   4 in sage.graphs.strongly_regular_db.SRG_416_100_36_20
[334 tests, 1 failure, 61.16 s]


So perhaps one of these packages at least tries to load the IO package if available, though doesn't strictly need it. In that case we should at least be silencing this message.

Yes, atlasrep does this. However, you might have a stale libgap workspace. I saw this warning, and then went away after I wiped the workspaces out.

### comment:49 in reply to: ↑ 47 Changed 3 years ago by dimpase

Another one that's a bit troubling is:

sage -t --long src/sage/tests/gap_packages.py
**********************************************************************
File "src/sage/tests/gap_packages.py", line 8, in sage.tests.gap_packages
Failed example:
test_packages(pkgs, only_failures=True)    # optional - gap_packages
Expected:
Status   Package   GAP Output
+--------+---------+------------+
Got:
#I  HAP warning: Set POLYMAKE_PATH manually if needed.
#I  HAP warning: Set BROWSER_PATH manually if needed.
#I  HAP warning: Set ASY_PATH manually if needed.
#I  You may wish to install the xgap package
#I  and enjoy the graphic capabilities of SONATA.
#I  qpa-version package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
Status    Package       GAP Output
+---------+-------------+------------+
Failure   polenta       fail
Failure   qpa-version   fail
Failure   resclasses    fail
**********************************************************************
1 of   5 in sage.tests.gap_packages
[10 tests, 1 failure, 3.98 s]


I get these warnings when loading the relevant GAP packages in a vanilla GAP source tree, so I suppose they're normal and/or expected, or at least not indicative of any specific bug on our part. But we probably need to at least temporarily silence them for the purposes of the code and tests they affect.

The one exception being the error about "qpa-version". The name of the package is just "qpa", but its source directory is unusually named "qpa-version-<x.y>", so there's some code that's making an invalid assumption about GAP package names :sigh:

Yes, that's where I am now too. I'll need to step away for an hour, I'll push what I have in a second.

### comment:50 Changed 3 years ago by git

• Commit changed from 01d85a7b1ebe5b95cb8a84d273dafd13c62f4a65 to 78db91a51010662e85dda67649495eaed4193025

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

 ​f31a8cd backport https://github.com/gap-packages/cohomolo/pull/19 ​78db91a known-buggification - see Trac #26924

### comment:51 Changed 3 years ago by dimpase

OK, over to you now.

### comment:52 Changed 3 years ago by embray

I think this change

• ## src/sage/algebras/quantum_groups/quantum_group_gap.py

diff --git a/src/sage/algebras/quantum_groups/quantum_group_gap.py b/src/sage/algebras/quantum_groups/quantum_group_gap.py
index 4ce98f4..0e4549b 100644
 a class QuantumGroupModule(Parent, UniqueRepresentation): sage: Q = QuantumGroup(['A',2])           # optional - gap_packages sage: V = Q.highest_weight_module([1,1])  # optional - gap_packages sage: V.gap()                             # optional - gap_packages <8-dimensional left-module over QuantumUEA( , Qpar = q )> <8-dimensional left-module over QuantumUEA( , Qpar = q )> """ return self._libgap

This one feels like a bug in the GAP and/or the QuaGroup? package to me, but I confirmed that this is in fact the output provided by GAP and not a bug on our end, so for the time being there's nothing to be done (other than possibly report it upstream)

This is one of the problems with GAP objects not having a real string representation that isn't tied to how exactly they're printed to the screen :|

### comment:53 Changed 3 years ago by embray

The problem here, accounting for all these test failures including the test above that was modified, is that the QuaGroups package seems (I have not looked for the exact code) to insert linebreaks into the representations of its types in different places depending on the terminal width. Not exactly sure if that's a GAP thing in general or something specific to this package, but this is the first place it's come up. In any case we apparently aren't dealing well with those unnecessary line breaks.

### comment:54 Changed 3 years ago by dimpase

linebreaks are inserted by GAP, it was always the case.

### comment:55 Changed 3 years ago by embray

I see now, the output splitting is indeed done by default by GAP. Again, this is no good, especially since it can depend on terminal width. I haven't found yet exactly where GAP determines this, but I need to figure out a way to force it to just not do this at all, if possible. That might be something to do as part of #22626 or as a follow-up.

### comment:56 Changed 3 years ago by git

• Commit changed from 78db91a51010662e85dda67649495eaed4193025 to fc197922d0c70666dfaa48efccbedfdeb7be1070

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

 ​fc19792 install deps of polenta, qpa, resclasses, fix test

### comment:57 in reply to: ↑ 47 Changed 3 years ago by dimpase

Another one that's a bit troubling is:

sage -t --long src/sage/tests/gap_packages.py
[.....]
+---------+-------------+------------+
Failure   polenta       fail
Failure   qpa-version   fail
Failure   resclasses    fail
**********************************************************************
1 of   5 in sage.tests.gap_packages
[10 tests, 1 failure, 3.98 s]


I get these warnings when loading the relevant GAP packages in a vanilla GAP source tree, so I suppose they're normal and/or expected, or at least not indicative of any specific bug on our part. But we probably need to at least temporarily silence them for the purposes of the code and tests they affect.

The one exception being the error about "qpa-version". The name of the package is just "qpa", but its source directory is unusually named "qpa-version-<x.y>", so there's some code that's making an invalid assumption about GAP package names :sigh:

the latest commit fixes these failures, which are two-fold: missing dependencies and weird -version-part which one should strip.

I should still figure out how to suppress these #I ... lines...

### comment:58 Changed 3 years ago by git

• Commit changed from fc197922d0c70666dfaa48efccbedfdeb7be1070 to 92517b495dc21eb739f9c8fcb39840674bc690f0

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

 ​92517b4 suppress "#I.." warnings

### comment:59 follow-up: ↓ 66 Changed 3 years ago by git

• Commit changed from 92517b495dc21eb739f9c8fcb39840674bc690f0 to 08ffb3396308b4643cfcbed1c56c0272c39f6a17

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

 ​08ffb33 more doctest fixes

### comment:60 Changed 3 years ago by dimpase

By the way, I tried installing IO package in the same way as this ticket builds packages needing compilation, but this fails due to a "missing" header (not sure what goes wrong):

[gap_packages-4.10.0] configure: creating ./config.status
[gap_packages-4.10.0] config.status: creating Makefile
[gap_packages-4.10.0] config.status: creating src/pkgconfig.h
[gap_packages-4.10.0] config.status: executing depfiles commands
[gap_packages-4.10.0] config.status: executing libtool commands
[gap_packages-4.10.0] make[2]: Entering directory '/mnt/opt/Sage/sage-dev/local/share/gap/pkg/io-4.5.4'
[gap_packages-4.10.0] /bin/sh ./libtool  --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. -I./src  -I/mnt/opt/Sage/sage-dev/local/var/tmp/sage/build/gap-4.10.0/src/gen -I/mnt/opt/Sage/sage-dev/local/var/tmp/sage/build/gap-4.10.0/src/src -I/mnt/opt/Sage/sage-dev/local/var/tmp/sage/build/gap-4.10.0/src -DHAVE_CONFIG_H -I/mnt/opt/Sage/sage-dev/local/include        -O2 -g   -g -O2 -MT src/io_la-io.lo -MD -MP -MF src/.deps/io_la-io.Tpo -c -o src/io_la-io.lo test -f 'src/io.c' || echo './'src/io.c
[gap_packages-4.10.0] libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I./src -I/mnt/opt/Sage/sage-dev/local/var/tmp/sage/build/gap-4.10.0/src/gen -I/mnt/opt/Sage/sage-dev/local/var/tmp/sage/build/gap-4.10.0/src/src -I/mnt/opt/Sage/sage-dev/local/var/tmp/sage/build/gap-4.10.0/src -DHAVE_CONFIG_H -I/mnt/opt/Sage/sage-dev/local/include -O2 -g -g -O2 -MT src/io_la-io.lo -MD -MP -MF src/.deps/io_la-io.Tpo -c src/io.c  -fPIC -DPIC -o src/.libs/io_la-io.o
[gap_packages-4.10.0] src/io.c:14:10: fatal error: src/compiled.h: No such file or directory
[gap_packages-4.10.0]  #include "src/compiled.h"          /* GAP headers                */
[gap_packages-4.10.0]           ^~~~~~~~~~~~~~~~


looks like the corresponding -I arguments all have an extra /src suffix, something that prevents this header to be found.

### comment:61 follow-up: ↓ 67 Changed 3 years ago by dimpase

..."thanks" to Gatwick drone operators, I can do this work now, and not sit on a plane off to Singapore...

### comment:62 Changed 3 years ago by dimpase

GAP's IO is on #26930

### comment:63 Changed 3 years ago by git

• Commit changed from 08ffb3396308b4643cfcbed1c56c0272c39f6a17 to c2e30ae0aff04ce2c88b9e6e9126caac305e85eb

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

 ​c2e30ae silence one more #I message from GAP

### comment:64 Changed 3 years ago by dimpase

• Status changed from needs_work to needs_review

OK, seems to be ready now

• ## src/sage/graphs/strongly_regular_db.pyx

diff --git a/src/sage/graphs/strongly_regular_db.pyx b/src/sage/graphs/strongly_regular_db.pyx
index 8df9bf5..06a0780 100644
 a def SRG_416_100_36_20(): EXAMPLES:: sage: from sage.graphs.strongly_regular_db import SRG_416_100_36_20 sage: libgap.eval("SetInfoLevel(InfoWarning,0)") # silence #I warnings sage: g = SRG_416_100_36_20()                # optional - gap_packages # long time sage: libgap.eval("SetInfoLevel(InfoWarning,1)") # silence #I warnings sage: g.is_strongly_regular(parameters=True) # optional - gap_packages # long time (416, 100, 36, 20) """

I don't like that this is done in the doctest; it distracts from the actual documentation. I think this should go in the function itself. We've already seen the warning in question, decided it is not important to us, and can go ahead and silence it.

Last edited 3 years ago by embray (previous) (diff)

### comment:66 in reply to: ↑ 59 ; follow-ups: ↓ 69 ↓ 72 Changed 3 years ago by embray

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

 ​08ffb33 more doctest fixes

This change doesn't seem to work for me. Those tests passed before this commit, and fail after. I tried clearing my existing workspaces but it still fails. That's troubling--it may still be some badly managed random state somewhere. Unless I'm missing something else.

### comment:67 in reply to: ↑ 61 ; follow-up: ↓ 68 Changed 3 years ago by embray

..."thanks" to Gatwick drone operators, I can do this work now, and not sit on a plane off to Singapore...

Huh?? What happened? :(

### comment:68 in reply to: ↑ 67 Changed 3 years ago by dimpase

..."thanks" to Gatwick drone operators, I can do this work now, and not sit on a plane off to Singapore...

Huh?? What happened? :(

Lucky you, you don't read news. :-) Gatwick was shut down for ~36 hours due to some not yet caught folks flying a largish drone over it several times, 700+ flights cancelled, 100s diverted. We were able to book an even cheaper flight, via Berlin, and get a full refund. They are open since this morning, and in fact the airline rebooked us for tonight, but we were in the full panic mode and got the other flight already...

### comment:69 in reply to: ↑ 66 Changed 3 years ago by dimpase

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

 ​08ffb33 more doctest fixes

This change doesn't seem to work for me. Those tests passed before this commit, and fail after. I tried clearing my existing workspaces but it still fails. That's troubling--it may still be some badly managed random state somewhere. Unless I'm missing something else.

they pass for me, both with and without --long, on Linux and on OSX.

I'd try rm -rf local/share/gap local/include/gap followed by ./sage -f gap gap_packages followed up by make build, and then testing...

(and wiping out ~/.sage/gap/ too)

Last edited 3 years ago by dimpase (previous) (diff)

### comment:70 follow-up: ↓ 71 Changed 3 years ago by git

• Commit changed from c2e30ae0aff04ce2c88b9e6e9126caac305e85eb to 1e5f679c41a7b27088ed330f986df91d03099a71

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

 ​1e5f679 move silencing into the function body

### comment:71 in reply to: ↑ 70 ; follow-up: ↓ 107 Changed 3 years ago by dimpase

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

 ​1e5f679 move silencing into the function body

### comment:72 in reply to: ↑ 66 Changed 3 years ago by embray

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

 ​08ffb33 more doctest fixes

This change doesn't seem to work for me. Those tests passed before this commit, and fail after. I tried clearing my existing workspaces but it still fails. That's troubling--it may still be some badly managed random state somewhere. Unless I'm missing something else.

Nevermind, that was my fault. Nevertheless, this still won't work: The outcomes of these tests depend on whether or not gap_packages is installed, so if it isn't then these will give different results. I'm not sure what to do: Maybe these tests should be rewritten to be more flexible, or the code that's being tested needs to be rewritten to be less sensitive to what GAP packages are installed (if that's even possible...)

### comment:73 Changed 3 years ago by dimpase

Let's just make gap_packages standard, as planned anyway. This will alleviate this problem.

### comment:74 follow-up: ↓ 76 Changed 3 years ago by embray

I don't think we should do that. It's bad enough that we're adding all the "default" packages, but that does seem defensible.

Otherwise there's no point in even having a separate spkg.

Meanwhile, I don't like that merely having some random package installed can cause actually different (even if not substantively) results, in ways that are very difficult to trace. This is a really annoying problem with GAP...

Last edited 3 years ago by embray (previous) (diff)

### comment:75 Changed 3 years ago by embray

Let's see... For the test that's failing in sage.libs.gap.element it should suffice to just put ellipses after ValueError: libGAP: Error, no method found!. That's the only part that's important; the rest are details.

The other two look like they might result from different PRNG states, so it might suffice for those two to explicitly re-seed the PRNG before them; let me try.

### comment:76 in reply to: ↑ 74 Changed 3 years ago by dimpase

I don't think we should do that. It's bad enough that we're adding all the "default" packages, but that does seem defensible.

Otherwise there's no point in even having a separate spkg.

for transition from several to one package it's OK, I think. We can merge them later on another ticket.

Meanwhile, I don't like that merely having some random package installed can cause actually different (even if not substantively) results, in ways that are very difficult to trace. This is a really annoying problem with GAP...

### comment:77 Changed 3 years ago by embray

I'm definitely -1 on making it a standard package, especially in the short time that we have. These have always been an optional add-on, and for the time being I don't want to add any more footprint to the default installation than we already are (keep in mind, this also increases the possibility of bugs in user code for which we don't have test cases).

### comment:78 Changed 3 years ago by embray

In the cases of the test we have failing currently, it appears that loading some of these packages just modifies the PRNG state, so the best solution for now is to explicitly re-seed them before the affected tests (not ideal, but it will ensure consistent results).

### comment:79 Changed 3 years ago by embray

• Branch changed from u/dimpase/GQ to public/ticket-26856
• Commit 1e5f679c41a7b27088ed330f986df91d03099a71 deleted
• Status changed from needs_review to needs_work

Public branch for this ticket. It is up-to-date with Dima's branch minus the last two commits that are contentious.

### comment:80 Changed 3 years ago by dimpase

your branch does not merge cleanly...

### comment:81 Changed 3 years ago by dimpase

How about tagging the previously untagged tests that differ if gap_packages are installed with # optional - gap_packages, and be done with it? It's not that many tests, right?

### comment:82 Changed 3 years ago by embray

It might need to be rebased again already; I don't know what Volker has been doing. I just didn't push it yet.

I already have another solution for these tests that is less fragile.

Last edited 3 years ago by embray (previous) (diff)

### comment:83 Changed 3 years ago by git

• Commit set to 3b3053bdff12aeb168bc0157aaa4cb3e850839e8

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

 ​5e2220d backport guava patch to allow no gcc ​29c82e8 put each installed package on its own line for readability ​d7bd012 remove unnecessary cruft ​8f22321 I don't think this is necessary anymore either ​d0b769d set GAP's notion of 'screen width' to the maximum allowed ​b50f488 backport https://github.com/gap-packages/cohomolo/pull/19 ​884bc4f install deps of polenta, qpa, resclasses, fix test ​bf2890e emphasize here that the pacakge's name is 'qpa', not 'qpa-version' ​8e81b42 the exact output of this test depends somewhat on which GAP packages happen to ​3b3053b add a convenience method on the libgap object to reset the random seed on

### comment:84 Changed 3 years ago by embray

For the remaining problematic tests I was able to fix them by manually calling (with some explanation in the prose) libgap.set_seed(0) before them (and modifying the expected output to correspond to that random seed).

It's unfortunate but possibly necessary, since the state of the PRNG can seem to depend on which packages were loaded and when, and for whatever reason those tests just happened to be very sensitive to this.

I'm re-running the full test suite now to see if any other examples like this turn up.

### comment:85 Changed 3 years ago by git

• Commit changed from 3b3053bdff12aeb168bc0157aaa4cb3e850839e8 to 78859fff771aa376e486d83e1ee4b6ff8ec9afb3

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

 ​78859ff clean up installation of compiled packages so that they are are also

### comment:86 Changed 3 years ago by embray

Oops, forgot to finish the tests for libgap.set_seed.

### comment:87 Changed 3 years ago by git

• Commit changed from 78859fff771aa376e486d83e1ee4b6ff8ec9afb3 to 728ad7faab01b0839317c3e77acef5c14e1a0ddb

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

 ​0611aa2 add a convenience method on the libgap object to reset the random seed on ​728ad7f clean up installation of compiled packages so that they are are also

### comment:88 Changed 3 years ago by dimpase

your branch does not have 8.5.rc1 merged, by the way. Just testing it might show wrong test faults...

### comment:89 in reply to: ↑ 8 ; follow-up: ↓ 92 Changed 3 years ago by embray

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


I don't suppose you ever found a solution and/or explanation for this failure? I have some workarounds for most everything else but this.

### comment:90 Changed 3 years ago by git

• Commit changed from 728ad7faab01b0839317c3e77acef5c14e1a0ddb to 6a2d88243b82b25d8e22362c4ea2aae03ca4b72d

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

 ​6a2d882 fix some tests where GAP output was improved by increasing the GAP output line width

### comment:91 Changed 3 years ago by git

• Commit changed from 6a2d88243b82b25d8e22362c4ea2aae03ca4b72d to f44ca4cd544b5d565166f4d04c3ef7e11e2267f0

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

 ​f44ca4c explicitly calling libgap.set_seed(0) for these tests allows consistent results for them

### comment:92 in reply to: ↑ 89 Changed 3 years ago by dimpase

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


I don't suppose you ever found a solution and/or explanation for this failure? I have some workarounds for most everything else but this.

see comment:11. Have you merged the corresponding commit? (from the ticket it refers to)

that's why #26889 is a dependence of this ticket.

Last edited 3 years ago by dimpase (previous) (diff)

### comment:93 follow-up: ↓ 94 Changed 3 years ago by embray

The issue here again seems to have something in part to do with the random number generator state in GAP. In the case of:

sage: MG = GU(3,2).as_matrix_group()
sage: PG = MG.as_permutation_group()
sage: PG2 = MG._permutation_group_()


usually PG and PG2 will be the same, but on rare occasion they will be different, and that just happened to be the case in this test. Could that be a bug in GAP, or expected behavior? That I don't know.

That code is still a mess on the Sage end too as well, so any effort to clean it up can't hurt.

### comment:94 in reply to: ↑ 93 ; follow-up: ↓ 112 Changed 3 years ago by dimpase

The issue here again seems to have something in part to do with the random number generator state in GAP. In the case of:

sage: MG = GU(3,2).as_matrix_group()
sage: PG = MG.as_permutation_group()
sage: PG2 = MG._permutation_group_()


usually PG and PG2 will be the same, but on rare occasion they will be different, and that just happened to be the case in this test. Could that be a bug in GAP, or expected behavior? That I don't know.

We have two instances of GAP talking to each other, how their RNGs are syncronised is an interesting question. Are they?

### comment:95 Changed 3 years ago by git

• Commit changed from f44ca4cd544b5d565166f4d04c3ef7e11e2267f0 to ca05b247f3c373950a1f883ac5c97f8bb1917781

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

 ​35abd34 Merge branch 'develop' of trac.sagemath.org:sage into gap410pkg ​db3f63e more libGAP in as_permutation(), less pexpect mess ​2d77eee follow up pyflake hints (all but one) ​7dd5fa6 reviewer's example and fix ​ca05b24 Merge branch 'u/dimpase/groups/better_as_permutation' of trac.sagemath.org:sage into gap410pkg

### comment:96 Changed 3 years ago by dimpase

new gap_packages installation does not work, breaks at cohomolo GAP package:

[gap_packages-4.10.0] cp calcpres.gap ..//..//bin/x86_64-pc-linux-gnu-default64/calcpres.gap
[gap_packages-4.10.0] chmod +x ..//..//bin/x86_64-pc-linux-gnu-default64/calcpres.gap
[gap_packages-4.10.0] make[3]: Leaving directory '/home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/gap_packages-4.10.0/src/pkg/cohomolo-1.6.7/standalone/progs.d'
[gap_packages-4.10.0] make[2]: Leaving directory '/home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/gap_packages-4.10.0/src/pkg/cohomolo-1.6.7'
[gap_packages-4.10.0] bin/ -> /home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/gap_packages-4.10.0/inst/home/scratch2/dimpase/sage/sage/local/lib/gap/pkg/cohomolo-1.6.7
[gap_packages-4.10.0] make[2]: Entering directory '/home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/gap_packages-4.10.0/src/pkg/cohomolo-1.6.7'
[gap_packages-4.10.0] cd standalone/progs.d; make clean
[gap_packages-4.10.0] make[3]: Entering directory '/home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/gap_packages-4.10.0/src/pkg/cohomolo-1.6.7/standalone/progs.d'
[gap_packages-4.10.0] /bin/rm -f *.o
[gap_packages-4.10.0] make[3]: Leaving directory '/home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/gap_packages-4.10.0/src/pkg/cohomolo-1.6.7/standalone/progs.d'
[gap_packages-4.10.0] make[2]: Leaving directory '/home/scratch2/dimpase/sage/sage/local/var/tmp/sage/build/gap_packages-4.10.0/src/pkg/cohomolo-1.6.7'
[gap_packages-4.10.0] ********************************************************************************
[gap_packages-4.10.0] Error: source file/directory bin does not exist
[gap_packages-4.10.0] ********************************************************************************


(this is on Fedora 26, in case it matters)

### comment:97 follow-up: ↓ 100 Changed 3 years ago by vbraun

#22626 breaks some of the # optional - database_gap doctests. I'll ignore that failure for now but please fix asap.

### comment:98 follow-up: ↓ 110 Changed 3 years ago by git

• Commit changed from ca05b247f3c373950a1f883ac5c97f8bb1917781 to bd849fbe2b32a6cc8d4b56b334e4293846061a6b

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

 ​bd849fb don't clean too early

### comment:99 Changed 3 years ago by dimpase

• Authors changed from Dima Pasechnik to Dima Pasechnik, Erik Bray
• Status changed from needs_work to needs_review

### comment:100 in reply to: ↑ 97 Changed 3 years ago by dimpase

#22626 breaks some of the # optional - database_gap doctests. I'll ignore that failure for now but please fix asap.

Yes, this ticket gets rid of database_gap (everything there will be a part of gap package), so this is taken care of here.

I've also fixed the build failure here, so it's ready for review, I think.

### comment:101 Changed 3 years ago by dimpase

So the current scheme is that gap_packages remains optional spkg, for the time being at least. I'd rather make it standard too, and then merge into gap package, but Erik thinks it's too big a change (I don't think so---I think the change between what we have in Sage 8.4 (and 8.5) is already very big, not the least due to GAP 4.10 being quite a big change from GAP 4.8, and forcing people to adjust their code several times is worse than asking to do so once--- but OK).

Last edited 3 years ago by dimpase (previous) (diff)

### comment:102 Changed 3 years ago by git

• Commit changed from bd849fbe2b32a6cc8d4b56b334e4293846061a6b to ff7722e4edb044fcadef0c83d78d72193a4aa013

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

 ​ff7722e suppress "#I.." warnings

### comment:103 Changed 3 years ago by dimpase

this commit went AWOL from the current branch. With it in, all tests pass

### comment:104 Changed 3 years ago by git

• Commit changed from ff7722e4edb044fcadef0c83d78d72193a4aa013 to bf405752a966b932afb7a7bb34d7be388c94324b

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

 ​bf40575 silencing of #I warning from GAP in the function body

### comment:105 Changed 3 years ago by dimpase

One more AWOL commit.

### comment:106 follow-up: ↓ 108 Changed 3 years ago by git

• Commit changed from bf405752a966b932afb7a7bb34d7be388c94324b to 37e23290bad2ee4d36463647f4c058c500b81eba

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

 ​37e2329 tagged # random - answer is always mathematically OK, outputs differ

### comment:107 in reply to: ↑ 71 Changed 3 years ago by embray

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

 ​1e5f679 move silencing into the function body

This should probably use InfoLevel(InfoWarning) to get the previous level, not just assume to restore it to 1. Probably unlikely to matter usually but it is still a bug waiting to happen.

### comment:108 in reply to: ↑ 106 Changed 3 years ago by embray

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

 ​37e2329 tagged # random - answer is always mathematically OK, outputs differ

I'm going to double-check but this shouldn't be necessary. This is what #26874 was fixing.

### comment:109 Changed 3 years ago by embray

Yes, merging in #26874 guarantees that test always works the same way, as the RNG state is consistent.

I'll deal with comment:107 and then do a couple final checks before setting positive_review.

### comment:110 in reply to: ↑ 98 Changed 3 years ago by embray

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

 ​bd849fb don't clean too early

This "fix" is a problem as well. The "cleanup" commands that were moved were deliberately before sdh_install * so that files that shouldn't be installed aren't installed. I'll see if I can reproduce the problem you had; I did not have the same problem on Ubuntu.

### comment:111 Changed 3 years ago by embray

• Status changed from needs_review to needs_work

### comment:112 in reply to: ↑ 94 Changed 3 years ago by embray

The issue here again seems to have something in part to do with the random number generator state in GAP. In the case of:

sage: MG = GU(3,2).as_matrix_group()
sage: PG = MG.as_permutation_group()
sage: PG2 = MG._permutation_group_()


usually PG and PG2 will be the same, but on rare occasion they will be different, and that just happened to be the case in this test. Could that be a bug in GAP, or expected behavior? That I don't know.

We have two instances of GAP talking to each other, how their RNGs are syncronised is an interesting question. Are they?

Nope. I do have an experimental branch where I modified Randstate.set_seed_gap() to seed both libgap and the pexpect GAP simultaneously, but I haven't decided whether or not to use it yet.

### comment:113 Changed 3 years ago by git

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

 ​f6c8734 When running doctests, use the current_randstate()'s seed for Interface.rand_seed() ​1dc5078 Add the generic InfoWarning info class to the 'standard' GAP globals ​e2c2c54 suppress "#I.." warnings ​5ad8209 followup to #22626: adds a prerm script to the gap SPKG

### comment:114 Changed 3 years ago by git

• Commit changed from 5ad8209875bf43561ff0ff86b797cae67e1d5c02 to 84a3d2f93f74ce2d09d19e0a55ee5ae8a9c7f45f

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

 ​2449886 When running doctests, use the current_randstate()'s seed for Interface.rand_seed() ​d9dbd00 Add the generic InfoWarning info class to the 'standard' GAP globals ​40b4108 suppress "#I.." warnings ​f3a938b followup to #22626: adds a prerm script to the gap SPKG ​84a3d2f make sure this make clean command is not run in parallel

### comment:115 Changed 3 years ago by embray

Can you check if the latest commit fixes the issue from comment:96 ? I can't reproduce that so I'm not really sure what's going on there (seeing more of the log might help too, if it's not fixed...)

### comment:116 follow-up: ↓ 136 Changed 3 years ago by slelievre

• Cc alexk arojas dimpase embray fbissey mantepse mmarco slelievre tscrim vbraun added
• Description modified (diff)
• Milestone changed from sage-8.5 to sage-8.6

Does this also fix #23054?

### comment:117 Changed 3 years ago by mantepse

I also have the problem from comment:96, on ubuntu 17.10, sage 8.6.beta0. I attached the log.

### comment:118 follow-up: ↓ 119 Changed 3 years ago by embray

Thanks--I was able to reproduce the problem in a docker container. My attempted fix doesn't seem to help. Not sure what the problem is though; it's an odd one...

### comment:119 in reply to: ↑ 118 Changed 3 years ago by dimpase

Thanks--I was able to reproduce the problem in a docker container. My attempted fix doesn't seem to help. Not sure what the problem is though; it's an odd one...

IMHO it should not hold up this ticket - open a follow-up ticket to fix it.

### comment:120 Changed 3 years ago by git

• Commit changed from 84a3d2f93f74ce2d09d19e0a55ee5ae8a9c7f45f to f3a938b92a922a0e78b955b90183b739c965d033

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

### comment:121 Changed 3 years ago by embray

• Dependencies changed from #22626, #26889 to #22626, #26889, #26965

### comment:122 Changed 3 years ago by git

• Commit changed from f3a938b92a922a0e78b955b90183b739c965d033 to 293e79ab78bf1574e469ddfa9ff8a93b7087c396

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

 ​3213d2c install deps of polenta, qpa, resclasses, fix test ​2a2eb73 emphasize here that the pacakge's name is 'qpa', not 'qpa-version' ​d50642b the exact output of this test depends somewhat on which GAP packages happen to ​b68c547 add a convenience method on the libgap object to reset the random seed on ​47e4b6e clean up installation of compiled packages so that they are are also ​2da5c32 fix some tests where GAP output was improved by increasing the GAP output line width ​508e6d8 explicitly calling libgap.set_seed(0) for these tests allows consistent results for them ​08f52a3 Add the generic InfoWarning info class to the 'standard' GAP globals ​2b65d0c suppress "#I.." warnings ​293e79a followup to #22626: adds a prerm script to the gap SPKG

### comment:123 Changed 3 years ago by embray

Rebased on 8.6.beta0

### comment:124 Changed 3 years ago by git

• Commit changed from 293e79ab78bf1574e469ddfa9ff8a93b7087c396 to 9854c3a3a59fd2ab42aacfb78f38346439f280d9

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

 ​5f49332 fix sdh_install of symlinks with invalid targets ​9854c3a Merge branch 'u/embray/build/sdh-install-symlinks' into public/ticket-26856

### comment:125 Changed 3 years ago by embray

• Status changed from needs_work to needs_review

Merged fix from #26965 so that this branch can be built successfully from a fresh install.

### comment:126 Changed 3 years ago by dimpase

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

### comment:127 follow-up: ↓ 129 Changed 3 years ago by 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 ?

### comment:128 Changed 3 years ago by embray

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

### comment:129 in reply to: ↑ 127 Changed 3 years ago by embray

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.

### comment:130 Changed 3 years ago by dimpase

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

### comment:131 Changed 3 years ago by embray

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.

### comment:132 Changed 3 years ago by git

• Commit changed from 9854c3a3a59fd2ab42aacfb78f38346439f280d9 to 8bf1044efda750d8a767d88aa6a2afda69edadbe

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

 ​8bf1044 disable doctesting of these modules

### comment:133 Changed 3 years ago by mantepse

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

Thank you!

### comment:134 Changed 3 years ago by dimpase

• Status changed from needs_review to positive_review

OK, great, all works.

New commits:

 ​8bf1044 disable doctesting of these modules

### comment:135 Changed 3 years ago by dimpase

• Reviewers set to Erik Bray, Dima Pasechnik

### comment:136 in reply to: ↑ 116 Changed 3 years ago by dimpase

Does this also fix #23054?

yes it does, thanks for pointing this out.

### comment:137 Changed 3 years ago by vbraun

• Branch changed from public/ticket-26856 to 8bf1044efda750d8a767d88aa6a2afda69edadbe
• Resolution set to fixed
• Status changed from positive_review to closed