Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#23748 closed enhancement (fixed)

Run doctests with limited memory

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: doctest framework Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 06d4307 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

There are various reasons to limit the maximal amount of memory that doctests can use:

  1. Bugs which cause large memory usage and which can cause trouble for the OS when running doctests.
  2. It is good to check that doctests do not use a large amount of memory.
  3. It allows to doctest graceful recovery when memory cannot be allocated by intentionally allocating more memory than available.

I suggest to limit the virtual memory for doctests. The following two files contain the tests which need the most memory:

src/sage/game_theory/matching_game.py
src/sage/schemes/elliptic_curves/heegner.py

This branch also reduces the memory needed for matching_game.py, such that heegner.py is now the doctest which requires the most memory.

Change History (46)

comment:1 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/run_doctests_with_limited_memory

comment:2 Changed 2 years ago by jdemeyer

  • Commit set to 6b2589ee117284cc3dc24d58beda0aa2b3aa5a0e
  • Status changed from new to needs_review

New commits:

6b2589eRun doctests with limited memory

comment:3 follow-ups: Changed 2 years ago by tscrim

Maybe this should be discussed on sage-devel, but my vote would be for 1Gb as IMO, doctests should in general not use a large amount of memory similar to the paradigm that they should be fast.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 2 years ago by jdemeyer

Replying to tscrim:

Maybe this should be discussed on sage-devel, but my vote would be for 1Gb as IMO, doctests should in general not use a large amount of memory similar to the paradigm that they should be fast.

With 1GB, you cannot even start Sage.

And some doctests in src/sage/schemes/elliptic_curves/heegner.py do not pass with a limit of only 3GB.

comment:5 in reply to: ↑ 3 Changed 2 years ago by jdemeyer

Replying to tscrim:

doctests should in general not use a large amount of memory similar to the paradigm that they should be fast.

That's precisely the reason for this patch.

comment:6 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:7 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

Maybe this should be discussed on sage-devel, but my vote would be for 1Gb as IMO, doctests should in general not use a large amount of memory similar to the paradigm that they should be fast.

With 1GB, you cannot even start Sage.

Oh, I was thinking this was the amount of memory specifically used by any particular doctest, not Sage + doctests. What about when doctests are run in parallel? Since it is using threads, we still have the same limit of 4Gb, so I am worried we will hit this limit on systems with larger amounts of cores.

And some doctests in src/sage/schemes/elliptic_curves/heegner.py do not pass with a limit of only 3GB.

Maybe we also want to consider a # high memory marker (another ticket?) or have # long time also allow higher memory?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 2 years ago by jdemeyer

Replying to tscrim:

Oh, I was thinking this was the amount of memory specifically used by any particular doctest, not Sage + doctests.

No, it is the virtual memory limit for the whole process. This includes code (Python + Python modules + libraries) and data, also unused but allocated memory (like the PARI stack).

What about when doctests are run in parallel? Since it is using threads

No! It uses processes, not threads. The limit is per process, so we do not need to worry about that.

And some doctests in src/sage/schemes/elliptic_curves/heegner.py do not pass with a limit of only 3GB.

Maybe we also want to consider a # high memory marker (another ticket?) or have # long time also allow higher memory?

Perhaps, but this is just a very simple limit, comparable to the absolute doctest time limit of 360s (short tests) or 1800s (long tests).

comment:9 Changed 2 years ago by jdemeyer

I'll try to lower the limit of 4GiB somewhat, but it certainly must be higher than 3GiB.

comment:10 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 2 years ago by git

  • Commit changed from 6b2589ee117284cc3dc24d58beda0aa2b3aa5a0e to f8fcb510bcbe16103fd133b868915b94e7555010

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f8fcb51Run doctests with limited memory

comment:13 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:14 in reply to: ↑ 8 ; follow-up: Changed 2 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

What about when doctests are run in parallel? Since it is using threads

No! It uses processes, not threads. The limit is per process, so we do not need to worry about that.

We should probably change this then:

travis@apricot:~/sage/src/sage/categories$ sage -tp category.py category_types.py 
[snip]
Doctesting 2 files using 8 threads.

Although I am somewhat surprised my memory usage is not higher when I am running parallel tests since each copy of Sage should take a minimum of 1Gb. *shrugs*, oh well, not important.

