#127

i started working on this, but i didnt reach anything useful. the main problem is the very dynamic nature of how Dot methods are generated.

it's easy enough to write a __reduce__ method in Dot saving __dict__ and returning it; but if you do that, then you're stumble by how pickle is unable to pickle objects with lambda functions, which are used in create_attribute_methods (https://github.com/pydot/pydot/blob/master/pydot.py#L528-L543); you can convert those lambdas in property() getter/setter, but then pickle fails on how the write_/save_XXX methods are created at https://github.com/pydot/pydot/blob/master/pydot.py#L1717-L1736

at this point i gave up, i think a reorganization of pydot code is necessary to be able to pickle Dot objects

with this simple change i am able to serialize objects using dill:

--- pydot.py    2018-12-22 17:10:42.564229985 -0500
+++ pydot.py     2019-10-03 14:36:20.398075364 -0400
@@ -427,7 +427,11 @@
 
     def __setstate__(self, state):
 
-        self.obj_dict = state
+        self.obj_dict = state['obj_dict']
+
+    def __reduce__(self):
+        state = self.__dict__.copy()
+        return (Common, (), state, )
 
 
     def __get_attribute__(self, attr):
@@ -1743,7 +1747,11 @@
 
     def __setstate__(self, state):
 
-        self.obj_dict = state
+        self.obj_dict = state['obj_dict']
+
+    def __reduce__(self):
+        state = self.__dict__.copy()
+        return (Dot, (), state, )
 
 
     def set_shape_files(self, file_paths):

it would be helpful if we can include this in pydot

Hi @sandrotosi,

First of all, thank you for maintaining the Debian package for pydot. I use Debian myself, so I have used your package as well. I am currently preparing a pydot 1.4.2 bugfix release and after that will open the master branch for 2.0.0 development. I doubt if we can make a final 2.0.0 release before the next Debian 11 bullseye freeze early 2021 though, because there is still a lot of work to do and I want to combine all the API breaking changes in one release and not have to jump to 3.0.0 within the year again. But we will see, there is still time.

About the pickle changes I am proposing (see below), I am afraid those are too breaking/risky for a mere bugfix release 1.4.2, so I am only planning to merge them for 2.0.0. However, as it is now, the changes are still Python 2 compatible and could apply on the 1.x branch. OTOH, I see you already patched the Debian package with the change you proposed above, so maybe you don't mind keeping that in until 2.0.0 comes out.

i think a reorganization of pydot code is necessary to be able to pickle Dot objects

And that's what I did. Please check out #242. I think it should solve your #216 and #217, as well as the earlier #127.

I had a look at your solution (comment above) as well. It works because the tuple returned by __reduce__ instructs pickle to call the object constructor Dot() on unpickling. This means that Dot.__init__() and Graph.__init__() get run again, setting Dot.prog and Dot.shape_files and creating all the dynamically created methods for the instance. However, this goes against the idea that __init__ should only get run once and not get called again during unpickling. For links see b6758685. It only works on Dot objects, btw, because Common does not have an __init__, but that does not matter because few people would pickle a pure Node or Edge object anyway, I guess. Another thing to note is that the entire object dictionary is pickled (state = self.__dict__.copy()), which makes the pickled representations heavy with all those instance methods (22 KiB vs. 351 bytes for a graph with 1 node), but in the end they are not used because __setstate__ only uses ['obj_dict'] from it. The reason the methods still end up in the unpickled object is not because they were pickled, but because __init__ gets run again, as first described. Finally, the __getstate__ methods could just as well be deleted, because they are not called from __reduce__ as they normally should. If I am not mistaking, this code has the same effect, but results in smaller pickled object representations and then also works with pickle:

def __setstate__(self, state):
    self.obj_dict = state

def __reduce__(self):
    state = self.__dict__['obj_dict'].copy()
    return (Dot, (), state, )

Still, I hope you can also have a look at my proposal in #242. It does away with all those instance-bound methods, __getstate__, __setstate__ and __reduce__. Hope you can let me know what you think. Thanks again. :)

@sandrotosi: This is in reply to your report in #256 that with newly released pydot 1.4.2 you get this error when running the unittests:

ERROR: test_graph_pickling (__main__.TestGraphAPI)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/pydot-1.4.2/test/pydot_unittest.py", line 114, in test_graph_pickling
    pickle.dumps(g)
AttributeError: Can't pickle local object 'Common.create_attribute_methods.<locals>.<lambda>'

This test does not fail on plain pydot 1.4.2 as we distribute it here on GitHub and on PyPI.org, but only after applying the additional patch for the Debian package debian/patches/serialize-via-dill.patch, applied since Debian pydot 1.4.1-2. The test failure was happening with Debian pydot 1.4.1-2 already, which is why the test suite was disabled for Debian pydot 1.4.1-3.

I reviewed that patch a few months ago (comment of 2020-10-27 above), but did not get any reply. As I wrote back then, full pickle support is planned only for pydot 2.0.0 through PR #242. In the mean time, I see 4 different solutions for you:

1. Change the serialize-via-dill.patch as I suggested in PyDot objects are not pickleble #217 (comment). Keep serializing only obj_dict as before any patch and let __reduce__() call __init__() to ensure the dynamic methods are created on unpacking. Then it also works with pickle. Maybe rename it to something like serialize-with-init.patch:

   diff --git a/pydot.py b/pydot.py
   index e3f7c6a5..9fe030c9 100644
   --- a/pydot.py
   +++ b/pydot.py
   @@ -420,17 +420,14 @@ class Common(object):
        """
    
    
   -    def __getstate__(self):
   -
   -        dict = copy.copy(self.obj_dict)
   -
   -        return dict
   -
   -
        def __setstate__(self, state):
    
            self.obj_dict = state
    
   +    def __reduce__(self):
   +        state = self.__dict__['obj_dict'].copy()
   +        return (Common, (), state, )
   +
    
        def __get_attribute__(self, attr):
            """Look for default attributes for this node"""
   @@ -1746,16 +1743,14 @@ class Dot(Graph):
                name = 'write_{fmt}'.format(fmt=frmt)
                self.__setattr__(name, new_method)
    
   -    def __getstate__(self):
   -
   -        dict = copy.copy(self.obj_dict)
   -
   -        return dict
   -
        def __setstate__(self, state):
    
            self.obj_dict = state
    
   +    def __reduce__(self):
   +        state = self.__dict__['obj_dict'].copy()
   +        return (Dot, (), state, )
   +
    
        def set_shape_files(self, file_paths):
            """Add the paths of the required image files.
   diff --git a/test/pydot_unittest.py b/test/pydot_unittest.py
   index c2accef6..8b7cd532 100644
   --- a/test/pydot_unittest.py
   +++ b/test/pydot_unittest.py
   @@ -100,18 +100,38 @@ class TestGraphAPI(unittest.TestCase):
            self.assertEqual(g.get_subgraph_list()[0].get_name(),
                             s.get_name())
    
   -
        def test_graph_pickling(self):
    
   -
   -        g = pydot.Graph()
   +        g = pydot.Dot()
            s = pydot.Subgraph("foo")
            g.add_subgraph(s)
            g.add_edge( pydot.Edge('A','B') )
            g.add_edge( pydot.Edge('A','C') )
   -        g.add_edge( pydot.Edge( ('D','E') ) )
   +        g.add_edge( pydot.Edge('D','E') )
            g.add_node( pydot.Node( 'node!' ) )
   -        pickle.dumps(g)
   +        p = pickle.dumps(g)
   +        g_new = pickle.loads(p)
   +        self.assertEqual(g.create_png(prog=TEST_PROGRAM),
   +                         g_new.create_png(prog=TEST_PROGRAM))
   +        self.assertEqual(str(g), str(g_new))
   +        self.assertEqual(dir(g), dir(g_new))
   +        self.assertEqual(g.prog, g_new.prog)
   +        self.assertEqual(g.shape_files, g_new.shape_files)
   +        self.assertEqual(hasattr(g_new, 'get_bgcolor'), True)
   +        #self.assertIs(g.get_bgcolor.__func__, g_new.get_bgcolor.__func__)
   +        self.assertEqual(hasattr(g_new, 'set_dpi'), True)
   +        #self.assertIs(g.set_dpi.__func__, g_new.set_dpi.__func__)
   +        self.assertEqual(hasattr(g_new, 'create_dot'), True)
   +        #self.assertIs(g.create_dot.__func__, g_new.create_dot.__func__)
   +        self.assertEqual(hasattr(g_new, 'write_png'), True)
   +        #self.assertIs(g.write_png.__func__, g_new.write_png.__func__)
   +        self.assertEqual(hasattr(g_new, 'write_raw'), True)
   +        #self.assertIs(g.write_raw.__func__, g_new.write_raw.__func__)
   +        pickle.loads(pickle.dumps(pydot.Graph()))
   +        pickle.loads(pickle.dumps(pydot.Subgraph()))
   +        pickle.loads(pickle.dumps(pydot.Cluster()))
   +        pickle.loads(pickle.dumps(pydot.Edge()))
   +        pickle.loads(pickle.dumps(pydot.Node()))
    
        def test_unicode_ids(self):
    

   Notes:

   • As you can see, I also added some of the additional pickle tests from PR Full pickle support #242, but you can choose whether the include those or not. Both the original and the extended test pass with this patch on pydot 1.4.2 and Python 2.7.16 and 3.7.3.
   • However, this has not had any real world testing yet. I never even made a PR for it, because it was just a suggestion to you for improvement of your patch. I do not know what side effects it might have on other users' workflow.
   • Same as with your original patch, the dynamically generated methods set_*(), get_*() for Common-subclasses such as Node and Edge are not generated on unpacking, because that is done from the subclass __init__(), not from (the non-existent) Common.__init__(). As said, because __setstate__() still only uses the obj_dict, it also does not unpack these methods from the serialized object. Perhaps the __init__() problem can be solved by replacing return (Common, (), state, ) with something like return (type(self), (), state, ) or return (self.__class__, (), state, ), but I am absolutely not sure and did not even try that out yet.

2. Drop serialize-via-dill.patch and instead apply PR Full pickle support #242 already. I just rebased it to current master, which is still basically pydot 1.4.2, so you can take the patch from there. Test suite passes when applied to pydot 1.4.2 on both Python 2.7.16 and 3.7.3. However, again note that PR Full pickle support #242 has not had any real world testing yet. It contains a major change in how pydot creates its dynamically generated methods (create_*(), write_*(), set_*(), get_*()) and this has not been extensively tested. It has not even be merged to development master yet and I did not get any comments on it yet.

3. Keep your change the way it is, but then also change the test suite to test with dill instead of pickle. For example, new serialize-via-dill-incl-test.patch:

   diff --git a/pydot.py b/pydot.py
   index e3f7c6a5..8cedf3c4 100644
   --- a/pydot.py
   +++ b/pydot.py
   @@ -429,7 +429,11 @@ class Common(object):
    
        def __setstate__(self, state):
    
   -        self.obj_dict = state
   +        self.obj_dict = state['obj_dict']
   +
   +    def __reduce__(self):
   +        state = self.__dict__.copy()
   +        return (Common, (), state, )
    
    
        def __get_attribute__(self, attr):
   @@ -1754,7 +1758,11 @@ class Dot(Graph):
    
        def __setstate__(self, state):
    
   -        self.obj_dict = state
   +        self.obj_dict = state['obj_dict']
   +
   +    def __reduce__(self):
   +        state = self.__dict__.copy()
   +        return (Dot, (), state, )
    
    
        def set_shape_files(self, file_paths):
   diff --git a/test/pydot_unittest.py b/test/pydot_unittest.py
   index c2accef6..4a26b1bb 100644
   --- a/test/pydot_unittest.py
   +++ b/test/pydot_unittest.py
   @@ -10,7 +10,7 @@ import argparse
    from hashlib import sha256
    import io
    import os
   -import pickle
   +import dill
    import string
    import subprocess
    import sys
   @@ -111,7 +111,7 @@ class TestGraphAPI(unittest.TestCase):
            g.add_edge( pydot.Edge('A','C') )
            g.add_edge( pydot.Edge( ('D','E') ) )
            g.add_node( pydot.Node( 'node!' ) )
   -        pickle.dumps(g)
   +        dill.dumps(g)
    
        def test_unicode_ids(self):
    
   

   Note that this just makes the test suite pass (when applied to pydot 1.4.2, with both Python 2.7.16 and 3.7.3). However, my original objections described in PyDot objects are not pickleble #217 (comment) still apply:

   • It does not work with pickle.
   • It results in much larger serialized objects because the dynamically generated instance methods are serialized as well (even though they are not even used on unpacking).
   • Same comment as before applies that the dynamically generated methods set_*(), get_*() for Common-subclasses such as Node and Edge are not generated on unpacking. See above for a possible solution.
   • I think that __getstate__() is not used anymore now that you have __reduce__().

4. Accept that serialization support (pickle/dill) is lacking in pydot 1.x.x and drop the Debian-specific patch. Just plain pydot 1.4.2. Limited serialization support (dynamically generated methods not available after unpacking), but the test suite passes with both Python 2.7.16 and 3.7.3.

I am not sure which option is best for you. It depends on how you weigh the risk of potential regressions against the benefit of having more or less serialization support. Please be aware that I made a conscious choice to not incorporate any of these patches in pydot 1.4.2 yet, because I wanted it to be a bug-fix release with minimal, targeted bug fixes only, to reduce the risk of regressions.

For at least the next few weeks, my time will be limited due to other obligations. I hope to still be able to answer a quick question now and then, but doing any further testing or investigation (like I did for this comment) will not be possible in the next few weeks. Hope you understand and that you can find a good solution. Please let me know.

@sandrotosi Btw, if you choose that 3rd option (changing the test suite to use dill instead of pickle), you may need to add dill to your build dependencies.