Replace use of sweet_pickle in naming, and delete sweet_pickle subpackage #199

aaronayres35 · 2020-11-10T20:12:08Z

fixes #133

This PR very possibly is biting the bullet too early on this, so I am happy to hold off on merging or close if need be. (we might not want to kill sweet_pickle!)

sweet_pickle was previously only used in a very minor way in apptools.naming, and was not used anywhere else in apptools. Additionally, AFAIK it is not used in other projects currently (this might not be the case in which case we can simply close this PR). This PR replaces its use in apptools.naming and deletes the sweet_pickle subpackage.

For more detailed commentary on this see #133 and specifically these comments: #133 (comment) #133 (comment)

It was also mentioned in the EEP that in envisage "The single_project plugin uses sweet_pickle and naming.". However, single_project was removed by enthought/envisage#331

Checklist

Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

Also, we could do a quick scream test on slack like we did for apptools.lru_cache.

…n apptools.naming to use apptools.persistence or pickle from the standard library instead

…ools into kill-sweet-pickle

kitchoi

Persistence is tricky because we also need to think about files that have been created in the past. Although pickling is not meant for long-term persistence, we still need to think about them...

Looks like Mayavi uses of apptools.naming (which uses sweet_pickle) is mostly transient: It is about binding variables to the Python shell.

As for the changes in apptools.naming.object_serializer, it seems those lines are indirectly covered by tests exercising apptools.naming.pyfs_context or apptools.naming.context._bind, and I can't find a test to ensure that round-tripping is working correctly, i.e. what is saved by ObjectSerializer.save can be loaded by ObjectSerializer.load correctly.

So I added the following test, which passes on master:

New test for roundtripping

# (C) Copyright 2005-2020 Enthought, Inc., Austin, TX
# All rights reserved.
#
# This software is provided without warranty under the terms of the BSD
# license included in LICENSE.txt and may be redistributed only under
# the conditions described in the aforementioned license. The license
# is also available online at http://www.enthought.com/licenses/BSD.txt
#
# Thanks for using Enthought open source!
import os
import shutil
import tempfile
import unittest

from traits.api import cached_property, HasTraits, Property, Str, Event

from apptools.naming.api import ObjectSerializer


class FooWithTraits(HasTraits):
    """ Dummy HasTraits class for testing ObjectSerizalizer."""

    full_name = Str()

    last_name = Property(depends_on="full_name")

    event = Event()

    @cached_property
    def _get_last_name(self):
        return self.full_name.split(" ")[-1]

class TestObjectSerializer(unittest.TestCase):

    def setUp(self):
        self.tmpdir = tempfile.mkdtemp()
        self.addCleanup(shutil.rmtree, self.tmpdir)
        self.tmp_file = os.path.join(self.tmpdir, "tmp.pickle")

    def test_save_load_roundtrip(self):
        # Test HasTraits objects can be serialized and deserialized as expected
        obj = FooWithTraits(full_name="John Doe")

        serializer = ObjectSerializer()
        serializer.save(self.tmp_file, obj)

        self.assertTrue(serializer.can_load(self.tmp_file))
        deserialized = serializer.load(self.tmp_file)

        self.assertIsInstance(deserialized, FooWithTraits)
        self.assertEqual(deserialized.full_name, "John Doe")
        self.assertEqual(deserialized.last_name, "Doe")

But it fails on this branch:

======================================================================
ERROR: test_save_load_roundtrip (apptools.naming.tests.test_object_serializer.TestObjectSerializer)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kchoi/Work/ETS/apptools/apptools/naming/tests/test_object_serializer.py", line 48, in test_save_load_roundtrip
    deserialized = serializer.load(self.tmp_file)
  File "/Users/kchoi/Work/ETS/apptools/apptools/naming/object_serializer.py", line 58, in load
    obj = VersionedUnpickler(f).load()
  File "/Users/kchoi/Work/ETS/apptools/apptools/persistence/versioned_unpickler.py", line 37, in load
    ret = Unpickler.load(self)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py", line 1210, in load
    dispatch[key[0]](self)
TypeError: 'classmethod' object is not callable

