Add `key` type and duplicates verification with hashing
State: closed | Author: NikhilBartwal (CONTRIBUTOR)

Closes #2230 
There is currently no verification for the data type and the uniqueness of the keys yielded by the `dataset_builder`.
This PR is currently a work in progress with the following goals:

-  [x] Adding `hash_salt` to `ArrowWriter` so that the keys belonging to different splits have different hash
-  [x] Add `key` arrtibute to `ArrowWriter.write()` for hashing
-  [x] Add a hashing class which takes an input key of certain type (`str`/`int`/anything convertible to string) and produces a 128-bit hash using `hashlib.md5`
-  [x] Creating a function giving a custom error message when non-unique keys are found 
   **[This will take care of type-checking for keys]**
-  [x] Checking for duplicate keys in `writer.write()` for each batch

[**NOTE**: This PR is currently concerned with `GeneratorBasedBuilder` only, for simplification. A subsequent PR will be made in future for `ArrowBasedBuilder`]

@lhoestq Thank you for the feedback. It would be great to have your guidance on this!

Comments: @lhoestq The tests for key type and duplicate keys have been added and verified successfully.
After generating with an intentionally faulty `mnist` script, when there is an incompatible key type, it shows:

```
Downloading and preparing dataset mnist/mnist (download: 11.06 MiB, generated: 60.62 MiB, post-processed: Unknown size, total: 71.67 MiB) to C:\Users\nikhil\.cache\huggingface\datasets\mnist\mnist\1.0.0\5064c25e57a1678f700d2dc798ffe8a6d519405cca7d33670fffda477857a994...
0 examples [00:00, ? examples/s]2021-04-26 02:50:03.703836: I tensorflow/stream_executor/platform/default/dso_loader.cc:49] Successfully opened dynamic library cudart64_110.dll

FAILURE TO GENERATE DATASET: Invalid key type detected
Found Key [0, 0] of type <class 'list'>
Keys should be either str, int or bytes type
```

In the case of duplicate keys, it now gives:

```
Downloading and preparing dataset mnist/mnist (download: 11.06 MiB, generated: 60.62 MiB, post-processed: Unknown size, total: 71.67 MiB) to C:\Users\nikhil\.cache\huggingface\datasets\mnist\mnist\1.0.0\5064c25e57a1678f700d2dc798ffe8a6d519405cca7d33670fffda477857a994...
0 examples [00:00, ? examples/s]2021-04-26 02:53:13.498579: I tensorflow/stream_executor/platform/default/dso_loader.cc:49] Successfully opened dynamic library cudart64_110.dll
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "f:\datasets\datasets-1\src\datasets\load.py", line 746, in load_dataset
    builder_instance.download_and_prepare(
  File "f:\datasets\datasets-1\src\datasets\builder.py", line 587, in download_and_prepare
    self._download_and_prepare(
  File "f:\datasets\datasets-1\src\datasets\builder.py", line 665, in _download_and_prepare
    self._prepare_split(split_generator, **prepare_split_kwargs)
  File "f:\datasets\datasets-1\src\datasets\builder.py", line 1002, in _prepare_split
    writer.write(example, key)
  File "f:\datasets\datasets-1\src\datasets\arrow_writer.py", line 321, in write
    self.check_duplicates()
  File "f:\datasets\datasets-1\src\datasets\arrow_writer.py", line 331, in check_duplicates
    raise DuplicatedKeysError(key)
datasets.keyhash.DuplicatedKeysError: FAILURE TO GENERATE DATASET !
Found duplicate Key: 234467
Keys should be unique and deterministic in nature
```
Please let me know if this is what we wanted to implement. Thanks!

This looks pretty cool !
We can make focus on the GeneratorBasedBuilder for now yes.

Do you think we could make the ArrowWriter not look for duplicates by default ?
This way we can just enable duplicate detections when instantiating the writer in the GeneratorBasedBuilder for now.

Thank you @lhoestq



> Do you think we could make the ArrowWriter not look for duplicates by default ?

We can definitely do that by including a `check_duplicates` argument while instantiating `ArrowWriter()`. 

However, since only `GeneratorBasedBuilder` uses the `write()` function (which includes the detection code) and the others like `ArrowBasedBuilder` use `write_table()` which remains as it was (without duplicate detection). I don't think it would be necessary.

Nonetheless, doing this would require just some small changes. Please let me know your thoughts on this. Thanks!

I like the idea of having the duplicate detection optional for other uses of the ArrowWriter.
This class is the main tool to write python data in arrow format so I'd expect it to be flexible.
That's why I think by default it shouldn't require users to provide keys or do any duplicates detection.

An alternative would be to subclass the writer to include duplicates detection in another class.

Both options are fine for me, let me know what you think !

> This class is the main tool to write python data in arrow format so I'd expect it to be flexible.
> That's why I think by default it shouldn't require users to provide keys or do any duplicates detection.

Well, that makes sense as the writer can indeed be used for other purposes as well.

> We can definitely do that by including a `check_duplicates` argument while instantiating `ArrowWriter()`.

I think that this would be the simplest and the more efficient option for achieving this as subclassing the writer only for this would lead to unnecessary complexity and code duplication (in case of `writer()`). 

I will be adding the changes soon. Thanks for the feedback @lhoestq!

@lhoestq I have pushed the final changes just now. 
Now, the keys and duplicate checking will be necessary only when the `ArrowWriter` is initialized with `check_duplicates=True` specifically (in this case, for `GeneratorBasedBuilders`)

Let me know if this is what was required. Thanks!

@lhoestq Thanks for the feedback! I will be adding the tests for the same very soon. 

However, I'm not sure as to what exactly is causing the `segmentation fault` in the failing CI tests. It seems to be something from `test_concatenation_table_cast` from `test_table.py`, but I'm not sure as to what exactly. Would be great if you could help. Thanks!

You can merge master into your branch to fix this issue.
Basically pyarrow 4.0.0 has a segfault issue (which has now been resolved on the master branch of pyarrow).
So until 4.0.1 comes out we changed to using `pyarrow<4.0.0` recently.

@lhoestq Thanks for the help with the CI failures. Apologies for the multiple merge commits. My local repo got messy while merging which led to this.
Will be pushing the commit for the tests soon!

Hey @lhoestq, I've just added the required tests for checking key duplicates and invalid key data types.
I think we have caught a nice little issue as 27 datasets are currently using non-unique keys (hence, the failing tests: All these datasets are giving `DuplicateKeysError` during testing). 
These datasets were not detected earlier as there was no key checking when `num_examples < writer_batch_size` due to which they passed the dummy data generation test. This bug was fixed by adding the test to `writer.finalize()` method as well for  checking any leftover examples from batches. 

I'd like to make changes to the faulty datasets' scripts. However, I was wondering if I should do that in this PR itself or open a new PR as this might get messy in the same PR. Let me know your thoughts on this. Thanks!

Hi ! Once https://github.com/huggingface/datasets/pull/2333 is merged, feel free to merge master into your branch to fix the CI :)

Thanks a lot for the help @lhoestq. Besides merging the new changes, I guess this PR is completed for now :)

I just merged the PR, feel free to merge `master` into your branch. It should fix most most of the CI issues. If there are some left we can fix them in this PR :)

@lhoestq Looks like the PR is completed now. Thanks for helping me out so much in this :)

Hey @lhoestq, I've added the test and corrected the Cl errors as well. Do let me know if this requires any change. Thanks!

Merging. I'll update the comment on the master branch (for some reason I can edit files on this branch)

@lhoestq Thank you for the help and feedback. Feels great to contribute!