package modular_resolution: Split out from p_group_cohomology

[mkoeppe]

As discussed in #28711 comment:64:

We split the current p_group_cohomology spkg:

In this ticket, we create the package modular_resolution, depending on meataxe-the-C-library, that installs:

• a standalone C library libmodres.so in $SAGE_LOCAL/lib
• some executables in $SAGE_LOCAL/bin, linked against libmodres
• database of cohomology rings

This also fixes the build problems of p_group_cohomology.

Fixing the doctest failures of p_group_cohomology is outside of the scope of this ticket. That's #34333.

New upstream:

• https://github.com/sagemath/modular_resolution

Depends on #34221
Depends on #34291

Upstream: Fixed upstream, in a later stable release.

CC: @simon-king-jena @dimpase @jhpalmieri @tscrim @kwankyu

Component: packages: optional

Keywords: sd111

Author: Matthias Koeppe, Dima Pasechnik, John Palmieri

Branch/Commit: c7108e9

Reviewer: John Palmieri, Matthias Koeppe, Dima Pasechnik, Travis Scrimshaw

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

[mkoeppe]

Dependencies: #28711

[simon-king-jena]

comment:2

I'll start with summing up what the current p_group_cohomology spkg comprises and how the parts depend upon each other.

1. the C library libmodres, which also links against libmtx and thus depends on the meataxe spkg, but not necessarily on sage-meataxe.
2. some executables linked against libmodres
3. some module for Singular, that should be installed in SAGE_LOCAL/share/singular/LIB/ (that's where Singular expect modules)
4. some package for GAP, which by Fix spkg-install of p_group_cohomology #28711 should be installed in SAGE_LOCAL/share/gap/pkg/ (that's where GAP expect packages)
5. some database of cohomology rings, to be installed in SAGE_LOCAL/share/pGroupCohomology (could in principle be moved to somewhere else, but is data and thus shouldn't be in places like SAGE_SRC)
6. some pip installable modules linked against libmodres and libmtx and also depending on sage.matrix.matrix_gfpn_dense being built and uses some Cython headers from sage.structure. The pip installable modules could in principle be put into the Sage src tree, analogously to sage-meataxe. 2.-5. are run time dependencies.

[simon-king-jena]

comment:3

Obviously, 1. and 2. from the previous comment should be in the new modular_resolution spkg. But at what point should 3.-5. be installed? Currently, 5. is installed when making modular_resolution, whereas 3.-4. are installed by p_group_cohomology's spkg-install.

Would it be better to install ALL of 3.-5. by modular_resolution's spkg-install (not by the make file)? Or ALL of 3.-5. by p_group_cohomology's spkg-install? Or something else?

[dimpase]

comment:4

The python/cython modules should be left alone, do not bundle anything with it please.

Have you thought about upstreaming the Singular library?

[simon-king-jena]

comment:5

Replying to @dimpase:

The python/cython modules should be left alone, do not bundle anything with it please.

Have you thought about upstreaming the Singular library?

No. I'm not sure if I should. It is about the computation of Dickson invariants, though. Since Singular has some capabilities of computing non-modular and modular invariant rings of finite group actions, it would somehow make sense to include it in Singular. (EDIT: I think I'll ask the Singular developers).

Concerning the analogous question on the GAP package: In this case I think it makes no sense to make it an upstream package. It calls the executables of modular_resolution, writing initial data needed for p_group_cohomology into several files.

[dimpase]

comment:6

Replying to @simon-king-jena:

Concerning the analogous question on the GAP package: In this case I think it makes no sense to make it an upstream package. It calls the executables of modular_resolution, writing initial data needed for p_group_cohomology into several files.

An important consideration is the ability to have spkg-check - e.g. for instance you can bundle the GAP package with modular_resolution, so that you can easily write some tests (in GAP) for all of them.

[simon-king-jena]

comment:7

Replying to @dimpase:

An important consideration is the ability to have spkg-check - e.g. for instance you can bundle the GAP package with modular_resolution, so that you can easily write some tests (in GAP) for all of them.

Good point!

If one bundles modular_resolution together with the cohomology database (which is currently the case) and with the GAP package, one could add the following self test: Call a function from the GAP package that uses an executable from modular_resolution to create initial data for, say, the cohomology ring of the dihedral group, which is part of the database, i.e., one can verify that the newly created initial data coincides what is found in the data base.

[mkoeppe]

comment:8

Replying to @simon-king-jena:

Obviously, 1. and 2. from the previous comment should be in the new modular_resolution spkg. But at what point should 3.-5. be installed? Currently, 5. is installed when making modular_resolution, whereas 3.-4. are installed by p_group_cohomology's spkg-install.

Would it be better to install ALL of 3.-5. by modular_resolution's spkg-install (not by the make file)? Or ALL of 3.-5. by p_group_cohomology's spkg-install? Or something else?

parts 3.-4. are outside of the scope of this ticket.

part 5. Is it used by 1.-2.? Then it should be included here; otherwise, outside of the scope of this ticket.

[simon-king-jena]

comment:9

Replying to @mkoeppe:

parts 3.-4. are outside of the scope of this ticket.

You argued in #28711, comment 58, that "an spkg should either be a pip-installable Python package, or it should be a non-Python package (which can install anywhere in SAGE_LOCAL)."

If I understand correctly, it means that not only the C library libmodres but also the database as well as the helpers for GAP and Singular shouldn't be installed by p_group_cohomology (whose main part IS pip-installable). So, 3.-5. should be split out as well and thus ARE in the scope of this ticket.

part 5. Is it used by 1.-2.? Then it should be included here; otherwise, outside of the scope of this ticket.

Then tell me by what source the database should be provided, if it shouldn't be p_group_cohomology.

[mkoeppe]

comment:10

Replying to @simon-king-jena:

Replying to @mkoeppe:

parts 3.-4. are outside of the scope of this ticket.

You argued in #28711, comment 58, that "an spkg should either be a pip-installable Python package, or it should be a non-Python package (which can install anywhere in SAGE_LOCAL)."

If I understand correctly, it means that not only the C library libmodres but also the database as well as the helpers for GAP and Singular shouldn't be installed by p_group_cohomology (whose main part IS pip-installable).

Yes, but this ticket is titled "package modular_resolution: Split out from p_group_cohomology", so it is more narrowly focused than a ticket titled "Split p_group_cohomology into its irreducible components".

[simon-king-jena]

comment:11

Replying to @mkoeppe:

Yes, but this ticket is titled "package modular_resolution: Split out from p_group_cohomology", so it is more narrowly focused than a ticket titled "Split p_group_cohomology into its irreducible components".

I don't understand what you suggest.

Do you suggest to change the title and description of the ticket? Or do you suggest to keep the Singular and Gap helpers in p_group_cohomology and additionally to move the database from modular_cohomology (that's where the database currently is) to p_group_cohomology, even though it is basically a pip installable module? Or something else?

[mkoeppe]

comment:12

Replying to @simon-king-jena:

Or do you suggest to keep the Singular and Gap helpers in p_group_cohomology

Yes, that's my suggestion for this ticket - to keep the ticket simple. Follow-up tickets can take care of further restructuring.

additionally to move the database from modular_cohomology (that's where the database currently is) to p_group_cohomology

If the modular_cohomology Makefile currently installs this database, there's probably no need to change it.

[mkoeppe]

comment:13

If the database is part of the modular_resolution package, I would suggest to install an executable script (perhaps modres-config) that reports the install location.

[simon-king-jena]

comment:14

Replying to @mkoeppe:

If the database is part of the modular_resolution package, I would suggest to install an executable script (perhaps modres-config) that reports the install location.

How can this be done? I mean, I know where the database ends up. Makefile.am is

ACLOCAL_AMFLAGS = -I m4
SUBDIRS = src

dbdir           = $(datadir)/pGroupCohomology
dist_db_DATA    = group_cohomology_database.tar

install-data-hook:
	cd $(DESTDIR)$(dbdir) && tar xf $(dist_db_DATA) && rm $(dist_db_DATA)

uninstall-hook:
	rm -r $(DESTDIR)$(dbdir)

and so the database will be in $SAGE_LOCAL/share/pGroupCohomology. But I guess a script returning that path (dependent on the value of SAGE_LOCAL is not what you suggest.

Is there a standard script in the autotools ecosystem that would provide the correct value of $(dbdir) both during and after (staged) installation?

[mkoeppe]

comment:15

Replying to @simon-king-jena:

Replying to @mkoeppe:

If the database is part of the modular_resolution package, I would suggest to install an executable script (perhaps modres-config) that reports the install location.

How can this be done? I mean, I know where the database ends up. Makefile.am is

ACLOCAL_AMFLAGS = -I m4
SUBDIRS = src

dbdir           = $(datadir)/pGroupCohomology
dist_db_DATA    = group_cohomology_database.tar

install-data-hook:
	cd $(DESTDIR)$(dbdir) && tar xf $(dist_db_DATA) && rm $(dist_db_DATA)

uninstall-hook:
	rm -r $(DESTDIR)$(dbdir)

and so the database will be in $SAGE_LOCAL/share/pGroupCohomology. But I guess a script returning that path (dependent on the value of SAGE_LOCAL is not what you suggest.

Not depending on SAGE_LOCAL - the upstream package should make sense without Sage.
Rather, depending on the configured install prefix.

Like this: Create template file modres-config.in

#! /bin/sh
echo "@datadir@/pGroupCohomology"

and add to configure.ac:

AC_CONFIG_FILES([modres-config], [chmod +x modres-config])

Is there a standard script in the autotools ecosystem that would provide the correct

value of $(dbdir) both during and after (staged) installation?

No, one would not want to read from DESTDIR.

[simon-king-jena]

comment:16

Replying to @mkoeppe:

Like this: Create template file modres-config.in

#! /bin/sh
echo "@datadir@/pGroupCohomology"

and add to configure.ac:

AC_CONFIG_FILES([modres-config], [chmod +x modres-config])

Is there a standard script in the autotools ecosystem that would provide the correct

value of $(dbdir) both during and after (staged) installation?

No, one would not want to read from DESTDIR.

Do I understand correctly that the syntax @datadir@ in modres-config.in makes it so that the value $(datadir) is inserted during creation of the modres-config script, so that calling it later provides the final destination of the database? Well, I simply didn't know how to achieve this and is what I meant by a "standard script in the autotools ecosystem".

To be clear: Do you suggest that this script will be called by the Python/Cython modules to find out the location of the database?

[mkoeppe]

comment:17

Replying to @simon-king-jena:

Do I understand correctly that the syntax @datadir@ in modres-config.in makes it so that the value $(datadir) is inserted during creation of the modres-config script, so that calling it later provides the final destination of the database?

That's right. ./configure will create the file modres-config. There are, of course, many other ways to do it, like generating this simple script in an install-hook.

Well, I simply didn't know how to achieve this and is what I meant by a "standard script in the autotools ecosystem".

Ok, great; but note that it does not do anything about DESTDIR (and should not).

To be clear: Do you suggest that this script will be called by the Python/Cython modules to find out the location of the database?

Yes.

[simon-king-jena]

comment:18

Replying to @mkoeppe:

To be clear: Do you suggest that this script will be called by the Python/Cython modules to find out the location of the database?

Yes.

Is this ticket only about creating a new modular_resolution spkg, or also to modify p_group_cohomology to use the new split-off spkg? Or shall the latter be done on a new ticket?

While we are at it: Is there a recommended way (at least recommended in Sage) to read from the output of system calls in Python? os.popen, or subprocess.Popen or something else?

[mkoeppe]

comment:19

Replying to @simon-king-jena:

Is this ticket only about creating a new modular_resolution spkg, or also to modify p_group_cohomology to use the new split-off spkg? Or shall the latter be done on a new ticket?

Both

While we are at it: Is there a recommended way (at least recommended in Sage) to read from the output of system calls in Python? os.popen, or subprocess.Popen or something else?

I just learned the hard way how to write this portably for Python >= 3.6 (all our currently supported Python versions): Take a look at the most recent version (after #30758) of src/sage/features/__init.py__ (package_systems).

        from subprocess import run, CalledProcessError, PIPE

        try:
            proc = run('sage-guess-package-system', shell=True, stdout=PIPE, stderr=PIPE, universal_newlines=True, check=True)
            _cache_package_systems = [PackageSystem(proc.stdout.strip())]
        except CalledProcessError:
            pass

[dimpase]

comment:20

Why would one want to create a custom script modres-config instead of hooking this up on pkg-config ?
(I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Then the Python interface is already there (with Python module pkgconfig - see e.g. src/sage/env.py)

[simon-king-jena]

comment:21

Replying to @dimpase:

Why would one want to create a custom script modres-config instead of hooking this up on pkg-config ?

I have no preference.

(I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Please do!

[mkoeppe]

comment:22

We were discussing how to advertise the install location of the data files. Pkgconfig for the library would also be nice of course

[simon-king-jena]

comment:23

Replying to @simon-king-jena:

(I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Please do!

In particular, I have no idea how to make pkgconfig aware of an autotoolized library: pkgconfig.list_all() contains neither libmtx nor mtx nor meataxe nor modres nor libmodres nor modular_resolution, although meataxe and modular_resolution are installed.

[dimpase]

comment:24

Replying to @simon-king-jena:

Replying to @simon-king-jena:

(I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Please do!

In particular, I have no idea how to make pkgconfig aware of an autotoolized library: pkgconfig.list_all() contains neither libmtx nor mtx nor meataxe nor modres nor libmodres nor modular_resolution, although meataxe and modular_resolution are installed.

As an example, have a look at e.g. cddlib/cddlib#40
(It changes a bit more than just adds cddlib.pc (adds some GMP-related stuff), so skip these parts)
Namely,

1. Makefile.am needed

pkgconfigdir       = $(libdir)/pkgconfig
pkgconfig_DATA     = cddlib.pc

2. cddlib.pc.in (a template) needed to be created

3. cddlib.pc had to be added to the list in AC_CONFIG_FILES

After all this, ./configure will fill in cddlib.pc.in and create cddlib.pc, which is then installed by make install.
Then Python pkgconfig module will be aware of that .pc file, no problem.

By the way, you can have a custom variable in .pc file - in your case, say, the path to the database - and request it using
pkg-config --variable=... ... calls (and from Python pkgconfig in a similar way)

[simon-king-jena]

comment:25

Thank you, Dima!

I wonder: Should I add pkg-config to SharedMeatAxe as well? After all, meataxe uses arithmetic tables that are of course stored in some default folder, and currently its location is presented by an environment variable. This variable is needed by the meataxe executables (that are also part of the package), whereas libmtx uses a C variable for the same purpose.

I took this design from the original MeatAxe from which I forked off SharedMeatAxe, but I've never been happy with it.

On the other hand, I'm not sure if pkg-config would be the right tool in this case: The location of the tables is in fact not determined by the autotoolised build system, but it is determined at run time -- and moreover the arithmetic tables are first created by spkg-postinst, not by the makefile of meataxe.

[dimpase]

comment:26

Replying to @simon-king-jena:

Thank you, Dima!

I wonder: Should I add pkg-config to SharedMeatAxe as well? After all, meataxe uses arithmetic tables that are of course stored in some default folder, and currently its location is presented by an environment variable. This variable is needed by the meataxe executables (that are also part of the package), whereas libmtx uses a C variable for the same purpose.

It certainly simplifies things, to be able to use pkg-config to check the library. E.g. imagine meataxe library available as a Debian package, and then naturally one would want to use it rather than to build Sage's one...

As to location of the tables/databases, naturally, there is no standard. Although I imagine there are defaults set in libmtx and in meataxe executables.

I took this design from the original MeatAxe from which I forked off SharedMeatAxe, but I've never been happy with it.

On the other hand, I'm not sure if pkg-config would be the right tool in this case: The location of the tables is in fact not determined by the autotoolised build system, but it is determined at run time -- and moreover the arithmetic tables are first created by spkg-postinst, not by the makefile of meataxe.

How is this determination happening at runtime? Checking an environment variable? Or there is a default value?

[tscrim]

comment:142

I mean specifically the one in the Sage library noted in comment:64.

[mkoeppe]

comment:143

Outside the scope of the ticket to add such annotations.

[tscrim]

Changed reviewer from John Palmieri, Matthias Koeppe, Dima Pasechnik to John Palmieri, Matthias Koeppe, Dima Pasechnik, Travis Scrimshaw

[tscrim]

comment:144

I disagree. I have pushed it.

---

New commits:

3641be4

Marking failing test as "# known bug".

[tscrim]

Changed commit from 0de2c98 to 3641be4

[tscrim]

Changed branch from u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology to u/tscrim/modular_resolution_split-30787

[mkoeppe]

comment:145

Do you know that it's a bug?

[tscrim]

comment:146

No as I can't (quickly) find this in the literature (as I don't know this area very well). However, since it is in a tests file, I am operating under the assumption it has been verified through some other means. Furthermore, none of the other results have changed.

Simon, perhaps you can comment briefly on this?

[mkoeppe]

comment:147

Then please clarify the annotation from # known bug to # known bug - at least one of the results is wrong

[sagetrac-git]

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

c7108e9

Updating "# known bug" description.

[sagetrac-git]

Changed commit from 3641be4 to c7108e9

[tscrim]

comment:149

Done.

[mkoeppe]

comment:150

This is fine with me.

[tscrim]

comment:151

I am setting it back to a positive review then.

Simon (or John or someone else if you know), if you could comment on the correctness of the test output, that would be appreciated.

[simon-king-jena]

comment:152

Replying to @tscrim:

No as I can't (quickly) find this in the literature (as I don't know this area very well). However, since it is in a tests file, I am operating under the assumption it has been verified through some other means. Furthermore, none of the other results have changed.

Simon, perhaps you can comment briefly on this?

In the test, an invariant of the group is calculated. So, if there are two different results than either the result computed by the old package version or the result computed by the new package version is wrong (or both). Therefore certainly one of the two versions has a bug.

I could ask Graham Ellis (my co-author in a publication on "persistant group cohomology") if he can compute this example: I know that he has software developed independently of p_group_cohomology.

[tscrim]

comment:153

Replying to @simon-king-jena:

I could ask Graham Ellis (my co-author in a publication on "persistant group cohomology") if he can compute this example: I know that he has software developed independently of p_group_cohomology.

Thank you for your thoughts. Could you please ask him? Independent corroboration would be great.

[vbraun]

Changed branch from u/tscrim/modular_resolution_split-30787 to c7108e9