Repository: huggingface/datasets
Pull request: Fix hashing for python 3.9
In python 3.9, pickle hashes the `glob_ids` dictionary in addition to the `globs` of a function.

Therefore the test at `tests/test_fingerprint.py::RecurseDumpTest::test_recurse_dump_for_function_with_shuffled_globals` is currently failing for python 3.9

To make hashing deterministic when the globals are not in the same order, we also need to make the order of `glob_ids` deterministic.

Right now we don't have a CI to test python 3.9 but we should definitely have one. For this PR in particular I ran the tests locally using python 3.9 and they're passing now.

Fix https://github.com/huggingface/datasets/issues/4506

Problem statement:
Failure to hash (and cache) a `.map(...)` (almost always) - using this method can produce incorrect results
## Describe the bug
Sometimes I get messages about not being able to hash a method:
`Parameter 'function'=<function StupidDataModule._separate_speaker_id_from_dialogue at 0x7f1b27180d30> of the transform datasets.arrow_dataset.Dataset.
_map_single couldn't be hashed properly, a random hash was used instead. Make sure your transforms and parameters are serializable with pickle or dill for the dataset fingerprinting and caching to work. If you reuse this transform, the caching mechanism will consider it to be different from the previous calls and recompute everything. This warning is only showed once. Subsequent hashing failures won't be showed.`
Whilst the function looks like this:
```python
@staticmethod
def _separate_speaker_id_from_dialogue(example: arrow_dataset.Example):
    speaker_id, dialogue = tuple(zip(*(example["dialogue"])))
    example["speaker_id"] = speaker_id
    example["dialogue"] = dialogue
    return example
```
This is the first step in my preprocessing pipeline, but sometimes the message about failure to hash is not appearing on the first step, but then appears on a later step.
This error is sometimes causing a failure to use cached data, instead of re-running all steps again.

## Steps to reproduce the bug
```python
import copy
import datasets
from datasets import arrow_dataset

def main():
    dataset = datasets.load_dataset("blended_skill_talk")
    res = dataset.map(method)
    print(res)

def method(example: arrow_dataset.Example):
    example['previous_utterance_copy'] = copy.deepcopy(example['previous_utterance'])
    return example

if __name__ == '__main__':
    main()
```
Run with:
```
python -m reproduce_error
```

## Expected results
Dataset is mapped and cached correctly.

## Actual results
The code outputs this at some point:
`Parameter 'function'=<function method at 0x7faa83d2a160> of the transform datasets.arrow_dataset.Dataset._map_single couldn't be hashed properly, a random hash was used instead. Make sure your transforms and parameters are serializable with pickle or dill for the dataset fingerprinting and caching to work. If you reuse this transform, the caching mechanism will consider it to be different from the previous calls and recompute everything. This warning is only showed once. Subsequent hashing failures won't be showed.`

## Environment info
<!-- You can run the command `datasets-cli env` and copy-and-paste its output below. -->
- `datasets` version:
- Platform: Ubuntu 20.04.3
- Python version: 3.9.12
- PyArrow version: 8.0.0
- Datasets version: 2.3.1


Datasets method `.map` not hashing
## Describe the bug
Datasets method `.map` not hashing, even with an empty no-op function

## Steps to reproduce the bug
```python
from datasets import load_dataset

# download 9MB dummy dataset
ds = load_dataset("hf-internal-testing/librispeech_asr_dummy", "clean")

def prepare_dataset(batch):
     return(batch)

ds = ds.map(
    prepare_dataset,
    num_proc=1,
    desc="preprocess train dataset",
)
```

## Expected results
Hashed and cached dataset preprocessing

## Actual results
Does not hash properly:
```
Parameter 'function'=<function prepare_dataset at 0x7fccb68e9280> of the transform datasets.arrow_dataset.Dataset._map_single couldn't be hashed properly, a random hash was used instead. Make sure your transforms and parameters are serializable with pickle or dill for the dataset fingerprinting and caching to work. If you reuse this transform, the caching mechanism will consider it to be different from the previous calls and recompute everything. This warning is only showed once. Subsequent hashing failures won't be showed.
```

## Environment info
<!-- You can run the command `datasets-cli env` and copy-and-paste its output below. -->
- `datasets` version: 2.3.3.dev0
- Platform: Linux-5.11.0-1028-gcp-x86_64-with-glibc2.31
- Python version: 3.9.12
- PyArrow version: 8.0.0
- Pandas version: 1.4.2

cc @lhoestq

Hints from issue discussion:
Important info:

