Broken doctest in src/sage/categories/fields.py

[jdemeyer]

The following doctest from #14058 breaks sometimes. I have seen it happen twice, but it's hard to reproduce:

sage -t --long src/sage/categories/fields.py
**********************************************************************
File "src/sage/categories/fields.py", line 104, in sage.categories.fields.Fields.__contains__
Failed example:
    len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)]) - n
Expected:
    0
Got:
    -1
**********************************************************************

CC: @robertwb @nbruin @simon-king-jena @jpflori @seblabbe @videlec

Component: memleak

Keywords: random_fail

Author: Simon King

Branch/Commit: c289fb0

Reviewer: Jeroen Demeyer

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

[simon-king-jena]

Branch: u/SimonKing/broken_doctest_in_src_sage_categories_fields_py

[simon-king-jena]

Author: Simon King

[simon-king-jena]

comment:2

I have seen that repeatedly. I think at some point I suggested to test that the difference is not zero but is at most zero. Now I suggest a different solution.

We have seen the following phenomenon while we introduced Sage's weak coercion dictionaries. It is possible that gc.collect() collects some objects, and by this collection other objects become collectable, which however will only be collected during a subsequent cyclic garbage collection. One can construct examples such that n cyclic garbage collections will not collect a certain object, but the (n+1)-st cyclic garbage collection will collect it.

It could be that we see this phenomenon here in real life. I.e., it could be that the two gc.collect() commands in the test do not make a certain field collectable that was created in a different doctest. However, a third collection step that may or may not happen in the for-loop does make it collectable.

Thus, depending on whether or not a third collection happens, we see a failure or not.

If my explanation is correct, then disabling the garbage collection till we really want it to happen should fix the test permanently.

---

New commits:

0d97c7d

Make garbage collection deterministic during a test against a memory leak

[simon-king-jena]

Commit: 0d97c7d

[simon-king-jena]

comment:3

The following approach might be even more stable.

What do we want to test? We want to test that the NEWLY created fields can be garbage collected.

What happens if the test fails? There is an additional garbage collection of a PREVIOUSLY created field.

Hence, we must temporarily prevent all PREVIOUSLY created but uncollected objects from garbage collection.

This could be done by storing the output of gc.get_object() in a variable. It creates strong references to all previously created uncollected objects and will thus make the test stable.

What solution do you prefer? I prefer the second solution, since I am not sure whether the theory behind my previous solution is correct.

[jdemeyer]

comment:4

Replying to @simon-king-jena:

This could be done by storing the output of gc.get_object() in a variable. It creates strong references to all previously created uncollected objects and will thus make the test stable.

What solution do you prefer? I prefer the second solution, since I am not sure whether the theory behind my previous solution is correct.

I agree.

[sagetrac-git]

Changed commit from 0d97c7d to 65ece65

[sagetrac-git]

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

65ece65

A different approach to make the test deterministic

[simon-king-jena]

comment:6

Make your choice :-)

[jdemeyer]

comment:7

What's the point of this?

+            sage: len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)]) > n
+            True

[simon-king-jena]

comment:8

Replying to @jdemeyer:

What's the point of this?

+            sage: len([X for X in gc.get_objects() if isinstance(X, sage.rings.finite_rings.integer_mod_ring.IntegerModRing_generic)]) > n
+            True

At time t, we determine the number n of fields in cache. At time t+2 we test that the number of fields in cache is again n. With the above line, I want to demonstrate that at time t+1, we really have more rings in cache than at time t. Otherwise, we actually haven't shown that a garbage collection has happened between time t+1 and time t+2. And that's the aim of the whole test: We have to show that at some point a ring exists and later it has vanished. The line shows that the ring exists.

[jdemeyer]

comment:9

Could you add some text in the doctest to explain what you just explained here?

[sagetrac-git]

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

c289fb0

Explain rationale of the doctest modification

[sagetrac-git]

Changed commit from 65ece65 to c289fb0

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:12

Since the old broken doctest is hard to reproduce, I can't say for sure if this fixes it. But at least I believe your analysis.

[vbraun]

Changed branch from u/SimonKing/broken_doctest_in_src_sage_categories_fields_py to c289fb0