-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
doctest memlimit being hit in sage.combinat.tableau tests #28106
Comments
comment:1
On #27681 it was reported that the error doesn't occur when running the tests with
Since this is a pretty random issue, it possible that running with |
comment:2
At this point I'm not even certain it has anything to with libgap. That's just the context in which this first started appearing for me. It could also be a CPython bug. |
comment:3
Sometimes I forget that on Linux we run the tests by default with I wonder if that's all it is. |
comment:4
Yep, the crash is just occurring upon hitting the 3300MB memlimit. Simple and mundane as that. It could still be indirectly related to libgap. When investigating this earlier in #27681 we thought it might have something to do with GAP workspaces. This is because I first discovered the problem while working on #18267, in which libgap is used much more extensively directly in sage (as opposed to pexpect gap which is running outside the main sage process and so doesn't hit this memory limit as easily). It also seemed to go away if I deleted my existing GAP workspaces. It could be that in the previous case, merely loading a pre-existing GAP workspace was enough to push over the memory limit. |
comment:5
Of note: With the default of While running the tests, libgap reserves initially what amounts to I wonder, if nothing else, as long as we're capping the virtual memory limit for doctests, the default amount reserved for Pari should be lower. |
comment:6
One little thing we can do that happens to be good-enough for this case, it seems, is to further limit the amount of workspace Pari allocates by default, just when running the test suite. Since many tests won't require that much memory for Pari this seems sensible. In the case of this particular test the other issue is just reducing the amount of memory used when instantiating large Tableaux classes. I have opened #28114 for that. |
Author: Erik Bray |
Branch: u/embray/ticket-28106 |
Commit: |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:8
I am a little split on this. I agree that we don't really need it for the doctests, but I don't like making the behavior different than what is done within a Sage session. Or is there something special with Cygwin and/or the doctest framework going on here? |
comment:9
This problem can also be observed with a doctest for I tried the branch here, but it does not solve the issue for me. Both the tests in On the other hand, raising the |
comment:10
I'm also hitting this on the buildbot, unsurprisingly. IMHO its reasonable to provide less memory specifically for doctests, if only to encourage authors to limit resource usage when writing tests. |
Reviewer: Volker Braun |
comment:11
I'm still running sometimes out of memory
|
comment:12
I still see this on my Fedora desktop, as I run tests with |
comment:13
Can we just reduce This is on Debian fwiw. |
comment:14
hmm, but it is fast (for me, on Debian 10):
It is not constructing this huge set for sure... |
comment:15
Replying to @dimpase:
Which is the point of this doctest, the set is huge and it is using an actual computation. However,
Now the
instead of
Now unfortunately I do not see a simple way to keep that family done lazily. However, changing it to 40 should still do an appropriate test but uses far less memory:
|
comment:16
The current branch does not work as intended because |
comment:32
Replying to @embray:
I wouldn't be surprised if some tests (in particular |
comment:33
Replying to @jdemeyer:
It just doesn't seem "fair", in a sense, that we impose a default hard limit on virtual memory allocation, and then have a large chunk of that eaten up by Pari, even when most of it isn't needed for most other tests. When Pari runs out of memory isn't there some way to "catch" that, increase its stack, and re-try the operation? Perhaps tests/code that are expected to eat up the Pari stack should actually explicitly do that (it would be good to test that anyways). |
comment:34
Replying to @embray:
I really don't understand at all what the word "fair" (even you put it in quotes) means here. This isn't about fairness, it's about solving a practical problem, namely preventing excessive memory usage by doctests. As I said: the limit of 3300 MB is chosen pretty arbitrarily and I wouldn't mind increasing it.
Sure, it probably could be done, but why bother? It's much easier to just allocate a large virtual stack. I just don't see the problem with that. |
comment:35
Its also the only bound on actual ram usage during tests, and we should try to keep that manageable especially since tests tend to be run in parallel and cpu core count is increasing faster than ram prices are declining. Also, something <= 4gb is going to be the upper bound on 32-bit anyways. |
comment:36
Replying to @jdemeyer:
That's what I'm talking about: Normally it's fine to just allocate a large stack for PARI. But RLIMIT_AS limits the amount of virtual address space available to the process, and a large chunk of that gets used, without exception, for the PARI stack even for tests that don't need it so much, thus creating stricter limitations on what those tests are allowed to do. This is what I mean by "fair". |
comment:37
I've since seen various other instances of this issue, not so much on my own machine, but others seem to be encountering it in various tests. Although I can't prove myself that this is the reason (they would have to demonstrate that the failing tests are hitting the VM limit, or that the problems go away with But ISTM we need to do something about this. If not in the PARI interface I could also see about reducing the amount libgap needs to allocate, as I believe it's the increased usage of libgap in permutation groups that is largely responsible for the increase in issues like this. |
comment:38
Why aren't we just doing the obvious thing and increasing the limit from 3300MB to 3400MB? There is nothing magical about that 3300MB limit, it's mostly an arbitrary number. |
comment:39
Sure, I obviously wouldn't mind increasing the limit. I just wonder how sustainable that is. Perhaps a few more tests should be marked It also makes me wonder if there aren't memory leaks that should be investigated. |
comment:40
Ticket retargeted after milestone closed |
comment:43
Setting new milestone based on a cursory review of ticket status, priority, and last modification date. |
comment:44
Ticket #31395 removed |
Changed reviewer from Volker Braun to Volker Braun, Dima Pasechnik |
This is a follow-up to #27681 which is now closed, as it fixed one issue related to libgap and pexpect gap possibly interacting in bad ways.
However, the original issue, which for a while I could not reproduce, seems to be coming up again. For example if I run just:
I get
There is something about
sage.combinat.tableau
that the problem is always occurring there, though I'm not sure why. It's usually crashing on the same tests, but not always with the exact same traceback, suggesting some pseudo-deterministic memory corruption.Component: interfaces
Keywords: gap libgap combinat
Author: Erik Bray
Branch/Commit: u/embray/ticket-28106 @
c6ff771
Reviewer: Volker Braun, Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/28106
The text was updated successfully, but these errors were encountered: