Random failure in coerce_action.pyx

[vbraun]

Zmod(6) can be collected at the wrong time

sage -t --long --warn-long 47.9 src/sage/structure/coerce_actions.pyx
**********************************************************************
File "src/sage/structure/coerce_actions.pyx", line 78, in sage.structure.coerce_actions.LAction
Failed example:
    sage.structure.coerce_actions.GenericAction(QQ, Zmod(6), True, check=False)
Expected:
    Left action by Rational Field on Ring of integers modulo 6
Got:
    <repr(<sage.structure.coerce_actions.GenericAction at 0x7f8650aca938>) failed: RuntimeError: This action acted on a set that became garbage collected>
**********************************************************************
1 item had failures:
   1 of   4 in sage.structure.coerce_actions.LAction
    [100 tests, 1 failure, 0.82 s]

Idem with

sage -t --long src/sage/structure/coerce.pyx
**********************************************************************
File "src/sage/structure/coerce.pyx", line 1452, in sage.structure.coerce.CoercionModel_cache_maps.discover_action
Failed example:
    cm.discover_action(GF(5)['x'], ZZ, operator.div)
Expected:
    Right inverse action by Finite Field of size 5 on Univariate Polynomial Ring in x over Finite Field of size 5
    with precomposition on right by Natural morphism:
      From: Integer Ring
      To:   Finite Field of size 5
Got:
    <repr(<sage.categories.action.PrecomposedAction at 0x2b41eb2a2050>) failed: RuntimeError: This action acted on a set that became garbage collected>
**********************************************************************

CC: @simon-king-jena

Component: coercion

Keywords: random_fail

Author: Vincent Delecroix, Simon King

Branch: 60fcedc

Reviewer: Simon King

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

[nbruin]

comment:1

Reliable reproduction of the error condition:

import gc
A=sage.structure.coerce_actions.GenericAction(QQ, Zmod(6), True, check=False)
gc.collect()
C=repr(A)

The reason is simple: sage.categories.action.Action.__init__ only stores a weakref to the set acted on, and nothing else. If this "action" object is mainly meant to operate inside the coercion framework then this is probably the appropriate behaviour. Note that if the action ever needs to get used, then there is a group element to act and a set element to be acted upon, so the space has to be alive. Whoever is storing this action object should have a callback on the weakref, though, to throw away this action object to avoid a memory leak.

It may just be that this action object does get used properly and that this doctest is broken. In that case the fix is:

sage: Z6 = Zmod(6)
sage: sage.structure.coerce_actions.GenericAction(QQ, Z6, True, check=False)

[nbruin]

comment:3

cm.discover_action(GF(5)['x'], ZZ, operator.div)

Also for this one: the coercion framework will never be asked to discover an action on an otherwise unreferenced parent. At the same time, the action object does get cached (globally!) somewhere in the coercion framework and hence should probably not hold a strong reference (at least to one of the sets it acts on). The container it gets stored in is probably a TripleDict, keyed on the two parents involved. It takes care of having a callback to delete the action object when one of the domains gets deleted, so the action objects will normally not outlive the shortest lifetime of the parents involved.

If we have an absolute need of handling actions outside the coercion framework, we should probably make other action objects that do keep strong references. We do this for maps that get used for conversion and coercion. But these actions are showing desirable behaviour for use in the coercion framework. I think the bug is in the doctests.

[videlec]

comment:4

Replying to @nbruin:

I think the bug is in the doctests.

Me too!

[vbraun]

comment:5

Somebody please write a patch instead of me-too-ing ;-)

[videlec]

Commit: ced7bfc

[videlec]

comment:6

Might be other wrong doctests... not sure how to find them.

---

New commits:

ced7bfc

Trac 18157: fix garbage collected objects in doctest

[videlec]

Branch: public/18157

[pjbruin]

comment:7

There is another one in src/sage/matrix/action.pyx; it is already fixed in #10513, so no need to treat that one here.

[videlec]

comment:8

