t20062443/AT1ds huggingface dataset:
M-Salti (CONTRIBUTOR) commented on 2021-02-21 05:47:23:
So I was curious why the issue reported at #1919 wasn't caught in [this test](https://github.com/huggingface/datasets/blob/248104c4bdb2e01c036b7578867199191fbff181/tests/test_arrow_dataset.py#L209), so I did some digging.
I tried to save to a temporary directory (just like in the test), like this:
```python
with tempfile.TemporaryDirectory() as requested_tempdir:
    squad.save_to_disk(requested_tempdir)     # no error
```
and it executes succesfuly without problems.
So why does it work, but this doesn't?
```python
squad.save_to_disk("./squad")   # error
```
It's because `save_to_disk` also creates a temporary directory (let's call it `tempdir`), and since `tempdir` and `requested_tempdir` share the same parents, the `Path.joinpath` method [(here)](https://github.com/huggingface/datasets/blob/248104c4bdb2e01c036b7578867199191fbff181/src/datasets/arrow_dataset.py#L469) will keep `requested_tempdir` as it is and the *train* directory will be created under `requested_tempdir` and hence no errors will arise.

But in the second case (where we are saving to a local dir), the *train* directory is created under *squad* which in turn is created under `tempdir`, not under `.` (current dir).

So, all of this probably doesn't help solving the issue but it might help creating a better test, and it also makes me wonder why are we saving to a temporary dir in `save_to_disk` anyway? I mean, won't it be removed with all its contents upon execution completion? what's the point then?
lhoestq (MEMBER) commented on 2021-02-21 20:24:13:
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.
smile0925 (NONE) commented on 2021-02-22 07:59:43:
Hi  @ZhengxiangShi 
You can first try whether your network can access these files. I need to use VPN to access these files, so I download the files that cannot be accessed to the local in advance, and then use them in the code. Like this,
`train_data = datasets.load_dataset("xsum.py", split="train")`
lhoestq (MEMBER) commented on 2021-02-22 10:35:59:
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 !
lhoestq (MEMBER) commented on 2021-02-22 16:05:46:
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 ?
patrickvonplaten (CONTRIBUTOR) commented on 2021-02-22 16:56:39:
Hey @BirgerMoell,

I tweaked your dataset file a bit to have a first working version. To test this dataset downloading script, you can do the following:

- 1) Download the Common Voice Georgian dataset from https://commonvoice.mozilla.org/en/datasets (It's pretty small which is why I chose it)
- 2) Run the following command using this branch: 

```python
from datasets import load_dataset

ds = load_dataset("./../datasets/datasets/common_voice", "Georgian", data_dir="./cv-corpus-6.1-2020-12-11/ka/", split="train")
```

Note that I'm loading a local version of the dataset script (`"./../datasets/datasets/common_voice/"` points to the folder in your branch) and that I also insert the downloaded data with the `data_dir` arg.

-> You'll see that the data is correctly loaded and that `ds` contains all the information we need.

Now there are a lot of different datasets on Common Voice, so it probably takes too much time to test all of those, but maybe you can test whether the current script works as well *e.g.* for Swedish, 3,4 other languages.

It would be very nice if we can use the exact same structure for all languages, meaning that we don't have to change the `datasets.Features(...)` structure depending on the language, but can use the exact same one for every language.

If everything works as expected we can then go over to cleaning the script and seeing how to add dummy data tests for it.
justin-yan (CONTRIBUTOR) commented on 2021-02-22 17:12:26:
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!
lhoestq (MEMBER) commented on 2021-02-22 19:33:09:
I just tested and using the Arrow File format doesn't improve the speed... This will need further investigation.

My guess is that it has to iterate over the record batches or chunks of a ChunkedArray in order to retrieve elements.

However if we know in advance in which chunk the element is, and at what index it is, then we can access it instantaneously. But this requires dealing with the chunked arrays instead of the pyarrow Table directly which is not practical.
albertvillanova (MEMBER) commented on 2021-02-23 08:16:07:
@lhoestq I have refactorized the logic. Instead of the previous hierarchy call (local temp file opening -> remote call -> use again temp local file logic but from within the remote caller scope), now it is flattened. Schematically:
```python
with src.open() as src_file, dst.open() as dst_file:
    src_file.fetch(dst_file)
```

