t20062443/AT1ds huggingface dataset:
mahesh1amour (NONE) commented on 2020-11-23 11:36:37:
Thanks thomwolf and julien-c

I'm still confusion on what you guys said, 

I have solved the problem as follows:

1. read the csv file using pandas from s3 
2. Convert to dictionary key as column name and values as list column data
3. convert it to Dataset using 
`from datasets import Dataset`
`train_dataset = Dataset.from_dict(train_dict)`
BramVanroy (CONTRIBUTOR) commented on 2020-11-24 08:09:04:
@thomwolf @AndreasMadsen I think this is a terrible idea, n/o, and I am very much against it. Modifying internals of an array in such a hacky way is bound to run into other (user) issues down the line. To users it would not be clear at all what is going on e.g. when they check for write access (which will return True) but then they get a warning that the array is not writeable. That's extremely confusing. 

If your only goal is to get rid of warnings in your code, then you can just use a [simplefilter](https://docs.python.org/3.8/library/warnings.html#temporarily-suppressing-warnings) for UserWarnings in your own code. Changing the code-base in such an intuitive way does not seem like a good way to go and sets a bad precedent, imo. 

(Feel free to disagree, of course.)

IMO a warning can stay (as they can be filtered by users anyway), but it can be clarified why the warning takes place.
lhoestq (MEMBER) commented on 2020-11-24 10:08:35:
It looks like there was an issue while building the imdb dataset.
Could you provide more information about your OS and the version of python and `datasets` ?

Also could you try again with 
```python
dataset = datasets.load_dataset("imdb", split="train", download_mode="force_redownload")
```
to make sure it's not a corrupted file issue ?
AndreasMadsen (NONE) commented on 2020-11-24 11:09:19:
> To users it would not be clear at all what is going on e.g. when they check for write access (which will return True) but then they get a warning that the array is not writeable. That's extremely confusing.

Confusion can be resolved with a helpful error message. In this case, that error message can be controlled by huggingface/datasets. The right argument here is that if code depends on `.flags.writable` being truthful (not just for warnings), then it will cause unavoidable errors. Although, I can't imagine such a use-case.

> If your only goal is to get rid of warnings in your code, then you can just use a simplefilter for UserWarnings in your own code. Changing the code-base in such an intuitive way does not seem like a good way to go and sets a bad precedent, imo.

I don't want to ignore all `UserWarnings`, nor all warnings regarding non-writable arrays. Ignoring warnings leads to hard to debug issues.

> IMO a warning can stay (as they can be filtered by users anyway), but it can be clarified why the warning takes place.

Plain use cases should really not generate warnings. It teaches developers to ignore warnings which is a terrible practice.

---

The best solution would be to allow non-writable arrays in `DataLoader`, but that is a PyTorch issue.
rabeehk (CONTRIBUTOR) commented on 2020-11-24 12:01:25:
hey
i do the exact same commands. for me it fails i guess might be issues with
caching maybe?
thanks
best
rabeeh

On Tue, Nov 24, 2020, 10:24 AM Quentin Lhoest <notifications@github.com>
wrote:

> Hi ! It runs on my side without issues. I tried
>
> from datasets import load_datasetload_dataset("boolq")
>
> What version of datasets and tensorflow are your runnning ?
> Also if you manage to get a minimal reproducible script (on google colab
> for example) that would be useful.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/huggingface/datasets/issues/879#issuecomment-732769114>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABP4ZCGGDR2FUMRKZTIY5CTSRN3VXANCNFSM4T7R3U6A>
> .
>
NielsRogge (CONTRIBUTOR) commented on 2020-11-24 12:20:39:
@thomwolf here's a slightly adapted version of the code from the [official Tapas repository](https://github.com/google-research/tapas/blob/master/tapas/utils/interaction_utils.py) that is used to turn the `answer_coordinates` and `answer_texts` columns into true Python lists of tuples/strings:

```
import pandas as pd
import ast

data = pd.read_csv("/content/sqa_data/random-split-1-dev.tsv", sep='\t')

def _parse_answer_coordinates(answer_coordinate_str):
  """Parses the answer_coordinates of a question.
  Args:
    answer_coordinate_str: A string representation of a Python list of tuple
      strings.
      For example: "['(1, 4)','(1, 3)', ...]"
  """

  try:
    answer_coordinates = []
    # make a list of strings
    coords = ast.literal_eval(answer_coordinate_str)
    # parse each string as a tuple
    for row_index, column_index in sorted(
        ast.literal_eval(coord) for coord in coords):
      answer_coordinates.append((row_index, column_index))
  except SyntaxError:
    raise ValueError('Unable to evaluate %s' % answer_coordinate_str)
  
  return answer_coordinates


def _parse_answer_text(answer_text):
  """Populates the answer_texts field of `answer` by parsing `answer_text`.
  Args:
    answer_text: A string representation of a Python list of strings.
      For example: "[u'test', u'hello', ...]"
  """
  try:
    answer = []
    for value in ast.literal_eval(answer_text):
      answer.append(value)
  except SyntaxError:
    raise ValueError('Unable to evaluate %s' % answer_text)

  return answer

data['answer_coordinates'] = data['answer_coordinates'].apply(lambda coords_str: _parse_answer_coordinates(coords_str))
data['answer_text'] = data['answer_text'].apply(lambda txt: _parse_answer_text(txt))
```

Here I'm using Pandas to read in one of the TSV files (the dev set).
BramVanroy (CONTRIBUTOR) commented on 2020-11-24 12:32:05:
> The right argument here is that if code depends on `.flags.writable` being truthful (not just for warnings), then it will cause unavoidable errors. Although, I can't imagine such a use-case.

That's exactly the argument in my first sentence. Too often someone "cannot think of a use-case", but you can not foresee the use-cases of a whole research community.
 
> I don't want to ignore all `UserWarnings`, nor all warnings regarding non-writable arrays. Ignoring warnings leads to hard to debug issues.

That's fair.

> Plain use cases should really not generate warnings. It teaches developers to ignore warnings which is a terrible practice.

But this is not a plain use-case (because Pytorch does not support these read-only tensors). Manually setting the flag to writable will solve the issue on the surface but is basically just a hack to compensate for something that is not allowed in another library. 

What about an "ignore_warnings" flag in `set_format` that when True wraps the offending code in a block to ignore userwarnings at that specific step in [_convert_outputs](https://github.com/huggingface/datasets/blob/880c2c76a8223a00c303eab2909371e857113063/src/datasets/arrow_dataset.py#L821)? Something like:

```python
def _convert_outputs(..., ignore_warnings=True):
    ...
    with warnings.catch_warnings():
        if ignore_warnings:
            warnings.simplefilter("ignore", UserWarning)
        return torch.tensor(...)
# continues without warning filter after context manager...
```
lhoestq (MEMBER) commented on 2020-11-25 13:32:52:
Could you check if it works on the master branch ?
You can use `load_dataset("boolq", script_version="master")` to do so.
We did some changes recently in boolq to remove the TF dependency and we changed the way the data files are downloaded in https://github.com/huggingface/datasets/pull/881
lhoestq (MEMBER) commented on 2020-11-25 15:24:40:
Yes right now `ArrayXD` can only be used as a column feature type, not a subtype.
With the current Arrow limitations I don't think we'll be able to make it work as a subtype, however it should be possible to allow dimensions of dynamic sizes (`Array3D(shape=(None, 137, 2), dtype="float32")` for example since the [underlying arrow  type](https://github.com/huggingface/datasets/blob/master/src/datasets/features.py#L236) allows dynamic sizes.

For now I'd suggest the use of nested `Sequence` types. Once we have the dynamic sizes you can update the dataset.
What do you think ?
lhoestq (MEMBER) commented on 2020-11-25 17:57:37:
I just tested the tool with some datasets and found out that it's not working for datasets that download files using `download_and_extract(file_url)` (where file_url is a `str`). That's because in that case the dummy_data.zip is not a folder but a single zipped file.

I think we have to fix that or we can have unexpected behavior when a scripts calls `download_and_extract(file_url)` several times, since it would always point to the same dummy data file.

So I decided to change that to have a folder containing the dummy files instead but it breaks around 90 tests so I need to update 90 dummy data files to follow this scheme. I'll probably fix them tomorrow morning.

What do you guys think ? Also cc @patrickvonplaten to make sure I understand things correctly
shexuan (NONE) commented on 2020-11-26 02:05:29:
> Hi ! Thanks for reporting.
> Do you have the same slowdown when you iterate through the raw dataset object as well ? (no dataloader)
> It would be nice to know whether it comes from the dataloader or not

I did not iter data from raw dataset, maybe I will test later. Now I iter all files directly from `open(file)`,  around 20000it/s.
lhoestq (MEMBER) commented on 2020-11-26 11:29:31:
Ok I changed to use the dummy_data.zip content to be a folder even for single url calls to `dl_manager.download_and_extract`. Therefore the automatic dummy data generation tool works for most datasets now.

To avoid having to change all the old dummy_data.zip files I added backward compatiblity. 

The only test failing is `tests/test_dataset_common.py::RemoteDatasetTest::test_load_dataset_xcopa`
It is expected to fail since I had modify its dummy data structure that was wrong. It was causing issue with backward compatibility. It will be fixed as soon as this PR is merged
patrickvonplaten (CONTRIBUTOR) commented on 2020-11-27 09:43:31:
Just to make sure this is what we want here. If we add one config per language, 

this means that this dataset ends up with well over 100 different configs most of which will have the same `train` split. The train split is always in English. Also, I'm not sure whether it's better for the user to be honest. 

I think it could be quite confusing for the user to have

```python
train_dataset = load_dataset("xglue", "ner-de", split="train")
```

in English even though it's `ner-de`.

To be honest, I'd prefer:

```python
train_dataset = load_dataset("xglue", "ner", split="train")
test_dataset_de = load_dataset("xglue", "ner", split="test-de")
test_dataset_fr = load_dataset("xglue", "ner", split="test-fr")
```

here
lhoestq (MEMBER) commented on 2020-11-27 10:38:29:
Oh yes right I didn't notice the train set was always in english sorry.
Moreover it seems that the way this dataset is used is to pick a pretrained multilingual model, fine-tune it on the english train set and then evaluate on each test set (one per language).
So to better fit the usual usage of this dataset, I agree that it's better to have one test split per language. 

Something like your latest example patrick is fine imo :
```python
train_dataset = load_dataset("xglue", "ner", split="train")
test_dataset_de = load_dataset("xglue", "ner", split="test.de")
```

I just replace test-de with test.de since `-` is not allowed for split names (it has to follow the `\w+` regex), and usually we specify the language after a point.
JoelNiklaus (CONTRIBUTOR) commented on 2020-11-27 12:44:01:
Thanks for the comments. I addressed them and pushed again.
When I run "make quality" I get the following error but I don't know how to resolve it or what the problem ist respectively:
would reformat /Users/joelniklaus/NextCloud/PhDJoelNiklaus/Code/datasets/datasets/ler/ler.py
Oh no! 💥 💔 💥
1 file would be reformatted, 257 files would be left unchanged.
make: *** [quality] Error 1
JoelNiklaus (CONTRIBUTOR) commented on 2020-11-27 14:19:47:
I did that now. But still getting the following error:
black --check --line-length 119 --target-version py36 tests src benchmarks datasets metrics
All done! ✨ 🍰 ✨
258 files would be left unchanged.
isort --check-only tests src benchmarks datasets metrics
flake8 tests src benchmarks datasets metrics
datasets/ler/ler.py:46:96: W291 trailing whitespace
datasets/ler/ler.py:47:68: W291 trailing whitespace
datasets/ler/ler.py:48:102: W291 trailing whitespace
datasets/ler/ler.py:49:112: W291 trailing whitespace
datasets/ler/ler.py:50:92: W291 trailing whitespace
datasets/ler/ler.py:51:116: W291 trailing whitespace
datasets/ler/ler.py:52:84: W291 trailing whitespace
make: *** [quality] Error 1

However: When I look at the file I don't see any trailing whitespace
lhoestq (MEMBER) commented on 2020-11-27 17:43:13:
Alright create the test in #905 .
The windows CI is failing for all the datasets that have bad usage of `os.path.join` for urls.
There are of course the ones you fixed in this PR (thanks again !) but I found others as well such as pg19 and blimp.
You can check the full list by looking at the CI failures of the commit 1ce3354
JoelNiklaus (CONTRIBUTOR) commented on 2020-11-28 00:14:42:
Now I updated to: 3.8.4 (mccabe: 0.6.1, pycodestyle: 2.6.0, pyflakes: 2.2.0) CPython 3.8.5 on Darwin

And now I even get additional errors:
black --check --line-length 119 --target-version py36 tests src benchmarks datasets metrics
All done! ✨ 🍰 ✨
258 files would be left unchanged.
isort --check-only tests src benchmarks datasets metrics
flake8 tests src benchmarks datasets metrics
datasets/polyglot_ner/polyglot_ner.py:123:64: F541 f-string is missing placeholders
datasets/ler/ler.py:46:96: W291 trailing whitespace
datasets/ler/ler.py:47:68: W291 trailing whitespace
datasets/ler/ler.py:48:102: W291 trailing whitespace
datasets/ler/ler.py:49:112: W291 trailing whitespace
datasets/ler/ler.py:50:92: W291 trailing whitespace
datasets/ler/ler.py:51:116: W291 trailing whitespace
datasets/ler/ler.py:52:84: W291 trailing whitespace
datasets/math_dataset/math_dataset.py:233:25: E741 ambiguous variable name 'l'
metrics/coval/coval.py:236:31: F541 f-string is missing placeholders
make: *** [quality] Error 1

I do this on macOS Catalina 10.15.7 in case this matters
joeddav (CONTRIBUTOR) commented on 2020-11-28 19:38:39:
@lhoestq Okay great, I added a test as well as two new inspect functions: `get_dataset_config_names` and `get_dataset_infos` (the latter is something I've been wanting anyway). As a quick hack, you can also just pass a random config name (e.g. an empty string) to `load_dataset` to get the config names in the error msg as before. Also added a couple paragraphs to the adding new datasets doc.

I'll send a separate PR incorporating this in existing datasets so we can get this merged before our sprint on Monday.

Any ideas on the failing tests? I'm having trouble making sense of it. **Edit**: nvm, it was master.
reshinthadithyan (CONTRIBUTOR) commented on 2020-11-29 18:04:15:
Hello, thanks. I had a talk with the dataset authors, found out that the data now is obsolete and they'll get a stable version soon. So temporality closing the PR.
 Although I have a question, What should _id_ be in the return statement? Should that be something like a start index (or) the type of split will do? Thanks.
TathagataChakraborti (NONE) commented on 2020-11-29 18:08:54:
@reshinthadithyan we should hold off on this for a couple of weeks till NeurIPS concludes. The [NLC2CMD](http://nlc2cmd.us-east.mybluemix.net/) data will be out then; which includes a cleaner version of this NL2Bash data. The older data is sort of obsolete now.
lhoestq (MEMBER) commented on 2020-11-29 22:43:37:
Ok found why it doesn't detect the problems fixed by #907 . That's because for all those datasets the urls are actually fine (no backslash) on windows, even if it uses `os.path.join`.

This is because of the behavior of `os.path.join` on windows when the first path ends with a slash : 

```python
import os
os.path.join("https://test.com/foo", "bar.txt")
# 'https://test.com/foo\\bar.txt'
os.path.join("https://test.com/foo/", "bar.txt")
# 'https://test.com/foo/bar.txt'
```

However even though the urls are correct, this is definitely bad practice and we should never use `os.path.join` for urls
lhoestq (MEMBER) commented on 2020-11-29 23:09:28:
Thanks for adding this one :) 

To fix the check_code_quality, please run `make style` with the latest version of black, isort, flake8
To fix the test_no_encoding_on_file_open, make sure to specify the encoding each time you call `open()` on a text file.
For example : `encoding="utf-8"`
To fix the test_load_dataset_pec , you must add the dummy_data.zip file. It is used to test the dataset script and make sure it runs fine. To add it, please refer to the steps in https://github.com/huggingface/datasets/blob/master/CONTRIBUTING.md#how-to-add-a-dataset
zhongpeixiang (CONTRIBUTOR) commented on 2020-11-30 04:36:01:
> Thanks for adding this one :)
> 
> To fix the check_code_quality, please run `make style` with the latest version of black, isort, flake8
> To fix the test_no_encoding_on_file_open, make sure to specify the encoding each time you call `open()` on a text file.
> For example : `encoding="utf-8"`
> To fix the test_load_dataset_pec , you must add the dummy_data.zip file. It is used to test the dataset script and make sure it runs fine. To add it, please refer to the steps in https://github.com/huggingface/datasets/blob/master/CONTRIBUTING.md#how-to-add-a-dataset

Thank you for the detailed suggestion.

I have added dummy_data but it still failed the DistributedDatasetTest check. My dataset has a central file (containing a python dict) that needs to be accessed by each data example. Is it because the central file cannot be distributed (which would lead to a partial dictionary)?

Specifically, the central file contains a dictionary of speakers with their attributes. Each data example is also associated with a speaker. As of now, I keep the central file and data files separately. If I remove the central file by appending the speaker attributes to each data example, then there would be lots of redundancy because there are lots of duplicate speakers in the data files.
reshinthadithyan (CONTRIBUTOR) commented on 2020-11-30 11:52:55:
Testing command doesn't work
###trace
-- Docs: https://docs.pytest.org/en/stable/warnings.html
========================================================= short test summary info ========================================================== 
ERROR tests/test_dataset_common.py - absl.testing.parameterized.NoTestsError: parameterized test decorators did not generate any tests. Ma...
====================================================== 2 warnings, 1 error in 54.23s ======================================================= 
ERROR: not found: G:\Work Related\hf\datasets\tests\test_dataset_common.py::LocalDatasetTest::test_load_real_dataset_concode
(no name 'G:\\Work Related\\hf\\datasets\\tests\\test_dataset_common.py::LocalDatasetTest::test_load_real_dataset_concode' in any of [<Module test_dataset_common.py>])
lhoestq (MEMBER) commented on 2020-11-30 13:27:07:
Code quality test now passes, thanks :) 

To fix the other tests failing I think you can just rebase from master.
Also make sure that the dummy data test passes with
```python
RUN_SLOW=1 pytest tests/test_dataset_common.py::LocalDatasetTest::test_load_dataset_all_configs_ler
```
lhoestq (MEMBER) commented on 2020-11-30 13:36:57:
Working with Arrow can be quite fun sometimes.
You can fix this issue by trying to reduce the writer batch size (same trick than the one used to reduce the RAM usage in https://github.com/huggingface/datasets/issues/741).

Let me know if it works.
I haven't investigated yet on https://github.com/huggingface/datasets/issues/741 since I was preparing this week's sprint to add datasets but this is in my priority list for early next week.
reshinthadithyan (CONTRIBUTOR) commented on 2020-11-30 13:45:35:
Hello @lhoestq Test checks are passing in my local, but the commit fails in ci. Any idea onto why?   
#### Dummy Dataset Test 
====================================================== 1 passed, 6 warnings in 7.14s ======================================================= 
#### Real Dataset Test 
====================================================== 1 passed, 6 warnings in 25.54s ======================================================
yjernite (MEMBER) commented on 2020-11-30 14:35:01:
> Awesome good job !
> 
> Could you also add a dataset card using the template guide here : https://github.com/huggingface/datasets/blob/master/templates/README_guide.md
> If you can't fill some fields then just leave `[N/A]`

Quick amendment: `[N/A]` is for fields that are not relevant: if you can't find the information just leave `[More Information Needed]`
yjernite (MEMBER) commented on 2020-11-30 15:30:34:
Hi @AmitMY, sorry for leaving you hanging for a minute :) 

We've developed a new pipeline for adding datasets with a few extra steps, including adding a dataset card. You can find the full process [here](https://github.com/huggingface/datasets/blob/master/ADD_NEW_DATASET.md)

Would you be up for adding the tags and description in the README.md so we can merge this cool dataset?
yjernite (MEMBER) commented on 2020-11-30 15:37:31:
Hi @zaidalyafeai, really excited to get more Arabic coverage in the lib, thanks for your contribution!

Couple of last comments:
- this PR seems to modify some files that are unrelated to your dataset. Could you rebase from master? It should take care of that.
- The dataset card is a good start! Can you describe the task in a few words and add more information in the Data Structure part, including listing and describing the fields? Also, if you don't know how to fill out a paragraph, or if you have some information but think more would be beneficial, please leave `[More Information Needed]` instead of `[N/A]`
yjernite (MEMBER) commented on 2020-11-30 15:39:50:
> That's really cool thank you !
> 
> Could you also add a dataset card ?
> You can find a template here : https://github.com/huggingface/datasets/blob/master/templates/README.md

The full information for adding a dataset card can be found here :) 
https://github.com/huggingface/datasets/blob/master/ADD_NEW_DATASET.md#manually-tag-the-dataset-and-write-the-dataset-card
yjernite (MEMBER) commented on 2020-11-30 15:42:01:
> Really cool thanks !
> 
> Could you make the dummy files smaller ? For example by reducing the size of persona.txt ?
> I also left a comment about the files concatenation. It would be cool to replace that with simple iterations through the different files.
> 
> Then once this is done, you can add a dataset card using the template guide here : https://github.com/huggingface/datasets/blob/master/templates/README_guide.md
> If some fields can't be filled, just leave `[N/A]`

Small change: if you don't have the information for a field, please leave `[More Information Needed]` rather than `[N/A]`

The full information can be found [here](https://github.com/huggingface/datasets/blob/master/ADD_NEW_DATASET.md#manually-tag-the-dataset-and-write-the-dataset-card)
yjernite (MEMBER) commented on 2020-11-30 15:53:28:
Hi @sumanthd17 

Looks like a good start! You will also need to add a Dataset card, following the instructions given [here](https://github.com/huggingface/datasets/blob/master/ADD_NEW_DATASET.md#manually-tag-the-dataset-and-write-the-dataset-card)
yjernite (MEMBER) commented on 2020-11-30 17:26:19:
> I followed the instructions mentioned there but my dataset isn't showing up in the dropdown list. Am I missing something here? @yjernite

The best way is to run the tagging app locally and provide it the location to the `dataset_infos.json` after you've run the CLI:
https://github.com/huggingface/datasets-tagging