Something still needs fixing?

apptools/persistence/versioned_unpickler.py

apptools/naming/object_serializer.py

aaronayres35 · 2020-11-18T21:38:30Z

Note there is this integration test for VersionedUnpickler: https://github.com/enthought/apptools/blob/master/integrationtests/persistence/test_persistence.py

(I'm simply linking this because I saw it existed, it is very possibly irrelevant / we may just want to delete it)

aaronayres35 · 2020-11-20T21:45:30Z

So I added the following test, which passes on master:

New test for roundtripping
But it fails on this branch:

======================================================================
ERROR: test_save_load_roundtrip (apptools.naming.tests.test_object_serializer.TestObjectSerializer)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/kchoi/Work/ETS/apptools/apptools/naming/tests/test_object_serializer.py", line 48, in test_save_load_roundtrip
    deserialized = serializer.load(self.tmp_file)
  File "/Users/kchoi/Work/ETS/apptools/apptools/naming/object_serializer.py", line 58, in load
    obj = VersionedUnpickler(f).load()
  File "/Users/kchoi/Work/ETS/apptools/apptools/persistence/versioned_unpickler.py", line 37, in load
    ret = Unpickler.load(self)
  File "/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/pickle.py", line 1210, in load
    dispatch[key[0]](self)
TypeError: 'classmethod' object is not callable

Something still needs fixing?

So, I was able to get this test (and all the tests) to pass on this branch, by simply removing the @classmethod decorator I had previously added to the load_build method on VersionedUnpickler. I had originally added it to match the VersionedUnpickler from sweet_pickle and to follow what was said in the comment:

apptools/apptools/persistence/versioned_unpickler.py

Lines 93 to 104 in a693c79

    
           # Make this a class method since dispatch is a class variable. 
        
           # Otherwise, supposing the initial sweet_pickle.load call (which would 
        
           # have overloaded the load_build method) makes a pickle.load call at some 
        
           # point, we would have the dispatch still pointing to 
        
           # NewPickler.load_build whereas the object being passed in will be an 
        
           # Unpickler instance, causing a TypeError. 
        
           def load_build(cls, obj): 
        
               # Just save the instance in the list of objects. 
        
               if isinstance(obj, NewUnpickler): 
        
                   obj.objects.append(obj.stack[-2]) 
        
               Unpickler.load_build(obj) 
        
           load_build = classmethod(load_build)

It appears this code was copy pasted from sweet_pickle and then this decorator likely got removed because it was causing bugs somehow? In any case I am still quite confused as your test passes on master, where in sweet_pickle that @classmethod decorator is there, however, adding it here makes that test fail. I need to investigate more

kitchoi · 2020-11-23T11:02:13Z

It appears this code was copy pasted from sweet_pickle and then this decorator likely got removed because it was causing bugs somehow? In any case I am still quite confused as your test passes on master, where in sweet_pickle that @classmethod decorator is there, however, adding it here makes that test fail. I need to investigate more

I think that's because this line wasn't removed after @classmethod was added:

apptools/apptools/persistence/versioned_unpickler.py

Line 96 in 6de956b

load_build = classmethod(load_build)

This line is equivalent to adding @classmethod to def load_build, but we don't need both.

kitchoi · 2020-11-23T18:35:58Z

@aaronayres35 The roundtripping test above is quite useful for making sure the changes in this PR has not broken object serializer (in the most trivial ways at least). I think it would be good to add the test as part of this PR, it will also serve as future reference as to what extent the object serializer remains to work.

I saw that you have removed apptools/sweet_pickle/tests/__init__.py. While the tests in that folder are not discovered and run by unittest discover any more, the test files are still in the folders. It would be a strong proof to the claim "users of sweet_pickle can transition to using apptools.persistance and pickle from the python standard library" if we can convert the existing tests in sweet_pickle to use persistence, what do you think?

aaronayres35 · 2020-11-23T18:47:31Z

@aaronayres35 The roundtripping test above is quite useful for making sure the changes in this PR has not broken object serializer (in the most trivial ways at least). I think it would be good to add the test as part of this PR, it will also serve as future reference as to what extent the object serializer remains to work.

Yeah that makes sense, I will add the test to this PR and have it sit in naming as it is intended for ObjectSerializer

I saw that you have removed apptools/sweet_pickle/tests/__init__.py. While the tests in that folder are not discovered and run by unittest discover any more, the test files are still in the folders. It would be a strong proof to the claim "users of sweet_pickle can transition to using apptools.persistance and pickle from the python standard library" if we can convert the existing tests in sweet_pickle to use persistence, what do you think?

Somehow when merging / addressing conflicts I must have made a mistake and readded sweet_pickle files. I had intended to remove the entire sub package in this PR. What you said does make sense though as far as supporting the "users of sweet_pickle can transition to using apptools.persistance and pickle from the python standard library" claim.
But in that case where should these tests sit? Should we leave a basically empty sweet_pickle package keeping the old tests? Should I move the tests into persistence after they have been modified (I think that would make sense)

kitchoi · 2020-11-23T18:56:53Z

Somehow when merging / addressing conflicts I must have made a mistake and readded sweet_pickle files. I had intended to remove the entire sub package in this PR.

I see! That's probably why I did not see them when I checked out the branch and now they reappear.

Should I move the tests into persistence after they have been modified (I think that would make sense)

Adding them to persistence would make sense to me too.

…pickle and remove test_global registry

aaronayres35 · 2020-11-23T22:15:36Z

Note sweet_pickle is also used in codetools, but all that is used is sweet_pickle.load and sweet_pickle.dump which can be replaced as is done here in apptools.naming.

aaronayres35 · 2020-11-23T22:30:39Z

I saw that you have removed apptools/sweet_pickle/tests/__init__.py. While the tests in that folder are not discovered and run by unittest discover any more, the test files are still in the folders. It would be a strong proof to the claim "users of sweet_pickle can transition to using apptools.persistance and pickle from the python standard library" if we can convert the existing tests in sweet_pickle to use persistence, what do you think?

Some of the sweet_pickle tests are a bit sweet_pickle specific. For example, the tests in
https://github.com/enthought/apptools/blob/master/apptools/sweet_pickle/tests/test_updater.py
basically each just test each specific method on the Updater class. persistence also has an Updater class but it is definitely not the same. It seems they go about doing things in different ways. Some of these tests I'm not sure how to directly modify to use persistence.

In fact, I am not confident that persistence can replace all of the functionality provided by sweet_pickle. I am confident thought that it can replace all the functionality of sweet_pickle that I have seen used. Which at this point all I have really seen is calls to sweet_pickle.load, sweet_pickle.loads, sweet_pickle.dump, and sweet_pickle.dumps, which can be replaced like they are in this PR in apptools.naming

kitchoi · 2020-11-24T11:06:56Z

In fact, I am not confident that persistence can replace all of the functionality provided by sweet_pickle. I am confident thought that it can replace all the functionality of sweet_pickle that I have seen used. Which at this point all I have really seen is calls to sweet_pickle.load, sweet_pickle.loads, sweet_pickle.dump, and sweet_pickle.dumps, which can be replaced like they are in this PR in apptools.naming

I see. Let's try to convert the tests that can be converted. Then we can more confidently say which functionality can be migrated and which are being discarded.

aaronayres35 · 2020-11-24T18:18:40Z

apptools/sweet_pickle/tests/test_class_mapping.py

-    def test_infinite_loop_detection(self):
-        """Validates that the class mapping framework detects infinite
-        loops of class mappings.
-        """
-        # Add mappings to the registry
-        self.registry.add_mapping_to_class(Foo.__module__, Foo.__name__, Bar)
-        self.registry.add_mapping_to_class(Bar.__module__, Bar.__name__, Baz)
-        self.registry.add_mapping_to_class(Baz.__module__, Baz.__name__, Foo)
-
-        # Validate that an exception is raised when trying to unpickle an
-        # instance anywhere within the circular definition.
-        def fn(o):
-            sweet_pickle.loads(sweet_pickle.dumps(o))
-
-        self.assertRaises(sweet_pickle.UnpicklingError, fn, Foo())
-        self.assertRaises(sweet_pickle.UnpicklingError, fn, Bar())
-        self.assertRaises(sweet_pickle.UnpicklingError, fn, Baz())


I believe cycles aren't possible with how persistence is set up, so I deleted this test. The following test I modified slightly as there is only one transition that can occur. ie with persistence you can't go Foo->Bar->Baz in one roundtrip

aaronayres35 · 2020-11-24T19:56:54Z

apptools/persistence/tests/test_state_function.py

-    def test_unpickled_chain_functionality(self):
-        """Validates that the registered state functions are used when
-        unpickling.
-        """


I believe it may be possible to modify this test, to get a similar flavor of test for apptools.persistence by using the setstates attribute on an Updater instance that is passed to a VersionedUnpickler. Unfortunately I was unable to get it to work atm.

I looked into this briefly... IMO, it looks evil! I opened #238.

aaronayres35 · 2020-11-24T20:04:04Z

Orthogonal to this PR, but I think apptools could really benefit from improved docstrings (numpy style). Obviously documentation could be improved in general, but I find myself spending a lot of time just trying to understand what arguments are, etc. For example when workng on this PR I noticed that the apptools.persistence.updater.Updater class definition never mentions a setstates attribute. See here:

apptools/apptools/persistence/updater.py

Lines 9 to 36 in d794275

    
           class Updater: 
        
               """An abstract class to provide functionality common to the updaters.""" 
        
               def get_latest(self, module, name): 
        
                   """The refactorings dictionary contains mappings between old and new 
        
                   module names.  Since we only bump the version number one increment 
        
                   there is only one possible answer. 
        
                   """ 
        
                   if hasattr(self, "refactorings"): 
        
                       module = self.strip(module) 
        
                       name = self.strip(name) 
        
                       # returns the new module and name if it exists otherwise defaults 
        
                       # to using the original module and name 
        
                       module, name = self.refactorings.get( 
        
                           (module, name), (module, name) 
        
                       ) 
        
                   return module, name 
        
               def strip(self, string): 
        
                   # Who would have thought that pickle would pass us 
        
                   # names with \013 on the end? Is this after the files have 
        
                   # manually edited? 
        
                   if ord(string[-1:]) == 13: 
        
                       return string[:-1] 
        
                   return string

It is however used here:

apptools/apptools/persistence/versioned_unpickler.py

Line 157 in d794275

fn = self.updater.setstates.get((module, name), False)

I discovered it here:

apptools/integrationtests/persistence/update1.py

Lines 39 to 41 in d794275

    
           self.setstates = { 
        
               ("cplab.project", "Project"): update_project 
        
           }

docstrings would save a lot of time spent trying to follow the code

kitchoi

Thank you very much for converting the tests, it really helps us to understand what can or cannot be migrated!
Some suggestions for the tests and the news fragment given the new information. The rest looks good to me.

docs/releases/upcoming/199.removal.rst

apptools/persistence/tests/test_two_stage_unpickler.py

apptools/persistence/tests/test_state_function.py

kitchoi · 2020-11-25T10:33:46Z

apptools/persistence/tests/test_state_function.py

-    def test_unpickled_chain_functionality(self):
-        """Validates that the registered state functions are used when
-        unpickling.
-        """


I looked into this briefly... IMO, it looks evil! I opened #238.

kitchoi

LGTM

aaronayres35 and others added 4 commits November 10, 2020 14:06

delete sweet_pickle subpackage and references to it. Update its use i…

53a9a72

…n apptools.naming to use apptools.persistence or pickle from the standard library instead

Merge branch 'master' into kill-sweet-pickle

cbfe318

adding news fragment

71bba6e

Merge branch 'kill-sweet-pickle' of https://github.com/enthought/appt…

37b1c16

…ools into kill-sweet-pickle

kitchoi self-requested a review November 16, 2020 14:26

kitchoi reviewed Nov 16, 2020

View reviewed changes

apptools/persistence/versioned_unpickler.py Show resolved Hide resolved

apptools/naming/object_serializer.py Show resolved Hide resolved

remove @classmethod on load_build

d8f436a

aaronayres35 added 2 commits November 20, 2020 16:00

Merge branch 'master' into kill-sweet-pickle

4c52201

finish merge

6de956b

aaronayres35 mentioned this pull request Nov 23, 2020

VersionedUnpickler depends on private implementation details of _Unpickler #232

Open

Merge branch 'master' into kill-sweet-pickle

ae85123

aaronayres35 added 3 commits November 23, 2020 13:11

add roundtrip test for object serializer

9e4f029

rewrite test_two_stage_unpickler to use persistence instead of sweet …

084d27d

…pickle and remove test_global registry

flake8

28f236e

convert test_class_mapping to use persistence

ba44f1c

aaronayres35 commented Nov 24, 2020

View reviewed changes

aaronayres35 added 4 commits November 24, 2020 11:51

flake8

6b057bb

update test_state_function to use persistence

ef2cd50

delete apptools.sweet_pickle

595a7ef

flake8

c3d8f77

aaronayres35 commented Nov 24, 2020

View reviewed changes

aaronayres35 requested a review from kitchoi November 24, 2020 20:11

remove/update any references to sweet_pickle in the comments

91968f2

mdickinson mentioned this pull request Nov 25, 2020

Drop Python 2 support #120

Closed

kitchoi reviewed Nov 25, 2020

View reviewed changes

apply suggestions from code review

9d09a05

kitchoi approved these changes Nov 25, 2020

View reviewed changes

aaronayres35 merged commit 4d71eb5 into master Nov 25, 2020

aaronayres35 deleted the kill-sweet-pickle branch November 25, 2020 14:59

coreysharris mentioned this pull request Feb 12, 2021

Should PersistableMixin fall back to stdlib pickle library if apptools is not installed enthought/codetools#45

Closed

aaronayres35 mentioned this pull request Mar 16, 2021

Replace use of apptools sweetpickle enthought/codetools#48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace use of sweet_pickle in naming, and delete sweet_pickle subpackage #199

Replace use of sweet_pickle in naming, and delete sweet_pickle subpackage #199

aaronayres35 commented Nov 10, 2020 •

edited

Loading

kitchoi left a comment

aaronayres35 commented Nov 18, 2020

aaronayres35 commented Nov 20, 2020

kitchoi commented Nov 23, 2020

kitchoi commented Nov 23, 2020

aaronayres35 commented Nov 23, 2020

kitchoi commented Nov 23, 2020

aaronayres35 commented Nov 23, 2020

aaronayres35 commented Nov 23, 2020

kitchoi commented Nov 24, 2020

aaronayres35 Nov 24, 2020

aaronayres35 Nov 24, 2020

kitchoi Nov 25, 2020

aaronayres35 commented Nov 24, 2020

kitchoi left a comment

kitchoi Nov 25, 2020

kitchoi left a comment

Replace use of sweet_pickle in naming, and delete sweet_pickle subpackage #199

Replace use of sweet_pickle in naming, and delete sweet_pickle subpackage #199

Conversation

aaronayres35 commented Nov 10, 2020 • edited Loading

kitchoi left a comment

Choose a reason for hiding this comment

aaronayres35 commented Nov 18, 2020

aaronayres35 commented Nov 20, 2020

kitchoi commented Nov 23, 2020

kitchoi commented Nov 23, 2020

aaronayres35 commented Nov 23, 2020

kitchoi commented Nov 23, 2020

aaronayres35 commented Nov 23, 2020

aaronayres35 commented Nov 23, 2020

kitchoi commented Nov 24, 2020

aaronayres35 Nov 24, 2020

Choose a reason for hiding this comment

aaronayres35 Nov 24, 2020

Choose a reason for hiding this comment

kitchoi Nov 25, 2020

Choose a reason for hiding this comment

aaronayres35 commented Nov 24, 2020

kitchoi left a comment

Choose a reason for hiding this comment

kitchoi Nov 25, 2020

Choose a reason for hiding this comment

kitchoi left a comment

Choose a reason for hiding this comment

aaronayres35 commented Nov 10, 2020 •

edited

Loading