huggingface / huggingface/datasets
_legacy_no_dict_keys_sorting has no reader since #7817, so the 2.15.0 legacy cache lookup computes the wrong config_id
- Dominant language
- Python
- Stars
- 22k
- Forks
- 3.4k
- Avg merge
- 5d 7h
- Merged PRs (30d)
- 17
Description
### Describe the bug
`Pickler._legacy_no_dict_keys_sorting` has a definition and a writer, but since #7817 it has no reader, so the flag does nothing.
`DatasetBuilder._check_legacy_cache2` patches it on to recompute the `config_id` that 2.15.0 would have produced, because 2.15.0 hashed dicts without sorting their items:
```python
# src/datasets/builder.py:493-495
with patch.object(Pickler, "_legacy_no_dict_keys_sorting", True):
config_id = self.config.name + "-" + Hasher.hash({"data_files": self.config.data_files})
```
#7817 rewrote `_batch_setitems` to accept the extra argument Python 3.14 passes, and in the same hunk removed the early return that read the flag:
```diff
- def _batch_setitems(self, items):
- if self._legacy_no_dict_keys_sorting:
- return super()._batch_setitems(items)
+ def _batch_setitems(self, items, *args, **kwargs):
# Ignore the order of keys in a dict
```
That PR is about the 3.14 signature change and says nothing about the legacy branch, so the removal looks incidental rather than deliberate. At HEAD the flag appears exactly twice in the tree, and neither is a read:
```console
$ grep -rn '_legacy_no_dict_keys_sorting' src/
src/datasets/builder.py:494: with patch.object(Pickler, "_legacy_no_dict_keys_sorting", True):
src/datasets/utils/_dill.py:30: _legacy_no_dict_keys_sorting = False
```
`_check_legacy_cache2` builds `{cache_dir}/{namespace}___{dataset_name}/{config_id}/0.0.0/{hash}` from that `config_id` and returns it only `if os.path.isdir(legacy_cache_dir)`, so a `config_id` that no longer matches the one 2.15.0 wrote means the old directory is not found and the lookup silently misses.
### Steps to reproduce the bug
`data_files` is a dict, and the two branches differ only when its keys are not already in sorted order, which is the common `{"train": ..., "test": ...}` case:
```python
from unittest.mock import patch
from datasets.fingerprint import Hasher
from datasets.utils._dill import Pickler
obj = {"train": ["train.csv"], "test": ["test.csv"]}
print("normal:", Hasher.hash(obj))
with patch.object(Pickler, "_legacy_no_dict_keys_sorting", True):
print("legacy:", Hasher.hash(obj))
```
Run against several versions on Python 3.11, one venv each, the 2.15.0 column being what that release actually produces for the same object:
| datasets | `normal` | `legacy` | matches 2.15.0 |
|---|---|---|---|
| 2.15.0 | `711511d8f1d9bc25` | n/a | n/a |
| 4.3.0 | `cfd3b51f0f8e9fd8` | `711511d8f1d9bc25` | yes |
| 4.4.0 | `cfd3b51f0f8e9fd8` | `cfd3b51f0f8e9fd8` | no |
| main @ b7cb10b0 | `cfd3b51f0f8e9fd8` | `cfd3b51f0f8e9fd8` | no |
4.4.0 is the first release containing #7817 (`git tag --contains f7c8e46ec`), so the behaviour changes exactly at that boundary.
### Expected behavior
Under the patched flag, `Hasher.hash({"train": ..., "test": ...})` should reproduce the 2.15.0 value `711511d8f1d9bc25`, so `_check_legacy_cache2` looks for the directory 2.15.0 actually wrote. At HEAD it returns the sorted-key hash instead.
Two reasonable resolutions, and I do not know which you would prefer:
1. Restore the early return in `_batch_setitems`, keeping the `*args`/`**kwargs` passthrough that #7817 added.
2. Retire the compat path deliberately, dropping the flag and its use in `_check_legacy_cache2`, if the 2.14/2.15 cache layout is no longer worth carrying.
I have a PR ready for option 1, since it is the smaller change and preserves the stated intent of #6514. Happy to switch it to option 2 if you would rather retire the path.
One thing I did not test: I did not build a real 2.15.0 cache directory and load it under HEAD. The hash mismatch above is measured, and the path construction and `os.path.isdir` check are read from the source rather than exercised end to end.
### Environment info
- `datasets` 5.0.2.dev0 (main @ `b7cb10b0e38ab8bbefdb142d0279bc85ac8c7ab8`), plus 4.4.0, 4.3.0 and 2.15.0 from PyPI
- Python 3.11.15, `dill` 0.4.1, Linux x86_64, all runs in clean containers
Contributor guide
Research direction
Start in src/datasets/utils/_dill.py at Pickler._batch_setitems and compare its current behavior with the legacy flag, then inspect src/datasets/builder.py around _check_legacy_cache2. Run the supplied Hasher.hash reproduction and verify that the patched flag produces the 2.15.0 hash so the legacy cache directory can be found.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- data
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100