SergeiGKS/huggingface-dataset-issues huggingface dataset:
Issue: [experiment] missing default_experiment-1-0.arrow

Comment:
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.
Issue: [experiment] missing default_experiment-1-0.arrow

Comment:
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.
Issue: [experiment] missing default_experiment-1-0.arrow

Comment:
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?
Issue: [experiment] missing default_experiment-1-0.arrow

Comment:
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.
Issue: [experiment] missing default_experiment-1-0.arrow

Comment:
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
Issue: [experiment] missing default_experiment-1-0.arrow

Comment:
> 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.
Issue: [experiment] missing default_experiment-1-0.arrow

Comment:
I just opened #1966 to fix this :)
@stas00 if have a chance feel free to try it !
Issue: Loading of FAISS index fails for index_name = 'exact'

Comment:
Works great 👍 I just put a minor comment on the commit, I think you meant to pass the `train_size` from the one obtained from the config.

Thanks for a quick response!
Issue: Side effect when filtering data due to `does_function_return_dict` call in `Dataset.map()`

Comment:
Thanks for the report !

Currently we don't have a way to let the user easily disable this behavior.
However I agree that we should support stateful processing functions, ideally by removing `does_function_return_dict`.

We needed this function in order to know whether the `map` functions needs to write data or not. if `does_function_return_dict` returns False then we don't write anything.

Instead of checking the output of the processing function outside of the for loop that iterates through the dataset to process it, we can check the output of the first processed example and at that point decide if we need to write data or not.

Therefore it's definitely possible to fix this unwanted behavior, any contribution going into this direction is welcome :)
Issue: [firewalled env] OFFLINE mode

Comment:
Thanks for reporting and for all the details and suggestions.

I'm totally in favor of having a HF_DATASETS_OFFLINE env variable to disable manually all the connection checks, remove retries etc.

Moreover you may know that the use case that you are mentioning is already supported from `datasets` 1.3.0, i.e. you already can:
- first load datasets and metrics from an instance with internet connection
- then be able to reload datasets and metrics from another instance without connection (as long as the filesystem is shared)

This is already implemented, but currently it only works if the requests return a `ConnectionError` (or any error actually). Not sure why it would hang instead of returning an error.

Maybe this is just a issue with the timeout value being not set or too high ?
Is there a way I can have access to one of the instances on which there's this issue (we can discuss this offline) ?
Issue: [firewalled env] OFFLINE mode

Comment:
I'm on master, so using all the available bells and whistles already.

If you look at the common issues - it for example tries to look up files if they appear in `_PACKAGED_DATASETS_MODULES` which it shouldn't do.

--------------

Yes, there is a nuance to it. As I mentioned it's firewalled - that is it has a network but making any calls outside - it just hangs in:

```
sin_addr=inet_addr("xx.xx.xx.xx")}, [28->16]) = 0
close(5)                                = 0
socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_TCP) = 5
connect(5, {sa_family=AF_INET, sin_port=htons(3128), sin_addr=inet_addr("yy.yy.yy.yy")}, 16^C) = ? ERESTARTSYS (To be restarted if SA_RESTART is set)
```
until it times out.

That's why we need to be able to tell the software that there is no network to rely on even if there is one (good for testing too).

So what I'm thinking is that this is a simple matter of pre-ambling any network call wrappers with:

```
if HF_DATASETS_OFFLINE:
    assert "Attempting to make a network call under Offline mode"
```

and then fixing up if there is anything else to fix to make it work.

--------------

Otherwise I think the only other problem I encountered is that we need to find a way to pre-cache metrics, for some reason it's not caching it and wanting to fetch it from online.

Which is extra strange since it already has those files in the `datasets` repo itself that is on the filesystem.

The workaround I had to do is to copy `rouge/rouge.py` (with the parent folder) from the datasets repo to the current dir - and then it proceeded.
Issue: [firewalled env] OFFLINE mode

Comment:
Ok understand better the hanging issue.
I guess catching connection errors is not enough, we should also avoid all the hangings.
Currently the offline mode tests are only done by simulating an instant connection fail that returns an error, let's have another connection mock that hangs instead.

I'll also take a look at why you had to do this for `rouge`.
Issue: [firewalled env] OFFLINE mode

Comment:
FWIW, I think instant failure on the behalf of a network call is the simplest solution to correctly represent the environment and having the caller to sort it out is the next thing to do, since here it is the case of having no functional network, it's just that the software doesn't know this is the case, because there is some network. So we just need to help it to bail out instantly rather than hang waiting for it to time out. And afterwards everything else you said.
Issue: [firewalled env] OFFLINE mode

Comment:
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.
Issue: [firewalled env] OFFLINE mode

