Improvements regarding caching and fingerprinting
MEMBER
I few comments here for discussion:
- I'm not convinced yet the end user should really have to understand the difference between "caching" and 'fingerprinting", what do you think? I think fingerprinting should probably stay as an internal thing. Is there a case where we want cahing without fingerprinting or vice-versa?
- while I think the random fingerprint mechanism is smart, I have one question: when we disable caching or fingerprinting we also probably don't want the disk usage to grow so we should then try to keep only one cache file. Is it the case currently?
- the warning should be emitted only once per session if possible (we have a mechanism to do that in transformers, you should ask Lysandre/Sylvain)


2021-01-08T09:37:06Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-756654301
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc1NjY1NDMwMQ==
https://api.github.com/repos/huggingface/datasets/issues/comments/756654301/reactions
2021-01-08T09:37:06Z
https://api.github.com/repos/huggingface/datasets/issues/comments/756654301
https://avatars.githubusercontent.com/u/7353373?v=4
https://api.github.com/users/thomwolf/events{/privacy}
https://api.github.com/users/thomwolf/followers
https://api.github.com/users/thomwolf/following{/other_user}
https://api.github.com/users/thomwolf/gists{/gist_id}
https://github.com/thomwolf
thomwolf
MDQ6VXNlcjczNTMzNzM=
https://api.github.com/users/thomwolf/orgs
https://api.github.com/users/thomwolf/received_events
https://api.github.com/users/thomwolf/repos
https://api.github.com/users/thomwolf/starred{/owner}{/repo}
https://api.github.com/users/thomwolf/subscriptions
User
https://api.github.com/users/thomwolf
public
MEMBER
About your points:
- Yes I agree, I just wanted to bring the discussion on this point. Until now fingerprinting hasn't been blocking for user experience. I'll probably remove the enable/disable fingerprinting function to keep things simple from the user's perspective.
- Right now every time a not in-place transform (i.e. map, filter) is applied, a new cache file is created. It is the case even if caching is disabled since disabling it only means that the cache file won't be reloaded. Therefore you're right that it might end up filling the disk with files that won't be reused. I like the idea of keeping only one cache file. Currently all the cache files are kept on disk until the user clears the cache. To be able to keep only one, we need to know if a dataset that has been transformed is still loaded or not. For example
```python
# case 1 - keep both cache files (dataset1 and dataset2)
dataset2 = dataset1.map(...)
# case 2 - keep only the new cache file
dataset1 = dataset1.map(...)
```
In python it doesn't seem trivial to detect such changes. One thing that we can actually do on the other hand is store the cache files in a temporary directory that is cleared when the session closes. I think that's a good a simple solution for this problem.
- Yes good idea ! I don't like spam either :) 
2021-01-08T11:06:19Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-756696815
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc1NjY5NjgxNQ==
https://api.github.com/repos/huggingface/datasets/issues/comments/756696815/reactions
2021-01-08T11:06:19Z
https://api.github.com/repos/huggingface/datasets/issues/comments/756696815
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
MEMBER
> * To be able to keep only one, we need to know if a dataset that has been transformed is still loaded or not. For example
> 
> ```python
> # case 1 - keep both cache files (dataset1 and dataset2)
> dataset2 = dataset1.map(...)
> # case 2 - keep only the new cache file
> dataset1 = dataset1.map(...)
> ```

