Repository: huggingface/datasets
Pull request: Print error message with filename when malformed CSV
Print error message specifying filename when malformed CSV file.

Close #1821

Problem statement:
Provide better exception message when one of many files results in an exception
I find when I process many files, i.e.

```
train_files = glob.glob('rain*.csv')
validation_files = glob.glob(validation*.csv')
datasets = load_dataset("csv", data_files=dict(train=train_files, validation=validation_files))
```

I sometimes encounter an error due to one of the files being misformed (i.e. no data, or a comma in a field that isn't quoted, etc).

For example, this is the tail of an exception which I suspect is due to a stray comma.

>   File "pandas/_libs/parsers.pyx", line 756, in pandas._libs.parsers.TextReader.read
>   File "pandas/_libs/parsers.pyx", line 783, in pandas._libs.parsers.TextReader._read_low_memory
>   File "pandas/_libs/parsers.pyx", line 827, in pandas._libs.parsers.TextReader._read_rows
>   File "pandas/_libs/parsers.pyx", line 814, in pandas._libs.parsers.TextReader._tokenize_rows
>   File "pandas/_libs/parsers.pyx", line 1951, in pandas._libs.parsers.raise_parser_error
> pandas.errors.ParserError: Error tokenizing data. C error: Expected 2 fields in line 559, saw 3

It would be nice if the exception trace contained the name of the file being processed (I have 250 separate files!)

Hints from issue discussion:
Hi!

Thank you for reporting this issue. I agree that the information about the exception should be more clear and explicit.

I could take on this issue.

On the meantime, as you can see from the exception stack trace, HF Datasets uses pandas to read the CSV files. You can pass arguments to `pandas.read_csv` by passing additional keyword arguments to `load_dataset`. For example, you may find useful this argument:
- `error_bad_lines` : bool, default True
  Lines with too many fields (e.g. a csv line with too many commas) will by default cause an exception to be raised, and no DataFrame will be returned. If False, then these “bad lines” will be dropped from the DataFrame that is returned.

You could try:
```python
datasets = load_dataset("csv", data_files=dict(train=train_files, validation=validation_files), error_bad_lines=False)
```

Commit message: Print error message with filename when malformed CSV

Patch to review:
diff --git a/src/datasets/packaged_modules/csv/csv.py b/src/datasets/packaged_modules/csv/csv.py
index f010f6dfecd..c298fc7672b 100644
--- a/src/datasets/packaged_modules/csv/csv.py
+++ b/src/datasets/packaged_modules/csv/csv.py
@@ -126,9 +126,15 @@ def _generate_tables(self, files):
                 float_precision=self.config.float_precision,
                 chunksize=self.config.chunksize,
             )
-            for batch_idx, df in enumerate(csv_file_reader):
-                pa_table = pa.Table.from_pandas(df, schema=schema)
-                # Uncomment for debugging (will print the Arrow table size and elements)
-                # logger.warning(f"pa_table: {pa_table} num rows: {pa_table.num_rows}")
-                # logger.warning('\n'.join(str(pa_table.slice(i, 1).to_pydict()) for i in range(pa_table.num_rows)))
-                yield (file_idx, batch_idx), pa_table
+
+            try:
+                for batch_idx, df in enumerate(csv_file_reader):
+                    pa_table = pa.Table.from_pandas(df, schema=schema)
+                    # Uncomment for debugging (will print the Arrow table size and elements)
+                    # logger.warning(f"pa_table: {pa_table} num rows: {pa_table.num_rows}")
+                    # logger.warning('\n'.join(str(pa_table.slice(i, 1).to_pydict()) for i in range(pa_table.num_rows)))
+                    yield (file_idx, batch_idx), pa_table
+            except ValueError as e:
+                # logger.error(f"Failed to read file '{csv_file_reader.f}' with error {type(e)}: {e}")
+                print(f"Failed to read file '{csv_file_reader.f}' with error {type(e)}: {e}")
+                raise
diff --git a/tests/test_packaged_modules.py b/tests/test_packaged_modules.py
new file mode 100644
index 00000000000..4d78ee5aba2
--- /dev/null
+++ b/tests/test_packaged_modules.py
@@ -0,0 +1,45 @@
+import textwrap
+
+import pytest
+
+from datasets.packaged_modules.csv.csv import Csv
+
+
+@pytest.fixture
+def csv_file(tmp_path):
+    filename = tmp_path / "malformed_file.csv"
+    data = textwrap.dedent(
+        """\
+        header1,header2
+        1,2
+        10,20
+        """
+    )
+    with open(filename, "w") as f:
+        f.write(data)
+    return str(filename)
+
+
+@pytest.fixture
+def malformed_csv_file(tmp_path):
+    filename = tmp_path / "malformed_file.csv"
+    data = textwrap.dedent(
+        """\
+        header1,header2
+        1,2
+        10,20,
+        """
+    )
+    with open(filename, "w") as f:
+        f.write(data)
+    return str(filename)
+
+
+def test_csv_generate_tables_raises_error_with_malformed_csv(csv_file, malformed_csv_file, capsys):
+    csv = Csv()
+    generator = csv._generate_tables([csv_file, malformed_csv_file])
+    with pytest.raises(ValueError, match="Error tokenizing data"):
+        for _ in generator:
+            pass
+    captured = capsys.readouterr()
+    assert f"Failed to read file '{malformed_csv_file}'" in captured.out


