Add imagefolder dataset
Opened by nateraw (closed)

A generic imagefolder dataset inspired by `torchvision.datasets.ImageFolder`. 

Resolves #2508 

---

Example Usage:

[![Open In Colab](https://colab.research.google.com/assets/colab-badge.svg)](https://colab.research.google.com/gist/nateraw/954fa8cba4ff806f6147a782fa9efd1a/imagefolder-official-example.ipynb)

Comments:
@lhoestq @albertvillanova it would be super cool if we could get the Image Classification task to work with this. I'm not sure how to have the dataset find the unique label names _after_ the dataset has been loaded. Is that even possible? 

My hacky community version [here](https://huggingface.co/datasets/nateraw/image-folder) does this, but it wouldn't pass the test suite here. Any thoughts?

Hi ! Dataset builders that require some `data_files` like `csv` or `json` are handled differently that actual dataset scripts.

In particular:
- they are placed directly in the `src` folder of the lib so that you can use it without internet connection (more exactly in `src/datasets/packaged_modules/<builder_name>.py`). So feel free to move the dataset python file there. You also need to register it in `src/datasets/packaked_modules.__init__.py`
- they are handled a bit differently in our test suite (see the `PackagedDatasetTest` class in `test_dataset_common.py`). To be able to test the builder with your dummy data, you just need to modify `get_packaged_dataset_dummy_data_files` in `test_dataset_common.py` to return the right `data_files` for your builder. The dummy data can stay in `datasets/image_folder/dummy`

Let me know if you have questions or if I can help !

Hey @lhoestq , I actually already did both of those things. I'm trying to get the `image-classification` task to work now.  

For example...When you run `ds = load_dataset('imagefolder', data_files='my_files')`, with a directory called `./my_files` that looks like this:

```
my_files
----| Cat
--------| image1.jpg
--------| ...
----| Dog
--------| image1.jpg
--------| ...
```

...We should set the dataset's `labels` feature to `datasets.features.ClassLabel(names=['cat', 'dog'])`  dynamically with class names we find by getting a list of directories in `my_files` (via `data_files`). Otherwise the `datasets.tasks.ImageClassification` task will break, as the `labels` feature is not a `ClassLabel`.

I couldn't figure out how to access the `data_files` in the builder's `_info` function in a way that would pass in the test suite. 

Nice ! Then maybe you can use `self.config.data_files` in `_info()` ?
What error are you getting in the test suite ?

Also note that `data_files` was first developed to accept paths to actual files, not directories. In particular, it fetches the metadata of all the data_files to get a unique hash for the caching mechanism. So we may need to do a few changes first.

I'm trying to make it work by getting the label names in the _info automatically.
I'll let you know tomorrow how it goes :)

Also cc @mariosasko since we're going to use #3163 

Right now I'm getting the label name per file by taking the first word (from regex `\w+`) after the common prefix of all the files per split

Data files resolution takes too much time on my side for a dataset of a few 10,000s of examples. I'll speed it up with some multihreading tomorrow, and maybe by removing the unnecessary checksum verification

The code is a bit ugly for my taste. I'll try to simplify it tomorrow by avoiding the `os.path.commonprefix` computation and do something similar to @nateraw's ImageFolder instead, where only the second-to-last path component is considered a label (and see if I can update the class labels lazily in `_generate_examples`).

Also, as discussed offline with @lhoestq,  I reverted the automatic directory globbing change in `data_files.py` and will investigate if we can use `data_dir` for that (e.g. `load_dataset("imagefolder", data_dir="path/to/data")` would be equal to `load_dataset("imagefolder", data_files=["path/to/data/**/*", "path/to/data/*"])`. The only problem with `data_dir`  that it's equal to `dl_manager.manual_dir`, which would break scripts with `manul_download_instructions`, so maybe we can limit this behavior only to the packaged loaders? WDYT?

An updated example of usage: [![Open In Colab](https://colab.research.google.com/assets/colab-badge.svg)](https://colab.research.google.com/drive/1zJkAQm0Dk372EHcBq6hYgHuunsK4rR0r?usp=sharing)

> The code is a bit ugly for my taste. I'll try to simplify it tomorrow by avoiding the os.path.commonprefix computation and do something similar to @nateraw's ImageFolder instead, where only the second-to-last path component is considered a label (and see if I can update the class labels lazily in _generate_examples).

Sounds good ! It's fine if we just support the same format as pytorch ImageFolder.

Regarding the `data_dir` parameter, what do you think is best ?

1. `dl_manager.data_dir = data_dir`
2. `dl_manager.data_files = resolve(os.path.join(data_dir, "**"))`

or something else ?

> The only problem with data_dir that it's equal to dl_manager.manual_dir, which would break scripts with manul_download_instructions, so maybe we can limit this behavior only to the packaged loaders? WDYT?

We can still have `dl_manager.manual_dir = data_dir` though

The example colab is amazing !

@lhoestq 
>Regarding the `data_dir` parameter, what do you think is best ?
>
>1. `dl_manager.data_dir = data_dir`
>2. `dl_manager.data_files = resolve(os.path.join(data_dir, "**"))`

The second option. Basically, I would like `data_files` to be equal to:
```python
def _split_generators(self, dl_manager):
    data_files = self.config.data_files
    if data_files is None:    
        data_files = glob.glob("{self.config.data_dir}/**", recursive=True)
    else:
        raise ValueError(f"At least one data file must be specified, but got data_files={data_files}")
```
in the scripts of packaged modules. It's probably better to do the resolution in `data_files.py` tho (to handle relative file paths on the Hub, for instance)

> The second option. Basically, I would like data_files to be equal to:
> ```python
> def _split_generators(self, dl_manager):
>     data_files = self.config.data_files
>     if data_files is None:    
>         data_files = glob.glob("{self.config.data_dir}/**", recursive=True)
>     else:
>         raise ValueError(f"At least one data file must be specified, but got data_files={data_files}")
> ```
> in the scripts of packaged modules. It's probably better to do the resolution in data_files.py tho (to handle relative file paths on the Hub, for instance)

sounds good !

🙌

Hey @mariosasko are we still actually able to load an image folder?

For example...

```
! wget https://download.microsoft.com/download/3/E/1/3E1C3F21-ECDB-4869-8368-6DEBA77B919F/kagglecatsanddogs_3367a.zip
! unzip kagglecatsanddogs_3367a.zip
```

followed by

```python
from datasets import load_dataset

# Does not work
ds = load_dataset('imagefolder', data_files='/PetImages')

# Also doesn't work
ds = load_dataset('imagefolder', data_dir='/PetImages')
```

Are we going forward with the assumption that the user always wants to download from URL and that they won't have a dataset locally already? This at least gets us part of the way, but is technically not an "imagefolder" as intended. 

Either way, was delighted to see the colab notebook work smoothly outside of the case I just described above. ❤️ thanks so much for the work here.

> Hey @mariosasko are we still actually able to load an image folder?
> 
> For example...
> 
> ```
> ! wget https://download.microsoft.com/download/3/E/1/3E1C3F21-ECDB-4869-8368-6DEBA77B919F/kagglecatsanddogs_3367a.zip
> ! unzip kagglecatsanddogs_3367a.zip
> ```
> 
> followed by
> 
> ```python
> from datasets import load_dataset
> 
> # Does not work
> ds = load_dataset('imagefolder', data_files='/PetImages')

I ran into this too when I was trying to out. At the moment you can still load from a local on disk directory using a glob pattern i.e. 

```python
from datasets import load_dataset
ds = load_dataset("imagefolder", data_files="PetImages/**/*")
```
[Colab example](https://colab.research.google.com/drive/1IvAyYSAADHphzbtJMt02OXmnGwGtku3k?usp=sharing). I'm not sure if that is the intended behaviour or not. If it is, I think it would be good to document this because I also assumed the approach @nateraw used would work.