Add support for different configs with `push_to_hub`
State: closed
Labels: description: New feature or request
name: enhancement

will solve  #5151

@lhoestq @albertvillanova @mariosasko 
This is still a super draft so please ignore code issues but I want to discuss some conceptually important things.

I suggest a way to do `.push_to_hub("repo_id", "config_name")` with pushing parquet files to directories named as `config_name` (inside `data/` dir as it is now), for example: 
```
data
   |__config-v1
        train-00000-00002-...-.parquet
        train-00001-00002-...-.parquet
        ...
   |__config-v2
        ....
```

When loading a dataset, I parse these configs from repository data files (only for `"data/{split}-[0-9][0-9][0-9][0-9][0-9]-of-[0-9][0-9][0-9][0-9][0-9]*.*"` pattern that is used for parquet datasets pushed with `.push_to_hub`). 
Therefore, 
- when user tries to load a dataset that has configs parsed from data files dir names without providing a config (like `load_dataset("repo")` instead of `load_dataset("repo", "config-v1")`) - raise error and asks for config - to be aligned with how it works in datasets with scripts.
- for backward compatibility: if user tries to `.push_to_hub(""repo", "config_name")` to an existing parquet repo with no configurations (all parquet files are directly in `data/` dir) - raise error. My initial idea was to raise a warning and move these files to another dir with name (config) like "default" or smth but in a PR and suggest user to merge it on the Hub. But there is no support for renaming (moving) files via `HfApi` yet so it would require deleting and pushing again if I understand it right.

This parsing approach can be extended to other Hub packaged modules, and to local packaged modules and other data files patterns
(except for cases when splits are in dir names `KEYWORDS_IN_DIR_NAME_BASE_PATTERNS` because we allow for arbitrary depth of directory hierarchy).
Do you think it's reasonable? Not sure how to provide flexibility (and backward compatibility) to not parsing configs and load all the data in a single config as it is now.

I also thought about getting information about configs from Readme.md `dataset_info` ([example](https://huggingface.co/datasets/polinaeterna/test_push_two_configs/blob/main/README.md)). But that way we 
are dependent on if it exists. It is created automatically with `.push_to_hub` but what if it is 
accidentally deleted or smth). 

Also, what I don't like is that this parsing is a part of Module/DataFiles logic, not Builder's one, which is not aligned with datasets with custom scripts. But I don't know to implement the second approach in current library's logic. 

 What do you think about this all? Am I missing smth?


TODO:

- [ ] save cache in the same dir for configs of the same datasets
- [ ] fix verification errors
- [ ] correctly update `dataset_infos.json` too
- [ ] ...
 

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

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/datasets/pr_5213). All of your documentation changes will be reflected on that endpoint.

The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/datasets/pr_5213). All of your documentation changes will be reflected on that endpoint.

Nice thanks !

Would it be possible to have the new folders at the same level as "data" ? This way they're all separated
```
├─ config-v1/
│  ├── train-00000-00002-...-.parquet
│  └── train-00001-00002-...-.parquet
└ config-v2/
    ├── train-00000-00002-...-.parquet
    └── train-00001-00002-...-.parquet
```
and if you don't provide a config name, it goes in a folder named "default" instead, that would be loaded by default.

We could also write in the YAML something like
```yaml
configs:
- name: config-v1
  data_dir: config-v1
- name: config-v2
  data_dir: config-v2
```
and loading `config-v1` would be equivalent to run `load_dataset(ds_name, "config-v1", data_dir="config-v1")`

Do you think it would make sense ?

For backward compatibility we can just keep the "data/*" pattern. It's ok to expect users to have an updated version of `datasets` to be able to load datasets with configurations.

@lhoestq thank you for the feedback! i'll reflect on this on Moday, my mind just melted because of the fever.

@mariosasko  @albertvillanova what do you think?

Thanks for addressing this, @polinaeterna. It is good:
- we support configs for datasets without scripts
- we align the behavior to datasets with scripts as much as possible

Maybe adding some tests will help clarify what is the expected behavior...

After some discussion with @lhoestq we decided that it's better to rely on metadata file than on data files patterns. 

So we decided to introduce a new field to yaml (like `configs` or smth like that) that would contain arbitrary configs kwargs to be passed to loader, including `data_dir` and `data_files`. 
This is more aligned with datasets with custom scripts where we explicitly write all the supported configs and config parameters in the code and is extendable to all packaged modules.
This would solve https://github.com/huggingface/datasets/issues/5209

(@lhoestq was right 21 days ago, this is a more general solution idk why i ignored this...)

closed in favor of https://github.com/huggingface/datasets/pull/5331