`pretty_name` for dataset in YAML tags
Opened by bhavitvyamalik (closed)

I'm updating `pretty_name` for datasets in YAML tags as discussed with @lhoestq. Here are the first 10, please let me know if they're looking good.

If dataset has 1 config, I've added `pretty_name` as `config_name: full_name_of_dataset` as config names were `plain_text`, `default`, `squad` etc (not so important in this case) whereas when dataset has >1 configs, I've added `config_name: full_name_of_dataset+config_name` so as to let user know about the `config` here. 

Comments: Initially I removed the ` - ` since there was only one `pretty_name` per config but turns out it was breaking here in `from_yaml_string`https://github.com/huggingface/datasets/blob/74751e3f98c74d22c48c6beb1fab0c13b5dfd075/src/datasets/utils/metadata.py#L197 in `/utils/metadata.py`

@lhoestq I guess this will also need some validation?

Looks like the parser doesn't allow things like
```
pretty_name:
  config_name1: My awesome config number 1
  config_name2: My amazing config number 2
```
therefore you had to use `-` and consider them as a list.

I would be nice to add support for this case in the validator.

There's one thing though: the DatasetMetadata object currently corresponds to the yaml tags that are flattened: the config names are just ignored, and the lists are concatenated.

Therefore I think we would potentially need to instantiate several `DatasetMetadata` objects: one per config. Otherwise we would end up with a list of several pretty_name while we actually need at most 1 per config.

What do you think @gchhablani ?

I was thinking of returning `metada_dict` (on line 193) whenever `load_dataset_card` is called (we can pass an extra parameter to `from_readme` or `from_yaml_string` for that to happen).

One just needs config_name as key for the dictionary inside `pretty_name` dict and for single config, there would be only one value to print. We can do this for other fields as well like `size_categories`, `languages` etc. This will obviate the need to flatten the YAML tags so that don't have to instantiate several DatasetMetadata objects. What do you guys think @lhoestq @gchhablani? 

Update: I was thinking of returning the whole dictionary before flattening so that user can access whatever they want with specific configs. Let's say [this](https://pastebin.com/eJ84314f) is my `metadata_dict` before flattening (the loaded YAML string), so instead of validating it and then returning the items individually we can return it just after loading the YAML string.

Hi @lhoestq @bhavitvyamalik 

@bhavitvyamalik, I'm not sure I understand your approach, can you please elaborate? The `metadata_dict` is flattened before instantiating the object, do you want to remove that? Still confused.

Few things come to my mind after going through this PR. They might not be entirely relevant to the current task, but I'm just trying to think about possible cases and discuss them here.

1. Instead of instantiating a new `DatasetMetadata` for each config with flattened tags, why can't we make it more flexible and validate only non-dict items? However, in that case, the types wouldn't be as strict for the class attributes. It would also not work for cases that are like `Dict[str,List[Dict[str,str]]`, but I guess that won't be needed anyway in the foreseeable future?

    Ideally, it would be something like - Check the metadata tag type (root), do a DFS, and find the non-dict objects (leaves), and validate them. Is this an overkill to handle the problem?
2. For single-config datasets, there can be slightly different validation for `pretty_names`, than for multi-config. The user shouldn't need to provide a config name for single-config datasets, wdyt @bhavitvyamalik @lhoestq? Either way, for multi-config, the validation can use the dictionary keys in the path to that leaf node to verify `pretty_names: ... (config)` as well. This will check whether the config name is same as the key (might be unnecessary but prevents typos, so less work for the reviewer(s)). For future, however, it might be beneficial to have something like this.
3. Should we have a default config name for single-config datasets? People use any string they feel like. I've seen `plain_text`, `default` and the dataset name. I've used `image` for a few datasets myself AFAIR. For smarter validation (again, a future case ;-;), it'd be easier for us to have a simple rule for naming configs in single-config datasets. Wdyt @lhoestq?

Btw, `pretty_names` can also be used to handle this during validation :P 

```
-# Dataset Card for [Dataset Name]
+# Dataset Card for Allegro Reviews
```

This is where `DatasetMetadata` and `ReadMe` should be combined. But there are very few overlaps, I guess.


@bhavitvyamalik @lhoestq What about adding a pretty name across all configs, and then config-specific names?

Like

```yaml
pretty_names:
    all_configs: X (dataset_name)
    config_1: X1 (config_1_name)
    config_2: X2 (config_2_name)
```
Then, using the `metadata_dict`, the ReadMe header can be validated against `X`.

Sorry if I'm throwing too many ideas at once.

@bhavitvyamalik

