t20062443/AT1ds huggingface dataset:
albertvillanova (MEMBER) commented on 2021-02-25 14:19:25:
Thanks @lhoestq. I was trying to follow @thomwolf suggestion about integrating that script but as `from_json` method...
> Note that I don't think this is necessary a breaking change, we can still keep the old scripts around

Do you think there is a better way of doing it?

I was trying to implement more or less the same logic as in the script, but I confess I assumed the target was in-memory only...
albertvillanova (MEMBER) commented on 2021-02-25 14:33:36:
Basically, I was trying to reimplement `Json(datasets.ArrowBasedBuilder)._generate_tables`, and no writing to arrow file (I assumed only in-memory usage). I started with the first "else" clause... 

I was planning to remove my `_cast_table_to_info_features` and use `paj.read_json(parse_options=...)` instead (like in the script).
stas00 (CONTRIBUTOR) commented on 2021-02-25 18:58:26:
Thank you for clarifying that the metrics files are to be found elsewhere, @lhoestq 

> The cache at ~/.cache/huggingface/metrics stores the users data for metrics computations (hence the arrow files).

could it be renamed to reflect that? otherwise it misleadingly suggests that it's the metrics. Perhaps `~/.cache/huggingface/metrics-user-data`?

And there are so many `.lock` files w/o corresponding files under `~/.cache/huggingface/metrics/`. Why are they there? 

for example after I wipe out the dir completely and do one training I end up with:
```
~/.cache/huggingface/metrics/sacrebleu/default/default_experiment-1-0.arrow.lock
```
what is that lock file locking when nothing is running?
lhoestq (MEMBER) commented on 2021-02-25 19:39:33:
Update on this: 

I managed to create a mock environment for tests that makes the connections hang until timeout.
I managed to reproduce the issue you're having in this environment.

I'll update the offline test cases to also test the robustness to connection hangings, and make sure we set proper timeouts where it's needed in the code. This should cover the _automatic_ section you mentioned.
stas00 (CONTRIBUTOR) commented on 2021-02-25 19:44:44:
are you sure you need an external lock file? if it's a single purpose locking in the same scope you can lock the caller `__file__` instead, e.g. here is how one can `flock` the script file itself to ensure atomic printing:

```
import fcntl
def printflock(*msgs):
    """ print in multiprocess env so that the outputs from different processes don't get interleaved """
    with open(__file__, "r") as fh:
        fcntl.flock(fh, fcntl.LOCK_EX)
        try:
            print(*msgs)
        finally:
            fcntl.flock(fh, fcntl.LOCK_UN)
```
stas00 (CONTRIBUTOR) commented on 2021-02-25 20:00:59:
Thank you for explaining the intention, @lhoestq 

1. Could it be then made more human-friendly? Currently the hex gibberish tells me nothing of what's really going on. e.g. the following is instructive, IMHO:

```
WARNING: wmt16/ro-en/train dataset was loaded from cache instead of being recomputed
WARNING: wmt16/ro-en/validation dataset was loaded from cache instead of being recomputed
WARNING: wmt16/ro-en/test dataset was loaded from cache instead of being recomputed
```
note that it removes the not so useful hex info and tells the user instead which split it's referring to - but probably no harm in keeping the path if it helps the debug. But the key is that now the warning is telling me what it is it's warning me about.
```
Warning:Loading cache path
```
on the other hand isn't telling what it is warning about.

And I still suggest this is INFO level, otherwise you need to turn all 'using cache' statements to WARNING to be consistent. The user is most likely well aware the cache is used for models, etc. So this feels very similar.

2. Should there be a way for a user to void warranty by having a flag - `I know I'm expecting the cached version to load if it's available - please do not warn me about it=True`

To explain the need: Warnings are a problem, they constantly take attention away because they could be the harbinger of a problem. Therefore I prefer not to have any warnings in the log, and if I get any I usually try to deal with those so that my log is clean. 

It's less of an issue for somebody doing long runs. It's a huge issue for someone who does a new run every few minutes and on the lookout for any potential problems which is what I have been doing a lot of integrating DeepSpeed and other things. And since there are already problems to deal with during the integration it's nice to have a clean log to start with. 

I hope my need is not unreasonable and I was able to explain it adequately. 

Thank you.
justin-yan (CONTRIBUTOR) commented on 2021-02-25 23:56:39:
OK! I just whipped up the `from_pandas()` portion of this PR, and it works, though I'm not *super* familiar with the available APIs so I'm not sure if there's a more "vectorized" way of doing all of these updates - so happy to get some feedback and iterate!

