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: sage-6.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:

Status badges

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 SimonKing

  • Branch set to u/SimonKing/broken_doctest_in_src_sage_categories_fields_py

comment:2 Changed 7 years ago by SimonKing

  • Authors set to Simon King
  • Commit set to 0d97c7db960aa058b7cb1c60aea839dbc3d161e9
  • Component changed from categories to memleak
  • Status changed from new to needs_review

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:

0d97c7dMake garbage collection deterministic during a test against a memory leak

comment:3 follow-up: Changed 7 years ago by SimonKing

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 jdemeyer

  • 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 git

  • Commit changed from 0d97c7db960aa058b7cb1c60aea839dbc3d161e9 to 65ece650612dfb15bd1139245f9ac72bd94c9b68

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

65ece65A different approach to make the test deterministic

comment:6 Changed 7 years ago by SimonKing

  • Status changed from needs_work to needs_review

Make your choice :-)

comment:7 follow-up: Changed 7 years ago by 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

comment:8 in reply to: ↑ 7 Changed 7 years ago by SimonKing

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 jdemeyer

  • 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 git

  • Commit changed from 65ece650612dfb15bd1139245f9ac72bd94c9b68 to c289fb06c7788a935a1ee9a4852fa9bcc662676c

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

c289fb0Explain rationale of the doctest modification

comment:11 Changed 7 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:12 Changed 7 years ago by jdemeyer

  • 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 vbraun

  • 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
Note: See TracTickets for help on using tickets.