Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the system GAP #36792

Merged
merged 64 commits into from
Dec 19, 2023
Merged

Use the system GAP #36792

merged 64 commits into from
Dec 19, 2023

Conversation

orlitzky
Copy link
Contributor

Resurrect #29644 and add an spkg-configure.m4 for GAP.

Some other changes were made to make this possible / nicer:

  1. GAP_LIB_DIR and GAP_SHARE_DIR are merged into GAP_ROOT_PATHS. This was suggested by @tornaria and is the right approach, especially now that GAP_SO has been removed. Nothing else needs those two directories and GAP itself doesn't care about them -- it only cares about the package search path, i.e. GAP_ROOT_PATHS. So changing the variable(s) brings us in line with the way GAP works.
  2. We no longer pass the -r flag to GAP. Particularly when using the system GAP, we do want the user to be able to install his own packages and run his own initialization.
  3. We begin to pass -A to GAP. This tells GAP not to autoload the big set of "recommended" packages at start-up, which avoids the inevitable error messages when those packages are not installed on the system GAP. We could check for all of them in spkg-configure.m4, but it's a long list, and loading them slows down GAP initialization for no benefit -- Sage itself uses only one such package. Users can autoload them via gaprc or gap.ini in light of (2).
  4. After adding -A to the initialization, we try to load the PolyCyclic package if it's installed. This is the one "recommended" package that Sage itself uses.
  5. The "recommended" packages are all moved from the gap SPKG to gap_packages because we no longer need them, except (maybe) for PolyCyclic. Should keep polycyclic installed by default? The tests all pass without it, but it is used a few places in sagelib.
  6. Various stale doctest fixes.
  7. The expected output from a few tests has changed. Where possible, I've made the test more robust. In one case I had to drop the printing of a matrix, because if you dig into the source code for GAP's NormalSubgroups(), it chooses a Representative() inconsistently, and that eventually affects the entries of the matrix.

All tests are passing afterwards... except one, a heisenbug:

sage -t --warn-long 187.7 --random-seed=96688270013898650573232209016248663009 src/sage/groups/matrix_gps/finitely_generated_gap.py
**********************************************************************
File "src/sage/groups/matrix_gps/finitely_generated_gap.py", line 123, in sage.groups.matrix_gps.finitely_generated_gap.FinitelyGeneratedMatrixGroup_gap.as_permutation_group
Failed example:
    P == Psmaller
Expected:
    False
Got:
    True
**********************************************************************

If anyone knows what's going on there, I'd be grateful. The GAP docs for SmallerDegreePermutationRepresentation() say,

The methods used might involve the use of random elements and the permutation representation (or even the degree of the representation) is not guaranteed to be the same for different calls of SmallerDegreePermutationRepresentation.

So maybe this isn't really a bug, but I would find it strange if that randomness could mean that sometimes the smaller degree option just wouldn't work at all.

Anyway, have at it, and let me know what you think.

@dimpase
Copy link
Member

dimpase commented Nov 30, 2023

is there GAP on a Gentoo overlay?

@dimpase
Copy link
Member

dimpase commented Nov 30, 2023

these "recommended" packages better be installed on the system. Otherwise it's hard to predict exact results (they amend behaviour of some GAP routines)

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot say much about the gap interface, so just two general suggestions.

