t20062443/AT1ds huggingface dataset:
lhoestq (MEMBER) commented on 2020-05-22 12:44:50:
If your dataset is a collection of sub-datasets, you should probably consider having one config per sub-dataset. For example for Glue, we have sst2, mnli etc.
If you want to have multiple train sets (for example one per stage). The easiest solution would be to name them `nlp.Split("train_stage1")`, `nlp.Split("train_stage2")`, etc. or something like that.
yjernite (MEMBER) commented on 2020-05-22 13:31:07:
Thanks for the tip! I ended up setting up three different versions of the dataset with their own configs.

for the named splits, I was trying with `nlp.Split("train-stage1")`, which fails. Changing to `nlp.Split("train_stage1")` works :) I looked for examples of what works in the code comments, it may be worth adding some examples of valid/invalid names in there?
lhoestq (MEMBER) commented on 2020-05-22 13:38:52:
There was an issue in the format, thanks.
Now you can do
```python3
squad_dset = nlp.load_dataset("squad")
squad_metric = nlp.load_metric("/Users/quentinlhoest/Desktop/hf/nlp-bis/metrics/squad")
predictions = [
    {"id": v["id"], "prediction_text": v["answers"]["text"][0]}  # take first possible answer
    for v in squad_dset["validation"]
]
squad_metric.compute(predictions, squad_dset["validation"])
```

and the expected format is 
```
Args:
    predictions: List of question-answers dictionaries with the following key-values:
        - 'id': id of the question-answer pair as given in the references (see below)
        - 'prediction_text': the text of the answer
    references: List of question-answers dictionaries with the following key-values:
        - 'id': id of the question-answer pair (see above),
        - 'answers': a Dict {'text': list of possible texts for the answer, as a list of strings}
```
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-22 14:11:00:
Hi @EntilZha - sorry for waiting so long until taking action here. We created a new command and a new recipe of how to add dummy_data. Can you maybe rebase to `master` as explained in 7. of https://github.com/huggingface/nlp/blob/master/CONTRIBUTING.md#how-to-contribute-to-nlp and check that your dummy data is correct following the instructions here: https://github.com/huggingface/nlp/blob/master/CONTRIBUTING.md#how-to-add-a-dataset ? 

If the tests described in 5. of https://github.com/huggingface/nlp/blob/master/CONTRIBUTING.md#how-to-add-a-dataset pass we can merge the PR :-)
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-22 14:11:59:
The PR looks awesome! 
Since you have already added a dataset I imagine the tests as described in 5. of https://github.com/huggingface/nlp/blob/master/CONTRIBUTING.md#how-to-add-a-dataset all pass, right @stefan-it ?

I think we are then good to merge this :-) @lhoestq
lhoestq (MEMBER) commented on 2020-05-22 14:23:33:
Nice !

One thing though: I saw that you copied the `dataset_info.json` (one split info), which is different from the `dataset_infos.json` (split infos of all configs) that we expect.

Could you generate the `dataset_infos.json` file using this command please ?
```
python nlp-cli test datasets/wnut_17 --save_infos --all_configs
```
thomwolf (MEMBER) commented on 2020-05-24 17:55:03:
Hi @tylerroost, we don't have a timeline for this at the moment.
If you want to give it a look we would be happy to review a PR on it.
Also, the library is one week old so everything is quite barebones, in particular the doc.
You should expect some bumps on the road.

To get you started, you can check the datasets scripts in the `./datasets` folder on the repo and find the one on math_datasets that will need to be modified. Then you should check the original repository on the math_dataset to see where the other files to download are located and what is the expected format for the various parts of the dataset.

