Add cloudpickle serialization support

[Peque]

A very basic implementation. Just to know what you think about this and to see what Travis says as well. 😉

[Peque]

It seems it fails. Only 2 errors:

• test_serialize:124
• test_serialize:386

Works with Python 2.7 and PyPy. Surprisingly, it seems to work with Python 3.4.4 as well. 😕

[Peque]

To reproduce test_serialize:124 error:

import cloudpickle
import Pyro4


proxy = Pyro4.core.Proxy("PYRO:9999@host.com:4444")
proxy._pyroTimeout = 2

print(cloudpickle.dumps(proxy))

Trackeback:

Traceback (most recent call last):
  File "/cache/src/Pyro4/src/Pyro4/core.py", line 505, in __pyroCreateConnection
    sock = socketutil.createSocket(connect=connect_location, reuseaddr=config.SOCK_REUSE, timeout=self.__pyroTimeout, nodelay=config.SOCK_NODELAY)
  File "/cache/src/Pyro4/src/Pyro4/socketutil.py", line 292, in createSocket
    sock.connect(connect)
socket.timeout: timed out

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "serialize124.py", line 9, in <module>
    print(cloudpickle.dumps(proxy))
  File "/cache/miniconda/envs/pyro4/lib/python3.6/site-packages/cloudpickle/cloudpickle.py", line 829, in dumps
    cp.dump(obj)
  File "/cache/miniconda/envs/pyro4/lib/python3.6/site-packages/cloudpickle/cloudpickle.py", line 233, in dump
    return Pickler.dump(self, obj)
  File "/cache/miniconda/envs/pyro4/lib/python3.6/pickle.py", line 409, in dump
    self.save(obj)
  File "/cache/miniconda/envs/pyro4/lib/python3.6/pickle.py", line 521, in save
    self.save_reduce(obj=obj, *rv)
  File "/cache/miniconda/envs/pyro4/lib/python3.6/site-packages/cloudpickle/cloudpickle.py", line 697, in save_reduce
    if hasattr(obj, '__transient__'):
  File "/cache/src/Pyro4/src/Pyro4/core.py", line 272, in __getattr__
    self._pyroGetMetadata()
  File "/cache/src/Pyro4/src/Pyro4/core.py", line 587, in _pyroGetMetadata
    self.__pyroCreateConnection()
  File "/cache/src/Pyro4/src/Pyro4/core.py", line 539, in __pyroCreateConnection
    raise ce
Pyro4.errors.CommunicationError: cannot connect to ('host.com', 4444): timed out

It seems cloudpickle is trying to check if hasattr(obj, '__transient__'), and then Pyro gets stuck when calling self._pyroGetMetadata(), as it ends up calling self.__pyroCreateConnection() (resulting in a timeout as expected, as the proxy address is just a dummy address).