Apologies for multiple commits - I realized how to solve a few different problems right after I gave up and pushed with the intent to ask for help :-)

I wanted to get some guidance on how to handle the reverse direction: I think there are two main areas to look at, `.to_pandas()` and also `.set_format('pandas')` and then pulling out a dataframe like so: `dataset[:]`.  Is there a single place where I can handle both of these cases at once or do these need to be handled independently?
gchhablani (CONTRIBUTOR) commented on 2021-02-26 01:16:02:
Hi @lhoestq,

Sorry again, the last couple of weeks were a bit busy for me. I am wondering how do you want me to achieve that. Using a custom BuilderConfig which takes in whether it is the regular data or "pid2name"? "pid2name" is only useful for "train_wiki", "val_nyt" and "val_wiki". So, based on my understanding, it would look like this:

```python
wiki_data = load_dataset('few_rel','train_wiki')
id2name = load_dataset('few_rel','pid2name')
```
and this will be handled in the multiple configs.


A better alternative could be providing name of the relationship in only "train_wiki", "val_nyt" and "val_wiki" as an extra feature in the dataset, and doing away with "pid2name" entirely. I'll only download pid2name if any of those datasets are requested, and then during generation I'll return the list with the dataset under "names" feature. How does this sound?

EDIT:
There is one issue with the second approach, the entire pid2name is saved with all three datasets - "train_wiki", "val_nyt" and "val_wiki" ([see code below](https://github.com/huggingface/datasets/pull/1823#issuecomment-786402026)). In dummy data, I can address this by manually editing the pid2name to contain only a few id-name pairs, those matching with the examples in the corresponding example file. But this seems to be inefficient for the entire dataset - storing the same file in multiple places.
gchhablani (CONTRIBUTOR) commented on 2021-02-26 02:10:15:
Hi @lhoestq,

Sorry for the delay, I have added the changes from the review. For the ISO format language codes, I just selected the first two characters from the names, hoping those are correct. Let me know if you want me to verify :P

Thanks for taking the time to add such a detailed review. I'll keep all these changes in mind the next time I'm adding a dataset.

Thanks,
Gunjan
lhoestq (MEMBER) commented on 2021-02-26 09:49:55:
Thanks ! This is awesome :) 
Could you also add a test ? There is already `test_to_pandas` in test_arrow_dataset.py
Feel free to complete this test to make sure it works for Categorical :)

To make it work with the "pandas" formating (when you do `set_format("pandas")` and then query `dataset[0]`, `dataset[:]`, etc.), you can take a look and the `PandasFormatter` in formatting.py
It takes a pyarrow table as input of its formatting methods (one method for rows, one for columns and one for batches) and returns a pandas DataFrame (or a Series for the method for formatting a column). You can cast to Categorical in each one of the formatter methods and it should work directly when you use a pandas-formatted dataset.

This formatter can then also be used in `to_pandas` (currently it does `pa_table.to_pandas()` but `PandasFormatter().format_batch(pa_table)` can be used instead).
lhoestq (MEMBER) commented on 2021-02-26 10:06:51:
When you're using metrics in a distributed setup, there are two cases:
1. you're doing two completely different experiments (two evaluations) and the 2 metrics jobs have nothing to do with each other
2. you're doing one experiment (one evaluation) but use multiple processes to feed the data to the metric.

In case 1. you just need to provide two different `experiment_id` so that the metrics don't collide.
In case 2. they must have the same experiment_id (or use the default one), but in this case you also need to provide the `num_processes` and `process_id`

If understand correctly you're in situation 2.

If so, you make sure that you instantiate the metrics with both the right `num_processes` and `process_id` parameters ?

If they're not set, then the cache files of the two metrics collide it can cause issues. For example if one metric finishes before the other, then the cache file is deleted and the other metric gets a FileNotFoundError
There's more information in the [documentation](https://huggingface.co/docs/datasets/loading_metrics.html#distributed-setups) if you want

Hope that helps !
lhoestq (MEMBER) commented on 2021-02-26 10:51:39:
Hi @gchhablani :)
Thanks for proposing your help !

I'll be doing a refactor of some parts related to filtering in the scope of https://github.com/huggingface/datasets/issues/1877
So I would first wait for this refactor to be done before working on the filtering. In particular because I plan to make things simpler to manipulate.

