t20062443/AT1ds huggingface dataset:
thomwolf (MEMBER) commented on 2020-04-15 09:46:10:
Ok great, so as discussed today, let's:
- have a main dataset directory inside the lib with sub-directories hashed by the content of the file
- keep a cache for downloading the scripts from S3 for now
- later: add methods to list and clean the local versions of the datasets (and the distant versions on S3 as well)

Side question: do you often use `builder_kwargs` for other things than supplying file paths? I was thinking about having a more easy to read and remember `data_files` argument maybe.
jplu (CONTRIBUTOR) commented on 2020-04-15 09:55:08:
Good plan!

Yes I do use `builder_kwargs` for other things such as:
- dataset name
- properties to know how to properly read a CSV file: do I have to skip the first line in a CSV, which delimiter is used, and the columns ids to use.
- properties to know how to properly read a JSON file: which properties in a JSON object to read
patrickvonplaten (CONTRIBUTOR) commented on 2020-04-23 07:38:53:
I think this can be quickly reproduced. 
I use `black, version 19.10b0`. 

When running: 
`black nlp/src/arrow_reader.py` 
it gives me: 

```
error: cannot format /home/patrick/hugging_face/nlp/src/nlp/arrow_reader.py: cannot use --safe with this file; failed to parse source file.  AST error message: invalid syntax (<unknown>, line 78)
Oh no! 💥 💔 💥
1 file failed to reformat.
```

The line in question is: 
https://github.com/huggingface/nlp/blob/6922a16705e61f9e31a365f2606090b84d49241f/src/nlp/arrow_reader.py#L78

What is weird is that the trainer file in `transformers` has more or less the same syntax and black does not fail there: 
https://github.com/huggingface/transformers/blob/cb3c2212c79d7ff0a4a4e84c3db48371ecc1c15d/src/transformers/trainer.py#L95

I googled quite a bit about black & typing hints yesterday and didn't find anything useful. 
Any ideas @thomwolf @julien-c @LysandreJik ?
patrickvonplaten (CONTRIBUTOR) commented on 2020-04-23 07:48:03:
> I think this can be quickly reproduced.
> I use `black, version 19.10b0`.
> 
> When running:
> `black nlp/src/arrow_reader.py`
> it gives me:
> 
> ```
> error: cannot format /home/patrick/hugging_face/nlp/src/nlp/arrow_reader.py: cannot use --safe with this file; failed to parse source file.  AST error message: invalid syntax (<unknown>, line 78)
> Oh no! 💥 💔 💥
> 1 file failed to reformat.
> ```
> 
> The line in question is:
> https://github.com/huggingface/nlp/blob/6922a16705e61f9e31a365f2606090b84d49241f/src/nlp/arrow_reader.py#L78
> 
> What is weird is that the trainer file in `transformers` has more or less the same syntax and black does not fail there:
> https://github.com/huggingface/transformers/blob/cb3c2212c79d7ff0a4a4e84c3db48371ecc1c15d/src/transformers/trainer.py#L95
> 
> I googled quite a bit about black & typing hints yesterday and didn't find anything useful.
> Any ideas @thomwolf @julien-c @LysandreJik ?

Ok I found the problem. It was the one Julien mentioned and has nothing to do with this line. Black's error message is a bit misleading here, I guess
patrickvonplaten (CONTRIBUTOR) commented on 2020-04-23 09:31:46:
> I think I'm fine with this.
> 
> The alternative would be to host a small subset of the dataset on the S3 together with the testing script. But I think having all (test file creation + actual tests) in one file is actually quite convenient.
> 
> Good for me!
> 
> One question though, will we have to create one test file for each of the 100+ datasets or could we make some automatic conversion from tfds dataset test files?

I think if we go the way shown in the PR we would have to create one test file for each of the 100+ datasets. 

As far as I know the tfds test files all rely on the user having created a special download folder structure in `tensorflow-datasets/tensorflow_datasets/testing/test_data/fake_examples`. 

My hypothesis was: 
Becasue, we don't want to work with PRs, no `dataset_script` is going to be in the official repo, so no `dataset_script_test` can be in the repo either. Therefore we can also not have any "fake" test folder structure in the repo. 

**BUT:** As you mentioned @thom, we could have a fake data structure on AWS. To add a test the user has to upload multiple small test files when uploading his data set script. 

