Fix multiple versioned pickle wrappers

[corranwebster]

Apptools provides the sweet_pickle subpackage to handle versioning of pickled objects to allow unpickling of objects after changes to the class structure.

The persistence subpackage has a slightly different focus: it is more about extracting state as a dictionary, but it has similar themes.

Mayavi uses persistence, but it also uses apptools.naming which internally uses sweet_pickle, so we cannot simply deprecate sweet_pickle.

[aaronayres35]

persistence coverage report:

sweet_pickle coverage report:

[aaronayres35]

The following are all the uses of sweet_pickle in naming:

apptools/apptools/naming/object_serializer.py

Line 25 in a693c79

import apptools.sweet_pickle as sweet_pickle

apptools/apptools/naming/object_serializer.py

Line 59 in a693c79

obj = sweet_pickle.load(f)

apptools/apptools/naming/object_serializer.py

Line 89 in a693c79

sweet_pickle.dump(obj, f, 1)

Looking at sweet_pickle.load and sweet_pickle.dump more closely:

apptools/apptools/sweet_pickle/__init__.py

Lines 112 to 113 in a693c79

	def load(file, max_pass=-1):
	return Unpickler(file).load(max_pass)

The Unpickler here is apptools.sweet_pickle.versioned_unpickler.VersionedUnpickler which inherits the load method from NewUnpickler here:

apptools/apptools/sweet_pickle/versioned_unpickler.py

Lines 112 to 131 in a693c79

	def load(self, max_pass=-1):
	"""Read a pickled object representation from the open file.
	Return the reconstituted object hierarchy specified in the file.
	"""
	# List of objects to be unpickled.
	self.objects = []
	# We overload the load_build method.
	dispatch = self.dispatch
	dispatch[BUILD[0]] = NewUnpickler.load_build
	# call the super class' method.
	ret = Unpickler.load(self)
	self.initialize(max_pass)
	self.objects = []
	# Reset the Unpickler's dispatch table.
	dispatch[BUILD[0]] = Unpickler.load_build
	return ret

Note this is identical to the same method on the apptools.persistence.versioned_unpickler.VersionUnpickler class which inherits the load method from VersionedUnpickler here:

apptools/apptools/persistence/versioned_unpickler.py

Lines 37 to 56 in a693c79

	def load(self, max_pass=-1):
	"""Read a pickled object representation from the open file.
	Return the reconstituted object hierarchy specified in the file.
	"""
	# List of objects to be unpickled.
	self.objects = []
	# We overload the load_build method.
	dispatch = self.dispatch
	dispatch[BUILD[0]] = NewUnpickler.load_build
	# call the super class' method.
	ret = Unpickler.load(self)
	self.initialize(max_pass)
	self.objects = []
	# Reset the Unpickler's dispatch table.
	dispatch[BUILD[0]] = Unpickler.load_build
	return ret

So it seems this could easily be replaced to use persistence instead.

Likewise, looking closer at dump:

apptools/apptools/sweet_pickle/__init__.py

Lines 126 to 131 in a693c79

	# Implement the dump and dumps methods so that all traits in a HasTraits object
	# get included in the pickle.
	def dump(obj, file, protocol=2):
	_flush_traits(obj)
	from six.moves.cPickle import dump as d
	return d(obj, file, protocol)

It seems the only thing sweet_pickle provides over six.moves.cPickle is the _flush_traits function:

apptools/apptools/sweet_pickle/__init__.py

Lines 161 to 168 in a693c79

	def _flush_traits(obj):
	if hasattr(obj, 'trait_names'):
	for name, value in obj.traits().items():
	if value.type == 'trait':
	try:
	getattr(obj, name)
	except AttributeError:
	pass

However, this docstring for a function of the same name in persistence.StatePickler suggests this isn't needed:

apptools/apptools/persistence/state_pickler.py

Lines 316 to 321 in a693c79

	def _flush_traits(self, obj):
	"""Checks if the object has traits and ensures that the traits
	are set in the `__dict__` so we can pickle it.
	"""
	# Not needed with Traits3.
	return

There are also other cases where apptools.naming imports: import six.moves.cPickle as pickle.

Overall it seems to me that sweet_pickle can very easily be replaced by persistence / six.moves.cPickle in naming. This is the only use of sweet_pickle inside apptools aside from the apptools.sweet_pickle subpackage itself. Additionally, it seems sweet_pickle is not used by any other active projects. With that it seems we should be fine to make the replacement in naming and then kill sweet_pickle altogether?

[aaronayres35]

Looking at using persistence instead of sweet_pickle to call load

So it seems this could easily be replaced to use persistence instead.

I tried this and there was actually one other minor problem. In persistence, NewUnpickler subclasses Unpickler which comes from from pickle import *. On the other hand, sweet_pickle does the following:

apptools/apptools/sweet_pickle/versioned_unpickler.py

Lines 25 to 28 in a693c79

	if sys.version_info[0] >= 3:
	from pickle import _Unpickler as Unpickler
	else:
	from pickle import Unpickler

As a result, when running the naming test suite there is a failure with the traceback that ends in:

File "/Users/aayres/Desktop/apptools/apptools/persistence/versioned_unpickler.py", line 46, in load
    dispatch = self.dispatch
AttributeError: 'VersionedUnpickler' object has no attribute 'dispatch'

Since we are soon to drop support for python 2, it seems we will always want _Unpickler here anyway, so I went ahead and added from pickle import _Unpickler as Unpickler at the top of the persistence.unversioned_pickler.py file and after that change all the tests pass.