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

Pickle fails with models using FieldTracker #83

Closed
ondrowan opened this issue Oct 8, 2013 · 20 comments
Closed

Pickle fails with models using FieldTracker #83

ondrowan opened this issue Oct 8, 2013 · 20 comments

Comments

@ondrowan
Copy link

ondrowan commented Oct 8, 2013

PicklingError: Can't pickle <function save at 0x3e10320>: it's not found as model_utils.tracker.save

This happens with custom User model in Django 1.5 (not sure if it matters). Haven't had a chance to try it with earlier versions or 1.6.

kezabelle added a commit to kezabelle/django-model-utils that referenced this issue Apr 20, 2014
@kezabelle
Copy link
Collaborator

Verified with a dirt simple test in a21b9cc
Because the patch_save method closes over the new save method, which isn't visible at module scope, the above error is exhibited (see https://docs.python.org/2/library/pickle.html#what-can-be-pickled-and-unpickled)
It should fail under any version, but for the purposes of clarity, I ran it under Python 2.7.6 and Django 1.6.2
I haven't got a suggestion for how it could be fixed, if it even can be, but at least the ticket is triaged.

@ondrowan
Copy link
Author

Unfortunatelly, my only solution was not pickling it in given scenario.

@schinckel
Copy link
Contributor

The code in question, I believe, could be replaced by a signal.

Working on a patch now.

schinckel added a commit to schinckel/django-model-utils that referenced this issue May 12, 2014
References jazzband#83.

Instead of patching the save method of a tracked model class, we can use
a signal handler on post_save, which means we can still pickle our model
class.