Your feedback on this refactor would also be appreciated since it also aims at making the core code more accessible (basically my goal is that no one's ever "having troubles getting started" ^^)

This will be available in a few days, I will be able to give you more details at that time if you don't mind waiting a bit !
stas00 (CONTRIBUTOR) commented on 2021-02-26 17:52:35:
I never said the calls were hanging indefinitely, what we need is quite different - in the firewalled env with a network, there should be no network calls or they should fail instantly.

To make this work I suppose on top of this PR we need:
1. `DATASETS_OFFLINE` env var to force set timeout to 0 globally (or to 0.0001 if 0 has a special meaning of no timeout)
2. `DATASETS_OFFLINE` should guard against failing network calls and not fail the program if it has all the data it needs locally.

Bottom line - if the logic wants to check online if the local file matches online dataset name, let it go wild, but it should fail instantly, recover and use the local file - if one is specified explicitly or cache if there is one. And only if neither was found only then assert.

I hope this makes sense and is doable.

I have started on the same approach for transformers https://github.com/huggingface/transformers/pull/10407

Thank you, @lhoestq
stas00 (CONTRIBUTOR) commented on 2021-02-26 17:56:35:
Thank you for explaining that in a great way, @lhoestq 

So the bottom line is that the `transformers` examples are broken since they don't do any of that. At least `run_seq2seq.py` just does `metric = load_metric(metric_name)`

What test would you recommend to reliably reproduce this bug in `examples/seq2seq/run_seq2seq.py`?
lhoestq (MEMBER) commented on 2021-02-26 18:16:45:
Yes that was the first step to add DATASETS_OFFLINE :)

