#25907 closed enhancement (fixed)

Better handling of memory limits on tests

Reported by: embray Owned by:
Priority: critical Milestone: sage-8.4
Component: doctest framework Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 331d4d5 (Commits) Commit: 331d4d5d47b1f4760750b069ad42c10b69052e5d
Dependencies: Stopgaps:

Description

Open to different ideas about this, but here's a proposal for better handling of tests that have the possibility of eating up all memory (such as the test toward the beginning of matrix_mod2_dense.pyx:

            sage: import resource
            sage: if resource.RLIMIT_AS == getattr(resource, 'RLIMIT_RSS', None):
            ....:     # Skip this test if RLIMIT_AS is not properly
            ....:     # supported like on OS X, see Trac #24190
            ....:     raise RuntimeError("matrix allocation failed")
            ....: else:  # Real test
            ....:     MatrixSpace(GF(2), 2^30)(1)
            Traceback (most recent call last):
            ...
            RuntimeError: matrix allocation failed

This fixes #25884, and also maintains the fix for OSX, since now by default this test will not run on those platforms unless the user explicitly passes --memlimit=0 for no memory limit on tests.

Change History (22)

comment:1 Changed 13 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 13 months ago by jdemeyer

Can't you handle high mem through the usual optional tag mechanism?

comment:3 Changed 13 months ago by embray

That's what it's already doing, but it works like --long in that there's also a command-line flag for it. The difference is that it's spelled --memlimit=0, and the same --memlimit option can also be used to set a higher or lower (but non-zero) memory limit.

comment:4 Changed 13 months ago by jdemeyer

Actually, I find the name high mem a bit misleading: this is a test specifically designed to allocate a large amount of memory and fail doing that. So it has a very specific meaning and I'm not convinced that we need support in the doctest framework for that.

Why not just fix the doctest that is breaking?

comment:5 follow-up: Changed 13 months ago by embray

I think there are other cases where this might be useful (if not currently, in the past and in the future). For example a comment in the runtests script says of the old 3300 MB memory limit:

It is in particular doctests in src/sage/schemes/elliptic_curves/heegner.py which need this much memory.

Except I didn't notice any high memory usage in that module once I tested this. Perhaps there was a memory leak that was fixed or something else.

"Just" fix the test is not that great either, because the "fix" is a convoluted workaround with the express purpose of effectively skipping that test, duplicating logic that is already needed in sage-runtests to check if it actually makes sense to set a memory limit on some platform or other. This moves all the platform-specific stuff to one place, and sets a flag as to whether or not the test can be safely run.

So I do believe it's a useful addition. I'm open to different nomenclature.

comment:6 in reply to: ↑ 5 Changed 13 months ago by jdemeyer

Replying to embray:

"Just" fix the test is not that great either, because the "fix" is a convoluted workaround

Isn't it just a matter of checking whether a memory limit was set? You could add a small helper function somewhere for that.

Adding new doctest infrastructure to fix a single broken doctest looks like way overkill to me.

comment:7 follow-up: Changed 13 months ago by embray

What do you mean? The fact that a memory limit is set at all is something that the doctest runner does (albeit without any customizability or knowledge of the the user). Therefore "checking whether a memory limit was set" is a question that must be interrogated of the doctest framework (just as # long time is used to mark tests that take long to run, it also makes sense to mark tests that might run out of memory).

This isn't adding any "infrastructure". It's just adding an option within the existing infrastructure. It's not just one test either--this could easily come up elsewhere and probably does from time to time. I don't see what your problem is.

Last edited 13 months ago by embray (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 13 months ago by jdemeyer

Replying to embray:

I don't see what your problem is.

Maybe my problem is that you should make it very clear what the semantics of high mem are (and change the name to something that more accurately reflects the semantics, like # optional - memlimit). You are giving too much the incorrect impression that # high mem is analogous to # long time, but for memory instead of running time.

When you write "tests that might run out of memory" in 7, I have the impression that even you don't understand it (and I really don't mean to offend!). It's a test which is specifically written to allocate more memory than available.

comment:9 Changed 13 months ago by embray

Okay, that's a very different "problem" than "this adds 'infrastructure' just to fix one test".

I do understand the test clearly, and problem is that on Cygwin it's simply not possible to set a virtual memory limit at all (at least not with setrlimit) and on OSX RLIMIT_AS is set equivalent to RLIMIT_RSS, so while the test may pass, it will gobble up physical memory needlessly :( Therefore it's better that the test be skipped.

I'd be happy with # optional - memlimit though it's not clear to me how that's much different. I get confused about what # optional is supposed to be used for, since it usually (but not always?) seems to pertain to optional packages...

comment:10 Changed 13 months ago by embray

I see now that there's also # optional - internet. So in retrospect I do like the spelling # optional - memlimit implying that the memlimit option has been set to something > 0.

comment:11 Changed 13 months ago by git

  • Commit changed from 20a121c668843f7970568c5b1a987ec70da4a994 to ed90ea94457d0b32850e8a2ffab827bdd929f36c

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

ed90ea9use '# optional - memlimit' instead; this explicitly states that

comment:12 Changed 13 months ago by embray

Perhaps something more like this then? Tests marked # optional - memlimit will only run if setting RLIMIT_AS actually worked, or if --optional=memlimit was given explicitly (this allows forcing the test to run).

comment:13 Changed 13 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work
  1. There are still some traces of high mem in the branch.
  1. I consider the --memlimit option quite specialized, so I would drop the short -m option for it.
  1. Maybe if memlimit > 0 and memlimit <= sys.maxsize -> if 0 < memlimit <= sys.maxsize
  1. Can you add a doctest test for this? See src/sage/doctest/test.py

comment:14 Changed 13 months ago by jdemeyer

Apart from these comments LGTM!

comment:15 Changed 13 months ago by embray

Ok; agreed with all of the above.

comment:16 Changed 13 months ago by embray

I'm not exactly sure how best to test this, given that the result of using the memlimit flag depends largely on the platform, and there's no obvious way around that without writing a largely tautological test.

comment:17 Changed 13 months ago by jdemeyer

I have an idea for a test. I'll push a commit.

comment:18 Changed 13 months ago by jdemeyer

  • Branch changed from u/embray/doctest/memlimit to u/jdemeyer/doctest/memlimit

comment:19 Changed 13 months ago by jdemeyer

  • Commit changed from ed90ea94457d0b32850e8a2ffab827bdd929f36c to 331d4d5d47b1f4760750b069ad42c10b69052e5d

New commits:

331d4d5Add test for --memlimit option

comment:20 Changed 13 months ago by embray

Originally the way I was going to test this was not even going to be to test the rlimit itself, but just test the behavior with regard to skipping the tests are not, as well as what setting --memlimit=0 does.

This could all be added as well of course. If you're OK with making the test so platform-specific then I could add such tests as well. I was just trying to avoid that, but it also seems impossible to avoid completely...

comment:21 Changed 12 months ago by embray

  • Status changed from needs_work to positive_review

Let's just get this finished; if more tests are needed later I'll add them.

comment:22 Changed 12 months ago by vbraun

  • Branch changed from u/jdemeyer/doctest/memlimit to 331d4d5d47b1f4760750b069ad42c10b69052e5d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.