Fix splits in local packaged modules, local datasets without script and hub datasets without script
NONE
_The documentation is not available anymore as the PR was closed or merged._
2022-04-11T14:07:44Z
https://github.com/huggingface/datasets/pull/4144#issuecomment-1095098930
https://api.github.com/repos/huggingface/datasets/issues/4144
IC_kwDODunzps5BReIy
https://api.github.com/repos/huggingface/datasets/issues/comments/1095098930/reactions
2022-04-28T21:09:17Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1095098930
https://avatars.githubusercontent.com/u/99929124?v=4
https://api.github.com/users/HuggingFaceDocBuilderDev/events{/privacy}
https://api.github.com/users/HuggingFaceDocBuilderDev/followers
https://api.github.com/users/HuggingFaceDocBuilderDev/following{/other_user}
https://api.github.com/users/HuggingFaceDocBuilderDev/gists{/gist_id}
https://github.com/HuggingFaceDocBuilderDev
HuggingFaceDocBuilderDev
U_kgDOBfTMJA
https://api.github.com/users/HuggingFaceDocBuilderDev/orgs
https://api.github.com/users/HuggingFaceDocBuilderDev/received_events
https://api.github.com/users/HuggingFaceDocBuilderDev/repos
https://api.github.com/users/HuggingFaceDocBuilderDev/starred{/owner}{/repo}
https://api.github.com/users/HuggingFaceDocBuilderDev/subscriptions
User
https://api.github.com/users/HuggingFaceDocBuilderDev
public
MEMBER
Thanks !
I'm in favor of this change, even though it's a breaking change:

if you had a dataset
```
data/
  train.csv
  test.csv
```

then running this code would now return both train and test splits:
```python
load_dataset("csv", data_dir="data/")
```
whereas right now it returns only a train split with the data from both CSV files.

In my opinion it's ok do do this breaking change because:
- it makes this behavior consistent with `load_dataset("path/to/data")` that also returns both splits: data_files resolution must be the same
- I don't expect too many affected users (unless people really wanted to group train and test images in the train split on purpose ?) compared to the many new users to come (especially with #4069 )
- this usage will become more and more common as we add packaged builder and imagefolder/audiofolder usage grows, so it may be better to do this change early