Comment:
I lost access to the firewalled setup, but I emulated it with:

```
sudo ufw enable
sudo ufw default deny outgoing
```
(thanks @mfuntowicz)

I was able to test `HF_DATASETS_OFFLINE=1` and it worked great - i.e. didn't try to reach out with it and used the cached files instead.

Thank you!
Issue: Anonymous Dataset Addition (i.e Anonymous PR?)

Comment:
Hi !
I guess you can add a dataset without the fields that must be kept anonymous, and then update those when the anonymity period is over.
You can also make the PR from an anonymous org.
Pinging @yjernite just to make sure it's ok
Issue: Anonymous Dataset Addition (i.e Anonymous PR?)

Comment:
Hello,
I would prefer to do the reverse: adding a link to an anonymous paper without the people names/institution in the PR. Would it be conceivable ?
Cheers
Issue: How to update the "wino_bias" dataset

Comment:
Hi @JieyuZhao !

You can edit the dataset card of wino_bias to update the URL via a Pull Request. This would be really appreciated :)

The dataset card is the README.md file you can find at https://github.com/huggingface/datasets/tree/master/datasets/wino_bias
Also the homepage url is also mentioned in the wino_bias.py so feel free to update it there as well.

You can create a Pull Request directly from the github interface by editing the files you want and submit a PR, or from a local clone of the repository.

Thanks for noticing !
Issue: Failure to save with save_to_disk

Comment:
Hi thanks for reporting and for proposing a fix :)

I just merged a fix, feel free to try it from the master branch !
Issue: Unable to download `wiki_dpr`

Comment:
Thanks for reporting ! This is a bug. For now feel free to set `ignore_verifications=False` in `load_dataset`.
I'm working on a fix
Issue: Unable to download `wiki_dpr`

Comment:
I just merged a fix :)

We'll do a patch release soon. In the meantime feel free to try it from the master branch
Thanks again for reporting !
Issue: Saving processed dataset running infinitely

Comment:
am suspicious of this thing? what's the purpose of this? pickling and unplickling
`self = pickle.loads(pickle.dumps(self))`

```
    def save_to_disk(self, dataset_path: str, fs=None):
        """
        Saves a dataset to a dataset directory, or in a filesystem using either :class:`datasets.filesystem.S3FileSystem` or any implementation of ``fsspec.spec.AbstractFileSystem``.

        Args:
            dataset_path (``str``): path (e.g. ``dataset/train``) or remote uri (e.g. ``s3://my-bucket/dataset/train``) of the dataset directory where the dataset will be saved to
            fs (Optional[:class:`datasets.filesystem.S3FileSystem`,``fsspec.spec.AbstractFileSystem``],  `optional`, defaults ``None``): instance of :class:`datasets.filesystem.S3FileSystem` or ``fsspec.spec.AbstractFileSystem`` used to download the files from remote filesystem.
        """
        assert (
            not self.list_indexes()
        ), "please remove all the indexes using `dataset.drop_index` before saving a dataset"
        self = pickle.loads(pickle.dumps(self))
  ```
Issue: Saving processed dataset running infinitely

Comment:
Tried finding the root cause but was unsuccessful.
I am using lazy tokenization with `dataset.set_transform()`, it works like a charm with almost same performance as pre-compute.
Issue: Saving processed dataset running infinitely

Comment:
Hi ! This very probably comes from the hack you used.

The pickling line was added an a sanity check because save_to_disk uses the same assumptions as pickling for a dataset object. The main assumption is that memory mapped pyarrow tables must be reloadable from the disk. In your case it's not possible since you altered the pyarrow table.
I would suggest you to rebuild a valid Dataset object from your new pyarrow table. To do so you must first save your new table to a file, and then make a new Dataset object from that arrow file.

You can save the raw arrow table (without all the `datasets.Datasets` metadata) by calling `map` with `cache_file_name="path/to/outut.arrow"` and `function=None`. Having `function=None` makes the `map` write your dataset on disk with no data transformation.

Once you have your new arrow file, load it with `datasets.Dataset.from_file` to have a brand new Dataset object :)

In the future we'll have a better support for the fast filtering method from pyarrow so you don't have to do this very unpractical workaround. Since it breaks somes assumptions regarding the core behavior of Dataset objects, this is very discouraged.
Issue: DBPedia14 Dataset Checksum bug?

Comment:
Hi ! :)

This looks like the same issue as https://github.com/huggingface/datasets/issues/1856 
Basically google drive has quota issues that makes it inconvenient for downloading files.

If the quota of a file is exceeded, you have to wait 24h for the quota to reset (which is painful).

The error says that the checksum of the downloaded file doesn't match because google drive returns a text file with the "Quota Exceeded" error instead of the actual data file.
Issue: Feature Request: Support for Pandas `Categorical`

