Opened 7 years ago
Closed 7 years ago
#19244 closed defect (fixed)
Broken doctest in src/sage/categories/fields.py
Reported by:  jdemeyer  Owned by:  

Priority:  blocker  Milestone:  sage6.9 
Component:  memleak  Keywords:  random_fail 
Cc:  robertwb, nbruin, SimonKing, jpflori, slabbe, vdelecroix  Merged in:  
Authors:  Simon King  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  c289fb0 (Commits, GitHub, GitLab)  Commit:  c289fb06c7788a935a1ee9a4852fa9bcc662676c 
Dependencies:  Stopgaps: 
Description
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 **********************************************************************
Change History (13)
comment:1 Changed 7 years ago by
 Branch set to u/SimonKing/broken_doctest_in_src_sage_categories_fields_py
comment:2 Changed 7 years ago by
 Commit set to 0d97c7db960aa058b7cb1c60aea839dbc3d161e9
 Component changed from categories to memleak
 Status changed from new to needs_review
comment:3 followup: ↓ 4 Changed 7 years ago by
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.
comment:4 in reply to: ↑ 3 Changed 7 years ago by
 Status changed from needs_review to needs_work
Replying to SimonKing:
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.
comment:5 Changed 7 years ago by
 Commit changed from 0d97c7db960aa058b7cb1c60aea839dbc3d161e9 to 65ece650612dfb15bd1139245f9ac72bd94c9b68
Branch pushed to git repo; I updated commit sha1. New commits:
65ece65  A different approach to make the test deterministic

comment:6 Changed 7 years ago by
 Status changed from needs_work to needs_review
Make your choice :)
comment:7 followup: ↓ 8 Changed 7 years ago by
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
comment:8 in reply to: ↑ 7 Changed 7 years ago by
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.
comment:9 Changed 7 years ago by
 Status changed from needs_review to needs_work
Could you add some text in the doctest to explain what you just explained here?
comment:10 Changed 7 years ago by
 Commit changed from 65ece650612dfb15bd1139245f9ac72bd94c9b68 to c289fb06c7788a935a1ee9a4852fa9bcc662676c
Branch pushed to git repo; I updated commit sha1. New commits:
c289fb0  Explain rationale of the doctest modification

comment:11 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 7 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
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.
comment:13 Changed 7 years ago by
 Branch changed from u/SimonKing/broken_doctest_in_src_sage_categories_fields_py to c289fb06c7788a935a1ee9a4852fa9bcc662676c
 Resolution set to fixed
 Status changed from positive_review to closed
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 forloop 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:
Make garbage collection deterministic during a test against a memory leak