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

remove __reduce__ method from Mutability #30281

Closed
mjungmath opened this issue Aug 3, 2020 · 17 comments
Closed

remove __reduce__ method from Mutability #30281

mjungmath opened this issue Aug 3, 2020 · 17 comments

Comments

@mjungmath
Copy link

The class Mutability used to be a mixin class for mutable objects. However, not every class inheriting from it uses pickling and even then it must be overloaded manually in most cases anyway.

We suggest to remove that method from Mutability.

CC: @tscrim @mkoeppe @egourgoulhon

Component: misc

Author: Michael Jung

Branch/Commit: f0230f0

Reviewer: Travis Scrimshaw, Matthias Koeppe

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

@mjungmath mjungmath added this to the sage-9.2 milestone Aug 3, 2020
@mjungmath
Copy link
Author

Author: Michael Jung

@mjungmath

This comment has been minimized.

@mjungmath

This comment has been minimized.

@mjungmath
Copy link
Author

@mjungmath
Copy link
Author

New commits:

f0230f0Trac #30281: `__reduce__` method removed

@mjungmath
Copy link
Author

Commit: f0230f0

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2020

comment:5

Is this for a ticket where the existence of this method is a creating a problem?

@mjungmath
Copy link
Author

comment:6

Yes. See comment:10 in #30261 and all the following by Travis.

@mjungmath
Copy link
Author

comment:7

It seems that inheriting from Mutability causes a pickling test. Probably that is the reason why it is endowed with this method.

@mkoeppe
Copy link
Member

mkoeppe commented Aug 3, 2020

comment:8

Yes. When there is a __reduce__ method, then it is tested.
The correct fix is to implement pickling for your class in #30261.

@mjungmath
Copy link
Author

comment:9

Replying to @mkoeppe:

Yes. When there is a __reduce__ method, then it is tested.
The correct fix is to implement pickling for your class in #30261.

This is exactly the reason why we remove __reduce__ from Mutability. But even then, pickling is tested (see #30280).

@tscrim
Copy link
Collaborator

tscrim commented Aug 4, 2020

comment:10

This __reduce__ method is bad because it it tries to unpickle anything as a Mutability object. So a Python class that inherits from this must then contend with this, which puts more of burden on the implementer. Perhaps there is a better way to do this, but it is not immediate to me how to do this...

@tscrim
Copy link
Collaborator

tscrim commented Aug 7, 2020

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 7, 2020

comment:11

I really think this should be removed. However, we need to make sure that when we pickle and unpickle an immutable object X it remains immutable.

@mkoeppe
Copy link
Member

mkoeppe commented Aug 7, 2020

comment:12

Fine with me too

@mkoeppe
Copy link
Member

mkoeppe commented Aug 18, 2020

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Aug 20, 2020

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

4 participants