So for a cli this could look like:
`python nlp-cli upload <data_set_script> --testfiles <relative path to test file 1> <relative path to test file 2> ...` 

or even easier if the user just creates the dataset folder with the script inside and the testing folder structure, then the API could look like:

`python nlp-cli upload <path/to/dataset/folder>`

and the dataset folder would look like
```
squad
- squad.py
- fake_data  # this dir would have to have the exact same structure we get when downloading from the official squad data url
```

This way I think we wouldn't even need any test files at all for each dataset script. For special datasets like `c4` or `wikipedia` we could then allow to optionally upload another test script. 
We just assume that this is our downloaded `url` and check all functionality from there. 

Thinking a bit more about this solution sounds a) much less work and b) even easier for the user.

A small problem I see here though:
1) What do we do when the depending on the config name the downloaded folder structure is very different? I think for each dataset config name we should have one test, which could correspond to one "fake" folder structure on AWS

@thomwolf What do you think? I would actually go for this solution instead now.
@mariamabarham You have written many more tfds dataset scripts and tests than I have - what do you think?
patrickvonplaten (CONTRIBUTOR) commented on 2020-04-23 09:39:51:
Regarding the tfds tests, I don't really see a point in keeping them because:

1) If you provide a fake data structure, IMO there is no need for each dataset to have an individual test file because (I think) most datasets have the same functions `_split_generators` and `_generate_examples` for which you can just test the functionality in a common test file. For special functions like these beam / pipeline functionality you probably need an extra test file. But @mariamabarham I think you have seen more than I have here as well 

2) The dataset test design is very much intertwined with the download manager design and contains a lot of code. I would like to seperate the tests into a) tests for downloading in general b) tests for post download data set pre-processing. Since we are going to change the download code anyways quite a lot, my plan was to focus on b) first.
mariamabarham (CONTRIBUTOR) commented on 2020-04-23 13:09:26:
> > I think I'm fine with this.
> > The alternative would be to host a small subset of the dataset on the S3 together with the testing script. But I think having all (test file creation + actual tests) in one file is actually quite convenient.
> > Good for me!
> > One question though, will we have to create one test file for each of the 100+ datasets or could we make some automatic conversion from tfds dataset test files?
> 
> I think if we go the way shown in the PR we would have to create one test file for each of the 100+ datasets.
> 
> As far as I know the tfds test files all rely on the user having created a special download folder structure in `tensorflow-datasets/tensorflow_datasets/testing/test_data/fake_examples`.
> 
> My hypothesis was:
> Becasue, we don't want to work with PRs, no `dataset_script` is going to be in the official repo, so no `dataset_script_test` can be in the repo either. Therefore we can also not have any "fake" test folder structure in the repo.
> 
> **BUT:** As you mentioned @thom, we could have a fake data structure on AWS. To add a test the user has to upload multiple small test files when uploading his data set script.
> 
> So for a cli this could look like:
> `python nlp-cli upload <data_set_script> --testfiles <relative path to test file 1> <relative path to test file 2> ...`
> 
> or even easier if the user just creates the dataset folder with the script inside and the testing folder structure, then the API could look like:
> 
> `python nlp-cli upload <path/to/dataset/folder>`
> 
> and the dataset folder would look like
> 
> ```
> squad
> - squad.py
> - fake_data  # this dir would have to have the exact same structure we get when downloading from the official squad data url
> ```
> 
> This way I think we wouldn't even need any test files at all for each dataset script. For special datasets like `c4` or `wikipedia` we could then allow to optionally upload another test script.
> We just assume that this is our downloaded `url` and check all functionality from there.
> 
> Thinking a bit more about this solution sounds a) much less work and b) even easier for the user.
> 
> A small problem I see here though:
> 
> 1. What do we do when the depending on the config name the downloaded folder structure is very different? I think for each dataset config name we should have one test, which could correspond to one "fake" folder structure on AWS
> 
> @thomwolf What do you think? I would actually go for this solution instead now.
> @mariamabarham You have written many more tfds dataset scripts and tests than I have - what do you think?