Do you think this is a cloudpickle problem? (i.e.: they should not be trying to check if hasattr(obj, '__transient__') Or could it be fixed in Pyro4?

[Peque]

The other error in test_serialize:386 seems to be the same (I do not think it is actually related to the circular reference test, but only to the proxy serialization problem):

import cloudpickle                                                                  
import Pyro4                                                                        
                                                                                    
                                                                                    
data = [42, "hello", Pyro4.core.Proxy("PYRO:dummy@dummy:4444")]                     
data.append(data)                                                                   
                                                                                    
print(cloudpickle.dumps(data))

[irmen]

I'm not familiar with cloudpickle nor the __transient__ dunder attribute it is using. What is it used for? Pyro doesn't recognise it as one of the "don't access this attribute remotely" attributes so it is indeed trying to fetch the value from the remote object (which isn't connected in the test case...)

[irmen]

By the way, you can tell Pyro to avoid looking up the metadata by setting the METADATA config item to False. Perhaps that allows the test to complete.

[Peque]

@irmen I think __transient__ is not a standard Python dunder attribute. For what I have seen in cloudpickle's source they use it in this way: if there is a __transient__ attribute, then all items in this attribute will be removed from __dict__ before serialization.

Would it make sense to consider that a special case in Pyro4? Or would you rather avoid it?

Cross reference: cloudpipe/cloudpickle#108

[Peque]

@irmen By the way, it seems Pyro4.config.METADATA = False:

import cloudpickle
import Pyro4


Pyro4.config.METADATA = False
proxy = Pyro4.core.Proxy("PYRO:9999@host.com:4444")
proxy._pyroTimeout = 2

print(cloudpickle.dumps(proxy))

Leads to another issue:

Traceback (most recent call last):
  File "asdf.py", line 9, in <module>
    print(cloudpickle.dumps(proxy))
  File "/cache/miniconda/envs/python/lib/python3.6/site-packages/cloudpickle/cloudpickle.py", line 829, in dumps
    cp.dump(obj)
  File "/cache/miniconda/envs/python/lib/python3.6/site-packages/cloudpickle/cloudpickle.py", line 233, in dump
    return Pickler.dump(self, obj)
  File "/cache/miniconda/envs/python/lib/python3.6/pickle.py", line 409, in dump
    self.save(obj)
  File "/cache/miniconda/envs/python/lib/python3.6/pickle.py", line 521, in save
    self.save_reduce(obj=obj, *rv)
  File "/cache/miniconda/envs/python/lib/python3.6/site-packages/cloudpickle/cloudpickle.py", line 699, in save_reduce
    state = state.copy()
AttributeError: 'tuple' object has no attribute 'copy'

The strange thing is that cloudpickle's state = state.copy() is inside an if statement which checks the __transient__ attribute existence, so I am not sure why it does even get there, because I guess Pyro4 is not, at least intentionally, ever setting that attribute.

Then the problem seems to be that when that is the case, it assumes state is a dictionary, not a tuple, because you will be actually modifying the state (i.e.: remove) according to the __transient__ values.

Have not investigate this further.

[Peque]

@irmen Yeah, it seems hasattr(proxy, '__transient__')) returns always True:

import Pyro4


Pyro4.config.METADATA = False
proxy = Pyro4.core.Proxy("PYRO:9999@host.com:4444")

print(hasattr(proxy, '__transient__'))

It actually returns True for anything you put on the right ('__transient__', 'asdf', ...).

Is that expected?

[Peque]

Maybe setting Pyro4.config.METADATA = False is not the best solution anyway, as serialization would still fail in the user code (unless they set that to False as well, which I do not know if may cause other issues or lead to unexpected behavior).

Perhaps adding a special case in Pyro4 is the easiest way to fix it (i.e.: set __transient__ to be a special case if cloudpickle is importable or even only if it is set in the accepted serializers set).

Would that be okay for you? Where would you suggest to add that fix/special case?

[irmen]

When metadata=off it is expected that hasattr(proxy, "something") always returns true. This is because Pyro can't know if the remote object actually has the requested attribute or not, so it has to pretend that the proxy has "all" attributes. Hence the metadata that is normally enabled - that fixes this issue.

For the AttributeError: 'tuple' object has no attribute 'copy' in cloudpickle, this I consider a bug in cloudpickle. It seems they assume __getstate__ always returns a dict. Which is not required: when you override __getstate__ AND __setstate__ (what Pyro does) you're free to use some other type, a tuple in Pyro's case. This is written in the Python docs for __setstate__. I think there's an easy fix for them, use the copy module rather than directly invoking the copy method.

Finally, it's weird they have something going on with __transient__ if the same can be achieved with properly returning from a __getstate__ just the attributes that you want to be pickled. I don't see the need for a non-standard dunder attribute.

I prefer not to make a special case just for __transient__ in Pyro. If anything happens in Pyro I'd like it to be a way to extend the set of special attributes, but ewwww.

Just for giggles, try adding "__transient__" to Pyro4.core.Proxy.__pyroAttributes and see if that makes it work.?

[Peque]

Yeah, it seems adding __transient__ to Pyro4.core.Proxy.__pyroAttributes fixes it. 😅

Here is the question/discussion I opened in cloudpickle. In case you want to follow it or add something.

[irmen]

@Peque yeah the unit test suite is not stable anymore on travis, I've seen these errors before (not enough workers) Often kicking travis and letting it run te tests again fixes it 50/50 chance. I haven't had the time to look into this yet.

[Peque]

Deleted comments related to the Travis errors (non-related to the cloudpickle integration) and opened a new issue #180. I will delete this one as well if you delete your last comment. 😉

Just to keep this discussion a bit cleaner...

[Peque]

We can wait to see if the cloudpickle team has something to say about this.

If they do not, it is up to you to decide whether or not this is to be merged. Maybe it could be merged like dill was (opening an issue to undo the "Fix cloudpickle serialization support" commit when there is a fix in cloudpickle). At least in this case, as opposed to the dill case, there is only one line that changes with the workaround. 😅

[irmen]

Yes it is a fairly trivial change in Pyro, but I'd like to know the rationale for that transient attribute. And perhaps it's fixable in cloudpickle, which avoids a third-party-lib-workaround in Pyro (no matter how tiny)

[Peque]

Seems they merged cloudpipe/cloudpickle#110 into cloudpickle, so we only need to wait for next release. 😊

[irmen]

Great news. And thanks a lot btw to take the time to also update the documentation in this PR

[irmen]

I guess we need to bump this to 0.3.2 to have their fix of removing the __transient__ attribute

[Peque]

Definitely. Will wait for next release to set the appropriate version (they might bump it to 0.4.0).

Maybe they do a release in the future 0.3.2 with just a couple of fixes to the old version but without breaking the transient support, so I think we should better wait to know the actual version number... 😉

[irmen]

good point, let's wait on an official number

[irmen]

we can drop this attribute here again

[Peque]

Sure. Once the new cloudpickle version is released, I will revert this as well to check Travis is happy with everything too. Might do it in new commits to make review easier for you, then I can squash everything in a single commit before you do the merge (don't think it makes sense to split this in more than 1 commit).

[irmen]

agree on the single commit, if possible :)

[irmen]

Seems like they released a new cloudpickle version, they bumped it to 0.4.0

[Peque]

Reverted the ugly "fix", bumped the minimum cloudpickle required version, squashed and rebased. Seems Travis is happy. 😊

[irmen]

Thanks a lot for this @Peque I'll add an entry to the changelog and perhaps try it out myself a little, maybe adding an example that utilizes a particular feature of cloudpickle (sending functions)

[Peque]

Thanks to you for merging! 😊