Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
Labels:None
Description
Users have requested (DM-9435) that the LSST exceptions in pex_exceptions have clearly documented purposes. The simplest way to do so is to add documentation blocks to the LSST_EXCEPTION_TYPE macros that define them; since Doxygen is configured to expand that macro, the text would appear in Doxygen/Sphinx.
I propose the following descriptions, motivated by the close (and presumably intentional) resemblance between the LSST hierarchy and the standard C++ one. I also propose a few tweaks to the existing hierarchy in places where I think the current behavior is harmful.
Exception
Inherits from std::exception (in C++) or __builtin__.Exception (in Python)
Provides consistent interface for LSST exceptions.
All exceptions defined by the LSST Stack are derived from this class. Code should not throw or catch Exception directly, but should instead be written in terms of the appropriate subclasses (e.g., catch RuntimeError to handle all unknown errors).
LogicError
Inherits from pex::exceptions::Exception
Reports errors in the logical structure of the program.
LogicError and its subclasses should be thrown to represent problems, such as violation of logical preconditions or class invariants, that are in principle preventable using defensive programming or other good practices. In many cases, it may not be appropriate to catch them.
@see RuntimeError
@see std::logic_error
DomainError
Inherits from pex::exceptions::LogicError
Reports arguments outside the domain of an operation.
This exception should be reserved for mathematical operations that are defined on a limited range of inputs. InvalidParameterError is more appropriate for non-mathematical operations.
@see std::domain_error
For consistency with the Python math module, I propose that pex::exceptions::DomainError inherit from __builtin__.ValueError.
InvalidParameterError
Inherits from pex::exceptions::LogicError
Reports invalid arguments.
This exception reports errors that arise because an argument value has not been accepted.
@see std::invalid_argument
This exception is identical in purpose to __builtin__.ValueError, and I propose that it inherit from it.
LengthError
Inherits from pex::exceptions::LogicError
Reports attempts to exceed implementation-defined length limits for some classes.
For example, some collection classes might not be able to handle more than some number of elements, or bit fields might support a limited number of flags.
@see std::length_error
OutOfRangeError
Inherits from pex::exceptions::LogicError
Reports attempts to access elements using an invalid index or key.
@see std::out_of_range
@note pybind11 wrappers should manually translate this exception to `py::index_error` or `py::key_error`, as appropriate. Some Python language constructs check for exceptions that are exactly `IndexError` or `KeyError` rather than a sub- or superclass.
RuntimeError
Inherits from pex::exceptions::Exception or __builtin__.RuntimeError (in Python)
Reports errors that are due to events beyond the control of the program.
RuntimeError and its subclasses represent problems that cannot be easily predicted or prevented. In other words, a RuntimeError is a possible outcome of calling a function or method even in well-written programs, and should be handled at the appropriate level.
@see LogicError
@see std::runtime_error
I strongly recommend that this exception no longer inherit from __builtin__.RuntimeError, since, while they have similar names, the two exceptions represent very different concepts. __builtin__.RuntimeError is a "miscellaneous exceptions" case rather than a superclass of, e.g., IoError, and more generally the Python standard library does not make a distinction between preventable and non-preventable exceptions.
IoError
Inherits from pex::exceptions::RuntimeError or __builtin__.IOError (in Python)
Reports errors in external input/output operations.
@see std::ios_base::failure
MemoryError
Inherits from pex::exceptions::RuntimeError or __builtin__.MemoryError (in Python)
I would recommend getting rid of this class. In the age of pybind11 and its exception translation, the main advantage of having LSST analogs for the C++ standard exceptions is the extra "this was thrown from this line" machinery, which is not helpful in debugging memory problems. Also, in practice, the main way a function can throw MemoryError is by catching std::bad_alloc and chaining it, which leads to a lot of boilerplate and is easy to forget to do. (Note: there are some places in the Stack where failed C struct allocations are translated to MemoryError, but they could throw bad_alloc just as easily.)
If we do keep this class, I propose the following description:
Reports failure to allocate storage for an object.
LSST C++ code should throw this exception instead of std::bad_alloc. LSST Python code may throw either this exception or the built-in `MemoryError`.
OverflowError
Inherits from pex::exceptions::RuntimeError or __builtin__.OverflowError (in Python)
Reports when the result of an arithmetic operation is too large for the destination type.
@see std::overflow_error
RangeError
Inherits from pex::exceptions::RuntimeError
Reports when the result of an operation cannot be represented by the destination type.
Situations covered by this exception include lossy type conversions.
@see OverflowError
@see UnderflowError
@see std::range_error
TimeoutError
Inherits from pex::exceptions::RuntimeError
I have no idea what this one was originally intended for; there are no obvious analogs in either the C++ or Python libraries.
Since this exception is used very rarely (only in the legacy_ap and daf_butler_experiment packages), I propose that it be removed from pex_exceptions and replaced with package-specific exceptions.
TypeError
Inherits from pex::exceptions::RuntimeError or __builtin__.TypeError (in Python)
Reports errors from accepting an object of an unexpected or inappropriate type.
I would recommend that this exception inherit from pex::exceptions::LogicError rather than RuntimeError, as type errors should be preventable by good program design.
UnderflowError
Inherits from pex::exceptions::RuntimeError or __builtin__.ArithmeticError (in Python)
Reports when the result of an arithmetic operation is too small for the destination type.
@see std::underflow_error
NotFoundError
Inherits from pex::exceptions::Exception or __builtin__.LookupError (in Python)
Reports attempts to access a resource that does not exist.
This exception should be thrown in cases where the item being looked up is not logically an index or key. For lookup errors in C++-like or Python-like collection objects, OutOfRangeError is generally more appropriate.
Attachments
Issue Links
Activity
I believe TimeoutError is supposed to indicate that an operation took longer than its allocated budget. I agree that its usage is limited, and it could be made package-specific.
I think we might be better off going for naming consistency with Python rather than naming consistency with the C++ standard library.
Also, for the cases where we have identified a clear one-to-one correspondence between one of our exceptions and one of the Python built-in exceptions, but with different names, I wonder if we should add the Python name as an alias and perhaps deprecate our current name.
I'm worried that that would snowball into a complete redesign of the exception system.
I'm worried about having our RuntimeError differ conceptually from __builtins__.RuntimeError, even if the ultimate problem is that __builtins__.RuntimeError and std::runtime_error fundamentally disagree. While I'm hoping someone can come up with some other solution, I think we might be better off going for naming consistency with Python rather than naming consistency with the C++ standard library.
I think that having a class similar to std::runtime_error (i.e., a shared base class for error conditions that aren't likely to be bugs) would be very useful, in that it would discourage (or at least not encourage) people from trying to catch Exception and accidentally catching too much. So I'm a little reluctant to do away with that part of the hierarchy.
I don't like the idea of remapping OutOfRangeError to different Python exceptions in the pybind11 layer, and I also think we currently tend to use NotFoundError as a synonym for KeyError, rather than the definition you've proposed. Could we just have OutOfRangeError map to __builtins__.IndexError and NotFoundError map to __builtins__.KeyError?
That was my original idea, until someone pointed out that one of the big users of NotFoundError is the Butler. If NotFoundError was originally created to indicate e.g. missing datasets, it would explain why NotFoundError and OutOfRangeError aren't sibling classes. (Edit: the original documentation also suggests NotFoundError was not intended to resemble KeyError).
Krzysztof Findeisen
added a comment - - edited
I think we might be better off going for naming consistency with Python rather than naming consistency with the C++ standard library.
Also, for the cases where we have identified a clear one-to-one correspondence between one of our exceptions and one of the Python built-in exceptions, but with different names, I wonder if we should add the Python name as an alias and perhaps deprecate our current name.
I'm worried that that would snowball into a complete redesign of the exception system.
I'm worried about having our RuntimeError differ conceptually from __builtins__.RuntimeError, even if the ultimate problem is that __builtins__.RuntimeError and std::runtime_error fundamentally disagree. While I'm hoping someone can come up with some other solution, I think we might be better off going for naming consistency with Python rather than naming consistency with the C++ standard library.
I think that having a class similar to std::runtime_error (i.e., a shared base class for error conditions that aren't likely to be bugs) would be very useful, in that it would discourage (or at least not encourage) people from trying to catch Exception and accidentally catching too much. So I'm a little reluctant to do away with that part of the hierarchy.
I don't like the idea of remapping OutOfRangeError to different Python exceptions in the pybind11 layer, and I also think we currently tend to use NotFoundError as a synonym for KeyError, rather than the definition you've proposed. Could we just have OutOfRangeError map to __builtins__.IndexError and NotFoundError map to __builtins__.KeyError?
That was my original idea, until someone pointed out that one of the big users of NotFoundError is the Butler. If NotFoundError was originally created to indicate e.g. missing datasets, it would explain why NotFoundError and OutOfRangeError aren't sibling classes. (Edit: the original documentation also suggests NotFoundError was not intended to resemble KeyError ). Sorry about the delayed reply.
I'm worried that [renaming our exceptions to match Python's] would snowball into a complete redesign of the exception system.
I don't think that would necessarily be a bad thing, in that I'm not sure we should really consider our exception system to have been thoroughly designed. But I completely agree that it's a lot bigger than the scope of the already good thing you're trying to accomplish here.
I think that having a class similar to std::runtime_error (i.e., a shared base class for error conditions that aren't likely to be bugs) would be very useful, in that it would discourage (or at least not encourage) people from trying to catch Exception and accidentally catching too much. So I'm a little reluctant to do away with that part of the hierarchy.
That's a very good point, and in the abstract I think C++'s definition of "runtime_error" is probably better than Python's. But I also still think that it's not gonna go well if we try to make the same symbol mean different things in different languages. Maybe the difference is subtle enough that we just don't care if people misuse them slightly, but if we don't rename ours, I think that's going to be inevitable.
[Mapping {{NotFoundError}} to {{KeyError}}] was my original idea, until someone pointed out that one of the big users of NotFoundError is the Butler. If NotFoundError was originally created to indicate e.g. missing datasets, it would explain why NotFoundError and OutOfRangeError aren't sibling classes.
I actually think that Butler usage of NotFoundError is pretty compatible with the definition of KeyError; butler lookup is not quite dict lookup, but it certainly is a lookup in some kind of little-m mapping. It'd probably be best to make the exception the Butler raises inherit from Python's LookupError and not KeyError, but I think having that exception inherit from KeyError would be a small price to pay for having a straightforward Python mapping for OutOfRangeError.
But I also still think that it's not gonna go well if we try to make the same symbol mean different things in different languages. Maybe the difference is subtle enough that we just don't care if people misuse them slightly, but if we don't rename ours, I think that's going to be inevitable.
Well, the exception already can't be used as a simple stand-in for __builtin__.RuntimeError: for example, under the current system pex::exceptions::IoError inherits from both __builtin__.RuntimeError and __builtin__.IOError, and will be caught by exception handlers looking for either (even though catching __builtin__.RuntimeError does not normally intercept I/O exceptions). It's more a question of what behavior would be less confusing.
Renaming an exception that's in common use would be very disruptive. Would you settle for keeping the current inheritance relation and combining it with the description proposed above, if we can't find a better solution?
I actually think that Butler usage of NotFoundError is pretty compatible with the definition of KeyError; butler lookup is not quite dict lookup, but it certainly is a lookup in some kind of little-m mapping. It'd probably be best to make the exception the Butler raises inherit from Python's LookupError and not KeyError, but I think having that exception inherit from KeyError would be a small price to pay for having a straightforward Python mapping for OutOfRangeError.
Ok then:
I propose that OutOfRangeError be auto-translated to (not inherit from) __builtin__.IndexError by pybind11, and have the following description:
Reports attempts to access elements outside a valid range of indices.
@see NotFoundError
@see std::out_of_range
I also propose that NotFoundError be auto-translated to __builtin__.KeyError (instead of inheriting from __builtin__.LookupError like at present), and that it have the following description:
Reports attempts to access elements using an invalid key.
This exception may represent lookup failures in classes that resemble C++ maps or Python dictionaries, but it may also be used when the relationship between an identifier and a resource is more abstract.
@see OutOfRangeError
Krzysztof Findeisen
added a comment - - edited But I also still think that it's not gonna go well if we try to make the same symbol mean different things in different languages. Maybe the difference is subtle enough that we just don't care if people misuse them slightly, but if we don't rename ours, I think that's going to be inevitable.
Well, the exception already can't be used as a simple stand-in for __builtin__.RuntimeError : for example, under the current system pex::exceptions::IoError inherits from both __builtin__.RuntimeError and __builtin__.IOError , and will be caught by exception handlers looking for either (even though catching __builtin__.RuntimeError does not normally intercept I/O exceptions). It's more a question of what behavior would be less confusing.
Renaming an exception that's in common use would be very disruptive. Would you settle for keeping the current inheritance relation and combining it with the description proposed above, if we can't find a better solution?
I actually think that Butler usage of NotFoundError is pretty compatible with the definition of KeyError ; butler lookup is not quite dict lookup, but it certainly is a lookup in some kind of little-m mapping. It'd probably be best to make the exception the Butler raises inherit from Python's LookupError and not KeyError , but I think having that exception inherit from KeyError would be a small price to pay for having a straightforward Python mapping for OutOfRangeError .
Ok then:
I propose that OutOfRangeError be auto-translated to ( not inherit from) __builtin__.IndexError by pybind11, and have the following description:
Reports attempts to access elements outside a valid range of indices.
@see NotFoundError
@see std::out_of_range
I also propose that NotFoundError be auto-translated to __builtin__.KeyError (instead of inheriting from __builtin__.LookupError like at present), and that it have the following description:
Reports attempts to access elements using an invalid key.
This exception may represent lookup failures in classes that resemble C++ maps or Python dictionaries, but it may also be used when the relationship between an identifier and a resource is more abstract.
@see OutOfRangeError Renaming an exception that's in common use would be very disruptive. Would you settle for keeping the current inheritance relation and combining it with the description proposed above, if we can't find a better solution?
Yes, that sounds reasonable.
I propose that OutOfRangeError be auto-translated to (not inherit from) _builtin_.IndexError by pybind11, and have the following description...
I also propose that NotFoundError be auto-translated to _builtin.KeyError (instead of inheriting from builtin_.LookupError like at present), and that it have the following description...
I have no objection to auto-translating to the Python builtins in principle, but I want to make sure we retain the ability to inject the (incomplete, but still quite useful) C++ traceback information into the printed traceback we see in Python. I don't recall exactly how that mechanism works, so I'm not sure if relies on raising a custom exception class in Python or not.
on your descriptions for both of these.
Pim Schellart [X], are there any technical problems with the proposed handling of OutOfRangeError and NotFoundError, like Jim Bosch suggests?
Krzysztof Findeisen
added a comment - - edited Pim Schellart [X] , are there any technical problems with the proposed handling of OutOfRangeError and NotFoundError , like Jim Bosch suggests? It doesn't do anything special. It just wraps the C++ exception in a (custom) Python object that prints e.what() as part of its __repr__.
I think we could special case it, at either the C++ or Python side to translate to a builtin.
Having said that, I'm not so happy about the proposed mapping.
Especially since it looks to be different from the one pybind11 uses: https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html
Having four different hierarchies (Python standard, C++ standard, pybind11 standard and pex) that are similar but not identical is a bit confusing.
What about instead just using C++ std exceptions (relying on pybind11's default translator) and only roll our own for things that are really different?
Granted, that may be a big change, out-of-scope for this ticket.
But I think we should either not change anything in the current behavior at all (just add documentation), or really think about the desired end result and do it right.
Pim Schellart [X] (Inactive)
added a comment - It doesn't do anything special. It just wraps the C++ exception in a (custom) Python object that prints e.what() as part of its __repr__ .
I think we could special case it, at either the C++ or Python side to translate to a builtin .
Having said that, I'm not so happy about the proposed mapping.
Especially since it looks to be different from the one pybind11 uses: https://pybind11.readthedocs.io/en/stable/advanced/exceptions.html
Having four different hierarchies (Python standard, C++ standard, pybind11 standard and pex) that are similar but not identical is a bit confusing.
What about instead just using C++ std exceptions (relying on pybind11's default translator) and only roll our own for things that are really different?
Granted, that may be a big change, out-of-scope for this ticket.
But I think we should either not change anything in the current behavior at all (just add documentation), or really think about the desired end result and do it right. Of course that doesn't quite answer your question.
No, I don't think there is a technical problem with it.
Pim Schellart [X] (Inactive)
added a comment - Of course that doesn't quite answer your question.
No, I don't think there is a technical problem with it. Good point about contradicting pybind11's mappings (though mapping almost everything to ValueError doesn't seem well thought out, either). So no changes to OutOfRangeError or NotFoundError's inheritance relations; documentation as proposed on Wednesday.
Krzysztof Findeisen
added a comment - Good point about contradicting pybind11's mappings (though mapping almost everything to ValueError doesn't seem well thought out, either). So no changes to OutOfRangeError or NotFoundError 's inheritance relations; documentation as proposed on Wednesday. Are you ready to adopt this RFC?
Adopted as amended in comments above.
Krzysztof Findeisen
added a comment - Adopted as amended in comments above. 
Thanks for putting this together! It's something we desperately need, and I agree with essentially everything you've proposed.
There are two problems you've identified that I'd like to be a bit more aggressive about trying to fix:
Also, for the cases where we have identified a clear one-to-one correspondence between one of our exceptions and one of the Python built-in exceptions, but with different names, I wonder if we should add the Python name as an alias and perhaps deprecate our current name.