I'm agreed with you just one thing, for some dataset like glue or xtreme you may have multiple datasets in it. so  I think a good way is to have one main fake folder and a subdirectory for each dataset inside
mariamabarham (CONTRIBUTOR) commented on 2020-04-23 13:20:55:
> Regarding the tfds tests, I don't really see a point in keeping them because:
> 
> 1. If you provide a fake data structure, IMO there is no need for each dataset to have an individual test file because (I think) most datasets have the same functions `_split_generators` and `_generate_examples` for which you can just test the functionality in a common test file. For special functions like these beam / pipeline functionality you probably need an extra test file. But @mariamabarham I think you have seen more than I have here as well
> 2. The dataset test design is very much intertwined with the download manager design and contains a lot of code. I would like to seperate the tests into a) tests for downloading in general b) tests for post download data set pre-processing. Since we are going to change the download code anyways quite a lot, my plan was to focus on b) first.

For _split_generator, yes. But I'm not sure for _generate_examples because there is lots of things that should be taken into account such as feature names and  types, data format (json, jsonl,  csv, tsv,..)
patrickvonplaten (CONTRIBUTOR) commented on 2020-04-24 13:14:39:
Ok, here is a second draft for the testing structure. 

I think the big difficulty here is "How can you generate tests on the fly from a given dataset name, *e.g.* `squad`"?

So, this morning I did some research on "parameterized testing" and pure `unittest` or `pytest` didn't work very well. 
I found the lib https://github.com/wolever/parameterized, which works very nicely for our use case I think. 
@thomwolf - would it be ok to have a dependence on this lib for `nlp`? It seems like a light-weight lib to me. 

This lib allows to add a `parameterization` decorator to a `unittest.TestCase` class so that the class can be instantiated for multiple different arguments (which are the dataset names `squad` etc. in our case).

What I like about this lib is that one only has to add the decorator and the each of the parameterized tests are shown, like this: 

