t20062443/AT1ds huggingface dataset:
ghazi-f (CONTRIBUTOR) commented on 2020-07-28 13:39:29:
This is the only fix I could come up with without touching the repo's code.
```python
from nlp.builder import FORCE_REDOWNLOAD
dataset = load_dataset('csv', data_file='./a.csv', download_mode=FORCE_REDOWNLOAD, version='0.0.1')
```
You'll have to change the version each time you want to load a different csv file.
If you're willing to add a ```print```, you can go to ```nlp.load``` and add ```print(builder_instance.cache_dir)``` right before the ```return ds``` in the ```load_dataset``` method. It'll print the cache folder, and you'll just have to erase it (and then you won't need the change here above).
lhoestq (MEMBER) commented on 2020-07-28 15:24:51:
I see. This is an interesting question.
Maybe as the dataset doesn't provide the mapping we shouldn't force an arbitrary one, and keep them as strings ?
Moreover for NER the labels are often different from a dataset to the other so it's probably good to keep strings (there is no conventional mapping).
Also as the column is called "ner_tags" (or "langs"), you can already assume that there is a fixed and predefined list of labels.
lhoestq (MEMBER) commented on 2020-07-29 08:31:59:
I tested the hashing speed [here](https://colab.research.google.com/drive/1hlhP84kLIHmOzMRQN1h8x10hKWpXXyud?usp=sharing).
I was able to get 8x speed with `xxhashlib` (42ms vs 345ms for 100MiB of data).
What do you think @thomwolf ?
thomwolf (MEMBER) commented on 2020-07-29 08:54:05:
Hi @ArneBinder, we have a few "generic" datasets which are intended to load data files with a predefined format:
- csv: https://github.com/huggingface/nlp/tree/master/datasets/csv
- json: https://github.com/huggingface/nlp/tree/master/datasets/json
- text: https://github.com/huggingface/nlp/tree/master/datasets/text

You can find more details about this way to load datasets here in the documentation: https://huggingface.co/nlp/loading_datasets.html#from-local-files

Maybe your brat loading script could be shared in a similar fashion?
lhoestq (MEMBER) commented on 2020-07-30 10:53:25:
I started to add metadata in the DatasetInfo.
Note that because there are new fields, **ALL the dataset_info[s].json generated after these changes won't be loadable from older versions of the lib**

Right now it looks like this:
```json
        "post_processing_resources_checksums": {
            "train": {
                "embeddings_index": {
                    "num_bytes": 30720045,
                    "checksum": "b04fb4f4f3ab83b9d1b9f6f9eb236f1c04a9fd61bef7cee16b12df8ac911766a"
                }
            }
        },
        "post_processing_size": 30720045,
```
lhoestq (MEMBER) commented on 2020-07-30 15:19:57:
I added:
- post processing features (inside a PostProcessedInfo object)
- backward compatibility for dataset info
- post processing tests (as_dataset and download_and_prepare) for map (change features), select (change number of elements) and add_faiss_index (add indexes)
And I fixed a bug in `map` that I found thanks to the new tests

Now I just have to move `post_processing_resources_checksums` to  PostProcessedInfo as well and everything should be good :)
Edit: done
tsellam (NONE) commented on 2020-07-30 15:25:50:
Following up on Ankur's comment---we are going to drop support for
positional (not named) arguments in the future releases because it seems to
cause bugs and confusion. I hope it doesn't create too much of a mess.

Le jeu. 30 juil. 2020 à 10:44, ankparikh <notifications@github.com> a
écrit :