And some doctests in src/sage/schemes/elliptic_curves/heegner.py do not pass with a limit of only 3GB.

Maybe we also want to consider a # high memory marker (another ticket?) or have # long time also allow higher memory?

Perhaps, but this is just a very simple limit, comparable to the absolute doctest time limit of 360s (short tests) or 1800s (long tests).

I don't mind 4Gb, I probably prefer it over the 3.x for simplicity.

comment:15 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 2 years ago by git

  • Commit changed from f8fcb510bcbe16103fd133b868915b94e7555010 to b4001c6dbad755c92abaaa4b5b2fb23867df74b8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b4001c6Run doctests with limited memory

comment:17 in reply to: ↑ 14 ; follow-up: Changed 2 years ago by jdemeyer

Replying to tscrim:

each copy of Sage should take a minimum of 1Gb.

Well, remember that this is virtual memory. Most of that 1GB is shared between processes. If you run 10 copies of Sage, the executable code of Sage needs to be in physical memory only once: the OS maps the same physical memory inside the 10 processes.

Then there is also memory which is reserved but typically not used: Sage might allocate a large PARI stack and GAP memory pool for example which counts against virtual memory. However, as long as this memory is not actually used, you don't need that much physical memory.

comment:18 follow-up: Changed 2 years ago by jdemeyer

It turns out that even 4GiB is not enough... This new version uses 4500MiB which passes make ptestlong on my machine.

comment:19 in reply to: ↑ 17 Changed 2 years ago by tscrim

Replying to jdemeyer:

Replying to tscrim:

each copy of Sage should take a minimum of 1Gb.

Well, remember that this is virtual memory. Most of that 1GB is shared between processes. If you run 10 copies of Sage, the executable code of Sage needs to be in physical memory only once: the OS maps the same physical memory inside the 10 processes.

Then there is also memory which is reserved but typically not used: Sage might allocate a large PARI stack and GAP memory pool for example which counts against virtual memory. However, as long as this memory is not actually used, you don't need that much physical memory.

Ah, right. Thank you for the explanation.

comment:20 in reply to: ↑ 18 Changed 2 years ago by tscrim

Replying to jdemeyer:

It turns out that even 4GiB is not enough... This new version uses 4500MiB which passes make ptestlong on my machine.

I am pretty sure it is this block of tests that takes the large amount of memory (with n = 10):

        sage: from itertools import permutations
        sage: suitr_preferences = list(permutations([-i-1 for i in range(n)]))
        sage: revr_preferences = list(permutations([i+1 for i in range(n)]))
        sage: for player in range(n):
        ....:     big_game.suitors()[player].pref = suitr_preferences[player]
        ....:     big_game.reviewers()[player].pref = revr_preferences[-player]
        sage: big_game.solve()
        {1: -1, 2: -8, 3: -9, 4: -10, 5: -7, 6: -6, 7: -5, 8: -4, 9: -3, 10: -2}

That is 2 * 10! * 10 == 72576000 integers plus other stuff. Might consider dropping n = 8 instead for this example.

comment:21 Changed 2 years ago by jdemeyer

Maybe, but I consider lowering the actual memory requirements of doctests outside the scope of this ticket.

comment:22 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This is failing badly on the patchbot

comment:23 Changed 2 years ago by jdemeyer

The problem seems to be that setrlimit() is done way too late. We need to limit the memory before starting Sage.

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:24 Changed 2 years ago by git

  • Commit changed from b4001c6dbad755c92abaaa4b5b2fb23867df74b8 to 38f982331680d86b3ddafbb40f58ebd63df6719b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

38f9823Run doctests with limited memory

comment:25 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to needs_review

New attempt, this time limiting the memory in the sage-runtests script. Let's see what the patchbot says...

comment:26 Changed 2 years ago by jdemeyer

Hmm, patchbots won't test this because it's "unsafe".

comment:27 follow-up: Changed 2 years ago by jdemeyer

In case it helps, I did test this on sardonis (ppc64le with lots of RAM) and on my laptop (x86_64 with 6GB of RAM) and it passed tests. 32-bit should also pass for the simple reason that the condition memlimit <= sys.maxsize() will be false.

I just got hit again by the issue of doctests taking infinite memory, apparently this creates an infinite list:

sage: L = [1, 2, 3]
sage: L.extend(-x for x in L)

