Pydicom: Error when using pickle on a dataset

[tboutelier]

Hi,
I am using python 3.6.8, with Pydicom version 1.3.0, on Windows 10.
I have an error when I try to save and load a dataset using the pickle format.
I manage (no error) to save the dataset using pickle.dump() function, but I get an error when I try to load it. Here is the code:

````python
import pydicom
import pickle

dcmname='C:\toto\IM-0511-0001.dcm' # path to the DICOM file
ds=pydicom.dcmread(dcmname) # load the DICOM file and obtain a dataset

filename='C:\toto\dcm.pkl' # Name of the file where the pickle data will be saved
f=open(filename,'wb'); # open the file
pickle.dump({'ds': ds}, f); # save the dataset
f.close(); # close the file

f=open(filename,'rb'); # open the file
obj=pickle.load(f); # load the pickle file
f.close(); # close the file
I get the following error python
Traceback (most recent call last):
File "", line 1, in
File "C:\toto.conda\envs\marvelous\lib\site-packages\pydicom\dataset.py", line 1649, in __setitem__
self._dict[tag] = data_element
File "C:\toto.conda\envs\my_env\lib\site-packages\pydicom\dataset.py", line 741, in __getattr__
return object.__getattribute__(self, name)
AttributeError: 'Dataset' object has no attribute '_dict'
I found a workaround by setting the [protocol ](https://docs.python.org/3.6/library/pickle.html#pickle.DEFAULT_PROTOCOL) option of the `pickle.dump()` function to a value lower or equal than 1: python
import pydicom
import pickle

dcmname='C:\toto\IM-0511-0001.dcm' # path to the DICOM file
ds=pydicom.dcmread(dcmname) # load the DICOM file and obtain a dataset

filename='C:\toto\dcm.pkl' # Name of the file where the pickle data will be saved
f=open(filename,'wb'); # open the file
pickle.dump({'ds': ds}, f, protocol=1); # save the dataset
f.close(); # close the file

f=open(filename,'rb'); # open the file
obj=pickle.load(f); # load the pickle file
f.close(); # close the file
````
Other protocol value does not work (3, the default value, and 4), while I would like to continue to be able to use the default value of the protocol.

I think that the problem was not existing on previous release of Pydicom, so I guess that it appeared with version 1.3.0.

I am sorry that I cannot share a DICOM file with you, but I have the problem with all my dicom files. I would be interested to know if someone else experienced the same problem.

Thanks!

[darcymason]

I get the same error loading pydicom's CT_small.dcm. Will look into it...

[rhaxton]

I have run into this as well. I had to revert to 1.2.2 to get it to work. It is not obvious to me what happened to break this, but maybe I can take some time to look at soon.

[mrbean-bremen]

@darcymason, @rhaxton:
Hm, I'm quite sure I broke this somehow with the Dataset inheritance stuff... Can easily be reproduced:

    def test_pickle(self):
        test_file = get_testdata_files('CT_small.dcm')[0]
        ds = pydicom.dcmread(test_file)
        s = pickle.dumps({'ds': ds})
        ds1 = pickle.loads(s)['ds']
        assert ds == ds1

As both of you are already working on this, I will leave that to you, but I'm ready to have a go, of course.

[mrbean-bremen]

Having said that, I have a workaround that fixed this, but it's not pretty. Will post it only if you don't have anything better 😈. I updated the test above to be able to test the fix.

[tboutelier]

Great! Let us know about your progress on the subject!

[mrbean-bremen]

To provide a bit more information: as far as I understand this, the problem happens during initialization of the data set from pickle (in Python 3 only). For some reason, Pickle deserializes the dictionary twice - the first time before _dict is set, which leads to the error. I have not yet understood why that happens.
An easy fix is to check for _dict (hasattr(self, '_dict')) in __setitem__ and return if it is not set. If we don't want to have this check on each call of __setattr__, we can move it to the fallback handling in __getattr__ and return an empty dict in this case, This is more of a hack, but has no impact performance-wise.
I thought about overwriting __setstate__, but that is called too late, but I'm sure there is a better fix... Any thoughts? @darcymason, @rhaxton, did you get anywhere?

[scaramallion]

Seems to work, but I have no idea if this is a good way to use __getnewargs__

def __getnewargs__(self):
    return (Dataset.__init__(self),)

Weirdly, I have a similar issue unpickling datasets previously pickled with protocol 3 using Python 3.6, but that exception is in MultiValue not Dataset. Python 3.5 works fine, but I'm still investigating to make sure it's not something I've done.

[darcymason]

Any thoughts? @darcymason, @rhaxton, did you get anywhere?

I only started to look into it a little bit - I don't have anything to add.

Seems to work, but I have no idea if this is a good way to use __getnewargs__

This looks like a relatively simple workaround, but I am not a pickle user, so hopefully others can comment on the appropriateness of this fix.

[mrbean-bremen]

Seems to work, but I have no idea if this is a good way to use __getnewargs__

This would fix the problem by creating the _dict variable before, so the first deserialization call will have no problem. My workaround basically did nothing at the first call instead. I think both are viable workarounds (I like yours better, as it is specifically related to pickle), but I still would like to understand the problem in the first way.

[mrbean-bremen]

I can confirm that the bug was introduced with deriving dataset from dict (#844).
I haven't looked further into this, though - maybe I will have another look later today.

[mrbean-bremen]

I had another look at the the __getnewargs__, and the proposed solution will not work, as far as I can see. It just strips both the original and pickled datasets of all tags, so that the test passes. I'll probably add a PR with a workaround, even if I don't understand the root cause completely.

[scaramallion]

It just strips both the original and pickled datasets of all tags, so that the test passes

Hah! Its alright as long as the test passes :stuck_out_tongue: