Repository: bids-standard/pybids (python)

Issue:
should pybids more lenient or at least get a dedicated exception like NonCompliantBIDSError
ATM pybids immediately throws a generic ValueError if found contained derivatives dataset is not fully compliant:

```shell
/tmp > datalad install https://github.com/OpenNeuroDatasets/ds001868 && cd ds001868
[INFO   ] Cloning https://github.com/OpenNeuroDatasets/ds001868 [1 other candidates] into '/tmp/ds001868' 
[INFO   ]   Remote origin not usable by git-annex; setting annex-ignore                                         
[INFO   ] access to 1 dataset sibling s3-PRIVATE not auto-enabled, enable with:
| 		datalad siblings -d "/tmp/ds001868" enable -s s3-PRIVATE 
install(ok): /tmp/ds001868 (dataset)

/tmp/ds001868 > python -c 'from bids import BIDSLayout; b=BIDSLayout(".", derivatives=True)' 
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/yoh/proj/bids/pybids/bids/layout/layout.py", line 227, in __init__
    index_metadata=index_metadata)
  File "/home/yoh/proj/bids/pybids/bids/layout/layout.py", line 555, in add_derivatives
    raise ValueError("Every valid BIDS-derivatives dataset must "
ValueError: Every valid BIDS-derivatives dataset must have a PipelineDescription.Name field set inside dataset_description.json.
```
But I wonder
- if this should be a warning and not error (optionally or not)
- there should be a dedicated exception (e.g. `NonCompliantBIDSError(ValueError)` and `NonCompliantDerivativesBIDSError(NonCompliantBIDSError)`) which would be thrown here instead of a generic ValueError which makes it fragile to handle such cases programmatically 

Pull request:
ENH: Introduce PyBIDS exceptions
This is an initial shot, which Closes #473 if merged since original concern would be addressed.

But
- I wonder if I have not overdone it a bit ;-)
- I think that many (if not all) of the `*BIDS*` exceptions should have ideally came from `bids_validator` package, and were not defined in PyBIDS
- I might have (or not) misinterpreted/attributed difference between "entity" (BIDS term) and target (more of a PyBIDS term).

But looking at the tests I think it becomes a bit clearer on what to expect to be raised, in comparison to IMHO abused `ValueError` (could come from anywhere).  I might have even made `BIDSValidationError` not a subclass of `ValueError` since we are not really talking about a value passed into a function in pybids (which is what typically ValueError is used for, and what I left it used for in a number of places), but rather about an entity on the harddrive (the BIDS dataset). 

Patch:
diff --git a/bids/exceptions.py b/bids/exceptions.py
new file mode 100644
index 00000000..7c599ea6
--- /dev/null
+++ b/bids/exceptions.py
@@ -0,0 +1,37 @@
+""" Exceptions.
+
+Exceptions relating to problems with BIDS itself, should carry BIDS in their
+name.  All exceptions should subclass from PyBIDSError
+"""
+
+
+class PyBIDSError(Exception):
+    """ Base class.  Typically for mix-in."""
+
+
+class ConfigError(ValueError, PyBIDSError):
+    """ Problems with config file. """
+
+
+class NoMatchError(ValueError, PyBIDSError):
+    """ No match found where it is required. """
+
+
+class BIDSEntityError(AttributeError, PyBIDSError):
+    """ An unknown entity. """
+
+
+class TargetError(ValueError, PyBIDSError):
+    """ An unknown target. """
+
+
+class BIDSValidationError(ValueError, PyBIDSError):
+    """ An invalid BIDS dataset. """
+
+
+class BIDSDerivativesValidationError(BIDSValidationError):
+    """ An invalid BIDS derivative dataset. """
+
+
+class BIDSConflictingValuesError(BIDSValidationError):
+    """ A value conflict (e.g. in filename and side-car .json) """
diff --git a/bids/layout/index.py b/bids/layout/index.py
index ee41acda..e181b463 100644
--- a/bids/layout/index.py
+++ b/bids/layout/index.py
@@ -6,6 +6,7 @@ from collections import defaultdict
 from bids_validator import BIDSValidator
 from .models import Config, Entity, Tag, FileAssociation
 from ..utils import listify, make_bidsfile
+from ..exceptions import BIDSConflictingValuesError
 
 
 def _extract_entities(bidsfile, entities):
@@ -367,8 +368,9 @@ class BIDSLayoutIndexer(object):
                             "filename {} (value='{}') versus its JSON sidecar "
                             "(value='{}'). Please reconcile this discrepancy."
                         )