I'm just saying that this ticket protects against stupid mistakes like this, which might otherwise cause the system to start to swap and become unresponsive.

comment:28 in reply to: ↑ 27 Changed 2 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Replying to jdemeyer:

In case it helps, I did test this on sardonis (ppc64le with lots of RAM) and on my laptop (x86_64 with 6GB of RAM) and it passed tests. 32-bit should also pass for the simple reason that the condition memlimit <= sys.maxsize() will be false.

Everything seems to be okay on my laptop as well (x86_64 with 8Gb of RAM).

I just got hit again by the issue of doctests taking infinite memory, apparently this creates an infinite list:

sage: L = [1, 2, 3]
sage: L.extend(-x for x in L)

I'm actually slightly surprised Python doesn't catch that as it does so well (sometimes too well) with mutating dicts.

I'm just saying that this ticket protects against stupid mistakes like this, which might otherwise cause the system to start to swap and become unresponsive.

Indeed. Let's kick this to the buildbots and into a beta to see if this causes any trouble.

comment:29 follow-up: Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

I'm getting

sage -t --warn-long 69.8 src/sage/game_theory/matching_game.py
**********************************************************************
File "src/sage/game_theory/matching_game.py", line 161, in sage.game_theory.matching_game.MatchingGame
Failed example:
    revr_preferences = list(permutations([i+1 for i in range(n)]))
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 885, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.game_theory.matching_game.MatchingGame[17]>", line 1, in <module>
        revr_preferences = list(permutations([i+Integer(1) for i in range(n)]))
    MemoryError

Here n=10, so 10! permutations...

PS: This is x86_64 with 32GB ram

Last edited 2 years ago by vbraun (previous) (diff)

comment:30 Changed 2 years ago by fbissey

Same as Volker. Also x86_64 with 32GB (although free says 12GB are free and 25GB available).

comment:31 in reply to: ↑ 29 Changed 2 years ago by jdemeyer

Replying to vbraun:

I'm getting

sage -t --warn-long 69.8 src/sage/game_theory/matching_game.py
**********************************************************************
File "src/sage/game_theory/matching_game.py", line 161, in sage.game_theory.matching_game.MatchingGame
Failed example:
    revr_preferences = list(permutations([i+1 for i in range(n)]))
Exception raised:
    Traceback (most recent call last):
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 515, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/mnt/disk/home/release/Sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 885, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.game_theory.matching_game.MatchingGame[17]>", line 1, in <module>
        revr_preferences = list(permutations([i+Integer(1) for i in range(n)]))
    MemoryError

Did you run all doctests? Is this the only file which fails?

comment:32 Changed 2 years ago by fbissey

Full failure

sage -t --long /usr/lib64/python2.7/site-packages/sage/game_theory/matching_game.py
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/game_theory/matching_game.py", line 161, in sage.game_theory.matching_game.MatchingGame
Failed example:
    revr_preferences = list(permutations([i+1 for i in range(n)]))
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 518, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 888, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.game_theory.matching_game.MatchingGame[17]>", line 1, in <module>
        revr_preferences = list(permutations([i+Integer(1) for i in range(n)]))
    MemoryError
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/game_theory/matching_game.py", line 162, in sage.game_theory.matching_game.MatchingGame
Failed example:
    for player in range(n):
        big_game.suitors()[player].pref = suitr_preferences[player]
        big_game.reviewers()[player].pref = revr_preferences[-player]
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 518, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 888, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.game_theory.matching_game.MatchingGame[18]>", line 3, in <module>
        big_game.reviewers()[player].pref = revr_preferences[-player]
    NameError: name 'revr_preferences' is not defined
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/game_theory/matching_game.py", line 165, in sage.game_theory.matching_game.MatchingGame
Failed example:
    big_game.solve()
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 518, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 888, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.game_theory.matching_game.MatchingGame[19]>", line 1, in <module>
        big_game.solve()
      File "/usr/lib64/python2.7/site-packages/sage/game_theory/matching_game.py", line 901, in solve
        self._is_complete()
      File "/usr/lib64/python2.7/site-packages/sage/game_theory/matching_game.py", line 668, in _is_complete
        raise ValueError("suitor preferences are not complete")
    ValueError: suitor preferences are not complete
**********************************************************************