Note we can't just listen for the signal from the class we have, but
instead listen for all post_save signals. This means we actually install
a new signal handler for each tracked model class, which fires on all
model save occurrences (and returns immediately if this handler doesn't care).

We probably could improve this to have a registry of tracked models, or
something, that allows us to just install one signal handler, and filter
according to membership.
@schinckel
Copy link
Contributor

So, this passes all tests (including an equivalent one for kezzabelle's test above).

However, I'm not 100% happy with it, as it installs a new signal handler for each tracked model, which listens to all models' post_save, and just exits early if it's not a subclass of the tracked model.

Which may be acceptable, I'm not sure.

@treyhunner
Copy link
Member

Fixed by gh-130.

@schinckel
Copy link
Contributor

I think this one can be closed now, @treyhunner.

@carljm
Copy link
Collaborator

carljm commented Aug 1, 2014

Reopening this, as I've just had to revert #130 due to #143.

@bforchhammer
Copy link

Just stumbled on this issue... would love to see a solution. ;)

@BodhiSukha
Copy link

Same here as @bforchhammer

@treyhunner
Copy link
Member

I do not plan to work on this issue as my development priorities have shifted away from field tracker.

For anyone who wants to try fixing this: pickling needs to be fixed while avoiding the problem addressed in #143 (see discussion of regression tests for that in #147).

@jeffreybrowning
Copy link

Seems like custom User models are ubiquitous enough that this issue is blocking for many django projects. Django's locmem cache uses pickling, so I can't pickle functions that return User querysets or specific User instances.

@Forever-Young
Copy link

As a workaround, I used dill (https://github.com/uqfoundation/dill) instead of pickle in my code (for storing a queryset into cache).

@MikeVL
Copy link

MikeVL commented Sep 27, 2016

Unable pickle model with FieldTracker:

tests/market/test_models.py:209: in test_market_offer_model_can_pickle
    pickle.dumps(self.market_offer)
/usr/lib/python2.7/pickle.py:1374: in dumps
    Pickler(file, protocol).dump(obj)
/usr/lib/python2.7/pickle.py:224: in dump
    self.save(obj)
/usr/lib/python2.7/pickle.py:331: in save
    self.save_reduce(obj=obj, *rv)
/usr/lib/python2.7/pickle.py:419: in save_reduce
    save(state)
/usr/lib/python2.7/pickle.py:286: in save
    f(self, obj) # Call unbound method with explicit self
/usr/lib/python2.7/pickle.py:649: in save_dict
    self._batch_setitems(obj.iteritems())
/usr/lib/python2.7/pickle.py:663: in _batch_setitems
    save(v)
/usr/lib/python2.7/pickle.py:286: in save
    f(self, obj) # Call unbound method with explicit self
/usr/lib/python2.7/pickle.py:748: in save_global
    (obj, module, name))
E   PicklingError: Can't pickle <function save at 0x7f9a0d358230>: it's not found as model_utils.tracker.save

@ghost
Copy link

ghost commented Mar 4, 2017

I have following which allows me to pickle while preserving post-save signal functionality.

`
def finalize_class(self, sender, **kwargs):

    super(PickleFieldTracker, self).finalize_class(sender, **kwargs)

    # Instead of patching the save method, we define the method
    # This will be called manually, since we want to control order of execution
    def handler(sender, instance, **kwargs):

        print "I am called"
        if not isinstance(instance, self.model_class):
            return

        update_fields = kwargs.get('update_fields')

        if not update_fields and update_fields is not None:  # () or []
            fields = update_fields
        elif update_fields is None:
            fields = None
        else:
            fields = (
                field for field in update_fields if
                field in self.fields
            )

        getattr(instance, self.attname).set_saved_fields(
            fields=fields
        )

    # We want to send our signal last, else post save signals would not work
    # We do not want to patch save, since that would not allow us to pickle the model.
    original_send = post_save.send

    def send(**kwargs):

        # All original Post Save Signals are received
        ret = original_send(**kwargs)

        # Call our own function
        resp = handler(**kwargs)
        ret.append((handler, resp))
        return ret

    post_save.send = send

def initialize_tracker(self, sender, instance, **kwargs):
    if not isinstance(instance, self.model_class):
        return  # Only init instances of given model (including children)
    tracker = self.tracker_class(instance, self.fields, self.field_map)
    setattr(instance, self.attname, tracker)
    tracker.set_saved_fields()

`

I just feel it is hackish to play around with signals execution order, so have not created PR it. If this seems fine, I will create PR for it.

@nomarek
Copy link

nomarek commented Jun 27, 2017

@csimple: In your solution, behavior of the field tracker in a save method is a little different than before. Currently, if you write:

class Foo(models.Model):
    bar = models.CharField(_('bar'), max_length=64)
    field_tracker = FieldTracker(fields=['bar'])

    def save(self, *args, **kwargs):
        print(1, self.field_tracker.has_changed('bar'))
        super().save(*args, **kwargs)
        print(2, self.field_tracker.has_changed('bar'))

obj = Foo.objects.create(bar='bar')
obj.bar = 'new_bar'
obj.save()

You will get:

1 True
2 True

But if you run it using PickleFieldTracker proposed by @csimple, you will get:

1 True
2 False

I am wondering which behavior is the correct one.

@ghost
Copy link

ghost commented Jun 28, 2017

@nomarek Since post save signals also return True (it returns True for both original as well as my solution), I think original behaviour was correct.

Which means my earlier assumption that this could be fixed by patching signal is not correct, and we are back to square one.

@ojomio
Copy link

ojomio commented Aug 8, 2017

Guys, I was so happy when found this repo, apparently, as much as I got disappointed when I discovered this bug.
It makes this library absolutely useless for us, because we use caching extensively. I very much appreciate if someone could fix this issue, because I spent a couple of hours but didn't figure it out myself :(

@aldenjenkins
Copy link

I also would very much benefit from being able to add an object which has a field_tracker field to my redis cache! Can't do it without being able to pickle it

@cyberbudy
Copy link

+1

@tumb1er
Copy link
Contributor

tumb1er commented Dec 15, 2019

Since 3.2.0 FieldTracker patches Model.save instead of instance.save, this should not be an issue anymore.

@Mogost Mogost closed this as completed Nov 27, 2023
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