Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-448

Document intended purposes of LSST exceptions

    XMLWordPrintable

    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

            Hide
            pschella 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.

            Show
            pschella 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.
            Hide
            pschella 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.

            Show
            pschella 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.
            Hide
            krzys 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.

            Show
            krzys 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.
            Hide
            tjenness Tim Jenness added a comment -

            Are you ready to adopt this RFC?

            Show
            tjenness Tim Jenness added a comment - Are you ready to adopt this RFC?
            Hide
            krzys Krzysztof Findeisen added a comment -

            Adopted as amended in comments above.

            Show
            krzys Krzysztof Findeisen added a comment - Adopted as amended in comments above.

              People

              Assignee:
              krzys Krzysztof Findeisen
              Reporter:
              krzys Krzysztof Findeisen
              Watchers:
              Jim Bosch, John Parejko, John Swinbank, Kian-Tat Lim, Krzysztof Findeisen, Pim Schellart [X] (Inactive), Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Planned End:

                  CI Builds

                  No builds found.