fix: files counted twice in inferred structure
Opened by borisdayma (closed)

Files were counted twice in a structure like:
```
my_dataset_local_path/
├── README.md
└── data/
    ├── train/
    │   ├── shard_0.csv
    │   ├── shard_1.csv
    │   ├── shard_2.csv
    │   └── shard_3.csv
    └── valid/
        ├── shard_0.csv
        └── shard_1.csv
```

The reason is that they were matching both `*train*/*` and `*train*/**/*`.

This PR fixes it. @lhoestq 

Comments:
I see it creates some errors in the tests.

Another solution if needed is to add something like `data_files = list(set(data_files))` after [this line](https://github.com/huggingface/datasets/blob/8555197a3fe826e98bd0206c2d031c4488c53c5c/src/datasets/data_files.py#L511)

Hi ! Thanks for the correction :)

Your change seems right, let me look at the errors and try to fix this

Not sure if it's due to this change but I tested `load_dataset('dalle-mini/encoded-vqgan_imagenet_f16_16384', streaming=True)` and the `validation` set is empty.

So indeed there was an issue with the patterns `*` and `**/*` that would return some files twice. This issue came from the fact that we were not using the right `glob`.

Indeed we were using `Path.rglob` for local files and `Path.match` for remote files. Since these two methods don't have the same behavior for such patterns, I decided to change that.

In particular, we now use `glob.glob` (same as `fsspec` glob) as a reference for data files resolution from patterns. This is the same as dask for example.

/!\ Here are some behaviors specific to `glob.glob` that are different from Path.glob, Path.match or fnmatch:
- '*' matches only first level files
- '**/*' matches only at least second level files

This way we have a consistent behavior with respect to other python data libraries and there's no overlap anymore between the two patterns.

Some implementations details:

To ensure that we have the same behavior for local files and for files in a remote dataset repository, I decided to use `fsspec` glob for both. This was made possible by implementing the `HfFileSystem` class as a `fsspec` filesystem.

I pushed those changes directly to your PR - I hope you don't mind. I'm still fixing the remaining tests.
Please let me know if that solves your problem, and then we can merge !

There's still an issue with fsspec's glob - I'll take a look this afternoon

I just found out that actually glob.glob and fsspec glob are different haha
glob.glob needs `**/*` and recursive=True to look into deep subdirectories, while fsspec only requires `**`

I think we can go with fsspec glob for consistency with dask and since it's our main tool for filesystems management

To recap:
```
We use fsspec glob as a reference for data files resolution from patterns.
This is the same as dask for example.

/!\ Here are some behaviors specific to fsspec glob that are different from glob.glob, Path.glob, Path.match or fnmatch:
- '*' matches only first level items
- '**' matches all items
- '**/*' matches all at least second level items

More generally:
- `*`` matches any character except a forward-slash (to match just the file or directory name)
- `**`` matches any character including a forward-slash /
```

lol Windows… Maybe `Pathlib` for the tests?

I tested streaming a repo and it worked perfectly now!