As hashes are generated randomly for functions, it leads to **false identifying some results as already hashed** (mapping function is not executed after a method update) when there's a `pytorch_lightning.seed_everything(123)`
@lhoestq
seems like quite critical stuff for me, if I'm not making a mistake
Hi ! Thanks for reporting. This bug seems to appear in python 3.9 using dill 3.5.1

As a workaround you can use an older version of dill:
```
pip install "dill<0.3.5"
```
installing `dill<0.3.5` after installing `datasets` by pip results in dependency conflict with the version required for `multiprocess`. It can be solved by installing `pip install datasets "dill<0.3.5"` (simultaneously) on a clean environment

Commit message: fix hashing for python 3.9

Patch to review:
diff --git a/src/datasets/utils/py_utils.py b/src/datasets/utils/py_utils.py
index 2f20ac2e38a..4adc56270d8 100644
--- a/src/datasets/utils/py_utils.py
+++ b/src/datasets/utils/py_utils.py
@@ -818,6 +818,22 @@ def save_function(pickler, obj):
                 else:
                     globs = {"__name__": obj.__module__}
 
+            # DONE: modified here for huggingface/datasets
+            # - globs is a dictionary with keys = var names (str) and values = python objects
+            # - globs_copy is a dictionary with keys = var names (str) and values = ids of the python objects
+            # however the dictionary is not always loaded in the same order
+            # therefore we have to sort the keys to make deterministic.
+            # This is important to make `dump` deterministic.
+            # Only these line are different from the original implementation:
+            # START
+            globs_is_globs_copy = globs is globs_copy
+            globs = {k: globs[k] for k in sorted(globs.keys())}
+            if globs_is_globs_copy:
+                globs_copy = globs
+            elif globs_copy is not None:
+                globs_copy = {k: globs_copy[k] for k in sorted(globs_copy.keys())}
+            # END
+
             if globs_copy is not None and globs is not globs_copy:
                 # In the case that the globals are copied, we need to ensure that
                 # the globals dictionary is updated when all objects in the
@@ -833,13 +849,6 @@ def save_function(pickler, obj):
                 else:
                     postproc_list.append((dill._dill._setitems, (globs, globs_copy)))
 
-            # DONE: globs is a dictionary with keys = var names (str) and values = python objects
-            # however the dictionary is not always loaded in the same order
-            # therefore we have to sort the keys to make deterministic.
-            # This is important to make `dump` deterministic.
-            # Only this line is different from the original implementation:
-            globs = {k: globs[k] for k in sorted(globs.keys())}
-
             if dill._dill.PY3:
                 closure = obj.__closure__
                 state_dict = {}


Code review comments:
diff_hunk: @@ -818,6 +818,22 @@ def save_function(pickler, obj):
                 else:
                     globs = {"__name__": obj.__module__}
 
+            # DONE: modified here for huggingface/datasets
+            # - globs is a dictionary with keys = var names (str) and values = python objects
+            # - globs_copy is a dictionary with keys = var names (str) and values = ids of the python objects
+            # however the dictionary is not always loaded in the same order
+            # therefore we have to sort the keys to make deterministic.
+            # This is important to make `dump` deterministic.
+            # Only these line are different from the original implementation:
+            # START
+            globs_is_globs_copy = globs is globs_copy
+            globs = {k: globs[k] for k in sorted(globs.keys())}
path: src/datasets/utils/py_utils.py
text: @user1:
```suggestion
            globs = dict(sorted(globs.items()))
```

diff_hunk: @@ -818,6 +818,22 @@ def save_function(pickler, obj):
                 else:
                     globs = {"__name__": obj.__module__}
 
+            # DONE: modified here for huggingface/datasets
+            # - globs is a dictionary with keys = var names (str) and values = python objects
+            # - globs_copy is a dictionary with keys = var names (str) and values = ids of the python objects
+            # however the dictionary is not always loaded in the same order
+            # therefore we have to sort the keys to make deterministic.
+            # This is important to make `dump` deterministic.
+            # Only these line are different from the original implementation:
+            # START
+            globs_is_globs_copy = globs is globs_copy
+            globs = {k: globs[k] for k in sorted(globs.keys())}
+            if globs_is_globs_copy:
+                globs_copy = globs
+            elif globs_copy is not None:
+                globs_copy = {k: globs_copy[k] for k in sorted(globs_copy.keys())}
path: src/datasets/utils/py_utils.py
text: @user1:
```suggestion
                globs_copy = dict(sorted(globs_copy.items()))
```