Discussion on version identifier & MockDataLoaderManager for test data
State: open | Author: EntilZha (CONTRIBUTOR)
Labels: name: generic discussion

Hi, I'm working on adding a dataset and ran into an error due to `download` not being defined on `MockDataLoaderManager`, but being defined in `nlp/utils/download_manager.py`. The readme step running this: `RUN_SLOW=1 pytest tests/test_dataset_common.py::DatasetTest::test_load_real_dataset_localmydatasetname` triggers the error. If I can get something to work, I can include it in my data PR once I'm done.

Comments: usually you can replace `download` in your dataset script with `download_and_prepare()` - could you share the code for your dataset here? :-) 

I have an initial version here: https://github.com/EntilZha/nlp/tree/master/datasets/qanta Thats pretty close to what I'll do as a PR, but still want to do some more sanity checks/tests (just got tests passing).

I figured out how to get all tests passing by adding a download command and some finagling with the data zip https://github.com/EntilZha/nlp/blob/master/tests/utils.py#L127



I'm quite positive that you can just replace the `dl_manager.download()` statements here: https://github.com/EntilZha/nlp/blob/4d46443b65f1f756921db8275594e6af008a1de7/datasets/qanta/qanta.py#L194 with `dl_manager.download_and_extract()` even though you don't extract anything. I would prefer to avoid adding more functions to the MockDataLoadManager and keep it as simple as possible (It's already to complex now IMO). 

Could you check if you can replace the `download()` function? 

I might be doing something wrong, but swapping those two gives this error:
```
>       with open(path) as f:
E       IsADirectoryError: [Errno 21] Is a directory: 'datasets/qanta/dummy/mode=first,char_skip=25/2018.4.18/dummy_data-zip-extracted/dummy_data'

src/nlp/datasets/qanta/3d965403133687b819905ead4b69af7bcee365865279b2f797c79f809b4490c3/qanta.py:280: IsADirectoryError

During handling of the above exception, another exception occurred:
```

So it seems like the directory name is getting passed. Is this not functioning as expected, or is there some caching happening maybe? I deleted the dummy files and re-ran the import script with no changes. I'm digging a bit in with a debugger, but no clear reason yet

From what I can tell here: https://github.com/huggingface/nlp/blob/master/tests/utils.py#L115

1. `data_url` is the correct http link
2. `path_to_dummy_data` is a directory, which is causing the issue

That path comes from `download_dummy_data`, which I think assumes that the data comes from the zip file, but isn't aware of individual files. So it seems like it data manager needs to be aware if the url its getting is for a file or a zip/directory, and pass this information along. This might happen in `download_dummy_data`, but probably better to happen in `download_and_extract`? Maybe a simple check to see if `os.path.basename` returns the dummy data zip filename, if not then join paths with the basename of the url?

I think the dataset script works correctly. Just the dummy data structure seems to be wrong. I will soon add more commands that should make the create of the dummy data easier.

I'd recommend that you won't concentrate too much on the dummy data.
If you manage to load the dataset correctly via:

```python 
# use local path to qanta
nlp.load_dataset("./datasets/qanta")
```

then feel free to open a PR and we will look into the dummy data problem together :-) 

Also please make sure that the Version is in the format 1.0.0 (three numbers separated by two points) - not a date. 

The script loading seems to work fine so I'll work on getting a PR open after a few sanity checks on the data.

On version, we currently have it versioned with YYYY.MM.DD scheme so it would be nice to not change that, but will it cause issues?

> The script loading seems to work fine so I'll work on getting a PR open after a few sanity checks on the data.
> 
> On version, we currently have it versioned with YYYY.MM.DD scheme so it would be nice to not change that, but will it cause issues?

It would cause issues for sure for the tests....not sure if it would also cause issues otherwise.

I would prefer to keep the same version style as we have for other models. You could for example simply add version 1.0.0 and add a comment with the date you currently use for the versioning.

 What is your opinion regarding the version here @lhoestq @mariamabarham @thomwolf ? 

Maybe use the YYYY.MM.DD as the config name ? That's what we are doing for wikipedia

> Maybe use the YYYY.MM.DD as the config name ? That's what we are doing for wikipedia

I'm not sure if this will work because the name should be unique and it seems that he has multiple config name in his data with the same version.
As @patrickvonplaten  suggested, I think  you can add a comment about the version in the data description.

Actually maybe our versioning format (inherited from tfds) is too strong for what we use it for?
We could allow any string maybe?

I see it more and more like an identifier for the user that we will back with a serious hashing/versioning system.- so we could let the user quite free on it.

I'm good with either putting it in description, adding it to the config, or loosening version formatting. I mostly don't have a full conceptual grasp of what each identifier ends up meaning in the datasets code so hard to evaluate the best approach.

For background, the multiple formats is a consequence of:

1. Each example is one multi-sentence trivia question
2. For training, its better to treat each sentence as an example
3. For evaluation, should test on: (1) first sentence, (2) full question, and (3) partial questions (does the model get the question right having seen the first half)

We use the date format for version since: (1) we expect some degree of updates since new questions come in every year and (2) the timestamp itself matches the Wikipedia dump that it is dependent on (so similar to the Wikipedia dataset).

perhaps this is better discussed in https://github.com/huggingface/nlp/pull/169 or update title?