With this PR, if a request times out (which couldn't happen before because no time out was set), it falls back on the local files with no error.

As you said, setting the timeout to something like 1e-16 makes the requests fail instantly, which is one step forward. One last thing left is to disable request retries and everything will be instant !
sgugger (CONTRIBUTOR) commented on 2021-02-27 00:03:50:
To give more context, we are just using the metrics for the `comput_metric` function and nothing else. Is there something else we can use that just applies the function to the full arrays of predictions and labels? Because that's all we need, all the gathering has already been done because the datasets Metric multiprocessing relies on file storage and thus does not work in a multi-node distributed setup (whereas the Trainer does).

Otherwise, we'll have to switch to something else to compute the metrics :-(
stas00 (CONTRIBUTOR) commented on 2021-02-27 21:15:41:
I tried to adjust `run_seq2seq.py` and trainer to use the suggested dist env:
```
    import torch.distributed as dist
    metric = load_metric(metric_name, num_process=dist.get_world_size(), process_id=dist.get_rank())
```
and in `trainer.py` added to call just for rank 0:
```
        if self.is_world_process_zero() and self.compute_metrics is not None and preds is not None and label_ids is not None:
            metrics = self.compute_metrics(EvalPrediction(predictions=preds, label_ids=label_ids))
```
and then the process hangs in a deadlock. 

Here is the tb:
```
  File "/mnt/nvme1/code/huggingface/datasets-master/src/datasets/utils/filelock.py", line 275 in acquire
  File "/mnt/nvme1/code/huggingface/datasets-master/src/datasets/metric.py", line 306 in _check_all_processes_locks
  File "/mnt/nvme1/code/huggingface/datasets-master/src/datasets/metric.py", line 501 in _init_writer
  File "/mnt/nvme1/code/huggingface/datasets-master/src/datasets/metric.py", line 440 in add_batch
  File "/mnt/nvme1/code/huggingface/datasets-master/src/datasets/metric.py", line 397 in compute
  File "examples/seq2seq/run_seq2seq.py", line 558 in compute_metrics
  File "/mnt/nvme1/code/huggingface/transformers-mp-pp/src/transformers/trainer.py", line 1876 in prediction_loop
  File "/mnt/nvme1/code/huggingface/transformers-mp-pp/src/transformers/trainer.py", line 1703 in evaluate
  File "/mnt/nvme1/code/huggingface/transformers-mp-pp/src/transformers/trainer_seq2seq.py", line 74 in evaluate
  File "examples/seq2seq/run_seq2seq.py", line 603 in main
  File "examples/seq2seq/run_seq2seq.py", line 651 in <module>
```

But this sounds right, since in the above diff I set up a distributed metric and only called one process - so it's blocking on waiting for other processes to do the same.

So one working solution is to leave:

```
    metric = load_metric(metric_name)
```
alone, and only call `compute_metrics` from rank 0
```
        if self.is_world_process_zero() and self.compute_metrics is not None and preds is not None and label_ids is not None:
            metrics = self.compute_metrics(EvalPrediction(predictions=preds, label_ids=label_ids))
```

so we now no longer use the distributed env as far as `datasets` is concerned, it's just a single process.

Are there any repercussions/side-effects to this proposed change in Trainer?  If it always gathers all inputs on rank 0 then this is how it should have been done in first place - i.e. only run for rank 0. It appears that currently it was re-calculating the metrics on all processes on the same data just to throw the results away other than for rank 0. Unless I missed something.
stas00 (CONTRIBUTOR) commented on 2021-02-27 21:39:09:
But no, since 
`
 metric = load_metric(metric_name)
`
is called for each process, the race condition is still there. So still getting:

```
ValueError: Error in finalize: another metric instance is already using the local cache file. Please specify an experiment_id to avoid colision between distributed metric instances.
```

i.e. the only way to fix this is to `load_metric` only for rank 0, but this requires huge changes in the code and all end users' code.
stas00 (CONTRIBUTOR) commented on 2021-02-27 22:01:05:
OK, here is a workaround that works. The onus here is absolutely on the user:

```
diff --git a/examples/seq2seq/run_seq2seq.py b/examples/seq2seq/run_seq2seq.py
index 2a060dac5..c82fd83ea 100755
--- a/examples/seq2seq/run_seq2seq.py
+++ b/examples/seq2seq/run_seq2seq.py
@@ -520,7 +520,11 @@ def main():

     # Metric
     metric_name = "rouge" if data_args.task.startswith("summarization") else "sacrebleu"
-    metric = load_metric(metric_name)
+    import torch.distributed as dist
+    if dist.is_initialized():
+        metric = load_metric(metric_name, num_process=dist.get_world_size(), process_id=dist.get_rank())
+    else:
+        metric = load_metric(metric_name)

     def postprocess_text(preds, labels):
         preds = [pred.strip() for pred in preds]
@@ -548,12 +552,17 @@ def main():
         # Some simple post-processing
         decoded_preds, decoded_labels = postprocess_text(decoded_preds, decoded_labels)

+        kwargs = dict(predictions=decoded_preds, references=decoded_labels)
+        if metric_name == "rouge":
+            kwargs.update(use_stemmer=True)
+        result = metric.compute(**kwargs) # must call for all processes
+        if result is None: # only process with rank-0 will return metrics, others None
+            return {}
+
         if metric_name == "rouge":
-            result = metric.compute(predictions=decoded_preds, references=decoded_labels, use_stemmer=True)
             # Extract a few results from ROUGE
             result = {key: value.mid.fmeasure * 100 for key, value in result.items()}
         else:
-            result = metric.compute(predictions=decoded_preds, references=decoded_labels)
             result = {"bleu": result["score"]}

         prediction_lens = [np.count_nonzero(pred != tokenizer.pad_token_id) for pred in preds]
```

This is not user-friendly to say the least. And it's still wasteful as we don't need other processes to do anything.

But it solves the current race condition.

Clearly this calls for a design discussion as it's the responsibility of the Trainer to handle this and not user's. Perhaps in the `transformers` land?
sgugger (CONTRIBUTOR) commented on 2021-02-27 23:58:26:
I don't see how this could be the responsibility of `Trainer`, who hasn't the faintest idea of what a `datasets.Metric` is. The trainer takes a function `compute_metrics` that goes from predictions + labels to metric results, there is nothing there. That computation is done on all processes  

The fact a `datasets.Metric` object cannot be used as a simple compute function in a multi-process environment is, in my opinion, a bug in `datasets`. Especially since, as I mentioned before, the multiprocessing part of `datasets.Metric` has a deep flaw since it can't work in a multinode environment. So you actually need to do the job of gather predictions and labels yourself.

The changes you are proposing Stas are making the code less readable and also concatenate all the predictions and labels `number_of_processes` times I believe, which is not going to make the metric computation any faster.
stas00 (CONTRIBUTOR) commented on 2021-02-28 00:02:41:
Right, to clarify, I meant it'd be good to have it sorted on the library side and not requiring the user to figure it out. This is too complex and error-prone and if not coded correctly the bug will be intermittent which is even worse.

Oh I guess I wasn't clear in my message - in no way I'm proposing that we use this workaround code - I was just showing what I had to do to make it work.

We are on the same page.

> The changes you are proposing Stas are making the code less readable and also concatenate all the predictions and labels number_of_processes times I believe, which is not going to make the metric computation any faster.

And yes, this is another problem that my workaround introduces. Thank you for pointing it out, @sgugger
lhoestq (MEMBER) commented on 2021-03-01 11:07:59:
> The fact a datasets.Metric object cannot be used as a simple compute function in a multi-process environment is, in my opinion, a bug in datasets

Yes totally, this use case is supposed to be supported by `datasets`. And in this case there shouldn't be any collision between the metrics. I'm looking into it :)
My guess is that at one point the metric isn't using the right file name. It's supposed to use one with a unique uuid in order to avoid the collisions.
lhoestq (MEMBER) commented on 2021-03-01 11:55:07:
Hi !

