GAP pexpect interface hangs when xgap is loaded

[timokau]

The pexpect interface hangs when xgap is loaded. That is known, which is why the loading of xgap should be prevented by adding it to ExcludeFromAutoload. Unfortunately that doesn't work in all cases, presumably when xgap is dependency of another package.

Instead PackagesToIgnore should be used, which just pretends the package is not there at all.

CC: @embray @kiwifb

Component: interfaces

Keywords: gap

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

[timokau]

comment:1

Even if I install gap without xgap, I get apparently related failures. For example PermutationGroup(['(1,2,3)(4,5)', '(3,4)']).order() will give the "this should never happen" warning that is thrown when @w GAP is trying to send a Window command and the interface hangs again.

Why don't we pass -A to sage and then load the necessary packages explicitly? I see that it was done in 5e61d7b but apparently didn't end up in the final version.

[embray]

comment:2

Replying to @timokau:

Even if I install gap without xgap, I get apparently related failures. For example PermutationGroup(['(1,2,3)(4,5)', '(3,4)']).order() will give the "this should never happen" warning that is thrown when @w GAP is trying to send a Window command and the interface hangs again.

The "this should never happen" warning is somewhat accurate: It really shouldn't happen. Please make sure to clear any old GAP workspaces. Also, it could be you're using some outdated, broken version of some package that is evoking xgap-related functionality when it shouldn't.

It would help if you could give precisely a case where this doesn't work; i.e. narrow it down to a specific package. Because this problem does not occur for the GAP packages that are packaged as Sage SPKGs for GAP 4.10.

Why don't we pass -A to sage and then load the necessary packages explicitly? I see that it was done in 5e61d7b but apparently didn't end up in the final version.

