Switch from dill to cloudpickle

[jan-janssen]

Primarily for consistency with our other developments

[pmrv]

Won't this break existing values dilled into HDF? At least we should have a fallback when cloud-unpickling fails.

[jan-janssen]

So far I have not found anything that does not work, two examples:

import dill
import cloudpickle
test_dict = {"a": 1, "b": 2}
cloudpickle.loads(dill.dumps(test_dict))

and

import dill
import cloudpickle
from ase.build import bulk
s = bulk("Al", cubic=True)
cloudpickle.loads(dill.dumps(s))

Still I agree this change is potentially dangerous and we should discuss very carefully when and how we want to do the migration. Nevertheless, given the advantages of pickle_by_value and pickle_by_reference that cloudpickle offers, I believe we have to do this switch sooner or later.

[pmrv]

Ah, I wasn't aware that they are interoperable. Is that something that they promise or does it just happen to be? Either way, moving to cloudpickle is a-ok with me, as long as we put a backwards test in.

[niklassiemer]

Would it harm to

try:
    cloudpickle.loads(hdf_item)
except (IDontKnowWhichError):
    dill.loads(hdf_item)

for a while? In each case I agree, it is good to use one framework to do such jobs :)

[pmrv]

So I haven't found any mention of the interoperability on the cloudpickle README. Maybe it's obvious when looking at the implementation, but barring that I suggest we add explicit tests that it works and a backwards compatibility test for tables. I think that's the only location where we use pickle/dill/cloud explicitly for long term storage. Then we can go ahead and swap it out.

[Leimeroth]

I think that's the only location where we use pickle/dill/cloud explicitly for long term storage. Then we can go ahead and swap it out.

From the cloudpickle github:

Using cloudpickle for long-term object storage is not supported and strongly discouraged.

This sounds like it will cause massive trouble with old objects at some point in the future.

[jan-janssen]

Using cloudpickle for long-term object storage is not supported and strongly discouraged.

The same applies for pickle and dill - in general serialized objects are not guaranteed to remain compatible.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jan-janssen]

@niklassiemer and @pmrv : I finally got back to this pull request and would like to merge it till the end of this year. As suggested by @pmrv I added two backwards compatibility tests based on the data_mining notebook in pyiron_atomistics https://github.com/pyiron/pyiron_atomistics/blob/main/notebooks/data_mining.ipynb . While the the system functions can be correctly unpickled by cloudpickle is they were initially pickled with dill, this does not apply to the user defined functions. These raise a ModuleNotFoundError error, so I followed @niklassiemer recommendation to fall back to dill if cloudpickle is not able to restore the object.

From my perspective this pull request is now ready to be reviewed again.

[pmrv]

I've browsed cloudpickles discussion wrt long term storage a bit and it seems that the statement concerning "cloudpickle is not meant for long-term storage" is mainly related to the fact that they use the highest pickle protocol by default and are hence potentially not backwards compatible. However for us that should mean as long as we do specify the pickling protocol we should be good.