Discuss whether support canonical datasets w/o dataset_infos.json and/or dummy data
Opened by albertvillanova (closed)

I open this PR to have a public discussion about this topic and make a decision.

As previously discussed, once we have the metadata in the dataset card (README file, containing both Markdown info and YAML tags), what is the point of having also the JSON metadata (dataset_infos.json file)?

On the other hand, the dummy data is necessary for testing (in our CI suite) that the canonical dataset loads correctly. However:
- the dataset preview feature is already an indirect test that the dataset loads correctly (it also tests it is streamable though)
- we are migrating canonical datasets to the Hub
Do we really need to continue testing them in out CI?

Also note that for generating both (dataset_infos.json file and dummy data), the entire dataset needs being downloaded. This can be an issue for huge datasets (like WIT, with 400 GB of data).

Feel free to ping other people for the discussion.

CC: @lhoestq @mariosasko @thomwolf @julien-c @patrickvonplaten @anton-l @LysandreJik @yjernite @nateraw 

Comments: IMO, the data streaming test is good enough of a test that the dataset works correctly (assuming that we can more or less ensure that if streaming works then the non-streaming case will also work), so that for datasets that have a working dataset preview, we can remove the dummy data IMO. On the other hand, it seems like not all datasets have streaming enabled yet and for those datasets (if they are used a lot), I think it would be nice to continue testing some dummy data.

I don't really have an opinion regarding the JSON metadata as I don't know enough about it.



I don't know all the details, but generally I'd be in favor of unifying the metadata formats into YAML inside .md (and so deprecating the dataset_infos.json) 

(Ultimately the CI can run on "HuggingFace Actions" instead of on GitHub)

The dataset_infos.json file currently has these useful infos for each dataset configuration, that I think can be moved to the dataset tags:
- Size of the dataset in MB: download size, arrow file size, and total size (sum of download + arrow)
- Size of each split in MB and number of examples. Again this can be moved to the dataset tags
- Feature type of each column
- supported task templates (it defines what columns correspond to the features and labels for example)

But it also has this, which I'm not sure if it should be in the tags or not:
- Checksums of the downloaded files for integrity verifications

So ultimately this file could probably be deprecated in favor of having the infos in the tags.

> Also note that for generating both (dataset_infos.json file and dummy data), the entire dataset needs being downloaded. This can be an issue for huge datasets (like WIT, with 400 GB of data).

To get the exact number of examples and size in MB of the dataset, one needs to download and generate it completely. IMO these infos are very important when someone considers using a dataset. Though using streaming we could do some extrapolation to have approximate values instead.

For the integrity verifications we also need the number of examples and the checksums of the downloaded files, so it requires the dataset to be fully downloaded once. This can be optional though.

> IMO, the data streaming test is good enough of a test that the dataset works correctly (assuming that we can more or less ensure that if streaming works then the non-streaming case will also work)

I agree with this. Usually if a dataset works in streaming mode, then it works in non-streaming mode (the other way around is not true though).

> On the other hand, it seems like not all datasets have streaming enabled yet and for those datasets (if they are used a lot), I think it would be nice to continue testing some dummy data.

Yes indeed, or at least make sure that it was tested on the true data.

(note that if we wanted to display sizes, etc we could also pretty easily parse the `dataset_infos.json` on the hub side)

I agree that we can move the relevant parts of `dataset_infos.json` to the YAML tags.

> On the other hand, it seems like not all datasets have streaming enabled yet and for those datasets (if they are used a lot), I think it would be nice to continue testing some dummy data. <
> > Yes indeed, or at least make sure that it was tested on the true data.

I like the idea of testing streaming and falling back to the dummy data test if streaming does not work. Generating dummy data can be very tedious, so this would be a nice incentive for the contributors to make their datasets streamable.  

CC: @severo 

About dummy data, please see e.g. this PR: https://github.com/huggingface/datasets/pull/3692/commits/62368daac0672041524a471386d5e78005cf357a
- I updated the previous dummy data: I just had to rename the file and its directory
  - the dummy data zip contains only a single file: `pubmed22n0001.xml.gz`