Would be nice if people would run the testsuite with this patch (and #10513)

for i in $(seq -w 1 100)
do
   sage -t --long structure/ rings/ modular/ matrix/| tee ~/testlong${i}.log
done

Or even better, does somebody know how to call gc.collect() after each command in doctests?

[jdemeyer]

comment:9

Replying to @videlec:

Or even better, does somebody know how to call gc.collect() after each command in doctests?

That wouldn't help, right?

[videlec]

comment:10

Replying to @jdemeyer:

Replying to @videlec:

Or even better, does somebody know how to call gc.collect() after each command in doctests?

That wouldn't help, right?

Right! Not for the two doctests mentioned in the description...

[simon-king-jena]

Reviewer: Simon King

[simon-king-jena]

comment:12

With one exception, the flaky tests constitute a misuse: Objects that are normally used internally are suddenly put out into the wild. It is clear that that can lead to disaster unless precautions are taken. Hence, I support the general approach in the attached branch: In these tests, one should work with permanent rather than temporary objects, to prevent premature garbage collection.

However, one should also explicitly say in these tests that we are talking about things that normally happen in the innards of Sage, and that the tests do not show how to work with them!

One test is different:

@@ -606,7 +624,8 @@ cdef class IntegerMulAction(Action):
 
         This used to hang before :trac:`17844`::
 
-            sage: E = EllipticCurve(GF(5), [4,0])
+            sage: GF5 = GF(5)
+            sage: E = EllipticCurve(GF5, [4,0])
             sage: P = E.random_element()
             sage: (-2^63)*P
             (0 : 1 : 0)

Defining an elliptic curve over a finite field should certainly be enough to prevent the finite field from garbage collection! So, here I do not agree with the fix. Apparently the test fails because of an internal bug, not because of misuse.

So, I put it to "needs work", because we should either find and fix the underlying bug here, or open a new ticket for the elliptic curve issue.

[simon-king-jena]

Work Issues: Find underlying problem for elliptic curve problem, or open a new ticket for it

[simon-king-jena]

comment:13

I don't see a problem with the elliptic curve example:

sage: import gc
sage: E = EllipticCurve(GF(5), [4,0])
sage: gc.collect()
156
sage: P = E.random_element()
sage: gc.collect()
0
sage: (-2^63)*P
(0 : 1 : 0)

So, why should it be changed?

[vbraun]

comment:14

Even if right now E is kept alive elsewhere (presumably another doctest leaked it) you shouldn't assume that this will not change in the future.

[videlec]

comment:15

Replying to @simon-king-jena:

One test is different:

Agreed. You can revert it.

[simon-king-jena]

comment:16

Replying to @vbraun:

Even if right now E is kept alive elsewhere (presumably another doctest leaked it) you shouldn't assume that this will not change in the future.

What are you talking about? In the test in question, the elliptic curve E certainly is kept alive in the test (not elsewhere), as it is strongly referenced. As I don't see this test fail, I agree with Vincent that the test should be reverted to its original state.

[sagetrac-git]

Changed commit from ced7bfc to ef3bb5d

[sagetrac-git]

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

ef3bb5d

Add a warning not to use coerce actions outside of the coercion model

[simon-king-jena]

Author: Vincent Delecroix, SImon King

[simon-king-jena]

comment:18

I added comments in appropriate places, telling to be careful when using coerce actions outside of coercion models, reverting the elliptic curve test, and fixing some more tests in which a premature garbage collection was possible.

From my point of view, it is a positive review, but of course someone should have a look at my commit.

---

New commits:

ef3bb5d

Add a warning not to use coerce actions outside of the coercion model

[sagetrac-git]

Changed commit from ef3bb5d to 60fcedc

[sagetrac-git]

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

a48cd06	Merge branch 'public/18157' of trac.sagemath.org:sage into develop
60fcedc	Trac 18157: adding reference to the trac ticket

[videlec]

comment:20

Hi Simon,

I merge on sage-6.7.beta1 (sorry if I did wrong). If you prefer you can only cherry-pick 60fcedc.

I agree with your commits and just added references to your ticket. I am ok for the positive review.

Vincent

[simon-king-jena]

comment:21

Replying to @videlec:

I merge on sage-6.7.beta1 (sorry if I did wrong). If you prefer you can only cherry-pick 60fcedc.

I suppose that Volker will tell you that (1) one should only merge if there is a good reason, and (2) one must not change the history, even if one has merged without a good reason.

So, just leave it as it is. I had a look at your changes, and they look fine to me. Provided that the patchbot gives a green blob, it should be positive review.

[simon-king-jena]

comment:22

The patchbot tells us that all tests pass, and based on the discussion above we have a positive review.

[simon-king-jena]

Changed work issues from Find underlying problem for elliptic curve problem, or open a new ticket for it to none

[vbraun]

Changed branch from public/18157 to 60fcedc

[kcrisman]

Changed author from Vincent Delecroix, SImon King to Vincent Delecroix, Simon King

[kcrisman]

Changed commit from 60fcedc to none