Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pickling be construction for element classes #23749

Open
simon-king-jena opened this issue Aug 30, 2017 · 2 comments
Open

Pickling be construction for element classes #23749

simon-king-jena opened this issue Aug 30, 2017 · 2 comments

Comments

@simon-king-jena
Copy link
Member

It was observed at #23707 that element classes do not correctly unpickle if the underlying base class changes from Python to Cython.

If P is a parent and P.Element is a Python class, then P.element_class is a dynamic class with bases P.Element and P.category().element_class. But if P.Element is a Cython class then currently (with not more than one exception) P.element_class is just P.Element.

If a pickle was created when P.Element was Python and later P.Element was changed to Cython, then the pickle still unpickles as dynamic class, whose bases are P.Element (now in Cython) and P.category().element_class. Problem: That is now different from P.element_class.

How to fix? When creating a dynamic class, one can provide a reduction, which here could be (getattr, (P,'element_class')). Then, a pickle of P.element_class would always unpickle to P.element_class.

However, I see a potential problem: Dynamic classes are cached. The cache is a weak cache, but that just means a weak value cache. However, there will be a strong reference to the key of the dictionary, which includes the reduction data. Which means that caching of the dynamic class would prevent P from garbage collection: Memory leak.

Obvious solution: We shouldn't include the reduction data in the cache key.

CC: @nthiery @jdemeyer

Component: pickling

Keywords: element class

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

@simon-king-jena
Copy link
Member Author

comment:1

The reduction data of an element class shouldn't be passed to the dynamic_class function, but should be assigned to the resulting class later, to circumvent the cache.

@simon-king-jena
Copy link
Member Author

comment:2

Replying to @simon-king-jena:

The reduction data of an element class shouldn't be passed to the dynamic_class function, but should be assigned to the resulting class later, to circumvent the cache.

On the other hand, pickling by construction probably is not the correct thing to do. Namely, the dynamic element classes belonging to different parents in the same category can easily be identical. Hence, pickling-by-construction would have a bad consequence: Assume you first construct a parent A and then a parent B with identical element classes; then, pickle an element x of B; when unpickling x, not only B but also A would be reconstructed, because the type(x) (the common element class of A and B) was, at the time of first constructing x, defined as getattr(A,'element_class').

I think building A when unpickling an element of B is a no-go. But unpickling a former dynamic Python element class as something that is not the element class after Cythonisation is a no-go as well.

Do you see a way out of that dilemma?

@mkoeppe mkoeppe removed this from the sage-8.1 milestone Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants