Repository: voxel51/fiftyone
Pull request: adding filename exception logging for failed xml parsing
Currently when an invalid or malformed xml file is parsed an `ExpatError` will be raised.  This change logs the filename that produced this error before re-raising it to the calling code.

Problem statement:
[BUG] Importing VOCDetectionDataset from disk fails due to bad XML - but what file?!
### System information

-   **OS Platform and Distribution (e.g., Linux Ubuntu 16.04)**: MacOS 10.15.7
-   **FiftyOne installed from (pip or source)**: pip
-   **FiftyOne version (run `fiftyone --version`)**: FiftyOne v0.15.1, Voxel51, Inc.
-   **Python version**: Python 3.6.8

### Commands to reproduce

As thoroughly as possible, please provide the Python and/or shell commands used
to encounter the issue. Application steps can be described in the next section.

From within a Jupyter notebook (so no source level debugging available). See code below.

### Describe the problem

One of my Pascal VOC format XML files evidently cannot be parsed for some reason. I'm not sure which one. FiftyOne fails with the following message. The exception provides no information about which file it failed on. So I cannot find and fix the problem in the Pascal VOC label file.
So the bug is that the exception needs to provide information about what file it failed on so that the user can fix or delete the offending file.

```
  23% |███|-------------| 201/893 [644.9ms elapsed, 2.2s remaining, 323.8 samples/s] 
---------------------------------------------------------------------------
ExpatError                                Traceback (most recent call last)
<ipython-input-23-18ab5bbecbe2> in <module>
----> 1 dataset = fo.Dataset.from_dir(dataset_type=fo.types.VOCDetectionDataset, data_path=dataset_path, labels_path=dataset_path, name=dataset_name)

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/core/dataset.py in from_dir(cls, dataset_dir, dataset_type, data_path, labels_path, name, label_field, tags, **kwargs)
   3748             label_field=label_field,
   3749             tags=tags,
-> 3750             **kwargs,
   3751         )
   3752         return dataset

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/core/dataset.py in add_dir(self, dataset_dir, dataset_type, data_path, labels_path, label_field, tags, expand_schema, add_info, **kwargs)
   2536             tags=tags,
   2537             expand_schema=expand_schema,
-> 2538             add_info=add_info,
   2539         )
   2540 

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/core/dataset.py in add_importer(self, dataset_importer, label_field, tags, expand_schema, add_info)
   3080             tags=tags,
   3081             expand_schema=expand_schema,
-> 3082             add_info=add_info,
   3083         )
   3084 

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/utils/data/importers.py in import_samples(dataset, dataset_importer, label_field, tags, expand_schema, add_info)
    128         samples = map(parse_sample, iter(dataset_importer))
    129         sample_ids = dataset.add_samples(
--> 130             samples, expand_schema=expand_schema, num_samples=num_samples
    131         )
    132 

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/core/dataset.py in add_samples(self, samples, expand_schema, validate, num_samples)
   1453         sample_ids = []
   1454         with fou.ProgressBar(total=num_samples) as pb:
-> 1455             for batch in batcher:
   1456                 sample_ids.extend(
   1457                     self._add_samples_batch(batch, expand_schema, validate)

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/core/utils.py in __next__(self)
    919             idx = 0
    920             while idx < batch_size:
--> 921                 batch.append(next(self._iter))
    922                 idx += 1
    923 

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/utils/voc.py in __next__(self)
    134         if labels_path:
    135             # Labeled image
--> 136             annotation = load_voc_detection_annotations(labels_path)
    137 
    138             # Use image filename from annotation file if possible

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/utils/voc.py in load_voc_detection_annotations(xml_path)
    742         a :class:`VOCAnnotation` instance
    743     """
--> 744     return VOCAnnotation.from_xml(xml_path)
    745 
    746 

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/utils/voc.py in from_xml(cls, xml_path)
    463             a :class:`VOCAnnotation`
    464         """
--> 465         d = fou.load_xml_as_json_dict(xml_path)
    466         return cls.from_dict(d)
    467 

~/envs/fiftyone/lib/python3.6/site-packages/fiftyone/core/utils.py in load_xml_as_json_dict(xml_path)
    664     """
    665     with open(xml_path, "rb") as f:
--> 666         return xmltodict.parse(f.read())
    667 
    668 

~/envs/fiftyone/lib/python3.6/site-packages/xmltodict.py in parse(xml_input, encoding, expat, process_namespaces, namespace_separator, disable_entities, process_comments, **kwargs)
    376         parser.Parse(b'',True)
    377     else:
--> 378         parser.Parse(xml_input, True)
    379     return handler.item
    380 

ExpatError: not well-formed (invalid token): line 1, column 1
```

### Code to reproduce issue

Provide a reproducible test case that is the bare minimum necessary to generate
the problem.

```
import fiftyone as fo
dataset_path = '.../images/trailcam/scraped_jan_2022/scraped'
dataset_name = 'scraped_jan_2022'
dataset = fo.Dataset.from_dir(dataset_type=fo.types.VOCDetectionDataset, data_path=dataset_path, labels_path=dataset_path, name=dataset_name)
```

### Other info / logs

Include any logs or source code that would be helpful to diagnose the problem.
If including tracebacks, please include the full traceback. Large logs and
files should be attached. Please do not use screenshots for sharing text. Code
snippets should be used instead when providing tracebacks, logs, etc.

### What areas of FiftyOne does this bug affect?

-   [ ] `App`: FiftyOne application issue
-   [x] `Core`: Core `fiftyone` Python library issue
-   [ ] `Server`: Fiftyone server issue

### Willingness to contribute

The FiftyOne Community encourages bug fix contributions. Would you or another
member of your organization be willing to contribute a fix for this bug to the
FiftyOne codebase?

-   [ ] Yes. I can contribute a fix for this bug independently.
-   [ ] Yes. I would be willing to contribute a fix for this bug with guidance
        from the FiftyOne community.
-   [ ] No. I cannot contribute a bug fix at this time.

Hints from issue discussion:
Definitely agree that a more informative error that includes the offending filename is called for here 💪

Commit message: adding filename exception logging for failed xml parsing

Patch to review:
diff --git a/fiftyone/core/utils.py b/fiftyone/core/utils.py
index cbc6462d98f..f30e145b95d 100644
--- a/fiftyone/core/utils.py
+++ b/fiftyone/core/utils.py
@@ -6,11 +6,6 @@
 |
 """
 import atexit
-from base64 import b64encode, b64decode
-from collections import defaultdict
-from contextlib import contextmanager
-from copy import deepcopy
-from datetime import date, datetime
 import hashlib
 import importlib
 import inspect
@@ -19,8 +14,8 @@
 import logging
 import ntpath
 import os
-import posixpath
 import platform
+import posixpath
 import signal
 import struct
 import subprocess
@@ -28,6 +23,15 @@
 import types
 import zlib
 
+from base64 import b64decode
+from base64 import b64encode
+from collections import defaultdict
+from contextlib import contextmanager
+from copy import deepcopy
+from datetime import date
+from datetime import datetime
+from xml.parsers.expat import ExpatError
+
 try:
     import pprintpp as _pprint
 
@@ -662,8 +666,12 @@ def load_xml_as_json_dict(xml_path):
     Returns:
         a JSON dict
     """
-    with open(xml_path, "rb") as f:
-        return xmltodict.parse(f.read())
+    try:
+        with open(xml_path, "rb") as f:
+            return xmltodict.parse(f.read())
+    except ExpatError as ex:
+        logger.error(f"Failed to read {xml_path}: {ex}")
+        raise
 
 
 def parse_serializable(obj, cls):


Code review comments:
diff_hunk: @@ -19,15 +14,24 @@
 import logging
 import ntpath
 import os
-import posixpath
 import platform
+import posixpath
 import signal
 import struct
 import subprocess
 import timeit
 import types
 import zlib
 
+from base64 import b64decode
path: fiftyone/core/utils.py
text: @user1:
nit: per [style guide](https://github.com/voxel51/fiftyone/blob/develop/STYLE_GUIDE.md#imports), we currently ignore `import`/`from` for grouping purposes.

IE, import blocks are defined solely based on whether it is builtin/third-party/project, not by import strategy used.

diff_hunk: @@ -662,8 +666,12 @@ def load_xml_as_json_dict(xml_path):
     Returns:
         a JSON dict
     """
-    with open(xml_path, "rb") as f:
-        return xmltodict.parse(f.read())
+    try:
+        with open(xml_path, "rb") as f:
+            return xmltodict.parse(f.read())
+    except ExpatError as ex:
+        logger.error(f"Failed to read {xml_path}: {ex}")
path: fiftyone/core/utils.py
text: @user1:
nit: per [style guide](https://github.com/voxel51/fiftyone/blob/develop/STYLE_GUIDE.md#logging) we currently prefer directly raising exceptions to `logger.error()`:

```py
raise ExpatError(f"Failed to read {xml_path}: {e}")
```