Latest dill release raises exception
State: closed | Author: albertvillanova (MEMBER)
Labels: name: bug

## Describe the bug
As reported by @sgugger, latest dill release is breaking things with Datasets.

```
______________ ExamplesTests.test_run_speech_recognition_seq2seq _______________


self = <multiprocess.pool.ApplyResult object at 0x7fa5981a1cd0>, timeout = None

    def get(self, timeout=None):
        self.wait(timeout)
        if not self.ready():
            raise TimeoutError
        if self._success:
            return self._value
        else:
>           raise self._value
E           TypeError: '>' not supported between instances of 'NoneType' and 'float'
```



Comments: Fixed by:
- #4380 

Just an additional insight, the latest dill (either 0.3.5 or 0.3.5.1) also broke the hashing/fingerprinting of any mapping function.

For example:
```
from datasets import load_dataset

d = load_dataset("rotten_tomatoes")
d.map(lambda x: x)
```

Returns the standard non-dillable error:
```
Parameter 'function'=<function <lambda> at 0x7fe7d18c9560> of the transform datasets.arrow_dataset.Dataset._map_single couldn't be hashed properly....
```

@albertvillanova ExamplesTests.test_run_speech_recognition_seq2seq is in which file?

Thanks a lot @gugarosa for the insight: we will incorporate it in our CI as regression testing for future dill releases.

Hi @anivegesana, that test is in `transformers` library:
- https://github.com/huggingface/transformers/blob/main/examples/pytorch/test_pytorch_examples.py#L449
- https://github.com/huggingface/transformers/blob/main/examples/pytorch/speech-recognition/run_speech_recognition_seq2seq.py 

@albertvillanova

I did a deep dive into @gugarosa's problem and found the issue and it might be related to the one @sgugger discovered. In dill 0.3.5(.1), I created a new `save_function` that fixes a bug in dill that prevented the pickling of recursive inner functions. It was a more complete solution to the problem that `dill._dill.stack` tried to solve in the internal API of dill. Since `dill._dill.stack` was no longer needed, I removed it. Since datasets copies the `save_function` directly from the dill API, it stops working with the new dill version since `dill._dill.stack` is no longer present and the `save_function` has been updated with new code.

https://github.com/huggingface/datasets/blob/95193ae61e92aa537d0c65d37a1fd9d2393aae89/src/datasets/utils/py_utils.py#L607-L678

~If the dill version is below 0.3.5, you should keep this function. If it is after, you would need to update your copy of `save_function` to use the code I introduced, or manually add a `stack` variable to `dill._dill` if it doesn't exist. Fortunately, in any version of Python 3.7+, dictionaries are always in insertion order and dill no longer supports Python 3.6 or older. So, any globals dictionary saved by dill 0.3.5+ will be deterministic given that the version of dill is held constant and this save_function is unnecessary for newer versions of dill.~

Ah. I see what is happening. I guess a different copy of the function code is needed that sorts the global variables by name.

```py
if dill.__version__.split('.') < ['0', '3', '5']:
    # current save_function code inside here
else:
    # new save_function code inside here with the following line inserted after creating the globals
    globs = {k: globs[k] for k in sorted(globs.keys())} 
```

Will look into the test case @sgugger pointed out after that and verify if this is causing the problem.

I am actually looking into rewriting the global variables code in uqfoundation/dill#466 and will keep this in mind and will try to create an easy way to modify the global variables in dill 0.3.6 (for example, sort them by key like datasets does).

Thanks a lot for your investigation @anivegesana.

Yes, we copied-pasted the old `save_function` function from `dill`, just adding a line to make deterministic the order of global variables `globs`. 

However, this function has changed a lot from version 0.3.5, after your PR (thank you for the fix in recursiveness, indeed):
- uqfoundation/dill#443

We have to address this change.

If finally your PR to sort global variables is merged into dill 0.3.6, that will make our life easier, as the tweak will no longer be necessary. ;)

I have included a regression test so that we are sure future releases of dill do not break `datasets`:
- #4385 

I should note that because Python 3.6 and older are now deprecated and Python 3.7 has insertion order dictionaries, the globals in dill will have a deterministic order, just not sorted. I would still keep it sorted like you have it to help with stability (for example, if someone reorders variables in a file, then sorting the globals would not invalidate the cache.)

It seems that the order is not quite deterministic in IPython. Huggingface datasets seems to do well in Jupyter regardless, so it is not a good idea to remove the sorting. uqfoundation/dill#19