To get a general overview on how datasets scripts are written and used, you can read the nice tutorial on how to add a new dataset for TensorFlow Dataset [here](https://www.tensorflow.org/datasets/add_dataset), our API is not exactly identical but it can give you a high-level overview.
mariamabarham (CONTRIBUTOR) commented on 2020-05-25 09:45:54:
cos_e v1.0 is related to CQA v1.0 but only CQA v1.11 dataset is available on their website. Indeed their is lots of ids in cos_e v1, which are  not in CQA v1.11  or the other way around.
@sarahwie, @thomwolf, @nazneenrajani,  @bmccann  do you know where I can  find CQA v1.0
sarahwie (NONE) commented on 2020-05-25 18:38:28:
I think I've resolved it. For others' reference: to convert from using the [`MultipleChoiceDataset` class](https://github.com/huggingface/transformers/blob/a34a9896ac2a4a33ff9cd805c76eed914c8d8965/examples/multiple-choice/utils_multiple_choice.py#L82)/[`run_multiple_choice.py`](https://github.com/huggingface/transformers/blob/a34a9896ac2a4a33ff9cd805c76eed914c8d8965/examples/multiple-choice/run_multiple_choice.py) script in Huggingface Transformers, I've done the following for hellaswag:

1. converted the `convert_examples_to_features()` function to only take one input and return a dictionary rather than a list:
```
def convert_examples_to_features(example, tokenizer, max_length):

    choices_inputs = defaultdict(list)
    for ending_idx, ending in enumerate(example['endings']['ending']):
        text_a = example['ctx']
        text_b = ending

        inputs = tokenizer.encode_plus(
            text_a,
            text_b,
            add_special_tokens=True,
            max_length=max_length,
            pad_to_max_length=True,
            return_overflowing_tokens=True,
        )
        if "num_truncated_tokens" in inputs and inputs["num_truncated_tokens"] > 0:
            logger.info(
                "Attention! you are cropping tokens (swag task is ok). "
                "If you are training ARC and RACE and you are poping question + options,"
                "you need to try to use a bigger max seq length!"
            )

        for key in inputs:
            choices_inputs[key].append(inputs[key])
            
        choices_inputs['label'] = int(example['label'])

    return choices_inputs
```
2. apply this directly (instance-wise) to dataset, convert dataset to torch tensors. Dataset is then ready to be passed to `Trainer` instance.

```
dataset['train'] = dataset['train'].map(lambda x: convert_examples_to_features(x, tokenizer, max_length), batched=False)
columns = ['input_ids', 'token_type_ids', 'attention_mask', 'label']
dataset['train'].set_format(type='torch', columns=columns)
```
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-25 19:57:04:
It's the dummy_data structure. You actually have to call the dummy data file name `dummy_data` (not .json anything). So there should not be a `dummy_data` folder but for each config only a `dummy_data` which contains your json dummy data. Can you maybe try once more - if it doesn't work I do it for you :-).
lhoestq (MEMBER) commented on 2020-05-26 14:08:57:
> I think that's not related to the config name but the filenames in the dummy data. Mostly it occurs with files downloaded from drive. In that case the dummy file name is extracted from the google drive link and it corresponds to what comes after `https://drive.google.com/`

The filenames of the dummy data are now encoded (see #173). So this is not a problem anymore.

The problem here is different and comes from the directory names where we save the arrow files (basically `dataset_name/config_name/version`). In this case we could have invalid directory names because of the config name
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-26 14:22:42:
I like the method, but I'm wondering whether it should just be a test method instead of a `__post_init__` function. From a logical point of view the only reason this error would be thrown is because of an invalid config name introduced when creating the dataset script / adding a new dataset => so I think it might be better to write a simple test for this in `test_dataset_common.py`...what do you think @lhoestq ?
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-26 14:29:24:
> `test_dataset_common.py` only tests canonical datasets no ? What if users wants to create their own script ?

It tests all dataset that can be loaded either locally or on AWS (which includes all non-canonical datasets as well)...by their own script you mean like a private dataset script that they don't want to be public? I guess even then they could locally run the test functions to check...
lhoestq (MEMBER) commented on 2020-05-26 14:44:22:
Let's say someone want to create his own private script. As the script is not meant to be shared, it's not going to be placed in `/datasets` right ? Maybe the script is going to be inside another project. If I'm not wrong in this case the `test_dataset_common.py` is not going to test his script.

Raising an error in the post init is a sanity check that would tell the user immediately what's wrong.
The error is raised if he tried to load the script or if he uses `nlp-cli test`
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-26 14:45:11:
> Let's say someone want to create his own private script. As the script is not meant to be shared, it's not going to be placed in `/datasets` right ? Maybe the script is going to be inside another project. If I'm not wrong in this case the `test_dataset_common.py` is not going to test his script.
> 
> Raising an error in the post init is a sanity check that would tell the user immediately what's wrong.
> The error is raised if he tried to load the script or if he uses `nlp-cli test`

OK, fair point! I'm good with this then :-)
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-26 14:47:14:
> We could have a bunch of simple consistency tests that run before uploading with the CLI (without loading data if we don't want to force the user to have dummy data)?

Yes! I guess that's a big question whether we should force the user to add dummy data. It's probably too tedious for the user...so when uploading to circle ci should we just check 
- 1) All configs can be instantiated (if there are any)
- 2) The BuilderClass can be instantiated ... 
- 3) ... maybe some more