Comment:
We already have a ClassLabel type that does this kind of mapping between the label ids (integers) and actual label values (strings).

I wonder if actually we should use the DictionaryType from Arrow and the Categorical type from pandas for the `datasets` ClassLabel feature type.
Currently ClassLabel corresponds to `pa.int64()` in pyarrow and `dtype('int64')` in pandas (so the label names are lost during conversions).

What do you think ?
Issue: Feature Request: Support for Pandas `Categorical`

Comment:
Now that I've heard you explain ClassLabel, that makes a lot of sense!  While DictionaryType for Arrow (I think) can have arbitrarily typed keys, so it won't cover all potential cases, pandas' Category is *probably* the most common use for that pyarrow type, and ClassLabel should match that perfectly?

Other thoughts:

- changing the resulting patype on ClassLabel might be backward-incompatible?  I'm not totally sure if users of the `datasets` library tend to directly access the `patype` attribute (I don't think we really do, but we haven't been using it for very long yet).
- would ClassLabel's dtype change to `dict[int64, string]`?  It seems like in practice a ClassLabel (when not explicitly specified) would be constructed from the DictionaryType branch of `generate_from_arrow_type`, so it's not totally clear to me that anyone ever actually accesses/uses that dtype?
- I don't quite know how `.int2str` and `.str2int` are used in practice - would those be kept?  Perhaps the implementation might actually be substantially smaller if we can just delegate to pyarrow's dict methods?

Another idea that just occurred to me: add a branch in here to generate a ClassLabel if the dict key is int64 and the values are string: https://github.com/huggingface/datasets/blob/master/src/datasets/features.py#L932 , and then don't touch anything else.

In practice, I don't think this would be backward-incompatible in a way anyone would care about since the current behavior just throws an exception, and this way, we could support *reading* a pandas Categorical into a `Dataset` as a ClassLabel.  I *think* from there, while it would require some custom glue it wouldn't be too hard to convert the ClassLabel into a pandas Category if we want to go back - I think this would improve on the current behavior without risking changing the behavior of ClassLabel in a backward-incompat way.

Thoughts?  I'm not sure if this is overly cautious.  Whichever approach you think is better, I'd be happy to take it on!
Issue: Feature Request: Support for Pandas `Categorical`

Comment:
I think we can first keep the int64 precision but with an arrow Dictionary for ClassLabel, and focus on the connection with arrow and pandas.

In this scope, I really like the idea of checking for the dictionary type:

> Another idea that just occurred to me: add a branch in here to generate a ClassLabel if the dict key is int64 and the values are string: https://github.com/huggingface/datasets/blob/master/src/datasets/features.py#L932 , and then don't touch anything else.

This looks like a great start.

Then as you said we'd have to add the conversion from classlabel to the correct arrow dictionary type. Arrow is already able to convert from arrow Dictionary to pandas Categorical so it should be enough.

I can see two things that we must take case of to make this change backward compatible:
- first we must still be able to load an arrow file with arrow int64 dtype and `datasets` ClassLabel type without crashing. This can be fixed by casting the arrow int64 array to an arrow Dictionary array on-the-fly when loading the table in the ArrowReader.
- then we still have to return integers when accessing examples from a ClassLabel column. Currently it would return the strings values since it's based on the pandas behavior for converting from pandas to python/numpy. To do so we just have to adapt the python/numpy extractors in formatting.py (it takes care of converting an arrow table to a dictionary of python objects by doing arrow table -> pandas dataframe -> python dictionary)

Any help on this matter is very much welcome :)
Issue: ALT dataset has repeating instances in all splits

Comment:
I just merged a fix, we'll do a patch release soon. Thanks again for reporting, and sorry for the inconvenience.
In the meantime you can load `ALT` using `datasets` from the master branch
Issue: Bug Report: timestamp[ns] not recognized

Comment:
Thanks for reporting !

You're right, `string_to_arrow` should be able to take `"timestamp[ns]"` as input and return the right pyarrow timestamp type.
Feel free to suggest a fix for `string_to_arrow` and open a PR if you want to contribute ! This would be very appreciated :)

To give you more context:

As you may know we define the features types of a dataset using the `Features` object in combination with feature types like `Value`. For example
```python
features = Features({
    "age": Value("int32")
})
```
However under the hood we are actually using pyarrow to store the data, and so we have a mapping between the feature types of `datasets` and the types of pyarrow.

For example, the `Value` feature types are created from a pyarrow type with `Value(str(pa_type))`.
However it looks like the conversion back to a pyarrow type doesn't work with `"timestamp[ns]"`.
This is the `string_to_arrow` function you highlighted that does this conversion, so we should fix that.