![Screenshot from 2020-04-24 15-13-14](https://user-images.githubusercontent.com/23423619/80216326-2bd9a680-863e-11ea-8a0f-460976f5309c.png)

With this structure we would only have to upload the dummy data for each dataset and would not require a specific testing file. 

What do you think @thomwolf @mariamabarham @lhoestq ?
lhoestq (MEMBER) commented on 2020-04-24 15:46:02:
The new download manager is ready. I removed the old folder and I fixed a few remaining dependencies.
I tested it on squad and a few others from the dataset folder and it works fine.

The only impact of these changes is that it breaks the `download_and_prepare` script that was used to register the checksums when we create a dataset, as the checksum logic is not implemented.

Let me know if you have remarks
thomwolf (MEMBER) commented on 2020-04-26 07:59:43:
Actually two additional things here @lhoestq (I merged too fast sorry, let's make a new PR for additional developments):
- I think we can remove some dependencies now (e.g. `promises`) in setup.py, can you have a look?
- also, I think we can remove the boto3 dependency like here: https://github.com/huggingface/transformers/pull/3968
patrickvonplaten (CONTRIBUTOR) commented on 2020-04-27 14:16:13:
UPDATE: 

This test design is ready now. I added dummy data to S3 for the dataests: `squad, crime_and_punish, sentiment140` . The structure can be seen on `https://s3.console.aws.amazon.com/s3/buckets/datasets.huggingface.co/nlp/squad/dummy/?region=us-east-1&tab=overview` for `squad`. 

All dummy data files have to be in .zip format and called `dummy_data.zip`. The zip file should thereby have the exact same folder structure one gets from downloading the "real" data url(s). 

To show how the .zip file looks like for the added datasets, I added the folder `nlp/datasets/dummy_data` in this PR. I think we can leave for the moment so that people can see better how to add dummy data tests and later delete it like `nlp/datasets/nlp`.
lhoestq (MEMBER) commented on 2020-04-30 08:53:26:
> * I'm not sure I understand the general organization of checksums. I see we have a checksum folder with potentially several checksum files but I also see that checksum files can potentially contain several checksums. Could you explain a bit more how this is organized?

It should look like this:
squad/
├── squad.py/
└── urls_checksums/
...........└── checksums.txt

In checksums.txt, the format is one line per (url, size, checksum)

I don't have a strong opinion between `urls_checksums/checksums.txt` or directly `checksums.txt` (not inside the `urls_checksums` folder), let me know what you think.


> * Also regarding your comment on checksum files for "canonical" datasets. I understand we can just create these with `nlp-cli test` and then upload them manually to our S3, right?

Yes you're right
lhoestq (MEMBER) commented on 2020-04-30 12:39:43:
Update of the commands:

- nlp-cli test \<dataset\> : Run download_and_prepare and verify checksums
  * --name \<name\> : run only for the name
  * --all_configs : run for all configs
  * --save_checksums : instead of verifying checksums, compute and save them
  * --ignore_checksums : don't do checksums verification

- nlp-cli upload \<dataset_folder\> : Upload a dataset
  * --upload_checksums : compute and upload checksums for uploaded files

TODO:
- don't overwrite checksums files on S3, to let the user upload a dataset in several steps if needed

Question:
- One idea from @patrickvonplaten : shall we upload checksums everytime we upload files ? (and therefore remove the upload_checksums parameter)
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-04 13:14:53:
Yeah, I saw :-) But I think we should add this as well since some datasets have an empty list of configs and then as the code is now it would fail. 

In this PR, I just make sure that the code jumps in the correct else if "there are no configs" as is the case for some datasets @mariamabarham
thomwolf (MEMBER) commented on 2020-05-06 11:23:40:
Ok here is a proposal for a more general API and workflow.

# New `ArrowBasedBuilder`

For all the formats that can be directly and efficiently loaded by Arrow (CSV, JSON, Parquet, Arrow), we don't really want to have to go through a conversion to python and back to Arrow. This new builder has a `_generate_tables` method to yield `Arrow.Tables` instead of single examples.
The tables can be directly casted in Arrow so it's not necessary to supply  `Features`, they can be deduced from the `Table` column.

# Central role of the `BuilderConfig` to store all the arguments necessary for the Dataset creation.
 
`BuilderConfig` provide a few defaults fields `name`, `version`,  `description`,  `data_files` and `data_dir` which can be used to store values necessary for the creation of the dataset. It can be freely extended to store additional information (see the example for `CsvConfig`).

On the contrary, `DatasetInfo` is designed as an organized and delimited information storage class with predefined fields.

`DatasetInfo` now store two names:
- `builder_name`: Name of the builder script used to create the dataset
- `config_name`: Name of the configuration used to create the dataset.

# Refactoring `load()` arguments and all the chain of processing including the `DownloadManager`

`load()` now accept a selection of arguments which are used to update the `BuilderConfig` and some kwargs which are used to handle the download process.

Supplying a `BuilderConfig` as `config` will override the config provided in the dataset. Supplying a `str` will get the associated config from the dataset. Default is to fetch the first config of the dataset.

Giving additional arguments to `load()` will override the arguments in the `BuilderConfig`.

# CSV script

The `csv.py` script is provided as an example, usage is:
```python
bbc = nlp.load('/Users/thomwolf/Documents/GitHub/datasets/datasets/nlp/csv',
               name='bbc',
               version="1.0.1",
               split='train',
               data_files={'train': ['/Users/thomwolf/Documents/GitHub/datasets/datasets/dummy_data/csv/test.csv']},
               skip_rows=10,
               download_mode='force_redownload')
```

# Checksums

We now don't raise an error if the checksum file is not found.

# `DownloadConfig`

We now have a download configuration class to handle all the specific arguments for file caching like proxies, using only local files or user-agents.
jplu (CONTRIBUTOR) commented on 2020-05-07 14:20:12:
Nice!! I tested it a bit and works quite well. I will do a my review once the #25 will be merged because there are several overlaps.

At least I can share my thoughts on your **Next** section:
1) I don't think it is a good thing to rely on tfds preprocessed datasets uploaded in their online storage, because they might be updated or deleted at any moment by Google and then possibly break our own processing.
2) Improves the pipeline is always a good direction, but in the meantime we might also share the preprocessed dataset in S3 storage. Which might be another way to see 1), instead of downloading Google preprocessed datasets, using our own ones.
3) Apache Beam can be easily integrated in Spark, so I don't see the need to replace Beam by Spark.
thomwolf (MEMBER) commented on 2020-05-07 21:13:02:
Ok merging this for now.

One general note is that it's a bit hard to handle the `ClassLabel` generally in both `nlp` and `Arrow` since a class label typically need some metadata for the class names. For now, I raise a `NotImplementedError` when an `ArrowBuilder` output a table with a `DictionaryType` is encountered (which could be a simple equivalent for a `ClassLabel` Feature in Arrow tables).

In general and if we need this in the future for some Beam Datasets for instance, I think we should use one of the `metadata` fields in the `Arrow` type or table's schema to store the relation with indices and class names.