and maybe suggest to the user to add dummy data using the dummy data command?
lhoestq (MEMBER) commented on 2020-05-26 14:50:29:
We store every dataset in the Arrow format. This is convenient as it supports nested types and memory mapping. If you are curious feel free to check the [pyarrow documentation](https://arrow.apache.org/docs/python/)

You can use this library to load your covid papers by creating a dataset script. You can find inspiration from the ones we've already written in `/datasets`. Here is a link to the steps to [add a dataset](https://github.com/huggingface/nlp/blob/master/CONTRIBUTING.md#how-to-add-a-dataset)
thomwolf (MEMBER) commented on 2020-05-26 15:38:03:
Hi so there are indeed two configurations in the datasets as you can see [here](https://github.com/huggingface/nlp/blob/master/datasets/scientific_papers/scientific_papers.py#L81-L82).

You can load either one with:
```python
dataset = nlp.load_dataset('scientific_papers', 'pubmed')
dataset = nlp.load_dataset('scientific_papers', 'arxiv')
```

This issues is actually related to a similar user-experience issue with GLUE. When several configurations are available and the first configuration is loaded by default (see issue #152 and #130), it seems to be unexpected for users.

I think we should maybe raise a (very explicit) error when there are several configurations available and the user doesn't specify one.

What do you think @lhoestq @patrickvonplaten @mariamabarham ?
lhoestq (MEMBER) commented on 2020-05-26 21:02:52:
I really like that we have a test with dummy data for canonical datasets. This is insurance that they'll keep working in the long run. 

On the other hand I understand that we will probably not force this practice for scripts uploaded on S3 by a user under his namespace (non-canonical), as it is tedious. As I understand right now the test is done for all the datasets on aws, even the non-canonical ? We should think about different tests for non-canonical datasets.

I also like the idea of a simple consistency test !
lhoestq (MEMBER) commented on 2020-05-26 21:19:23:
When you install `nlp` for the first time on a Colab runtime, it updates the `pyarrow` library that was already on colab. This update shows this message on colab:
```
WARNING: The following packages were previously imported in this runtime:
  [pyarrow]
You must restart the runtime in order to use newly installed versions.
```
You just have to restart the runtime and it should be fine.
honnibal (NONE) commented on 2020-05-27 16:13:48:
> If we add the constraint that this flat namespace also be shared with variable names this gets untractable pretty fast :)

I'm sort of confused by your point here. The namespace *is* shared by variable names. You should not use local variables that are named the same as modules, because then you cannot use the module within the scope of your function.

For instance,

```python

import nlp
import transformers

nlp = transformers.pipeline("sentiment-analysis")
```

This is a bug: you've just overwritten the module, so now you can't use it. Or instead:

```python

import transformers

nlp = transformers.pipeline("sentiment-analysis")
# (Later, e.g. in a notebook)
import nlp
```

This is also a bug: you've overwritten your variable with an import.

If you have a module named `nlp`, you should avoid using `nlp` as a variable, or you'll have bugs in some contexts and inconsistencies in other contexts. You'll have situations where you need to import differently in one module vs another, or name variables differently in one context vs another, which is bad.

> importing only the methods that they need like @thomwolf suggested

Okay but the same logic applies to naming the module *literally anything else*. There's absolutely no point in having a module name that's 3 letters if you always plan to do `import from`! It would be entirely better to name it `nlp_datasets` if you don't want people to do `import nlp`.

And finally:

> By the way, nlp will very likely not be only about datasets, and not even just about datasets and metrics.

So...it isn't a datasets library? https://twitter.com/Thom_Wolf/status/1261282491622731781

I'm confused 😕
lhoestq (MEMBER) commented on 2020-05-27 22:07:58:
An error is raised if the sub-dataset is not specified :)
```
ValueError: Config name is missing.
Please pick one among the available configs: ['cola', 'sst2', 'mrpc', 'qqp', 'stsb', 'mnli', 'mnli_mismatched', 'mnli_matched', 'qnli', 'rte', 'wnli', 'ax']
Example of usage:
	`load_dataset('glue', 'cola')`
```
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-28 14:45:13:
Here the full error trace:

```
ArrowInvalid                              Traceback (most recent call last)
<ipython-input-1-7aaf3f011358> in <module>
      1 import nlp
      2 ds = nlp.load_dataset("trivia_qa", "rc", split="validation[:1%]")  # this might take 2.3 min to download but it's cached afterwards...
----> 3 ds.map(lambda x: x, load_from_cache_file=False)

~/python_bin/nlp/arrow_dataset.py in map(self, function, with_indices, batched, batch_size, remove_columns, keep_in_memory, load_from_cache_file, cache_file_name, writer_batch_size, arrow_schema, disable_nullable)
    549
    550         if update_data:
--> 551             writer.finalize()  # close_stream=bool(buf_writer is None))  # We only close if we are writing in a file
    552
    553             # Create new Dataset from buffer or file

~/python_bin/nlp/arrow_writer.py in finalize(self, close_stream)
    182     def finalize(self, close_stream=True):
    183         if self.pa_writer is not None:
--> 184             self.write_on_file()
    185             self.pa_writer.close()
    186         if close_stream:

~/python_bin/nlp/arrow_writer.py in write_on_file(self)
    104         """
    105         if self.current_rows:
--> 106             pa_array = pa.array(self.current_rows, type=self._type)
    107             first_example = pa.array(self.current_rows[0:1], type=self._type)[0]
    108             # Sanity check

~/hugging_face/venv_3.7/lib/python3.7/site-packages/pyarrow/array.pxi in pyarrow.lib.array()

~/hugging_face/venv_3.7/lib/python3.7/site-packages/pyarrow/array.pxi in pyarrow.lib._sequence_to_array()

~/hugging_face/venv_3.7/lib/python3.7/site-packages/pyarrow/error.pxi in pyarrow.lib.check_status()

ArrowInvalid: Could not convert TagMe with type str: converting to null type
```
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-28 17:38:04:
Actually, I know what the problem is...I'm quite sure it's a bug. Here we take some test inputs: https://github.com/huggingface/nlp/blob/0e0ef12c14d2175e0b0bd7d8aa814b09e2cd7e1f/src/nlp/arrow_dataset.py#L472

It might be that in the test inputs, a `Sequence` type value is an emtpy list. So in my case I have `ds[0]["entity_pages']["wiki_context"] = []`. => this leads to an `arrow_schema` equal to `null`  for `["entity_pages']["wiki_context"]` => see line: https://github.com/huggingface/nlp/blob/0e0ef12c14d2175e0b0bd7d8aa814b09e2cd7e1f/src/nlp/arrow_dataset.py#L501  instead of list of string which it should for other examples. 

Guess it's an edge case, but it can happen.
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-28 17:48:58:
> Instead of flattening everything and rebuilding the example, maybe we can try to access the examples like this:
> 
> ```python
> for i in range(num_examples):
>     example = map_nested(lambda x: x[i], batch)
>     # ... then test to keep it or not
> ```

Awesome I'll check it out :-)
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-28 18:07:44:
> Instead of flattening everything and rebuilding the example, maybe we can try to access the examples like this:
> 
> ```python
> for i in range(num_examples):
>     example = map_nested(lambda x: x[i], batch)
>     # ... then test to keep it or not
> ```

Awesome this function is definitely much nicer!
lhoestq (MEMBER) commented on 2020-05-28 18:08:34:
Actually I just realized that `map_nested` might not work either as it applies the function at the very last list of the structure. However we can imagine that a single example has also a list in its structure:
```python
one_example = {
    "title": "blabla",
    "paragraphs": [
        "p1", "p2", ...
    ]
}
```
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-28 18:21:11:
> Actually I just realized that `map_nested` might not work either as it applies the function at the very last list of the structure. However we can imagine that a single example has also a list in its structure:
> 
> ```python
> one_example = {
>     "title": "blabla",
>     "paragraphs": [
>         "p1", "p2", ...
>     ]
> }
> ```

They both work. I'm using it on trivia_qa which is pretty nested. If you use the option `dict_only=True` I think it's fine.
patrickvonplaten (CONTRIBUTOR) commented on 2020-05-29 11:44:43:
Oh I see you might have a wrong version of pyarrow install on the colab -> could you try the following. Add these lines to the beginning of your notebook, restart the runtime and run it again:
```
!pip uninstall -y -qq pyarrow
!pip uninstall -y -qq nlp
!pip install -qq git+https://github.com/huggingface/nlp.git
```
lhoestq (MEMBER) commented on 2020-05-29 13:01:00:
For the rouge2 metric you can do

```python
rouge = nlp.load_metric('rouge')
with open("pred.txt") as p, open("ref.txt") as g:
    for lp, lg in zip(p, g):
        rouge.add(lp, lg)
score = rouge.compute(rouge_types=["rouge2"])
```

Note that I just did a PR to have both `.add` and `.add_batch` for metrics, that's why now this is `rouge.add(lp, lg)` and not `rouge.add([lp], [lg])`
lhoestq (MEMBER) commented on 2020-05-29 13:07:45:
I just ran it on colab and got this
```
[{'expected': SplitInfo(name='train', num_bytes=610252351, num_examples=532812,
dataset_name='blog_authorship_corpus'), 'recorded': SplitInfo(name='train',
num_bytes=611607465, num_examples=533285, dataset_name='blog_authorship_corpus')},
{'expected': SplitInfo(name='validation', num_bytes=37500394, num_examples=31277,
dataset_name='blog_authorship_corpus'), 'recorded': SplitInfo(name='validation',
num_bytes=35652716, num_examples=30804, dataset_name='blog_authorship_corpus')}]
```
which is different from the `dataset_infos.json` and also different from yours.

It looks like the script for generating examples is not consistent
lhoestq (MEMBER) commented on 2020-05-29 14:50:06:
Even if I fix the `TypeError: __init__() got an unexpected keyword argument 'cdn'` error, it looks like it still uploads to                                                                                                                               `https://s3.amazonaws.com/models.huggingface.co/bert/<namespace>/<dataset_name>` instead of `https://s3.amazonaws.com/datasets.huggingface.co/nlp/<namespace>/<dataset_name>`
lhoestq (MEMBER) commented on 2020-05-29 23:34:22:
When you install `nlp` for the first time on a Colab runtime, it updates the `pyarrow` library that was already on colab. This update shows this message on colab:
```
WARNING: The following packages were previously imported in this runtime:
  [pyarrow]
You must restart the runtime in order to use newly installed versions.
```
You just have to restart the runtime and it should be fine.
If you don't restart, then it breaks like in your first message
Sharvil97 (NONE) commented on 2020-05-31 17:03:42:
> Oh I see you might have a wrong version of pyarrow install on the colab -> could you try the following. Add these lines to the beginning of your notebook, restart the runtime and run it again:
> 
> ```
> !pip uninstall -y -qq pyarrow
> !pip uninstall -y -qq nlp
> !pip install -qq git+https://github.com/huggingface/nlp.git
> ```

Tried, having the same error.
yjernite (MEMBER) commented on 2020-06-01 13:45:37:
@Colanim unfortunately there are different implementations of the ROUGE metric floating around online which yield different results, and we had to chose one for the package :) We ended up including the one from the google-research repository, which does minimal post-processing before computing the P/R/F scores. If I recall correctly, files2rouge relies on the Perl, script, which among other things normalizes all numbers to a special token: in the case you presented, this should account for a good chunk of the difference.