I have created `RemotePath` (analogue to Path) with method `.open()` that returns `FtpFile`/`HttpFile` (analogue to file-like).

Now I am going to implement `RemotePath.exists()` method (analogue to the Path's method) to check if remote resource is accessible, using `Ftp/Http.head()`.
pdufter (CONTRIBUTOR) commented on 2021-02-23 08:48:14:
Hi, it seems I am somewhat stuck here. The failed test `ci/circleci: run_dataset_script_tests_pyarrow_1_WIN` seems to be caused by some broken connection (`ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host`). Any help on this is appreciated. 

Edit: Seems to be resolved now.
lhoestq (MEMBER) commented on 2021-02-23 14:37:05:
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 :)
patrickvonplaten (CONTRIBUTOR) commented on 2021-02-24 08:44:04:
@skyprince999, I think I'm getting the same error you're getting :-/

```
Sorry, you can't view or download this file at this time.

Too many users have viewed or downloaded this file recently. Please try accessing the file again later. If the file you are trying to access is particularly large or is shared with many people, it may take up to 24 hours to be able to view or download the file. If you still can't access a file after 24 hours, contact your domain administrator.
```

It would be great if you could write the authors to see whether they can fix it.
Also cc @lhoestq - do you think we could mirror the dataset?
patrickvonplaten (CONTRIBUTOR) commented on 2021-02-24 08:47:50:
Also there are huge those datasets. Think downloading MuST-C v1.2 amounts to ~ 1000GB... because there are 14 possible configs each around 60-70GB. I think users mostly will only use one of the 14 configs so that they would only need, in theory, will have to download ~60GB which is ok. But I think this functionality doesn't exist yet in `datasets` no? cc @lhoestq
avacaondata (NONE) commented on 2021-02-24 12:00:43:
In the end, to make it work I turned to A-100 gpus instead of TPUS, among other changes. Set_transform doesn't work as expected and slows down training very much even in GPUs, and applying map destroys the disk, as it multiplies by 100 the size of the data passed to it (due to inefficient implementation converting strings to int64 floats I guess). For that reason, I chose to use datasets to load the data as text, and then edit the Collator from Transformers to tokenize every batch it receives before processing it. That way, I'm being able to train fast, without memory breaks, without the disk being unnecessarily filled, while making use of GPUs almost all the time I'm paying for them (the map function over the whole dataset took ~15hrs, in which you're not training at all). I hope this info helps others that are looking for training a language model from scratch cheaply, I'm going to close the issue as the optimal solution I found after many experiments to the problem posted in it is explained above.
lhoestq (MEMBER) commented on 2021-02-24 15:26:28:
Thanks ! could you maybe add a few tests in test_arrow_dataset.py to make sure from_pandas works as expected with categorical types ?

In particular I'm pretty sure that if you now try to `cast` the dataset to the same features at its current features, it will break instead of just being a no-op.
This is because `features.type` returns an arrow int64 type for the classlabel column instead of the arrow dictionary type that you have in the arrow table. There are two issues in this case:
- it will try to replace the arrow type from dictionary to int64 instead of being a no-op
- it will crash because pyarrow is not able to cast a dictionary to int64 (even if it's actually possible do cast the column by hand by accessing the sub-array of the dictionary array containing the indices/integers)

It would be awesome to fix this case ! Ideally the arrow `pa_type` of classlabel ([here](https://github.com/huggingface/datasets/blob/7072e1becd69d421d863374b825e3da4c6551798/src/datasets/features.py#L558)) should be an arrow dictionary type. This should fix the issue. Then we can start working on backward compatibility.

Let me know if you have questions or if I can help.
In particular if there is some glue-ing to do I can take care of that if you want ;)

--------------

Also just a few information regarding the functions you mentioned

`int2str` and  `str2int` are used by users to transforms the labels if they want to. Here sine ClassLabel is instantiated without the class names, they would crash. I was about to make a PR to disallow the creation of an empty ClassLabel feature type.
Therefore can you provide class_names= when creating the ClassLabel ?

`encode_example` is mostly used with a dataset builder (e.g. squad.py) so it's not used when using .from_pandas.
pdufter (CONTRIBUTOR) commented on 2021-02-24 15:27:55:
I guess the `dummy_data.zip` is too large. I can reduce the languages that are contained there, but when testing it, it obviously throws an error, as not all files can be found. I guess I can either i) change the default value regarding which languages are loaded or ii) let the `_generate_examples` silently skip any language for which it cannot find files. Both solutions are not really pretty - is there another way around this?
lhoestq (MEMBER) commented on 2021-02-24 16:55:34:
> Also cc @lhoestq - do you think we could mirror the dataset?