I see what you mean. It's a tricky question. One option would be that if caching is deactivated we have a single memory mapped file and have copy act as a copy by reference instead of a copy by value. We will then probably want a `copy()` or `deepcopy()` functionality. Maybe we should think a little bit about it though.
2021-01-08T15:04:56Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-756802504
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc1NjgwMjUwNA==
https://api.github.com/repos/huggingface/datasets/issues/comments/756802504/reactions
2021-01-08T15:04:56Z
https://api.github.com/repos/huggingface/datasets/issues/comments/756802504
https://avatars.githubusercontent.com/u/7353373?v=4
https://api.github.com/users/thomwolf/events{/privacy}
https://api.github.com/users/thomwolf/followers
https://api.github.com/users/thomwolf/following{/other_user}
https://api.github.com/users/thomwolf/gists{/gist_id}
https://github.com/thomwolf
thomwolf
MDQ6VXNlcjczNTMzNzM=
https://api.github.com/users/thomwolf/orgs
https://api.github.com/users/thomwolf/received_events
https://api.github.com/users/thomwolf/repos
https://api.github.com/users/thomwolf/starred{/owner}{/repo}
https://api.github.com/users/thomwolf/subscriptions
User
https://api.github.com/users/thomwolf
public
MEMBER
- I like the idea of using a temporary directory per session!
- If the default behavior when caching is disabled is to re-use the same file, I'm a little worried about people making mistakes and having to re-download and process from scratch.
- So we already have a keyword argument for `dataset1 = dataset1.map(..., in_place=True)`?
2021-01-08T16:37:27Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-756858861
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc1Njg1ODg2MQ==
https://api.github.com/repos/huggingface/datasets/issues/comments/756858861/reactions
2021-01-08T17:16:25Z
https://api.github.com/repos/huggingface/datasets/issues/comments/756858861
https://avatars.githubusercontent.com/u/10469459?v=4
https://api.github.com/users/yjernite/events{/privacy}
https://api.github.com/users/yjernite/followers
https://api.github.com/users/yjernite/following{/other_user}
https://api.github.com/users/yjernite/gists{/gist_id}
https://github.com/yjernite
yjernite
MDQ6VXNlcjEwNDY5NDU5
https://api.github.com/users/yjernite/orgs
https://api.github.com/users/yjernite/received_events
https://api.github.com/users/yjernite/repos
https://api.github.com/users/yjernite/starred{/owner}{/repo}
https://api.github.com/users/yjernite/subscriptions
User
https://api.github.com/users/yjernite
public
MEMBER
> * If the default behavior when caching is disabled is to re-use the same file, I'm a little worried about people making mistakes and having to re-download and process from scratch.

We should distinguish between the caching from load_dataset (base dataset cache files) and the caching after dataset transforms such as map or filter (transformed dataset cache files). When disabling caching only the second type (for map and filter) doesn't reload from cache files.
Therefore nothing is re-downloaded. To re-download the dataset entirely the argument `download_mode="force_redownload"` must be used in `load_dataset`.
Do we have to think more about the naming to make things less confusing in your opinion ?

> * So we already have a keyword argument for `dataset1 = dataset1.map(..., in_place=True)`?

There's no such `in_place` parameter in map, what do you mean exactly ?
2021-01-08T17:29:50Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-756895331
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc1Njg5NTMzMQ==
https://api.github.com/repos/huggingface/datasets/issues/comments/756895331/reactions
2021-01-08T17:29:50Z
https://api.github.com/repos/huggingface/datasets/issues/comments/756895331
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
MEMBER
I updated the PR:
- I removed the enable/disable fingerprinting function
- if caching is disabled arrow files are written in a temporary directory that is deleted when session closes
- the warning that is showed when hashing a transform fails is only showed once
- I added the `set_caching_enabled` function to the docs and explained the caching mechanism and its relation with fingerprinting

I would love to have some feedback :) 
2021-01-15T13:59:50Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-760957733
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc2MDk1NzczMw==
https://api.github.com/repos/huggingface/datasets/issues/comments/760957733/reactions
2021-01-15T13:59:50Z
https://api.github.com/repos/huggingface/datasets/issues/comments/760957733
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
MEMBER
> > * So we already have a keyword argument for `dataset1 = dataset1.map(..., in_place=True)`?
> 
> There's no such `in_place` parameter in map, what do you mean exactly ?

Sorry, that wasn't clear at all. I was responding to your previous comment about case 1 / case 2. I don't think the behavior should depend on the command, but we could have:

```
# case 1 - keep both cache files (dataset1 and dataset2)
dataset2 = dataset1.map(...)
# case 2 - keep only the new cache file
dataset1 = dataset1.map(..., in_place=True)
```