Let me know if you think this is acceptable @mariosasko @albertvillanova or not, and if you think we need to first have a warning for some time before switching to this new behavior
2022-04-12T12:57:14Z
https://github.com/huggingface/datasets/pull/4144#issuecomment-1096694280
https://api.github.com/repos/huggingface/datasets/issues/4144
IC_kwDODunzps5BXjoI
https://api.github.com/repos/huggingface/datasets/issues/comments/1096694280/reactions
2022-04-12T13:03:43Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1096694280
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
CONTRIBUTOR
Also, if people really want to put train and test, say, images in a single train split they could do 
`load_dataset("imagefolder", data_files={"train": "/path/to/data/**})`. Probably (arguably :)), if this is a more counterintuitive case, then it should require manual files specification, not a default one (in which we expect that users do want to infer splits from filenames / dir structure but currently they have to pass smth like `{"train": "/path/to/data/train*", "test": "/path/to/data/test*"}` explicitly as `data_files`)  
2022-04-12T13:08:32Z
https://github.com/huggingface/datasets/pull/4144#issuecomment-1096706514
https://api.github.com/repos/huggingface/datasets/issues/4144
IC_kwDODunzps5BXmnS
https://api.github.com/repos/huggingface/datasets/issues/comments/1096706514/reactions
2022-04-12T13:11:22Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1096706514
https://avatars.githubusercontent.com/u/16348744?v=4
https://api.github.com/users/polinaeterna/events{/privacy}
https://api.github.com/users/polinaeterna/followers
https://api.github.com/users/polinaeterna/following{/other_user}
https://api.github.com/users/polinaeterna/gists{/gist_id}
https://github.com/polinaeterna
polinaeterna
MDQ6VXNlcjE2MzQ4NzQ0
https://api.github.com/users/polinaeterna/orgs
https://api.github.com/users/polinaeterna/received_events
https://api.github.com/users/polinaeterna/repos
https://api.github.com/users/polinaeterna/starred{/owner}{/repo}
https://api.github.com/users/polinaeterna/subscriptions
User
https://api.github.com/users/polinaeterna
public
COLLABORATOR
I also like this change, and  I don't think we even need a warning during the transition period, considering I've been asked several times since the release of `imagefolder` why splits are not correctly inferred if the directory structure is as follows:
```
data_dir
    train
        label_a
            0.jpg
            ...
        label_b 
            0.jpg
            ...
    test
        label_a
            0.jpg
            ...
        label_b 
            0.jpg
            ...
```
2022-04-13T12:44:14Z
https://github.com/huggingface/datasets/pull/4144#issuecomment-1098006442
https://api.github.com/repos/huggingface/datasets/issues/4144
IC_kwDODunzps5Bcj-q
https://api.github.com/repos/huggingface/datasets/issues/comments/1098006442/reactions
2022-04-13T12:44:14Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1098006442
https://avatars.githubusercontent.com/u/47462742?v=4
https://api.github.com/users/mariosasko/events{/privacy}
https://api.github.com/users/mariosasko/followers
https://api.github.com/users/mariosasko/following{/other_user}
https://api.github.com/users/mariosasko/gists{/gist_id}
https://github.com/mariosasko
mariosasko
MDQ6VXNlcjQ3NDYyNzQy
https://api.github.com/users/mariosasko/orgs
https://api.github.com/users/mariosasko/received_events
https://api.github.com/users/mariosasko/repos
https://api.github.com/users/mariosasko/starred{/owner}{/repo}
https://api.github.com/users/mariosasko/subscriptions
User
https://api.github.com/users/mariosasko
public
MEMBER
Cool ! Feel free to add a test (maybe something similar to `test_PackagedDatasetModuleFactory_with_data_dir` but with a data_dir that contains several splits) and mark this PR as ready for review then @polinaeterna :)
2022-04-13T13:43:54Z
https://github.com/huggingface/datasets/pull/4144#issuecomment-1098068588
https://api.github.com/repos/huggingface/datasets/issues/4144
IC_kwDODunzps5BczJs
https://api.github.com/repos/huggingface/datasets/issues/comments/1098068588/reactions
2022-04-13T13:43:54Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1098068588
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
CONTRIBUTOR
@lhoestq @mariosasko do you think it's a good idea to do the same with `HubDatasetModuleFactoryWithoutScript` and `LocalDatasetModuleFactoryWithoutScript` (see the latest change). If we agree on the current change, doing 
```python
ds = load_dataset("polinaeterna/jsonl_test", data_dir="data/")
```
on dataset with the following structure:
```
train.jsonl
test.jsonl
data/
   train.jsonl
   test.jsonl
```
will result in having two splits from files under `data/` dir in specified repo, while master version returns a single train split. 
The same would be for local dataset without script if doing smth like:
```python
ds = load_dataset("/home/polina/workspace/repos/jsonl_test", data_dir="/home/polina/workspace/repos/jsonl_test/data")
```
(though I'm not sure I understand this use case :D)
Let me know if you think we should preserve the same logic for all factories or if I should roll back this change.
2022-04-19T10:24:04Z
https://github.com/huggingface/datasets/pull/4144#issuecomment-1102476334
https://api.github.com/repos/huggingface/datasets/issues/4144
IC_kwDODunzps5BtnQu
https://api.github.com/repos/huggingface/datasets/issues/comments/1102476334/reactions
2022-04-20T08:40:19Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1102476334
https://avatars.githubusercontent.com/u/16348744?v=4
https://api.github.com/users/polinaeterna/events{/privacy}
https://api.github.com/users/polinaeterna/followers
https://api.github.com/users/polinaeterna/following{/other_user}
https://api.github.com/users/polinaeterna/gists{/gist_id}
https://github.com/polinaeterna
polinaeterna
MDQ6VXNlcjE2MzQ4NzQ0
https://api.github.com/users/polinaeterna/orgs
https://api.github.com/users/polinaeterna/received_events
https://api.github.com/users/polinaeterna/repos
https://api.github.com/users/polinaeterna/starred{/owner}{/repo}
https://api.github.com/users/polinaeterna/subscriptions
User
https://api.github.com/users/polinaeterna
public
CONTRIBUTOR
@lhoestq to test passing subdirectory (`base_path`) to data_files functions and methods, I extended the temporary test directory with data so that it contains subdirectory. Because of that the number of files in this directory increased, so I had to change some numbers and patterns to account for this change - [907ddf0](https://github.com/huggingface/datasets/pull/4144/commits/907ddf09d3afece5afbae18675c859d6e453f2bf)

Do you think it's ok? Another option is to create another tmp dir and do all the checks inside it. 
2022-04-28T14:51:17Z
https://github.com/huggingface/datasets/pull/4144#issuecomment-1112304270
https://api.github.com/repos/huggingface/datasets/issues/4144
IC_kwDODunzps5CTGqO
https://api.github.com/repos/huggingface/datasets/issues/comments/1112304270/reactions
2022-04-28T14:51:17Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1112304270
https://avatars.githubusercontent.com/u/16348744?v=4
https://api.github.com/users/polinaeterna/events{/privacy}
https://api.github.com/users/polinaeterna/followers
https://api.github.com/users/polinaeterna/following{/other_user}
https://api.github.com/users/polinaeterna/gists{/gist_id}
https://github.com/polinaeterna
polinaeterna
MDQ6VXNlcjE2MzQ4NzQ0
https://api.github.com/users/polinaeterna/orgs
https://api.github.com/users/polinaeterna/received_events
https://api.github.com/users/polinaeterna/repos
https://api.github.com/users/polinaeterna/starred{/owner}{/repo}
https://api.github.com/users/polinaeterna/subscriptions
User
https://api.github.com/users/polinaeterna
public
fixes #4150

I suggest to infer splits structure from files when `data_dir` is passed with `get_patterns_locally`, analogous to what's done in `LocalDatasetModuleFactoryWithoutScript` with `self.path`, instead of generating files with `data_dir/**` patterns and putting them all into a single default (train) split.

I would also suggest to align `HubDatasetModuleFactoryWithoutScript` and `LocalDatasetModuleFactoryWithoutScript` with this logic (remove `data_files = os.path.join(data_dir, "**")`). It's not reflected in the current code now as I'd like to discuss it cause I might be unaware of some use cases. @lhoestq @mariosasko @albertvillanova  WDYT?