That was discussed and decided against, since not loading any packages by default results in an ostensibly "broken" GAP (IMO this does not reflect well on GAP but from the Sage end of things it's better to give the best GAP experience we can so that users don't submit bogus bug reports to the GAP developers when really it's just a question of what settings the GAP installation in Sage uses).

[timokau]

comment:4

Replying to @embray:

Replying to @timokau:

Even if I install gap without xgap, I get apparently related failures. For example PermutationGroup(['(1,2,3)(4,5)', '(3,4)']).order() will give the "this should never happen" warning that is thrown when @w GAP is trying to send a Window command and the interface hangs again.

The "this should never happen" warning is somewhat accurate: It really shouldn't happen. Please make sure to clear any old GAP workspaces. Also, it could be you're using some outdated, broken version of some package that is evoking xgap-related functionality when it shouldn't.

Builds and tests happen in a sandbox with a clean $HOME and freshly build gap+pkgs, so that shouldn't be an issue.

It would help if you could give precisely a case where this doesn't work; i.e. narrow it down to a specific package. Because this problem does not occur for the GAP packages that are packaged as Sage SPKGs for GAP 4.10.

I can confirm that this doesn't happen if I only install the packages the spkg installs. I'll try to narrow it down to one offending package.

Why don't we pass -A to sage and then load the necessary packages explicitly? I see that it was done in 5e61d7b but apparently didn't end up in the final version.

That was discussed and decided against, since not loading any packages by default results in an ostensibly "broken" GAP (IMO this does not reflect well on GAP but from the Sage end of things it's better to give the best GAP experience we can so that users don't submit bogus bug reports to the GAP developers when really it's just a question of what settings the GAP installation in Sage uses).

But if the mere presence of packages that are not in the spkg changes gap behaviour, it seems like a nightmare to maintain without explicitly setting the set of loaded packages.

[timokau]

comment:5

I can't reproduce the PermutationGroup issue right now. Not sure why. I was probably testing some sketchy hack at the time to get things working.

However I get plenty of other issues with gap packages:

• utils-0.59 causes failures of the type

sage -t --long /nix/store/gh48l47qc1irys2xpbwf56g2lwa96pcs-sage-src-8.6.beta0/src/sage/libs/gap/element.pyx
**********************************************************************
File "/nix/store/gh48l47qc1irys2xpbwf56g2lwa96pcs-sage-src-8.6.beta0/src/sage/libs/gap/element.pyx", line 898, in sage.libs.gap.element.GapElement._sub_
Failed example:
    libgap(1) - libgap.CyclicGroup(2)
Expected:
    Traceback (most recent call last):
    ...
    ValueError: libGAP: Error, no method found!
    Error, no 1st choice method found for `-' on 2 arguments
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/nix/store/0jfdqivm0m2d6rap3lk9bbdyxy7fnyz5-python-2.7.15-env/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/nix/store/0jfdqivm0m2d6rap3lk9bbdyxy7fnyz5-python-2.7.15-env/lib/python2.7/site-packages/sag
e/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.libs.gap.element.GapElement._sub_[4]>", line 1, in <module>
        libgap(Integer(1)) - libgap.CyclicGroup(Integer(2))
      File "sage/structure/element.pyx", line 1359, in sage.structure.element.Element.__sub__ (build/cythonized/sage/structure/element.c:11384)
        return (<Element>left)._sub_(right)
      File "sage/libs/gap/element.pyx", line 911, in sage.libs.gap.element.GapElement._sub_ (build/cythonized/sage/libs/gap/element.c:9584)
        raise ValueError('libGAP: {}'.format(msg))
    ValueError: libGAP: Error, no method found! Error, no 1st choice method found for `AdditiveInverseMutable' on 1 arguments

• "io-4.5.4" causes failures of the type

src/doc/en/constructions/elliptic_curves.rst
Failed example:
    G.permutation_group()
Expected:
    Permutation Group with generators [(1,2,3,4,5)]
Got:
    #W dlopen() error: /nix/store/6z48grms4hjgyg5ia99lcvyqb27gikwh-gap-4.10.0/shar\
    e/gap/build-dir/pkg/io-4.5.4/bin/x86_64-pc-linux-gnu-default64/io.so: undefine\
    d symbol: ChangedBags
    Error, module '/nix/store/6z48grms4hjgyg5ia99lcvyqb27gikwh-gap-4.10.0/share/gap/build\
    -dir/pkg/io-4.5.4/bin/x86_64-pc-linux-gnu-default64/io.so' not found
    Error, was not in any namespace
    Syntax warning: Unbound global variable in /nix/store/6z48grms4hjgyg5ia99lcvyq\
    b27gikwh-gap-4.10.0/share/gap/build-dir/lib/init.g:803
        ColorPrompt( UserPreference( "UseColorPrompt" ) );
                   ^
    Error, SetGasmanMessageStatus: function is not yet defined
    Permutation Group with generators [(1,2,3,4,5)]

• "format-1.4a", "Browse" and others (haven't tested all because that is time
  consuming) cause a timeout when doing batch imports in
  src/sage/libs/gap/assigned_names.py and
  src/sage/libs/gap/all_documented_functions.py

So in summary, the pexpect interface only works reliably when just the package
set the spkg expects is installed. I'm not sure how gap package autoloading
works, but apparently some packages are autoloaded when they are available.

[embray]

comment:6

Replying to @timokau:

Replying to @embray:

Replying to @timokau:

Even if I install gap without xgap, I get apparently related failures. For example PermutationGroup(['(1,2,3)(4,5)', '(3,4)']).order() will give the "this should never happen" warning that is thrown when @w GAP is trying to send a Window command and the interface hangs again.

The "this should never happen" warning is somewhat accurate: It really shouldn't happen. Please make sure to clear any old GAP workspaces. Also, it could be you're using some outdated, broken version of some package that is evoking xgap-related functionality when it shouldn't.

Builds and tests happen in a sandbox with a clean $HOME and freshly build gap+pkgs, so that shouldn't be an issue.

One of the packages could still be buggy though, or somehow forcibly loading xgap. But I'm not sure.

That was discussed and decided against, since not loading any packages by default results in an ostensibly "broken" GAP (IMO this does not reflect well on GAP but from the Sage end of things it's better to give the best GAP experience we can so that users don't submit bogus bug reports to the GAP developers when really it's just a question of what settings the GAP installation in Sage uses).

But if the mere presence of packages that are not in the spkg changes gap behaviour, it seems like a nightmare to maintain without explicitly setting the set of loaded packages.

I agree of course, which is why I went with -A originally. However, there is a default set of packages that are autoloaded, and so the gap SPKG includes all of those packages and their dependencies--no more, no less. So that still gives a reasonably stable baseline as the "standard" out-of-the-box GAP experience.

Notably, xgap is not one of those packages, nor is it a package we're including in the gap_packages SPKGs. I had previously added xgap manually and tested with it installed and didn't get it to load.

[embray]

comment:7

Does it matter, from your perspective, if some sage doctests fail depending on which GAP packages are installed?

The issue with IO is known, for example, and has a fix that just hasn't been included yet since there is no SPKG that installs the IO package. See #22626 comment:424 and #26930

[timokau]

comment:9

Replying to @embray:

That was discussed and decided against, since not loading any packages by default results in an ostensibly "broken" GAP (IMO this does not reflect well on GAP but from the Sage end of things it's better to give the best GAP experience we can so that users don't submit bogus bug reports to the GAP developers when really it's just a question of what settings the GAP installation in Sage uses).

But if the mere presence of packages that are not in the spkg changes gap behaviour, it seems like a nightmare to maintain without explicitly setting the set of loaded packages.

I agree of course, which is why I went with -A originally. However, there is a default set of packages that are autoloaded, and so the gap SPKG includes all of those packages and their dependencies--no more, no less. So that still gives a reasonably stable baseline as the "standard" out-of-the-box GAP experience.

But why do packages like utils and io still cause breakage then, if they are not part of the default set of packages that are autoloaded?

Notably, xgap is not one of those packages, nor is it a package we're including in the gap_packages SPKGs. I had previously added xgap manually and tested with it installed and didn't get it to load.

You mean that you cannot reproduce the issue with having xgap installed?

Replying to @embray:

Does it matter, from your perspective, if some sage doctests fail depending on which GAP packages are installed?

Yes it matters. I'd like to maintain 0 doctest failures, so even "comsetic" failures matter. I think a testsuite looses a lot of its value if you can't rely on test breakage actually signifying real issues. Also a lot of the failures are not cosmetic but real issues.

With Nix I could install a separate gap package just for sage that has the expected package set. That is what I've been doing before the update, but using the regular gap package would be much nicer. Also most other package managers don't have that luxury.

If we are only officially supporting a subset of the available packages, I think we should only ever load that subset (even if more are available).

The issue with IO is known, for example, and has a fix that just hasn't been included yet since there is no SPKG that installs the IO package. See #22626 comment:424 and #26930

I just want to clarify that I'm very grateful that you are working on this. I know making big changes like this can be very thankless work because it inevitably breaks someones workflow and you usually only hear complaints afterwards. Thank you, the update was very necessary.

[embray]

comment:10

Replying to @timokau:

Replying to @embray:

That was discussed and decided against, since not loading any packages by default results in an ostensibly "broken" GAP (IMO this does not reflect well on GAP but from the Sage end of things it's better to give the best GAP experience we can so that users don't submit bogus bug reports to the GAP developers when really it's just a question of what settings the GAP installation in Sage uses).

But if the mere presence of packages that are not in the spkg changes gap behaviour, it seems like a nightmare to maintain without explicitly setting the set of loaded packages.

I agree of course, which is why I went with -A originally. However, there is a default set of packages that are autoloaded, and so the gap SPKG includes all of those packages and their dependencies--no more, no less. So that still gives a reasonably stable baseline as the "standard" out-of-the-box GAP experience.

But why do packages like utils and io still cause breakage then, if they are not part of the default set of packages that are autoloaded?

Well, some packages have "optional dependencies" that are nice to have (I think these are usually under Dependencies.SuggestedOtherPackages in PackageInfo.g) which are not required, but that the package may load if it happens to be present (same as with xgap). So in this sense I see your problem: Having those packages installed at all causes things to break even if they aren't explicitly used.

Notably, xgap is not one of those packages, nor is it a package we're including in the gap_packages SPKGs. I had previously added xgap manually and tested with it installed and didn't get it to load.

You mean that you cannot reproduce the issue with having xgap installed?

I cannot--last time I worked on this I had some 140 packages installed and the fix I added was good enough. I could be wrong though. I haven't tried it since I (supposedly) fixed it. It remains fixed for all the GAP packages that are installed by Sage SPKGs but that might not be good enough after all.

Replying to @embray:

Does it matter, from your perspective, if some sage doctests fail depending on which GAP packages are installed?

Yes it matters. I'd like to maintain 0 doctest failures, so even "comsetic" failures matter. I think a testsuite looses a lot of its value if you can't rely on test breakage actually signifying real issues. Also a lot of the failures are not cosmetic but real issues.

I agree that having 0 doctest failures matters. I'm just asking if it matters if all tests pass on all conceivable combinations of installed packages on the system?

With Nix I could install a separate gap package just for sage that has the expected package set. That is what I've been doing before the update, but using the regular gap package would be much nicer. Also most other package managers don't have that luxury.

Agreed.

If we are only officially supporting a subset of the available packages, I think we should only ever load that subset (even if more are available).

See above though: That requires somehow forcing other packages not to load optional dependencies. I'm not sure there's a way to prevent that across the board (other than maybe monkey-patching some things, which is worth a shot...)

The issue with IO is known, for example, and has a fix that just hasn't been included yet since there is no SPKG that installs the IO package. See #22626 comment:424 and #26930

I just want to clarify that I'm very grateful that you are working on this. I know making big changes like this can be very thankless work because it inevitably breaks someones workflow and you usually only hear complaints afterwards. Thank you, the update was very necessary.

Thanks!

[timokau]

comment:11

Replying to @embray:

Notably, xgap is not one of those packages, nor is it a package we're including in the gap_packages SPKGs. I had previously added xgap manually and tested with it installed and didn't get it to load.

You mean that you cannot reproduce the issue with having xgap installed?

I cannot--last time I worked on this I had some 140 packages installed and the fix I added was good enough. I could be wrong though. I haven't tried it since I (supposedly) fixed it. It remains fixed for all the GAP packages that are installed by Sage SPKGs but that might not be good enough after all.

In my case all packages that ship with the standard gap tarball are installed.

Replying to @embray:

Does it matter, from your perspective, if some sage doctests fail depending on which GAP packages are installed?

Yes it matters. I'd like to maintain 0 doctest failures, so even "comsetic" failures matter. I think a testsuite looses a lot of its value if you can't rely on test breakage actually signifying real issues. Also a lot of the failures are not cosmetic but real issues.

I agree that having 0 doctest failures matters. I'm just asking if it matters if all tests pass on all conceivable combinations of installed packages on the system?

I'd say yes. If we don't want to support all conceivable combinations we should explicitly restrict that.

With Nix I could install a separate gap package just for sage that has the expected package set. That is what I've been doing before the update, but using the regular gap package would be much nicer. Also most other package managers don't have that luxury.

Agreed.

I'll probably go that way for 8.6, with it already being in rc phase. Are you still planning to completely replace the pexpect interface with libgap? In that case the problem may solve itself to some degree, as I would expect libgap to be much more resilient to the installed packges.

If we are only officially supporting a subset of the available packages, I think we should only ever load that subset (even if more are available).

See above though: That requires somehow forcing other packages not to load optional dependencies. I'm not sure there's a way to prevent that across the board (other than maybe monkey-patching some things, which is worth a shot...)

Yeah either monkey patching SuggestedOtherPackages or maybe using PackagesToIgnore to produce a whitelist. I know nothing about gap, so I don't know if that would be possible.

[embray]

comment:12

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

[embray]

comment:13

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

[embray]

comment:14

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

[embray]

comment:15

Ticket retargeted after milestone closed

[mkoeppe]

comment:16

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

[mkoeppe]

comment:18

Moving this ticket to 9.4, as it seems unlikely that it will be merged in 9.3, which is in the release candidate stage

[mkoeppe]

comment:19

Setting a new milestone for this ticket based on a cursory review.

[mkoeppe]

comment:20

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.