> Sorry one nit: Could we use named arguments for the call to BLEURT?
>
> i.e.
> scores = self.scorer.score(references=references, candidates=predictions)
>
> (i.e. so it is less bug prone)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/nlp/pull/455#issuecomment-666414514>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABTMRNGAN2PMECS5K4DIHJDR6GBMLANCNFSM4PL323FA>
> .
>
yjernite (MEMBER) commented on 2020-07-30 15:28:59:
> Following up on Ankur's comment---we are going to drop support for positional (not named) arguments in the future releases because it seems to cause bugs and confusion. I hope it doesn't create too much of a mess. Le jeu. 30 juil. 2020 à 10:44, ankparikh <notifications@github.com> a écrit :
> […](#)
> Sorry one nit: Could we use named arguments for the call to BLEURT? i.e. scores = self.scorer.score(references=references, candidates=predictions) (i.e. so it is less bug prone) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <[#455 (comment)](https://github.com/huggingface/nlp/pull/455#issuecomment-666414514)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTMRNGAN2PMECS5K4DIHJDR6GBMLANCNFSM4PL323FA> .

Changed @ankparikh @tsellam, thanks for taking a look!
lhoestq (MEMBER) commented on 2020-07-30 16:57:24:
Hi ! Glad you managed to fix the version issue.

The command `
python nlp-cli dummy_data datasets/guardian_authorship --save_infos --all_configs` is supposed to generate a json file `dataset_infos.json` next to your dataset script, but I can't see it in the PR.
Can you make sure you have the json file on your side and that you have pushed it ?
orsharir (CONTRIBUTOR) commented on 2020-07-30 18:19:23:
Thanks for approving my patch.

I agree that if copying is needed then some locking mechanism should be put in place. But, I don't think a file should be needlessly copied without a check. So I guess the flow should be, lock => copy if needed => unlock, and add locks wherever else that file is being accessed.

I'll also add that my personal experience with filelock on a different project hasn't been that great, and on some occasions a process somehow got through the lock -- I've never gotten to the bottom of that but it tainted my view of that module. Perhaps it's been fixed (or I just miss used it), but thought you should know to take steps to test it.
lhoestq (MEMBER) commented on 2020-07-30 21:26:11:
Cool thanks for adding the tests :) 

Next step is merge master into this branch.
Not sure I understand what you did in your last commit, but it looks like you discarded all the changes from master ^^'

We've done some changes in the features logic on master, so let me know if you need help merging it.

As soon as we've merged from master, we'll have to make sure that we have extensive tests and we'll be good to do !
About the lxmert dataset, we can probably keep it for another PR as soon as we have working 2d features. What do you think ?
eltoto1219 (CONTRIBUTOR) commented on 2020-07-30 21:55:03:
haha, when I tried to rebase, I ran into some conflicts. In that last commit, I restored the features.py from the previous commit on the branch in my fork because upon updating to master, the pandasdtypemanger and pandas extension types disappeared.  If you actually could help me with merging in what is needed, that would actually help a lot. 

Other than that, ya let me go ahead and move the dataloader code out of this PR. Perhaps we could discuss in the slack channelk soon about what to do with that because we can either just support the pretraining corpus for lxmert or try to implement the full COCO and visual genome datasets (+VQA +GQA) which im sure people would be pretty happy about. 

Also we can talk more tests soon too when you are free. 

Goodluck on the release tomorrow guys!
lhoestq (MEMBER) commented on 2020-07-31 18:18:28:
One note on features inference:

if an arrow type is `struct of items` where each item is a `list`, then we return a `dict` in which each item is a `Sequence`.
It means that we don't use the Sequence <-> dict swap when we infer features.

It's fine because the swap is generally used in dataset scripts, in which features are defined (inferred features are discarded)
mariamabarham (CONTRIBUTOR) commented on 2020-08-03 08:26:13:
I think the problem is related to `wiki_dpr` dataset which is making the circle CI failed as you can see:
```
FAILED tests/test_dataset_common.py::AWSDatasetTest::test_load_dataset_wiki_dpr
FAILED tests/test_hf_gcp.py::TestDatasetOnHfGcp::test_script_synced_with_s3_wiki_dpr/dummy_psgs_w100_no_embeddings
FAILED tests/test_hf_gcp.py::TestDatasetOnHfGcp::test_script_synced_with_s3_wiki_dpr/dummy_psgs_w100_with_nq_embeddings
FAILED tests/test_hf_gcp.py::TestDatasetOnHfGcp::test_script_synced_with_s3_wiki_dpr/psgs_w100_no_embeddings
FAILED tests/test_hf_gcp.py::TestDatasetOnHfGcp::test_script_synced_with_s3_wiki_dpr/psgs_w100_with_nq_embeddings

```
I'm facing the same issues with my last commits, I tried to rebase from master but it still not working. Maybe @lhoestq can help with.
lhoestq (MEMBER) commented on 2020-08-03 17:07:39:
Hi ! Did you try to set the output format to pytorch ? (or tensorflow if you're using tensorflow)
It can be done with `dataset.set_format("torch", columns=columns)`  (or "tensorflow").

Note that for pytorch, string columns can't be converted to `torch.Tensor`, so you have to specify in `columns=` the list of columns you want to keep (`input_ids` for example)
lewtun (MEMBER) commented on 2020-08-03 20:18:34:
I've created a simple function that seems to do the trick:

```python
def apply_encoding_on_file_open(filepath: str):
    """Apply UTF-8 encoding for all instances where a non-binary file is opened."""
    
    with open(filepath, 'r', encoding='utf-8') as input_file:
        regexp = re.compile(r"""
                            (?!.*\b(?:encoding|rb|wb|wb+|ab|ab+)\b)
                            (open)
                            \((.*)\)
                            """)
        input_text = input_file.read()
        match = regexp.search(input_text)
        
        if match:
            print('Found match!', match.group())
            # append utf-8 encoding to matching groups in-place
            output = regexp.sub(lambda m: m.group()[:-1]+', encoding="utf-8")', input_text)
            with open(filepath, 'w', encoding='utf-8') as output_file:
                output_file.write(output)
        else:
            print("No match found!")
```

The regexp does a negative lookahead to avoid matching on cases where the encoding is already specified or when binary files are involved.

From an implementation perspective:

* Would it make sense to include this function in `nlp-cli` so that we can run something like
```
nlp-cli fix_encoding path/to/folder
```
and the command recursively fixes all files in the target?
* What is the desired behaviour in the CI test? Here we could either have a simple script that we run as a `job` in the CI and raises an error if a missing encoding is detected. Alternatively we could incorporate this behaviour into the CLI and run that in the CI.

Please let me know what you prefer among the alternatives.
HugToDebug (NONE) commented on 2020-08-04 01:01:53:
I'm trying to follow along with the following about datasets from the docs:

https://huggingface.co/nlp/loading_datasets.html
https://huggingface.co/nlp/processing.html

However the train_test_split method no longer works as it is expecting a dataset, rather than a datsetdict. How would I got about splitting a CSV into a train and test set? 

I'm trying to utilize the Trainer() class, but am having trouble converting my data from a csv into dataset objects to pass in.
lewtun (MEMBER) commented on 2020-08-08 12:42:49:
I pushed an improved docstring and the unit tests now pass, which suggests the previous failure on AWS was simply a timeout error. 

For some reason the docs are now failing to build, but does not seem related to my changes:
```
Warning, treated as error:
/home/circleci/nlp/src/nlp/dataset_dict.py:docstring of nlp.DatasetDict.filter:27:Inline interpreted text or phrase reference start-string without end-string.
make: *** [Makefile:20: html] Error 2
```

Any ideas what's going wrong?
stas00 (CONTRIBUTOR) commented on 2020-08-10 20:34:11:
I found `UNv1.0.en-ru.tar.gz` here: https://conferences.unite.un.org/uncorpus/en/downloadoverview, so it can be reconstructed with:
```
wget -c https://stuncorpusprod.blob.core.windows.net/corpusfiles/UNv1.0.en-ru.tar.gz.00
wget -c https://stuncorpusprod.blob.core.windows.net/corpusfiles/UNv1.0.en-ru.tar.gz.01
wget -c https://stuncorpusprod.blob.core.windows.net/corpusfiles/UNv1.0.en-ru.tar.gz.02
cat UNv1.0.en-ru.tar.gz.0* > UNv1.0.en-ru.tar.gz
```
it has other languages as well, in case https://storage.googleapis.com/tfdataset-data/downloadataset/uncorpus/ is gone
stas00 (CONTRIBUTOR) commented on 2020-08-11 05:04:48:
Further, `nlp.load_dataset('wmt19', 'ru-en')` has only the `train` and `val` datasets. `test` is missing.

Fixed locally for summarization needs, by running:
```
pip install sacrebleu
sacrebleu -t wmt19 -l ru-en --echo src > test.source
sacrebleu -t wmt19 -l ru-en --echo ref > test.target
```
h/t @sshleifer
lhoestq (MEMBER) commented on 2020-08-11 16:34:32:
I created a PR to fix the issue.
It was due to an overflow check that handled badly an empty list.

You can try the changes by using 
```
!pip install git+https://github.com/huggingface/nlp.git@fix-bad-type-in-overflow-check
```

Also I noticed that the first 1000 examples have an empty list in the `title_urls` field. The feature type inference in `.map` will consider it `null` because of that, and it will crash when it encounter the next example with a `title_urls` that is not empty.

Therefore to fix that, what you can do for now is increase the writer batch size so that the feature inference will take into account at least one example with a non-empty `title_urls`:

```python
# default batch size is 1_000 and it's not enough for feature type inference because of empty lists
valid_dataset = valid_dataset.map(make_input_target, writer_batch_size=3_000) 
```

I was able to run the frozen cell with these changes.
ratthachat (NONE) commented on 2020-08-12 00:08:28:
@lhoestq mapping the function `make_input_target` was passed by your fixing.

However, there is another error in the final step of `valid_dataset.map(convert_to_features, batched=True)`

`ArrowInvalid: Could not convert Thepiratebay.vg with type str: converting to null type`
(The [same colab notebook above with new error message](https://colab.research.google.com/drive/14wttOTv3ky74B_c0kv5WrbgQjCF2fYQk?usp=sharing#scrollTo=5sRrJ3_C8rLt))

Do you have some ideas? (I am really sorry I could not debug it by myself since I never used `pyarrow` before) 
Note that `train_dataset.map(convert_to_features, batched=True)` can be run successfully even though train_dataset is 27x bigger than `valid_dataset` so I believe the problem lies in some field of `valid_dataset` again .
lhoestq (MEMBER) commented on 2020-08-12 09:36:57:
I noticed that you also changed the Apache Beam `open` to also use utf-8. However it doesn't have an `encoding` parameter.
Therefore you should ignore lines like

```python
beam.io.filesystems.FileSystems.open(filepath)
```

I guess you could add a rule to your regex to only include the `open` call that have a space right before it.
lhoestq (MEMBER) commented on 2020-08-12 10:32:50:
In 0.4.0, the assertion in `concatenate_datasets ` is on the features, and not the schema.
Could you try to update `nlp` ?

Also, since 0.4.0, you can use `dset_wikipedia.cast_(dset_books.features)` to avoid the schema cast hack.
lhoestq (MEMBER) commented on 2020-08-12 13:30:08:
Thanks for reporting this issue

There was a bug where numpy arrays would get returned instead of tensorflow tensors.
This is fixed on master.

I tried to re-run the colab and encountered this error instead:

```
AttributeError: 'tensorflow.python.framework.ops.EagerTensor' object has no attribute 'to_tensor'
```

This is because the dataset returns a Tensor and not a RaggedTensor.
But I think we should always return a RaggedTensor unless the length of the sequence is fixed (it that case they can be stack into a Tensor).
Narsil (CONTRIBUTOR) commented on 2020-08-12 15:39:57:
Ok I tried to add the dummy dataset (I actually modified the dummy_data command to generate them for me because it was too painful to do that manually).

The dummy_data test seems to work:
```bash
RUN_SLOW=1 pytest tests/test_dataset_common.py::LocalDatasetTest::test_load_dataset_all_configs_iwslt2017
```

However the test on the full data fails, because the `**config_kwargs` don't include `pair, multilingual`.
I could add a default parameter for the Config (but that feels broken, how can one config be the "default" ?). If I do I still have errors, saying that something within the downloader is a directory so I'm not sure where that comes from.

I can share my auto_zip dummy data code if you want (I tried to keep it clean). [Edit: it's [here](https://github.com/Narsil/nlp/tree/auto_zip)]. 
The way it works is that it just keeps X line from the beginning of the original files, and Y lines at the end. It's good enough for my usage, but I guess it could work for most data files out there (as long as they're real text and not binary format)
lewtun (MEMBER) commented on 2020-08-12 20:12:09:
Good catch @lhoestq! Your suggestion to match on `open(...)` with a whitespace was a great idea - it allowed me to simplify the regexp considerably 😄.

I fixed the Apache Beam false positives and also caught a few problems in `json.load()`, e.g.
```python
relation_name_map = json.load(open(rel_info), encoding='utf-8')
```

I've tested that the new regexp doesn't reintroduce these false positives, so I think the PR is ready for another review.
jarednielsen (CONTRIBUTOR) commented on 2020-08-12 22:57:46:
I'm using the master branch. The assertion failure comes from the underlying `pa.concat_tables()`, which is in the pyarrow package. That method does check schemas.

Since `features.type` does not contain information about nullable vs non-nullable features, the `cast_()` method won't resolve the schema mismatch. There is information in a schema which is not stored in features.
lhoestq (MEMBER) commented on 2020-08-13 08:44:39:
Thanks for reporting !

To store the cache file, we compute a hash of the function given in `.map`, using our own hashing function.
The hash doesn't seem to stay the same over sessions for the tokenizer.
Apparently this is because of the regex at `tokenizer.pat` is not well supported by our hashing function.

I'm working on a fix
lhoestq (MEMBER) commented on 2020-08-13 17:07:54:
I took a look at the dummy data creation for this dataset.

Maybe it didn't work on your side might be because `master.zip` and `narrativeqa_full_text.zip` are supposed to be directories and not acutal zip files in the dummy data folder.

I managed to make it work with this `dummy_data.zip` file:
https://drive.google.com/file/d/1G9ZHAjelazNApbFI0ep2dnSAWklXgGMd/view?usp=sharing
lhoestq (MEMBER) commented on 2020-08-17 10:22:37:
Not sure why github thinks there are conflicts here, as I just rebased from the current master branch.
Merging into master locally works on my side without conflicts
```
git checkout master
git reset --hard origin/master
git merge --no-ff eltoto1219/support_multi_dim_tensors_for_images
Merge made by the 'recursive' strategy.
 datasets/lxmert_pretraining_beta/lxmert_pretraining_beta.py |  89 +++++++++++++++++++++++++++++++++++++
 datasets/lxmert_pretraining_beta/test_multi_array.py        |  45 +++++++++++++++++++
 datasets/lxmert_pretraining_beta/to_arrow_data.py           | 371 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/nlp/arrow_dataset.py                                    |  24 +++++-----
 src/nlp/arrow_writer.py                                     |  22 ++++++++--
 src/nlp/features.py                                         | 229 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 tests/test_array_2d.py                                      | 210 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 969 insertions(+), 21 deletions(-)
 create mode 100644 datasets/lxmert_pretraining_beta/lxmert_pretraining_beta.py
 create mode 100644 datasets/lxmert_pretraining_beta/test_multi_array.py
 create mode 100644 datasets/lxmert_pretraining_beta/to_arrow_data.py
 create mode 100644 tests/test_array_2d.py
```
thomwolf (MEMBER) commented on 2020-08-18 19:00:35:
Hi @vegarab yes feel free to open a discussion here.

This design choice was not very much thought about.

Since `dataset.select()` (like all the method without a trailing underscore) is non-destructive and returns a new dataset it has most of its properties initialized from scratch (except the table and infos).

Thinking about it I don't see a strong reason against transmitting the format from the parent dataset to its newly created child. It's probably what's expected by the user in most cases. What do you think @lhoestq?

By the way, I've been working today on a refactoring of all the samples re-ordering/selection methods (`select`, `sort`, `shuffle`, `shard`, `train_test_split`). The idea is to speed them up by a lot (like, really a lot) by working as much as possible with an indices mapping table instead of doing a deep copy of the full dataset as we've been doing currently. You can give it a look and try it here: https://github.com/huggingface/nlp/pull/513
Feedbacks are very much welcome
vegarab (CONTRIBUTOR) commented on 2020-08-18 19:21:40:
I failed to realise that this only applies to `shuffle()`. Whenever `keep_in_memory` is set to True, this is passed on to the `select()` function. However, if `cache_file_name` is None, it will be defined in the `shuffle()` function before it is passed on to `select()`. 

Thus, `select()` is called with `keep_in_memory=True` and a not None value for `cache_file_name`. 
This is essentially fixed in #513 

Easily reproducible:
```python
>>> import nlp
>>> data = nlp.load_dataset("cosmos_qa", split="train")
Using custom data configuration default
>>> data.shuffle(keep_in_memory=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/vegarab/.conda/envs/torch/lib/python3.7/site-packages/nlp/arrow_dataset.py", line 1398, in shuffle
    verbose=verbose,
  File "/home/vegarab/.conda/envs/torch/lib/python3.7/site-packages/nlp/arrow_dataset.py", line 1178, in select
    ), "Please use either `keep_in_memory` or `cache_file_name` but not both."
AssertionError: Please use either `keep_in_memory` or `cache_file_name` but not both.
>>>data.select([0], keep_in_memory=True)
# No error
```
vegarab (CONTRIBUTOR) commented on 2020-08-19 11:36:02:
My bad. This is actually not fixed in #513. Sorry about that...
The new `indices_cache_file_name` is set to a non-None value in the new `shuffle()` as well. 

The buffer and caching mechanisms used in the `select()` function are too intricate for me to understand why the check is there at all. I've removed it in my local build and it seems to be working fine for my project, without really considering other implications of the change.