Now, I think I better understand what you're saying. So you want to skip validation for the unflattened metadata and just return it? And let the validation run for the flattened version?

Exactly! Validation is important but once the YAML tags are validated I feel we shouldn't do that again while calling `load_dataset_card`. +1 for default config name for single-config datasets.

@bhavitvyamalik
Actually, I made the `ReadMe` validation similar to `DatasetMetadata` validation and the class was validating the metadata during the creation. 

Maybe we need to have a separate validation method instead of having it in `__post_init__`? Wdyt @lhoestq? 

I'm sensing too many things to look into. It'd be great to discuss these sometime. 

But if this PR is urgent then @bhavitvyamalik's logic seems good to me. It doesn't need major modifications in validation.

> Maybe we need to have a separate validation method instead of having it in __post_init__? Wdyt @lhoestq?

We can definitely have a `is_valid()` method instead of doing it in the post init.

> What about adding a pretty name across all configs, and then config-specific names?

Let's keep things simple to starts with. If we can allow both single-config and multi-config cases it would already be great :)

for single-config:
```yaml
pretty_name: Allegro Reviews
```

for multi-config:
```yaml
pretty_name:
  mrpc: Microsoft Research Paraphrase Corpus (MRPC)
  sst2: Stanford Sentiment Treebank
  ...
```

To support the multi-config case I see two options:
1. Don't allow DatasetMetadata to have dictionaries but instead have separate DatasetMetadata objects per config
2. allow DatasetMetadata to have dictionaries. It implies to remove the flattening step. Then we could get metadata for a specific config this way for example:
```python
from datasets import load_dataset_card

glue_dataset_card = load_dataset_card("glue")
print(glue_dataset_card.metadata)
# DatasetMetatada object with dictionaries since there are many configs
print(glue_dataset_card.metadata.get_metadata_for_config("mrpc"))
# DatasetMetatada object with no dictionaries since there are only the mrpc tags
```

Let me know what you think or if you have other ideas.

I think Option 2 is better.

Just to clarify, will `get_metadata_for_config`  also return common details, like language, say?

> Just to clarify, will get_metadata_for_config also return common details, like language, say?

Yes that would be more convenient IMO. For example a dataset card like this
```yaml
languages:
- en
pretty_name:
  config1: Pretty Name for Config 1
  config3: Pretty Name for Config 2
```

then `metadat.get_metadata_for_config("config1")`  would return something like
```python
DatasetMetadata(languages=["en"], pretty_name="Pretty Name for Config 1")
```

@lhoestq, should we do this post-processing in `load_dataset_card` by returning unflattened dictionary from `DatasetMetadata` or send this from `DatasetMetadata`? Since there isn't much to do I feel once we have the unflattened dictionary

Not sure I understand the difference @bhavitvyamalik , could you elaborate please ?

I was talking about this unflattened dictionary:

> I was thinking of returning the whole dictionary before flattening so that user can access whatever they want with specific configs. Let's say [this](https://pastebin.com/eJ84314f) is my metadata_dict before flattening (the loaded YAML string), so instead of validating it and then returning the items individually we can return it just after loading the YAML string.

Post-processing meant extracting config-specific fields from this dictionary and then return this `languages=["en"], pretty_name="Pretty Name for Config 1"`

I still don't understand what you mean by "returning unflattened dictionary from DatasetMetadata or send this from DatasetMetadata", sorry. Can you give an example or rephrase this ?

IMO load_dataset_card can return a dataset card object with a metadata field. If the metadata isn't flat (i.e. it has several configs), you can get the flat metadata of 1 specific config with `get_metadata_for_config`. But of course if you have better ideas or suggestions, we can discuss this

@lhoestq,  I think he is saying whatever `get_metadata_for_config` is doing can be done in `load_dataset_card` by taking the unflattened `metadata_dict` as input.

@bhavitvyamalik, I think it'd be better to have this "post-processing" in `DatasetMetadata` instead of `load_dataset_card`, as @lhoestq has shown. I'll quickly get on that.

---
Three things that are to be changed in `DatasetMetadata`:
1. Allow Non-flat elements and their validation.
2. Create a method to get metadata by config name.
3. Create a `validate()` method.

Once that is done, this PR can be updated and reviewed, wdys?

Thanks @gchhablani for the help ! Now that https://github.com/huggingface/datasets/pull/2436 is merged you can remove the `-` in the pretty_name @bhavitvyamalik  :)

Thanks @bhavitvyamalik.

I think this PR was superseded by these others also made by you:
-  #3498
- #3536

I'm closing this.