Fix spkg-install of p_group_cohomology

[simon-king-jena]

This is a reformulation of the ticket summary, after some discussion went on.

p_group_cohomology is divided into four parts:

• a C-library libmodres (autotoolized), also providing a data base.
• a pip-installable collection of Python / Cython modules
• some files containing GAP code (let's call it GAP helper module)
• a documentation using sphinx

Problem: Some of the modules refer to SAGE_LOCAL at installation or at run time, respectively. Moreover, the GAP helper module is currently installed into SAGE_LOCAL/share/sage, which has been removed.

Issues of each part:

• The C-library seems totally fine. One could consider to turn it into a stand-alone spkg.
• The Python / Cython modules seem mostly fine. Problems:
  • Its setup.py takes the job of installing the GAP helper module, in a folder whose location is determined from the value of SAGE_LOCAL and which actually doesn't exist any longer.
  • Some Python module makes GAP read the GAP helper module, thus also determines the import location from the value of SAGE_LOCAL.
    Proposed solution:
  • It shouldn't be the job of setup.py to install the GAP helper module.
  • It should be possible for GAP to load the helper module without explicitly constructing a path derived from SAGE_LOCAL.
    Side remark: It would be possible to move the Python / Cython modules into Sage's src tree, but that would created issues with testing and documentation.
• The files containing GAP code are not intended to be a proper GAP module, but should perhaps behave like a proper GAP module. A more or less clean solution would be to put it into GAP's pkg folder such that it can be loaded by LoadPackage("pGroupCohomologyHelpers");. Problem: How to determine GAP's pkg folder at installation time without referring to SAGE_LOCAL?
• Documentation is built using $MAKE html and then copied. It was suggested to replace cp -r by sdh_install.

Upstream of the package: https://github.com/sagemath/p_group_cohomology/releases/tag/v3.3.2

CC: @embray @jhpalmieri @orlitzky @kiwifb @dimpase

Component: packages: optional

Author: Simon King

Branch/Commit: f4e5f6a

Reviewer: Dima Pasechnik, Matthias Koeppe

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

[simon-king-jena]

comment:1

The spkg-install script does use sdh_configure, sdh_make and sdh_pip_install, but is also uses "$MAKE install" and "$MAKE html" and "cp -r". I guess "$MAKE install" should become "sdh_make_install", but I don't know what to do with "$MAKE html" and "cp -r" which is used to build and install the package documentation.

[jhpalmieri]

comment:3

Can you use $MAKE html to build the documentation and then sdh_install to install it?

[embray]

comment:4

Ticket retargeted after milestone closed

[mkoeppe]

comment:6

Also it looks like dependencies are not right.
https://github.com/mkoeppe/sage/runs/874569119

[jhpalmieri]

comment:8

There have for a long time been issues with installation of p_group_cohomology: it seems that you have to install meataxe and then run sage -b and then install p_group_cohomology. Something like that, anyway. If someone could straighten that out, that would be great.

[kiwifb]

comment:9

I have been reluctant to work on p_group_cohomology for a long time because of the state of meataxe - in the installation department. I am not planning to work on this until there is proof of improvement. In the past I would have been willing to help with that, nowadays I am considering myself overcommitted already.

[mkoeppe]

comment:10

Thanks for the info. Then let's postpone work on this until after modularization of sagelib, which will eliminate this complicated dependency.

[mkoeppe]

Dependencies: #30151

[simon-king-jena]

comment:13

There is an issue that came up at #30349: Apparently there is a problem with the installation folder of gap code that is part of the package. It isn't clear to me how this should be fixed.

[simon-king-jena]

comment:14

On the other ticket, Dima stated:

///

You can call GAP and examine GAPInfo.RootPaths. When looking for a package, GAP appends pkg/ to each directory in this list, cf. 9.2 GAP Root Directories in its manual.

IMHO, Python wheels are build to be relocateable, so it's futile to try to install GAP data via setup.py. I'd say, install a GAP package separately, not from setup.py from spkg-install. By the way, GAP now has its own package manager. Maybe you can use it (​https://github.com/gap-packages/PackageManager) - it's in gap_packages, I think.

///

So, I'll have a look at it.

Question: When using GAP's package manager, it would probably be possible to do it in the setup.py, using libgap. Should I keep it there, or is it cleaner to move installation of GAP code to spkg-install?

[dimpase]

comment:15

one can do this in setup.py regardless, getting a proper path to install a GAP package to is just

sage: next(filter(lambda l: l.sage().startswith(SAGE_LOCAL), libgap.eval('GAPInfo.RootPaths'))).sage()+'pkg/'                                                                                                                          
'/mnt/opt/Sage/sage-dev/local/share/gap/pkg/'

[dimpase]

comment:16

not sure how "pythonic" this is though.

[simon-king-jena]

comment:17

Replying to @simon-king-jena:

IMHO, Python wheels are build to be relocateable, so it's futile to try to install GAP data via setup.py. I'd say, install a GAP package separately, not from setup.py from spkg-install. By the way, GAP now has its own package manager. Maybe you can use it (​https://github.com/gap-packages/PackageManager) - it's in gap_packages, I think.

Do I understand correctly that GAP's PackageManager is for installing packages from the GAP ecosystem? What I want to do is to use some GAP code that is NOT an upstream GAP package (not even an unofficial "submitted" package).

Anyway, using some GAP infrastructure to install my files, rather than hacking paths, seems the right approach to me.

[simon-king-jena]

comment:18

I see one and a half clear disadvantages of the PackageManager:

The PackageManager is provided by the gap_packages spkg. Thus, p_group_cohomology would need yet another dependency. I'd rather avoid to add it. It should be enough to have the Small Groups library in GAP (which I think meanwhile is available without installing optional spkgs).

The "half" disadvantage is the GAP-typical lack of a clear documentation. The documentation of InstallPackage says that one way to install a package is to provide the URL of a valid PackageInfo.g file. But nowhere is defined (at least I couldn't find it at https://gap-packages.github.io/PackageManager/doc/chap0.html) what constitutes a "valid" PackageInfo.g file.

In other words, it is explained how to install a package, but it is not explained how to create a package that can be installed.

Do you have pointers how to create a GAP package?

[simon-king-jena]

comment:19

PS: I just installed the gap_package spkg, but apparently it does not provide the PackageManager.

So, I guess it is no option.

[simon-king-jena]

comment:20

I am unhappy, because I don't see a good solution for the installation problems of the GAP parts of the spkg.

WHAT NEEDS TO BE AVAILABLE:

• A couple of .g files, containing function definitions.

CURRENT APPROACH:

• Install the files into $SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/
• Let setup.py do the installation

CURRENT PROBLEM:

• As reported on Downgrade broken optional packages to experimental for Sage 9.2 #30349, Sage totally messes up the path. Note that I have never seen that problem, myself.
• It is argued that installation of the .g files should be done in spkg-install, not in setup.py.

ALTERNATIVE APPROACH:

• Put the files somewhere else, such as $SAGE_LOCAL/share/gap/pkg/
• Let spkg-install do the installation, perhaps via some GAP tool

PROBLEM WITH THE ALTERNATIVE APPROACH:

• I don't consider the couple of .g files a "package" for GAP.
• Potential GAP tools seem to be not part of the gap_packages spkg. Thus, aren't readily available in Sage.

So, I believe the current approach is just fine: We need a place for some GAP readable files, that logically belong to a Sage package (p_group_cohomology) but can hardly be seen a stand-alone GAP package. Thus, some place in $SAGE_LOCAL/share/sage/ext/gap seems just right to me.

Only we need to understand why, according to #30349, Sage puts stuff into $SAGE_LOCAL/lib/python3.7/site-packages/$SAGE_LOCAL/share/sage/ext/gap/modular_cohomology/, although setup.py clearly states, data_files=[(os.path.join(SAGE_SHARE,"sage","ext","gap","modular_cohomology").

[simon-king-jena]

comment:21

AHA! Can it be that $SAGE_LOCAL/share/sage has been removed?

sage: 'sage' in os.listdir(os.path.join(SAGE_LOCAL,"share"))                                                       
False
sage: 'gap' in os.listdir(os.path.join(SAGE_LOCAL,"share"))                                                        
True

Then of course a different location is needed. In that case, os.listdir(os.path.join(SAGE_LOCAL,"share","gap","pkg","p_group_cohomology")) seems good to me.

EDIT: Since it isn't really a GAP package, os.listdir(os.path.join(SAGE_LOCAL,"share","p_group_cohomology")) seems better!

[mkoeppe]

comment:22

Replying to @simon-king-jena:

I am unhappy, because I don't see a good solution for the installation problems of the GAP parts of the spkg.

WHAT NEEDS TO BE AVAILABLE:

• A couple of .g files, containing function definitions.

I think such data files should just be installed next to the python files of the package.
Just like src/sage/interfaces/sage-maxima.lisp

[mkoeppe]

comment:23

It should definitely not depend on the environment variable SAGE_LOCAL at all.
Background: pip-installable packages should be installable into various virtual environments.

[mkoeppe]

comment:116

Hm......

(sage-sh) mkoeppe@egret:sage-rebasing$ sage-download-file https://github.com/sagemath/p_group_cohomology/releases/tag/v3.3.2/p_group_cohomology-3.3.2.tar.xz > A
[......................................................................]
(sage-sh) mkoeppe@egret:sage-rebasing$ file A
A: HTML document text, UTF-8 Unicode text, with very long lines
(sage-sh) mkoeppe@egret:sage-rebasing$ head A





<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
  <link rel="dns-prefetch" href="https://github.githubassets.com">

[mkoeppe]

Changed branch from u/SimonKing/fix_spkg_install_of_p_group_cohomology to u/mkoeppe/fix_spkg_install_of_p_group_cohomology

[mkoeppe]

Changed reviewer from https://github.com/mkoeppe/sage/actions/runs/313004407 to https://github.com/mkoeppe/sage/actions/runs/313251166

[mkoeppe]

Changed commit from ae50157 to bd34c47

[mkoeppe]

New commits:

bd34c47

build/pkgs/p_group_cohomology/checksums.ini: Fix upstream_url

[simon-king-jena]

comment:119

Replying to @jhpalmieri:

I think the problem with the documentation is this: if you install the documentation in spkg-install.in, then the package hasn't been installed yet — it's in a staging area — so it cannot be imported into Python, which is necessary for the docbuilding process. So move the docbuilding stuff to spkg-postinst.in:

Sure, that makes sense! Thank you for spotting it!

I'll try, and if it works, I will also remove the comment from the doc that states that docbuilding during package installation is broken.

[simon-king-jena]

Changed branch from u/mkoeppe/fix_spkg_install_of_p_group_cohomology to u/SimonKing/fix_spkg_install_of_p_group_cohomology

[simon-king-jena]

comment:121

Thank you very much, when using spkg-postinst, building the documentation works!

---

New commits:

245560d

Fix docbuild of p_group_cohomology

[simon-king-jena]

Changed commit from bd34c47 to 245560d

[simon-king-jena]

comment:122

PS: The new tarball (removing the remark on broken docbuild) is on github.

[dimpase]

comment:123

OK, but one doctest is failing

$ ./sage -tp src/sage/tests/modular_group_cohomology.py
Running doctests with ID 2020-10-19-09-47-51-87beac43.
Git branch: public/graphs/30386
Using --optional=bliss,build,debian,dochtml,gap_packages,libsemigroups,meataxe,memlimit,p_group_cohomology,pip,sage,sage_spkg
Doctesting 1 file using 4 threads.
sage -t --warn-long 81.6 --random-seed=0 src/sage/tests/modular_group_cohomology.py
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 20, in sage.tests.modular_group_cohomology
Failed example:
    H == H0                                       # optional - p_group_cohomology
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   1 of  18 in sage.tests.modular_group_cohomology
    [17 tests, 1 failure, 3.98 s]
----------------------------------------------------------------------
sage -t --warn-long 81.6 --random-seed=0 src/sage/tests/modular_group_cohomology.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 4.0 seconds
    cpu time: 3.4 seconds
    cumulative wall time: 4.0 seconds

note that also at Sage prompt computing H and H0 gives

sage: H0.gens()                                                                                                                                                      
[1,
 a_2_1: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_2_2: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_4_4: 4-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_0: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_1: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_3_3: 3-Cocycle in H^*(SmallGroup(64,14); GF(2))]
sage: H.gens()                                                                                                                                                       
[1,
 a_2_1: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_2_2: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_4_4: 4-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_0: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_1: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_3_3: 3-Cocycle in H^*(SmallGroup(64,14); GF(2))]
sage: H0.is_isomorphic(H)                                                                                                                                            
('1*a_2_1', '1*c_2_2', '1*c_4_4', '1*a_1_0', '1*a_1_1', '1*a_3_3')
sage: H.is_isomorphic(H0)                                                                                                                                            
('1*a_2_1', '1*c_2_2', '1*c_4_4', '1*a_1_0', '1*a_1_1', '1*a_3_3')
sage: H==H0                                                                                                                                                          
False

not sure how serious it is - if it's not, and you don't have time to fix it now, just mark it as a known error, and open another ticket.

[simon-king-jena]

comment:124

Replying to @dimpase:

OK, but one doctest is failing

Thank you for spotting this! Indeed I forgot that there are some doctests in the Sage src tree, and only tested the package's own test suite.

note that also at Sage prompt computing H and H0 gives

sage: H0.gens()                                                                                                                                                      
[1,
 a_2_1: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_2_2: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_4_4: 4-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_0: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_1: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_3_3: 3-Cocycle in H^*(SmallGroup(64,14); GF(2))]
sage: H.gens()                                                                                                                                                       
[1,
 a_2_1: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_2_2: 2-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 c_4_4: 4-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_0: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_1_1: 1-Cocycle in H^*(SmallGroup(64,14); GF(2)),
 a_3_3: 3-Cocycle in H^*(SmallGroup(64,14); GF(2))]
sage: H0.is_isomorphic(H)                                                                                                                                            
('1*a_2_1', '1*c_2_2', '1*c_4_4', '1*a_1_0', '1*a_1_1', '1*a_3_3')
sage: H.is_isomorphic(H0)                                                                                                                                            
('1*a_2_1', '1*c_2_2', '1*c_4_4', '1*a_1_0', '1*a_1_1', '1*a_3_3')
sage: H==H0                                                                                                                                                          
False

Hm. I'll have a closer look. Two cohomology rings should evaluate equal if the two ring representations are identical. This should be the case here, as there is an isomorphism mapping the generators one to one.

... unless there is a tail reduction of the relations in one case but not in the other case.

not sure how serious it is - if it's not, and you don't have time to fix it now, just mark it as a known error, and open another ticket.

At least I'll have a look now.

[simon-king-jena]

comment:125

Interesting:

sage: H.set_ring()                                                                                  
sage: H.relation_ideal()                                                                            
a_1_0^2,
a_1_0*a_1_1,
a_1_1^3,
a_2_1*a_1_0,
a_2_1^2+a_2_1*a_1_1^2,
a_1_0*a_3_3+a_2_1*a_1_1^2,
a_1_1*a_3_3+a_2_1*a_1_1^2,
a_2_1*a_3_3,
a_3_3^2
sage: H0.set_ring()                                                                                 
sage: H0.relation_ideal()                                                                           
a_1_0^2,
a_1_0*a_1_1,
a_1_1^3,
a_2_1*a_1_0,
a_2_1^2+a_2_1*a_1_1^2,
a_1_0*a_3_3+a_2_1*a_1_1^2,
a_1_1*a_3_3+a_2_1*a_1_1^2,
a_2_1*a_3_3,
a_3_3^2
sage: H.rels()                                                                                      
['a_1_0^2',
 'a_1_0*a_1_1',
 'a_1_1^3',
 'a_2_1*a_1_0',
 'a_2_1^2+a_2_1*a_1_1^2',
 'a_1_0*a_3_3+a_2_1*a_1_1^2',
 'a_1_1*a_3_3+a_2_1*a_1_1^2',
 'a_2_1*a_3_3',
 'a_3_3^2']
sage: H0.rels()                                                                                     
['a_1_0^2',
 'a_1_0*a_1_1',
 'a_1_1^3',
 'a_2_1*a_1_0',
 'a_2_1^2+a_2_1*a_1_1^2',
 'a_1_1*a_3_3+a_2_1^2',
 'a_1_0*a_3_3+a_2_1^2',
 'a_2_1*a_3_3',
 'a_3_3^2']

So, Singular says that the two ideals are identical, but the string representation stored in the cohomology ring is different.

[dimpase]

comment:126

The relations of H and H0 are equal as sets, but not as lists:

sage: H.rels()                                                                                                                                                       
['a_1_0^2',
 'a_1_0*a_1_1',
 'a_1_1^3',
 'a_2_1*a_1_0',
 'a_2_1^2+a_2_1*a_1_1^2',
 'a_1_0*a_3_3+a_2_1*a_1_1^2',
 'a_1_1*a_3_3+a_2_1*a_1_1^2',
 'a_2_1*a_3_3',
 'a_3_3^2']
sage: H0.rels()                                                                                                                                                      
['a_1_0^2',
 'a_1_0*a_1_1',
 'a_1_1^3',
 'a_2_1*a_1_0',
 'a_2_1^2+a_2_1*a_1_1^2',
 'a_1_1*a_3_3+a_2_1^2',
 'a_1_0*a_3_3+a_2_1^2',
 'a_2_1*a_3_3',
 'a_3_3^2']

EDIT - oops, sorry, there are just a bit different - as you noticed already, I suppose.

[simon-king-jena]

comment:127

Indeed, the problem is the missing interreduction:

sage: H0.set_ring()                                                                                 
sage: I = singular.ideal(H0.rels())                                                                 
sage: str(I) == str(H0.relation_ideal())                                                            
False
sage: str(I.interred()) == str(H0.relation_ideal())                                                 
True

Hm. What to do about it?

[simon-king-jena]

comment:128

Replying to @dimpase:

The relations of H and H0 are equal as sets, but not as lists:

No, they aren't. They are equal after doing interreduction.

sage: set(H0.rels()) == set(H.rels())                                                               
False

[simon-king-jena]

comment:129

Back to the question what to do about it.

The point of the equality check is to provide a VERY QUICK test. So, I wouldn't like to do interreduction before comparing.

But one could fix the doctest by checking that the two rings are indeed isomorphic by mapping the list of generators one-to-one.

[dimpase]

comment:130

I would not call what we see here a "pythonic" equality. What we see here is more like
1/2==2/4 - the jury is still out on whether this should be True or False.

On the other hand what we have here is a stronger kind of equvalence than just a (graded) ring isomorphism, no?

[sagetrac-git]

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

f4e5f6a

Fix a doctest in sage/tests/modular_cohomology.py

[sagetrac-git]

Changed commit from 245560d to f4e5f6a

[simon-king-jena]

comment:132

Replying to @dimpase:

I would not call what we see here a "pythonic" equality. What we see here is more like
1/2==2/4 - the jury is still out on whether this should be True or False.

I chose "equality of cohomology rings" to mean "equality of ring representations".

So, with this choice, one would have 1/2==2/4 if and only if 1/2 is represented in the same way (namely as a REDUCED fraction) as 2/4.

Note that to check whether two ideals represent the same set of polynomials, one would need to compute Gröbner bases. But that's very expensive. Therefore, it was decided by Sage (at least in the past, I don't know if that has changed) to not do the expensive test, but to compare generators instead.

And that's what I do here, too.

On the other hand what we have here is a stronger kind of equvalence than just a (graded) ring isomorphism, no?

Sure. I wrote a research paper (with Bettina Eick) on classifying the cohomology rings of groups of order <=81 up to graded isomorphism. And that's a totally different problem.

Here, equality is not "isomorphism of graded rings", but "identity of ring representations".

[dimpase]

comment:133

OK, good.
One can still try to find more errors here: https://github.com/mkoeppe/sage/actions/runs/313251166
but I guess it's OK.

[dimpase]

Changed reviewer from https://github.com/mkoeppe/sage/actions/runs/313251166 to Dima Pasechnik

[mkoeppe]

comment:134

Looking good. On to #30787...?

[mkoeppe]

Changed reviewer from Dima Pasechnik to Dima Pasechnik, Matthias Koeppe

[vbraun]

Changed branch from u/SimonKing/fix_spkg_install_of_p_group_cohomology to f4e5f6a