Case 1 returns a new reference using the new cache file, case 2 returns the same reference
2021-01-15T15:03:18Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-760993154
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc2MDk5MzE1NA==
https://api.github.com/repos/huggingface/datasets/issues/comments/760993154/reactions
2021-01-15T15:03:18Z
https://api.github.com/repos/huggingface/datasets/issues/comments/760993154
https://avatars.githubusercontent.com/u/10469459?v=4
https://api.github.com/users/yjernite/events{/privacy}
https://api.github.com/users/yjernite/followers
https://api.github.com/users/yjernite/following{/other_user}
https://api.github.com/users/yjernite/gists{/gist_id}
https://github.com/yjernite
yjernite
MDQ6VXNlcjEwNDY5NDU5
https://api.github.com/users/yjernite/orgs
https://api.github.com/users/yjernite/received_events
https://api.github.com/users/yjernite/repos
https://api.github.com/users/yjernite/starred{/owner}{/repo}
https://api.github.com/users/yjernite/subscriptions
User
https://api.github.com/users/yjernite
public
MEMBER
> Sorry, that wasn't clear at all. I was responding to your previous comment about case 1 / case 2. I don't think the behavior should depend on the command, but we could have:
> 
> ```
> # case 1 - keep both cache files (dataset1 and dataset2)
> dataset2 = dataset1.map(...)
> # case 2 - keep only the new cache file
> dataset1 = dataset1.map(..., in_place=True)
> ```
> 
> Case 1 returns a new reference using the new cache file, case 2 returns the same reference

Ok I see !
`in_place` is a parameter that is used in general to designate a transform so I would name that differently (maybe `overwrite` or something like that).
Not sure if it's possible to update an already existing arrow file that is memory-mapped, let me check real quick.
Also it's possible to call `dataset2.cleanup_cache_files()` to delete the other cache files if we create a new one after the transform. Or even to get the cache file with `dataset1.cache_files` and let the user remove them by hand.

EDIT: updating an arrow file in place is not part of the current API of pyarrow, so we would have to make new files.

2021-01-15T15:25:08Z
https://github.com/huggingface/datasets/pull/1703#issuecomment-761006056
https://api.github.com/repos/huggingface/datasets/issues/1703
MDEyOklzc3VlQ29tbWVudDc2MTAwNjA1Ng==
https://api.github.com/repos/huggingface/datasets/issues/comments/761006056/reactions
2021-01-15T15:46:48Z
https://api.github.com/repos/huggingface/datasets/issues/comments/761006056
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
This PR adds these features:
- Enable/disable caching
    If disabled, the library will no longer reload cached datasets files when applying transforms to the datasets.
    It is equivalent to setting `load_from_cache` to `False` in dataset transforms.
```python
from datasets import set_caching_enabled

set_caching_enabled(False)
```
- Allow unpicklable functions in `map`
    If an unpicklable function is used, then it's not possible to hash it to update the dataset fingerprint that is used to name cache files. To workaround that, a random fingerprint is generated instead and a warning is raised.
```python
logger.warning(
    f"Transform {transform} 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."
)
```

and also (open to discussion, EDIT: actually NOT included):
- Enable/disable fingerprinting
    Fingerprinting allows to have one deterministic fingerprint per dataset state.
    A dataset fingerprint is updated after each transform.
    Re-running the same transforms on a dataset in a different session results in the same fingerprint.
    Disabling the fingerprinting mechanism makes all the fingerprints random.
    Since the caching mechanism uses fingerprints to name the cache files, then cache file names will be different.
    Therefore disabling fingerprinting will prevent the caching mechanism from reloading datasets files that have already been computed.
    Disabling fingerprinting may speed up the lib for users that don't care about this feature and don't want to use caching.
```python
from datasets import set_fingerprinting_enabled

set_fingerprinting_enabled(False)
```

Other details:
- I renamed the `fingerprint` decorator to `fingerprint_transform` since the name was clearly not explicit. This decorator is used on dataset transform functions to allow them to update fingerprints.
- I added some `ignore_kwargs` when decorating transforms with `fingerprint_transform`, to make the fingerprint update not sensible to kwargs like `load_from_cache` or `cache_file_name`.

Todo: tests for set_fingerprinting_enabled + documentation for all the above features