Yes we can mirror it if the authors are fine with it. You can create a dataset repo on huggingface.co (possibly under the relevant org) and add the mirrored data files.

> I think users mostly will only use one of the 14 configs so that they would only need, in theory, will have to download ~60GB which is ok. But I think this functionality doesn't exist yet in datasets no? cc @lhoestq

If there are different download links for each configuration we can make the dataset builder download only the files related to the requested configuration.
lhoestq (MEMBER) commented on 2021-02-24 17:58:52:
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) ?
stas00 (CONTRIBUTOR) commented on 2021-02-24 18:10:26:
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.
lhoestq (MEMBER) commented on 2021-02-24 18:26:34:
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`.
stas00 (CONTRIBUTOR) commented on 2021-02-24 18:37:35:
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.
skyprince999 (CONTRIBUTOR) commented on 2021-02-24 18:39:44:
I have written to the dataset authors, highlighting this issue. Waiting for their response. 

Update on 25th Feb: 
The authors have replied back, they are updating the download link and will revert back shortly! 

```
first of all thanks a lot for being interested in MuST-C and for building the data-loader.

Before answering your request, I'd like to clarify that the creation, maintenance, and expansion of MuST-c are not supported by any funded project, so this means that we need to find economic support for all these activities. This also includes permanently moving all the data to AWS or GCP.  We are working at this with the goal of facilitating the use of MuST-C, but this is not something that can happen today. We hope to have some news ASAP and you will be among the first to be informed.

I hope you understand our situation.
```
johncookds (NONE) commented on 2021-02-24 21:55:08:
Hi, 
I'm following up here as I found my exact issue. It was with saving and re-loading the tokenizer. When I trained then processed the data without saving and reloading it, it was 10x-100x faster than when I saved and re-loaded it.
Both resulted in the exact same tokenized datasets as well. 
There is additionally a bug where the older legacy tokenizer save does not preserve a learned tokenizing behavior if trained from scratch.
Understand its not exactly Datasets related but hope it can help someone if they have the same issue.
Thanks!
lhoestq (MEMBER) commented on 2021-02-25 11:22:19:
Yes that makes sense. I completely forgot that the label names of an arrow Dictionary type were not stored in the type but in the DictionaryArray.

This is made me realize that it's actually pretty unpractical and I feel that handling this can add unnecessary complexity in the handling of dtypes.
More specifically:
- it's not possible to create a DictionaryArray from a call to pyarrow.array with python objects, which is the function we use to convert python objects to pyarrow objects (or we would need to convert the python objects to pandas categorical series beforehand but it doesn't work for nested types)
- casting nested types containing Dictionary types would require a lot of array manipulations since it's not compatible with pyarrow.array.cast

I feel like the original feature request (support of pandas Categorical) should be addressable without adding so much complexity to the library.

If we admit that we don't want to deal with arrow Dictionary type, maybe we can simply convert the pandas categorical series to an int64 series and set the feature type to the right ClassLabel in `from_pandas`. We can have the reverse operation in `to_pandas`. This way we don't need to support the arrow DictionaryType and so we can keep simple/accessible code for conversion from python to arrow and also for type casting. Let me know what you think.

In the future depending on the usage of the ClassLabel types with pandas/pyarrow we might reconsider this but for now I believe this simple solution is enough.
lhoestq (MEMBER) commented on 2021-02-25 13:35:24:
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 :)
lhoestq (MEMBER) commented on 2021-02-25 13:43:12:
Hi !

The cache at `~/.cache/huggingface/metrics` stores the users data for metrics computations (hence the arrow files).

However python modules (i.e. dataset scripts, metric scripts) are stored in `~/.cache/huggingface/modules/datasets_modules`.

In particular the metrics are cached in `~/.cache/huggingface/modules/datasets_modules/metrics/`

Feel free to take a look at your cache and let me know if you find any issue that would help explaining why you had an issue with `rouge` with no connection. I'm doing some tests on my side to try to reproduce the issue you have