Also the (conda) tests are failing (there are more, but most of them are false positives as #36372 was not applied):

File "src/doc/en/reference/spkg/gap_packages.rst", line 18, in doc.en.reference.spkg.gap_packages
Failed example:
    gap.eval('LoadPackage("Grape")')
Expected nothing
Got:
    'fail'
**********************************************************************
File "src/doc/en/reference/spkg/gap_packages.rst", line 19, in doc.en.reference.spkg.gap_packages
Failed example:
    libgap.LoadPackage("Grape")
Expected nothing
Got:
    #I  grape package is not available. Check that the name is correct
    #I  and it is present in one of the GAP root directories (see '??RootPaths')
    fail

src/sage/env.py Outdated
# in the usual $PATH) over the system ones. In any case, the only
# thing to watch out for here is the possibility of the empty string
# after calling split(";").
GAP_ROOT_PATHS = ";".join([join(SAGE_LOCAL, "lib", "gap"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to store this as an array of strings (without empty ones) and not as a string with semicolons. Makes some of the logic below more easy to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll think about this along with @tornaria's suggestion about overriding GAP_ROOT_PATHS. GAP itself uses a semicolon-separated string, and the semicolons have meaning beyond just separating the paths. At the beginning of the string they mean "append these paths" and at the end of the string a semicolon means "prepend these paths." I think it would be more awkward to encode that magic behavior as an empty string in a list than the current level of awkwardness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the GAP_ROOT_PATHS mangling from sage.env into sage-conf and I still don't think using a list here would help, for two main reasons:

  1. The semicolon appending/prepending issue I already mentioned. How to we encode a leading or trailing semicolon in a python list of strings?
  2. Both the source (./configure, and hopefully soon, meson) and the sink (gap -l) for GAP_ROOT_PATHS use a semicolon-separated string. Neither can use a python list, so we would have to parse the string we get from the build system into a python list, and then immediately convert back to a semicolon string in sage.env. A list would be a tiny bit nicer for a human to type into conf.py by hand, but I think we all want to stop doing that ASAP.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal was to have something like GAP_ROOT_PATHS = var(...).split(';') in sage.env, but since you now need it as often as a colon-separated list and as a list of strings (and given the appending/prepending issue), I no longer think this makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better (less confusing) that sage.env.GAP_ROOT_PATHS matches the environment variable GAP_ROOT_PATHS, which is by definition a string.

src/sage/env.py Outdated
# Passing -A allows us to use a minimal GAP installation without
# producing errors at start-up. The files sage.g and sage.gaprc can be
# used to load any additional packages that may be available.
_gap_cmd = f'gap -A -l "{GAP_ROOT_PATHS}"'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this logic of determining the fallback of the gap command to interfaces/gap_workspace.py? It feels like that there is more context why this default command is the correct choice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is here so it can be overriden by environment SAGE_GAP_COMMAND

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the whole chunk is,

_gap_cmd  = f'gap -A -l "{GAP_ROOT_PATHS}"'
if SAGE_GAP_MEMORY is not None:
    _gap_cmd += " -s " + SAGE_GAP_MEMORY + " -o " + SAGE_GAP_MEMORY
SAGE_GAP_COMMAND = var('SAGE_GAP_COMMAND', _gap_cmd)

So _gap_cmd is being used as the default for SAGE_GAP_COMMAND. (And thus needs to be set before that call to var).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's reasonable to have here just:

SAGE_GAP_COMMAND = var("SAGE_GAP_COMMAND")

and move the logic for the "default" gap command to interfaces/gap_workspace.py as something like:

gap_cmd = SAGE_GAP_COMMAND
if gap_cmd is None:
    gap_cmd = f'gap -A -l "{GAP_ROOT_PATHS}"'
    if SAGE_GAP_MEMORY is not None:
        gap_cmd += " -s " + SAGE_GAP_MEMORY + " -o " + SAGE_GAP_MEMORY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks @tornaria, that's what I had in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done. Although I wound up moving it to sage.interfaces.gap (and not sage.interface.gap_workspace) because that's where most of the existing gap command processing takes place.

src/sage/env.py Outdated
Comment on lines 221 to 223
GAP_ROOT_PATHS = ";".join([join(SAGE_LOCAL, "lib", "gap"),
join(SAGE_SHARE, "gap"),
var("GAP_ROOT_PATHS", "")])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make it impossible to really override the value of GAP_ROOT_PATHS. If either SAGE_LOCAL/lib/gap or SAGE_SHARE/gap is undesired for some reason, they are nevertheless forced.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we do SAGE_LOCAL/share/gap instead of SAGE_SHARE/gap? For a regular installation of sage these two are the same, but for a system wide gap they are not.

For instance, I define SAGE_SHARE to be /usr/share/sagemath/ and I place sagemath specific data inside (e.g. all databases that are only used from sage). And for a system sagemath we possibly have SAGE_LOCAL=/usr (taken from SAGE_VENV which itself defaults to sys.prefix)

(ideally, SAGE_SHARE can be phased out as in conway_polynomials, etc.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have a similar setup for SAGE_SHARE, I am hoping it is on its way out in the near future but yes, I do a similar distinction and patching of some paths for it. So, I second the request of @tornaria

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can we do SAGE_LOCAL/share/gap instead of SAGE_SHARE/gap? For a regular installation of sage these two are the same, but for a system wide gap they are not.

Done. Our sdh_configure() doesn't specify --datadir anyway so it's literally using $SAGE_LOCAL/share.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to make it impossible to really override the value of GAP_ROOT_PATHS. If either SAGE_LOCAL/lib/gap or SAGE_SHARE/gap is undesired for some reason, they are nevertheless forced.

Done. I moved the hackiness out of sage.env and into sage-conf.

@dimpase
Copy link
Member

dimpase commented Nov 30, 2023

regrading SmallerDegree... failing test, it's really an heuristic. (Finding the smallest one would mean finding a subgroup of minimal index, a hard task)

It might be affected by the set of installed GAP packages too.

@dimpase
Copy link
Member

dimpase commented Nov 30, 2023

not loading recommend GAP packages is akin to using Python without ssl module. It sort of works, but YMMV.

@orlitzky
Copy link
Contributor Author

is there GAP on a Gentoo overlay?

Yes, thanks to Francois: https://github.com/cschwan/sage-on-gentoo

@orlitzky
Copy link
Contributor Author

regrading SmallerDegree... failing test, it's really an heuristic. (Finding the smallest one would mean finding a subgroup of minimal index, a hard task)

It might be affected by the set of installed GAP packages too.

It works in a CLI session but not while doctesting with the same set of packages. Big WTF. I'd ultimately be OK with declaring it not an error, it just strikes me as particularly suspicious.

@orlitzky
Copy link
Contributor Author

not loading recommend GAP packages is akin to using Python without ssl module. It sort of works, but YMMV.

Everything I know about GAP I learned in the past week, so I'll listen to the experts on this. FWIW my motivation was:

  1. Reduce the start-up time. There are 18 fewer packages to load whenever libgap is used for the first time.
  2. Support minimal installations. On Gentoo, those 18 packages are dependencies that would otherwise be unnecessary to run sage or for its test suite to pass.
  3. Make the ./configure (and eventually, meson) check easier. It's a lot easier to check for 3 packages than 21.

@orlitzky
Copy link
Contributor Author

File "src/doc/en/reference/spkg/gap_packages.rst", line 18, in doc.en.reference.spkg.gap_packages
Failed example:
    gap.eval('LoadPackage("Grape")')
Expected nothing
Got:
    'fail'
**********************************************************************
File "src/doc/en/reference/spkg/gap_packages.rst", line 19, in doc.en.reference.spkg.gap_packages
Failed example:
    libgap.LoadPackage("Grape")
Expected nothing
Got:
    #I  grape package is not available. Check that the name is correct
    #I  and it is present in one of the GAP root directories (see '??RootPaths')
    fail

Fixed this one. I forgot SPKG.rst was doctested.

@dimpase
Copy link
Member

dimpase commented Dec 1, 2023

not loading recommend GAP packages is akin to using Python without ssl module. It sort of works, but YMMV.

Everything I know about GAP I learned in the past week, so I'll listen to the experts on this. FWIW my motivation was:

1. Reduce the start-up time. There are 18 fewer packages to load whenever libgap is used for the first time.

only for the 1st time. Then it's in the workspace which we load.

2. Support minimal installations. On Gentoo, those 18 packages are dependencies that would otherwise be unnecessary to run sage or for its test suite to pass.

GAP does not test such super-minimal GAP configurations, nor should we.

3. Make the `./configure` (and eventually, meson) check easier. It's a lot easier to check for 3 packages than 21.

write a loop which runs 21 times isn't harder than a loop which runs 3 times :-)

Please don't let the packaging convenience override the intentions of GAP releases. They have these extra packages out of necessity, for historical reasons - people preferred adding packages to contributing to the core GAP, as GAP was telling the users that publishing GAP packages is like publishing papers - and contributors liked it an did that.
These "recommended" packages are an integral part of GAP proper.

dimpase
dimpase previously requested changes Dec 1, 2023
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please undo this package reshuffling.

build/pkgs/gap_packages/spkg-install.in Outdated Show resolved Hide resolved
src/sage/env.py Outdated Show resolved Hide resolved
@orlitzky
Copy link
Contributor Author

orlitzky commented Dec 2, 2023

All remaining conda CI failures are fixed in #36372

Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I leave setting it to "positive-review" to the gap experts.

@dimpase
Copy link
Member

dimpase commented Dec 3, 2023

we need to test for presence of system-wide library libgap, with headers (e.g., test for <gap/gap.h>), not only the executable gap.

@dimpase
Copy link
Member

dimpase commented Dec 3, 2023

Please also add distros/gentoo.txt (and perhaps for some other distros?) for gap and gap_packages, and libsemigroups). Figuring out what packages to install to cover all gap_packages isn't trivial. So far I only know about sci-libs/libsemigroups dev-gap/semigroups (the latter does install few other GAP packages)

With only libgap-dev, we are missing the "gap" executable.
We added this for the CI to try, and it didn't work.
Without this, when certain tests fail, we print a bunch of junk to the
screen (and to config.log).
The GAP subpackages listed at,

  https://packages.fedoraproject.org/pkgs/gap/

are not included with the superpackage install, so we have to list
the ones that we use explicitly.
…SPKG

If the user doesn't want to use either the sage package manager or his
system's package manager, then this should be available immediately.
@dimpase dimpase dismissed their stale review December 15, 2023 00:20

let's get this is, and look into gap_packages later

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's get this in. We'd sort gap_packages later

Copy link

Documentation preview for this PR (built with commit 404f23a; changes) is ready! 🎉

@orlitzky
Copy link
Contributor Author

Thanks. I've been working on the Gentoo GAP packages this week, trying to get them ready for the main tree.

@dimpase
Copy link
Member

dimpase commented Dec 16, 2023

@antonio-rojas - does this work on Arch?

@antonio-rojas
Copy link
Contributor

@antonio-rojas - does this work on Arch?

Works fine for the distro package (as in there are no regressions wrt the current beta). Does this address #31761 ? It looks like gap_reset_workspace still loads all installed packages.

@dimpase
Copy link
Member

dimpase commented Dec 17, 2023

No, #31761 has not been looked at.
This might require more work, but not in this PR.

@vbraun vbraun merged commit d6a2dcc into sagemath:develop Dec 19, 2023
15 of 26 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 19, 2023
@kiwifb
Copy link
Member

kiwifb commented Dec 19, 2023

I do get an error when building the documentation in sage-on-gentoo. But it may be that I have to remove some patch or settings I had in place before this ticket.

#I  gapdoc 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  GAP: needed package gapdoc cannot be loaded
Error, failed to load needed package `gapdoc' (version >= 1.2)
Syntax warning: Unbound global variable in /usr/share/gap/lib/init.g:740
    ColorPrompt( UserPreference( "UseColorPrompt" ) );
    ^^^^^^^^^^^
Error, SetGasmanMessageStatus: function is not yet defined
Error, Variable: 'L1_IMMUTABLE_ERROR' must have a value

and GAPDoc is installed at /usr/lib64/gap/pkg/gapdoc.

@kiwifb
Copy link
Member

kiwifb commented Dec 19, 2023

I need to update sage-conf in sage-on-gentoo, and then my problem should vanish.

@orlitzky
Copy link
Contributor Author

I need to update sage-conf in sage-on-gentoo, and then my problem should vanish.

I think all you have to do is replace GAP_LIB_DIR and GAP_SHARE_DIR with GAP_ROOT_PATHS (semicolon-separated, how gap itself expects it).

@kiwifb
Copy link
Member

kiwifb commented Dec 19, 2023

Yes, that was all it was.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 5, 2024
…stalling packages

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
As observed in
sagemath#36792 (comment), the
"CI Linux incremental" workflow, which is run when package scripts are
updated, is not helpful when testing new or modified `spkg-configure.m4`
scripts when the SPKG has been installed:

Although we uninstall packages with changed spkg-configure.m4 scripts
already, a re-run of configure is not (always?) triggered.

So here we invoke it explicitly, using a new makefile target `make
reconfigure`.
(This new target also simplifies instructions issued by the system
package advice facility.)

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36829
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 13, 2024
…stalling packages

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
As observed in
sagemath#36792 (comment), the
"CI Linux incremental" workflow, which is run when package scripts are
updated, is not helpful when testing new or modified `spkg-configure.m4`
scripts when the SPKG has been installed:

Although we uninstall packages with changed spkg-configure.m4 scripts
already, a re-run of configure is not (always?) triggered.

So here we invoke it explicitly, using a new makefile target `make
reconfigure`.
(This new target also simplifies instructions issued by the system
package advice facility.)

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36829
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee, Matthias Köppe
@orlitzky orlitzky deleted the system-gap branch January 24, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants