Fix splits in local packaged modules, local datasets without script and hub datasets without script
State: closed

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?

Comments:
_The documentation is not available anymore as the PR was closed or merged._

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

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`)  

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
            ...
```

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 :)

@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.

@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.