We may end up adding in more versions of the metric, but probably not for a while (@lhoestq correct me if I'm wrong). However, feel free to take a stab at adding it in yourself and submitting a PR if you're interested!
julien-c (MEMBER) commented on 2020-06-01 15:52:18:
@lhoestq The endpoints in https://github.com/huggingface/nlp/blob/master/src/nlp/hf_api.py should be (depending on the type of file):
```
POST   /api/datasets/presign
GET    /api/datasets/listObjs
DELETE /api/datasets/deleteObj
POST   /api/metrics/presign  
GET    /api/metrics/listObjs
DELETE /api/metrics/deleteObj
```

In addition to this, @thomwolf cleaned up the objects with dataclasses but you should revert this and re-align to the hf_api that's in this branch of transformers: https://github.com/huggingface/transformers/pull/4632 (so that potential new JSON attributes in the API output don't break existing versions of any library)
astariul (NONE) commented on 2020-06-01 23:41:29:
Thank you for your kind answer.

As a side question : Isn't it better to have a package that normalize more ?

I understand to idea of having a package that does minimal post-processing for transparency.

But it means that people using different architecture (with different tokenizers for example) will have difference in ROUGE scores even if their predictions are actually similar.  
The goal of `nlp` is to have _one package to rule them all_, right ?

I will look into it but I'm not sure I have the required skill for this ^^