Opened 4 years ago
Last modified 4 months ago
#26983 needs_info defect
GAP pexpect interface hangs when xgap is loaded
Reported by: | gh-timokau | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.7 |
Component: | interfaces | Keywords: | gap |
Cc: | embray, fbissey | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
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.
Change History (21)
comment:1 follow-up: ↓ 2 Changed 4 years ago by
comment:2 in reply to: ↑ 1 ; follow-up: ↓ 4 Changed 4 years ago by
Replying to gh-timokau:
Even if I install gap without
xgap
, I get apparently related failures. For examplePermutationGroup(['(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 5e61d7b6a0da3aa53d8176fa1fb9353cc559b098 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).
comment:3 Changed 4 years ago by
- Status changed from new to needs_info
comment:4 in reply to: ↑ 2 ; follow-up: ↓ 6 Changed 4 years ago by
Replying to embray:
Replying to gh-timokau:
Even if I install gap without
xgap
, I get apparently related failures. For examplePermutationGroup(['(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 5e61d7b6a0da3aa53d8176fa1fb9353cc559b098 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.
comment:5 Changed 4 years ago by
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
andsrc/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.
comment:6 in reply to: ↑ 4 Changed 4 years ago by
Replying to gh-timokau:
Replying to embray:
Replying to gh-timokau:
Even if I install gap without
xgap
, I get apparently related failures. For examplePermutationGroup(['(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.
comment:7 follow-up: ↓ 9 Changed 4 years ago by
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 comment:424:ticket:22626 and #26930
comment:8 Changed 4 years ago by
- Cc fbissey added
comment:9 in reply to: ↑ 7 ; follow-up: ↓ 10 Changed 4 years ago by
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 thegap
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 comment:424:ticket:22626 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.
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 4 years ago by
Replying to gh-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 thegap
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
andio
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 comment:424:ticket:22626 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!
comment:11 in reply to: ↑ 10 Changed 4 years ago by
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.
comment:12 Changed 4 years ago by
- Milestone changed from sage-8.6 to sage-8.7
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.
comment:13 Changed 3 years ago by
- Milestone changed from sage-8.7 to sage-8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:14 Changed 3 years ago by
- Milestone changed from sage-8.8 to sage-8.9
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).
comment:15 Changed 3 years ago by
- Milestone changed from sage-8.9 to sage-9.1
Ticket retargeted after milestone closed
comment:16 Changed 2 years ago by
- Milestone changed from sage-9.1 to sage-9.2
Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.
comment:17 Changed 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:18 Changed 17 months ago by
- Milestone changed from sage-9.3 to sage-9.4
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
comment:19 Changed 13 months ago by
- Milestone changed from sage-9.4 to sage-9.5
Setting a new milestone for this ticket based on a cursory review.
comment:20 Changed 8 months ago by
- Milestone changed from sage-9.5 to sage-9.6
Stalled in needs_review
or needs_info
; likely won't make it into Sage 9.5.
comment:21 Changed 4 months ago by
- Milestone changed from sage-9.6 to sage-9.7
Even if I install gap without
xgap
, I get apparently related failures. For examplePermutationGroup(['(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 5e61d7b6a0da3aa53d8176fa1fb9353cc559b098 but apparently didn't end up in the final version.