-                        raise ValueError(msg.format(md_key, bf.path, file_val,
-                                                    md_val))
+                        raise BIDSConflictingValuesError(
+                            msg.format(md_key, bf.path, file_val,
+                            md_val))
                     continue
                 if md_key not in all_entities:
                     all_entities[md_key] = Entity(md_key, is_metadata=True)
diff --git a/bids/layout/layout.py b/bids/layout/layout.py
index f0ef306c..de9777ea 100644
--- a/bids/layout/layout.py
+++ b/bids/layout/layout.py
@@ -24,6 +24,14 @@ from .models import (Base, Config, BIDSFile, Entity, Tag)
 from .index import BIDSLayoutIndexer
 from .utils import BIDSMetadata
 from .. import config as cf
+from ..exceptions import (
+    BIDSDerivativesValidationError,
+    BIDSEntityError,
+    BIDSValidationError,
+    ConfigError,
+    NoMatchError,
+    TargetError,
+)
 
 try:
     from os.path import commonpath
@@ -122,10 +130,10 @@ def add_config_paths(**kwargs):
     """
     for k, path in kwargs.items():
         if not os.path.exists(path):
-            raise ValueError(
+            raise ConfigError(
                 'Configuration file "{}" does not exist'.format(k))
         if k in cf.get_option('config_paths'):
-            raise ValueError('Configuration {!r} already exists'.format(k))
+            raise ConfigError('Configuration {!r} already exists'.format(k))
 
     kwargs.update(**cf.get_option('config_paths'))
     cf.set_option('config_paths', kwargs)
@@ -307,7 +315,7 @@ class BIDSLayout(object):
                 if sing in entities:
                     ent_name = sing
                 else:
-                    raise AttributeError(
+                    raise BIDSEntityError(
                         "'get_{}' can't be called because '{}' isn't a "
                         "recognized entity name.".format(ent_name, ent_name))
             return partial(self.get, return_type='id', target=ent_name)
@@ -480,7 +488,7 @@ class BIDSLayout(object):
         target = os.path.join(self.root, 'dataset_description.json')
         if not os.path.exists(target):
             if self.validate:
-                raise ValueError(
+                raise BIDSValidationError(
                     "'dataset_description.json' is missing from project root."
                     " Every valid BIDS dataset must have this file."
                     "\nExample contents of 'dataset_description.json': \n%s" %
@@ -494,9 +502,9 @@ class BIDSLayout(object):
             if self.validate:
                 for k in MANDATORY_BIDS_FIELDS:
                     if k not in self.description:
-                        raise ValueError("Mandatory %r field missing from "
-                                         "'dataset_description.json'."
-                                         "\nExample: %s" % (k, MANDATORY_BIDS_FIELDS[k])
+                        raise BIDSValidationError("Mandatory %r field missing from "
+                                                  "'dataset_description.json'."
+                                                  "\nExample: %s" % (k, MANDATORY_BIDS_FIELDS[k])
                         )
 
     def _validate_force_index(self):
@@ -805,13 +813,15 @@ class BIDSLayout(object):
             pipeline_name = description.get(
                 'PipelineDescription', {}).get('Name')
             if pipeline_name is None:
-                raise ValueError("Every valid BIDS-derivatives dataset must "
+                raise BIDSDerivativesValidationError(
+                                 "Every valid BIDS-derivatives dataset must "
                                  "have a PipelineDescription.Name field set "
                                  "inside 'dataset_description.json'. "
                                  "\nExample: %s" %
                                  MANDATORY_DERIVATIVES_FIELDS['PipelineDescription.Name'])
             if pipeline_name in self.derivatives:
-                raise ValueError("Pipeline name '%s' has already been added "
+                raise BIDSDerivativesValidationError(
+                                 "Pipeline name '%s' has already been added "
                                  "to this BIDSLayout. Every added pipeline "
                                  "must have a unique name!")
             # Default config and sources values
@@ -968,7 +978,7 @@ class BIDSLayout(object):
                 message = "Did you mean one of: {}?".format(suggestions)
             else:
                 message = "Valid targets are: {}".format(potential)
-            raise ValueError(("Unknown target '{}'. " + message)
+            raise TargetError(("Unknown target '{}'. " + message)
                              .format(target))
 
         results = []
@@ -992,7 +1002,7 @@ class BIDSLayout(object):
 
         elif return_type in ['id', 'dir']:
             if target is None:
-                raise ValueError('If return_type is "id" or "dir", a valid '
+                raise TargetError('If return_type is "id" or "dir", a valid '
                                  'target entity must also be specified.')
 
             results = [x for x in results if target in x.entities]
@@ -1254,7 +1264,7 @@ class BIDSLayout(object):
         if not filters.get('suffix'):
             f = self.get_file(path)
             if 'suffix' not in f.entities:
-                raise ValueError(
+                raise BIDSValidationError(
                     "File '%s' does not have a valid suffix, most "
                     "likely because it is not a valid BIDS file." % path
                 )
@@ -1424,7 +1434,7 @@ class BIDSLayout(object):
         images = self.get(extension=['nii', 'nii.gz'], scope=scope,
                           **filters)
         if not images:
-            raise ValueError("No functional images that match criteria found.")
+            raise NoMatchError("No functional images that match criteria found.")
 
         all_trs = set()
         for img in images:
@@ -1432,7 +1442,7 @@ class BIDSLayout(object):
             all_trs.add(round(float(md['RepetitionTime']), 5))
 
         if len(all_trs) > 1:
-            raise ValueError("Unique TR cannot be found given filters {!r}"
+            raise NoMatchError("Unique TR cannot be found given filters {!r}"
                              .format(filters))
         return all_trs.pop()
 
@@ -1515,7 +1525,8 @@ class BIDSLayout(object):
         to_check = os.path.join(os.path.sep, built)
 
         if validate and not BIDSValidator().is_bids(to_check):
-            raise ValueError("Built path {} is not a valid BIDS filename. "
+            raise BIDSValidationError(
+                             "Built path {} is not a valid BIDS filename. "
                              "Please make sure all provided entity values are "
                              "spec-compliant.".format(built))
 


Test patch:
diff --git a/bids/layout/tests/test_layout.py b/bids/layout/tests/test_layout.py
index e2a39375..20fb5af5 100644
--- a/bids/layout/tests/test_layout.py
+++ b/bids/layout/tests/test_layout.py
@@ -18,6 +18,13 @@ from bids.layout.utils import BIDSMetadata
 from bids.tests import get_test_data_path
 from bids.utils import natural_sort
 
+from bids.exceptions import (
+    BIDSValidationError,
+    ConfigError,
+    NoMatchError,
+    TargetError,
+)
+
 
 def test_layout_init(layout_7t_trt):
     assert isinstance(layout_7t_trt.files, dict)
@@ -194,11 +201,11 @@ def test_get_metadata_error(layout_7t_trt):
 
 
 def test_get_with_bad_target(layout_7t_trt):
-    with pytest.raises(ValueError) as exc:
+    with pytest.raises(TargetError) as exc:
         layout_7t_trt.get(target='unicorn')
         msg = exc.value.message
         assert 'subject' in msg and 'reconstruction' in msg and 'proc' in msg
-    with pytest.raises(ValueError) as exc:
+    with pytest.raises(TargetError) as exc:
         layout_7t_trt.get(target='sub')
         msg = exc.value.message
         assert 'subject' in msg and 'reconstruction' not in msg
@@ -440,11 +447,11 @@ def test_derivative_getters():
 
 def test_get_tr(layout_7t_trt):
     # Bad subject, should fail
-    with pytest.raises(ValueError) as exc:
+    with pytest.raises(NoMatchError) as exc:
         layout_7t_trt.get_tr(subject="zzz")
         assert exc.value.message.startswith("No functional images")
     # There are multiple tasks with different TRs, so this should fail
-    with pytest.raises(ValueError) as exc:
+    with pytest.raises(NoMatchError) as exc:
         layout_7t_trt.get_tr(subject=['01', '02'])
         assert exc.value.message.startswith("Unique TR")
     # This should work
@@ -543,10 +550,10 @@ def test_deriv_indexing():
 def test_add_config_paths():
     bids_dir = dirname(bids.__file__)
     bids_json = os.path.join(bids_dir, 'layout', 'config', 'bids.json')
-    with pytest.raises(ValueError) as exc:
+    with pytest.raises(ConfigError) as exc:
         add_config_paths(test_config1='nonexistentpath.json')
     assert str(exc.value).startswith('Configuration file')
-    with pytest.raises(ValueError) as exc:
+    with pytest.raises(ConfigError) as exc:
         add_config_paths(bids=bids_json)
     assert str(exc.value).startswith("Configuration 'bids' already")
     add_config_paths(dummy=bids_json)
@@ -625,7 +632,7 @@ def test_layout_save(tmp_path, layout_7t_trt):
 
 def test_indexing_tag_conflict():
     data_dir = join(get_test_data_path(), 'ds005_conflict')
-    with pytest.raises(ValueError) as exc:
+    with pytest.raises(BIDSValidationError) as exc:
         layout = BIDSLayout(data_dir)
         print(exc.value.message)
         assert exc.value.message.startswith("Conflicting values found")