I cannot talk for Volker but this is the only file for which I have a failure related to this ticket. The other failure I got was https://trac.sagemath.org/ticket/23497#comment:39

comment:33 Changed 2 years ago by vbraun

Thats the only file that failed for me, too

comment:34 Changed 2 years ago by jdemeyer

If you both have only that file failing, it's a good reason to revisit those doctests after all and make them use less memory.

comment:35 follow-ups: Changed 2 years ago by fbissey

But it didn't fail before this ticket and this ticket doesn't touch this particular file.

comment:36 Changed 2 years ago by git

  • Commit changed from 38f982331680d86b3ddafbb40f58ebd63df6719b to 06d43071962808d1d8adf6d527a4e762f34290b5

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

06d4307Simplify doctest in matching_game.py

comment:37 in reply to: ↑ 35 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Replying to fbissey:

But it didn't fail before this ticket and this ticket doesn't touch this particular file.

The point of this ticket is to limit the amount of memory that doctests are allowed to use. Apparently matching_game.py is the file which uses the most memory of all doctests. There is no reason why the testcase in the matching_game.py doctests must be so large. I just simplified that.

comment:38 in reply to: ↑ 35 ; follow-up: Changed 2 years ago by jdemeyer

Replying to fbissey:

But it didn't fail before this ticket and this ticket doesn't touch this particular file.

So what would you suggest instead? Simply increasing the limit from 3300 MiB to some higher number?

comment:39 in reply to: ↑ 38 Changed 2 years ago by fbissey

  • Status changed from needs_review to positive_review

Replying to jdemeyer:

Replying to fbissey:

But it didn't fail before this ticket and this ticket doesn't touch this particular file.

So what would you suggest instead? Simply increasing the limit from 3300 MiB to some higher number?

No, after reading the description again properly, the fact that it passed before for me is irrelevant to what you try to achieve. I'll put it back to positive review to send it back to the bots.

comment:40 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:41 Changed 2 years ago by jdemeyer

Thanks!

comment:42 Changed 2 years ago by fbissey

Now I get this on Volker's develop branch

sage -t --long /usr/lib64/python2.7/site-packages/sage/rings/integer.pyx
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/rings/integer.pyx", line 2828, in sage.rings.integer.Integer.divisors
Failed example:
    for i in range(20):  # long time
        try:
            alarm(RDF.random_element(1e-3, 0.5))
            _ = n.divisors()
            cancel_alarm()  # we never get here
        except AlarmInterrupt:
            pass
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 518, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib64/python2.7/site-packages/sage/doctest/forker.py", line 888, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.integer.Integer.divisors[19]>", line 4, in <module>
        _ = n.divisors()
      File "sage/rings/integer.pyx", line 2897, in sage.rings.integer.Integer.divisors (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/rings/integer.c:19409)
        ptr = <unsigned long*>check_allocarray(divisor_count, 3 * sizeof(unsigned long))
      File "memory.pxd", line 87, in cysignals.memory.check_allocarray (/dev/shm/portage/sci-mathematics/sage-9999/work/sage-9999/src-python2_7/build/cythonized/sage/rings/integer.c:45630)
    MemoryError: failed to allocate 33554432 * 24 bytes
**********************************************************************

I have another failure in that file that is not related to memory, so there is probably problems with other tickets touching that file. I'll see if I can find out which one(s).

comment:43 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/run_doctests_with_limited_memory to 06d43071962808d1d8adf6d527a4e762f34290b5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:44 follow-ups: Changed 2 years ago by vdelecroix

  • Commit 06d43071962808d1d8adf6d527a4e762f34290b5 deleted

Could this be the source of the problem in #24075?

comment:45 in reply to: ↑ 44 Changed 2 years ago by fbissey

Replying to vdelecroix:

Could this be the source of the problem in #24075?

I wouldn't think so but if you can repeat the failure you can just comment those two lines in sage-runtests

        if lim == resource.RLIM_INFINITY or lim > memlimit:
            resource.setrlimit(resource.RLIMIT_AS, (memlimit, hard))

and see if it improves things.

comment:46 in reply to: ↑ 44 Changed 2 years ago by jdemeyer

Replying to vdelecroix:

Could this be the source of the problem in #24075?

That would be very surprising. How can runnning doctests with limited memory make something work accidentally?

Note: See TracTickets for help on using tickets.