hdmf-dev / hdmf

The Hierarchical Data Modeling Framework
http://hdmf.readthedocs.io
Other
46 stars 26 forks source link

Plain base Exception instances should not be raised #340

Open yarikoptic opened 4 years ago

yarikoptic commented 4 years ago

Ideally many exceptions raised by HDMF should have some base classe(s) for exceptions of "HDMF nature". For some cases where it is indeed a typical IndexError (e.g. out of bounds like below), should be ok to raise it, but even then there could be class HDMFIndexError(IndexError).

Why am I here with this -- I just ran into:

  File "/usr/lib/python3/dist-packages/hdmf/build/objectmapper.py", line 969, in construct
    raise Exception(msg) from ex
Exception: Could not construct DynamicTableRegion object due to The index 63 is out of range for this DynamicTable of length 63

and code indeed has a good number of those:

$> git grep 'raise Except'
src/hdmf/backends/hdf5/h5tools.py:                    raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:                raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:            raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:            raise Exception("Could not create dataset %s in %s" % (name, parent.name)) from exc
src/hdmf/backends/hdf5/h5tools.py:                raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:            raise Exception(msg) from exc
src/hdmf/build/objectmapper.py:                raise Exception(msg)
src/hdmf/build/objectmapper.py:                        raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:                            raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:                        raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:                        raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:            raise Exception(msg) from ex
src/hdmf/container.py:            raise Exception('cannot reassign container_source')
src/hdmf/data_utils.py:            raise Exception('Data type could not be determined. Please specify dtype in DataChunkIterator init.')
src/hdmf/spec/spec.py:            raise Exception('Cannot re-assign parent')
src/hdmf/testing/testcase.py:                        raise Exception(err)
src/hdmf/utils.py:                raise Exception('docval for {}: keys {} are not supported by docval'.format(a['name'],
src/hdmf/utils.py:                raise Exception(msg)
src/hdmf/utils.py:                    raise Exception(msg)
src/hdmf/utils.py:                    raise Exception(msg)
src/hdmf/utils.py:                    raise Exception(msg)
src/hdmf/utils.py:                    raise ExceptionType(msg)

which makes it hard to impossible for outside code to really tell if it is something "core" (permission, IO, etc errors) spit out by standard libraries or something PyNWB or HDMF dislikes. It makes outside code to resort to just except Exception which IIRC even pep8 doesn't like ;-)

bendichter commented 3 years ago

Thanks @yarikoptic. This sounds valuable. We'd have to think about what we want our Exception ontology to be.

Anyone want to offer some suggestions?

yarikoptic commented 3 years ago

"ontology" -- ;-) Exception ontologies like a religion and might cause if not a war, then a stomach to rumble to start with.
I would recommend to review your favorite established libraries, and follow the suit. Overall, reusing standard exceptions https://docs.python.org/3/library/exceptions.html is a good strategy to follow, and HDMF largely already uses them. It is for those Exception's originally mentioned -- some could be replaced with standard ones or some more HDMF specific exceptions could be defined.

oruebel commented 3 years ago

I think especially in the ObjectMapping it will be useful to have our own exception types to more clearly communicate where the exception is coming from. In other cases it will be useful to user more specific standard exception types (e.g, when we raise issues due to HDF5).

rly commented 3 years ago

See https://github.com/NeurodataWithoutBorders/pynwb/issues/1144 for a need for a dedicated Exception class for missing namespaces.