So ping me if you meet Beam datasets which uses `ClassLabels` (cc @lhoestq @patrickvonplaten @mariamabarham).
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-08 08:22:07:
Very weird bug indeed! I think the problem was that when importing `setup_module` we overwrote `pytest's` setup_module function. I think this is the relevant code in pytest: https://github.com/pytest-dev/pytest/blob/9d2eabb397b059b75b746259daeb20ee5588f559/src/_pytest/python.py#L460.
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-08 08:49:27:
Also PR: #25 introduced some renaming: `DatasetBuilder.builder_config` -> `DatasetBuilder.config` so that we will have to change most of the dataset scripts (Just replace the "builder_config" with "config").

I think the renaming is a good idea and I can do the fix with a bash regex, but will have to re-upload most of the datasets. @thomwolf @mariamabarham
mariamabarham (CONTRIBUTOR) commented on 2020-05-08 09:03:28:
> Also PR: #25 introduced some renaming: `DatasetBuilder.builder_config` -> `DatasetBuilder.config` so that we will have to change most of the dataset scripts (Just replace the "builder_config" with "config").
> 
> I think the renaming is a good idea and I can do the fix with a bash regex, but will have to re-upload most of the datasets. @thomwolf @mariamabarham

I think if it only needs a re-uploading, we can rename it, `DatasetBuilder.config` is easier and sounds better
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-08 10:18:44:
> Also we should convert `tf.io.gfile.exists` into `os.path.exists` , `tf.io.gfile.listdir`into `os.listdir` and `tf.io.gfile.glob` into `glob.glob` (will need to add `import glob`)

We should probably open a new PR about this
thomwolf (MEMBER) commented on 2020-05-11 08:04:17:
> Really cool, I love it! I would just raise a tiny point, the distributive version of the metrics might not work properly with TF because it is a different way to do, why not to add a "framework" detection and raise warning when TF is used, saying something like "not available yet in TF switch to non distributive metric computation".
> 
> What do you think?

Good point @jplu I'm not sure how you should do distributed metrics evaluation for TF.
There is only one python script, right?
Maybe it's just the same as in the not-distributed case?
lhoestq (MEMBER) commented on 2020-05-11 20:09:46:
After a quick chat with Yacine : the 2Go test may not be sufficient actually, as I'm looking at the size of the array and not the size of the current_rows. If the test doesn't do the job I think I'll remove it and lower the batch size a bit to be sure that it never exceeds 2Go. I'll do more tests later
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-13 23:20:38:
> Ok, really clean!
> I like the logic (not a huge fan of using `_asdict_inner` but it makes sense).
> I think it's a nice improvement!
> 
> How should we update the files in the repo? Run a big job on a server or on somebody's computer who has most of the datasets already downloaded?

Maybe we can split the updates among us...IMO most datasets run very quickly. 
I think I've downloaded 50 datasets and 80% are loaded in <5min,  15% in <1h and then `wmt` which is still downloading (since 12h). 
I deleted my cache because the `wmt` downloads require quite a lot of space, so I only have parts of the `wmt` datasets on my computer. 

@mariamabarham I guess you have downloaded most of the datasets no?
jplu (CONTRIBUTOR) commented on 2020-05-14 12:37:53:
@julien-c Looks like what you have in mind?

```python
import nlp
nlp.load_dataset("datasets/webis", "tl_dr")

#Output: Downloading and preparing dataset webis/tl_dr (download: Unknown size, generated: Unknown size, total: Unknown size) to /home/jplu/.cache/huggingface/datasets/webis/tl_dr/1.0.0...
```
mariamabarham (CONTRIBUTOR) commented on 2020-05-14 13:48:45:
> Wikihow is an example that needs to manually download two files as stated in: https://github.com/mahnazkoupaee/WikiHow-Dataset.
> 
> The user can then store these files under a hard-coded name: `wikihowAll.csv` and `wikihowSep.csv` in this case in a directory of his choice, e.g. `~/wikihow/manual_dir`.
> 
> The dataset can then be loaded via:
> 
> ```python
> import nlp
> nlp.load_dataset("wikihow", data_dir="~/wikihow/manual_dir")
> ```
> 
> I added/changed so that there are explicit error messages when using manually downloaded files.

wouldn't be nicer if we can have `manual_dir/wikihow`?