Then I discover it fails: https://app.circleci.com/pipelines/github/huggingface/datasets/9800/workflows/173a4433-8feb-4fc6-ab9e-59762084e3e1/jobs/60437
```
No such file or directory: '.../dummy_data/pubmed22n0002.xml.gz'
```
- it needs dummy data for all the 1114 files: 
  `_URLs = [f"ftp://ftp.ncbi.nlm.nih.gov/pubmed/baseline/pubmed22n{i:04d}.xml.gz" for i in range(1, 1115)]`
- this confirms me that it never passed the test: these dummy data files were not present before my PR
- therefore, is it really useful the data test if we just ignore it when it does not pass?

In relation with JSON metadata, I'm generating the file for `pubmed` (see above) in a GCP instance: it's running for more than 3 hours and only 9 million examples generated so far (before my PR, it had 32 million, now it has more).

I mention in https://github.com/huggingface/datasets-server/wiki/Preliminary-design that the future "datasets server" could be in charge of generating both the dummy data and the dataset-info.json file if required (or their equivalent).

Hi ! I think dummy data generation is out of scope for the datasets server, since it's about generating the original data files.

That would be amazing to have it generate the dataset_infos.json though !

From some offline discussion with @mariosasko and especially for vision datasets, we'll probably not require dummy data anymore and use streaming instead :) This will make adding a new dataset much easier.
This should also make sure that streaming works as expected directly in the CI, without having to check the dataset viewer once the PR is merged

OK. I removed the "dummy data" item from the services of the dataset server

It seems that migration from dataset-info.json to dataset card YAML has been acted.

Probably it's a good idea, but I didn't find the pros and cons of this decision, so I put some I could think of:

pros:
- only one file to parse, share, sync
- it gives a hint to the users that if you write your dataset card, you should also specify the metadata

cons:
- the metadata header might be very long, before reaching the start of the README/dataset card. It might be surprising when you edit the file because the metadata is not shown on top when the dataset card is rendered (dataset page). It also somewhat prevents including large strings like the checksums
- YAML vs JSON: not sure which one is easier for users to fill and maintain
- two concepts are mixed in the same file (metadata and documentation). This means that if you're interested only in one of them, you still have to know how to parse the whole file.
- [low priority] besides the JSON file, we might want to support yaml or toml file if the user prefers (as [prettier](https://prettier.io/docs/en/configuration.html) and others do for their config files, for example). Inside the md, I understand that only YAML is allowed

> the metadata header might be very long, before reaching the start of the README/dataset card. It might be surprising when you edit the file because the metadata is not shown on top when the dataset card is rendered (dataset page). It also somewhat prevents including large strings like the checksums

Note that we could simply not have the checksums in the YAML metadata at all, or maybe at one point have a pointer to another file instead.

We can also choose to hide (collapse) certain sections in the YAML by default when we open the dataset card editor.

> two concepts are mixed in the same file (metadata and documentation). This means that if you're interested only in one of them, you still have to know how to parse the whole file.

I think it's fine for now. Later if we really end up with too many YAML sections we can see if we need to tweak the API endpoints or the `datasets`/`huggingface_hub` tools

> YAML vs JSON: not sure which one is easier for users to fill and maintain

Regarding YAML vs JSON: I think YAML is easier to write by hand, and I also think that it's better for consistency - i.e. we're using more and more YAML to configure models/datasets/spaces

I didn't know the decision was already taken. Good to know. 😅

> the metadata header might be very long, before reaching the start of the README/dataset card. It might be surprising when you edit the file because the metadata is not shown on top when the dataset card is rendered (dataset page). It also somewhat prevents including large strings like the checksums

We can definitely work on this on the hub side to make the UX better

Tensorflow Datasets catalog includes a community catalog where you can find and use HF datasets (see [here](https://www.tensorflow.org/datasets/community_catalog/huggingface)).

FYI I noticed today that they are using the exported dataset_infos.json files from github to get the metadata (see their code [here](https://github.com/tensorflow/datasets/blob/a482f01c036a10496f5e22e69a2ef81b707cc418/tensorflow_datasets/scripts/documentation/build_community_catalog.py#L261))

Metadata is now stored as YAML, and dummy data is deprecated, so I think we can close this issue.