Code review comments:
diff_hunk: @@ -126,9 +126,15 @@ def _generate_tables(self, files):
                 float_precision=self.config.float_precision,
                 chunksize=self.config.chunksize,
             )
-            for batch_idx, df in enumerate(csv_file_reader):
-                pa_table = pa.Table.from_pandas(df, schema=schema)
-                # Uncomment for debugging (will print the Arrow table size and elements)
-                # logger.warning(f"pa_table: {pa_table} num rows: {pa_table.num_rows}")
-                # logger.warning('\n'.join(str(pa_table.slice(i, 1).to_pydict()) for i in range(pa_table.num_rows)))
-                yield (file_idx, batch_idx), pa_table
+
+            try:
+                for batch_idx, df in enumerate(csv_file_reader):
+                    pa_table = pa.Table.from_pandas(df, schema=schema)
+                    # Uncomment for debugging (will print the Arrow table size and elements)
+                    # logger.warning(f"pa_table: {pa_table} num rows: {pa_table.num_rows}")
+                    # logger.warning('\n'.join(str(pa_table.slice(i, 1).to_pydict()) for i in range(pa_table.num_rows)))
+                    yield (file_idx, batch_idx), pa_table
+            except ValueError as e:
+                # logger.error(f"Failed to read file '{csv_file_reader.f}' with error {type(e)}: {e}")
path: src/datasets/packaged_modules/csv/csv.py
text: @author:
I am wondering why HF sets logging propagation to False...

@user1:
What's the issue exactly here ? Why use print instead of the logging ?

@author:
Yes. I first logged, but nothing appeared. Then I discovered that logging propagation is disabled. And I would like to know why you decided that. :)

@user1:
We're doing the same thing on `transformers`, I'm not 100% sure why though. The `logging` doc says
> Note If you attach a handler to a logger and one or more of its ancestors, it may emit the same record multiple times. In general, you should not need to attach a handler to more than one logger - if you just attach it to the appropriate logger which is highest in the logger hierarchy, then it will see all events logged by all descendant loggers, provided that their propagate setting is left set to True. A common scenario is to attach handlers only to the root logger, and to let propagation take care of the rest.

However having propagation disabled makes the `caplog`/`capsys` unable to get what's logged.

In `transformers` we're using some utilities to test the logging outputs. It can be used like this:
```python
from transformers import logging
from transformers.testing_utils import CaptureLogger

msg = "Testing 1, 2, 3"
logging.set_verbosity_info()
logger = logging.get_logger("transformers.models.bart.tokenization_bart")
with CaptureLogger(logger) as cl:
    logger.info(msg)
assert cl.out, msg + "\n"
```

Maybe we could have the same here ? The testing utils are in there: 

https://github.com/huggingface/transformers/blob/master/src/transformers/testing_utils.py#L443

We can have the same code in `datasets` for the tests

Also cc @user2 @user3 who may know more

@author:
Thanks for your detailed answer @user1. 

That is one of the reasons why I would like to know why you disabled propagation: current implementation does not allow any capture by `caplog`/`capsys`. If keeping propagation disabled, I agree with you that we should implement this alternative log-capturing mechanism for tests.

But there is another reason: end user. The typical end user normally does not custom configure HF logging in advance. Thus, currently, they do not have access to (error/warning) logs to the default sys.stderr. That is why in HF code `print`s are used instead: to make sure the user receives HF error/warning messages.

On the other hand, if propagation is enabled:
- our tests can normally use `caplog`/`capsys`
- the typical end user (who does not configure logging) sees the error/warning logs in the terminal. 

  This is so because of the "default" handler: Python logging package has a default handler (only used if the end user has not previously set any custom handler) which sends error/warning logs to the sys.stderr.
- if an advanced end user (or an application using HF) configures logging with custom handlers, then those are used instead of the "default" handler.
- no need of prints for error/warnings


@user1:
Ok I found why we had propagation disabled in the first place. This was related to TF 1.14 and an issue with absl (more info [here](https://github.com/tensorflow/tensorflow/issues/26691))

So now we should clearly re-enable propagation since the issue is fixed with absl>=0.8.0

Moreover I'm not a fan of adding a handler by default (see logging configuration advise [here](https://docs.python.org/3/howto/logging.html#configuring-logging-for-a-library)) so I would also remove the handler that was added when we disabled propagation.

I'm creating a PR right away to make these changes

@user1:
PR is here: https://github.com/huggingface/datasets/pull/1845

@author:
Thank you @user1. Much clear logging defaults now, following best practices!!!

I am going to adapt this PR, once yours is merged.

@user1:
PR is merged :) fee free to merge master into your branch