To fix 1, an you try to run this code ?
```python
from datasets import load_dataset

load_dataset("squad", download_mode="force_redownload")
```
Maybe the file your downloaded was corrupted, in this case redownloading this way should fix your issue 1.

Regarding your 2nd point, you're right that loading the raw json this way doesn't give you a dataset with the column "context", "question" and "answers". Indeed the squad format is a very nested format so you have to preprocess the data. You can do it this way:
```python
def process_squad(examples):
    """
    Process a dataset in the squad format with columns "title" and "paragraphs"
    to return the dataset with columns "context", "question" and "answers".
    """
    out = {"context": [], "question": [], "answers":[]} 
    for paragraphs in examples["paragraphs"]: 
        for paragraph in paragraphs: 
            for qa in paragraph["qas"]: 
                answers = [{"answer_start": answer["answer_start"], "text": answer["text"].strip()} for answer in qa["answers"]] 
                out["context"].append(paragraph["context"].strip()) 
                out["question"].append(qa["question"].strip()) 
                out["answers"].append(answers) 
    return out

datasets = load_dataset(extension, data_files=data_files, field="data")
column_names = datasets["train"].column_names

if set(column_names) == {"title", "paragraphs"}:
    datasets = datasets.map(process_squad, batched=True, remove_columns=column_names)
```

Hope that helps :)
gaceladri (NONE) commented on 2021-03-01 14:04:24:
Great comment @alexvaca0 . I think that we could re-open the issue as a reformulation of why it takes so much space to save the arrow. Saving a 1% of oscar corpus takes more thank 600 GB (it breaks when it pass 600GB because it is the free memory that I have at this moment) when the full dataset is 1,3 TB. I have a 1TB M.2 NVMe disk that I can not train on because the saved .arrow files goes crazily big. If you can share your Collator I will be grateful.
yavuzKomecoglu (CONTRIBUTOR) commented on 2021-03-01 18:23:21:
> Thanks for changing to ClassLabel :)
> This is all good now !
> 
> However I can see changes in other files than the ones for interpress_news_category_tr_lite, can you please fix that ?
> To do so you can create another branch and another PR to only include the interpress_news_category_tr_lite files.
> 
> Maybe this happened because of a git rebase ? Once you've already pushed your code, please use git merge instead of rebase in order to avoid this.

Thanks for the feedback.
New PR https://github.com/huggingface/datasets/pull/1967
stas00 (CONTRIBUTOR) commented on 2021-03-01 18:30:53:
Since the failure was originally intermittent, there is no 100% telling that the problem is gone. 
But if my artificial race condition setup https://github.com/huggingface/datasets/issues/1942#issuecomment-787124529 is to be the litmus test then the problem has been fixed, as with this PR branch that particular race condition is taken care of correctly.

Thank you for taking care of this, @lhoestq  - locking can be very tricky to do right!
lhoestq (MEMBER) commented on 2021-03-02 12:58:04:
Hi !
As far as I know not all faiss indexes can be computed in parallel and then merged. 
For example [here](https://github.com/facebookresearch/faiss/wiki/Special-operations-on-indexes#splitting-and-merging-indexes) is is mentioned that only IndexIVF indexes can be merged.
Moreover faiss already works using multithreading to parallelize the workload over your different CPU cores. You can find more info [here](https://github.com/facebookresearch/faiss/wiki/Threads-and-asynchronous-calls#internal-threading)
So I feel like the gains we would get by implementing a parallel `add_faiss_index` would not be that important, but let me know what you think.