openff-toolkit: Custom exceptions

Thanks for the feedback!

why does MessageException exist?

This has been in the codebase for ages (I think back when the PIs were the only developers) and I have always been confused by it as well. Things were different back in ~2017 … maybe there’s some Python 2 compatibility that I have never come across? Maybe it was part of a more elaborate idea that never got fully implemented. I don’t know what it does that the built-in exceptions don’t do, and therefore I’d like to ditch it.

Your last code snippet (with class ToolkitUnavailableException(MissingPackageError):) is exactly the design I’m hoping can be implemented here.

I agree that each docstring, on average, does a poor job of communicating which exceptions can be raised. I’d like to say that we could go through everything and make a good effort to list out everything that could be raised by any function/method call, but that’s probably a significant amount of work to wrangle up and a decent amount of work to keep things in sync over time. I’m even skeptical that it can be done well manually - I wonder if there are automated solutions for this?

I’m still not sure about this reasoning for so many different custom exceptions. … So long as the ModuleNotFoundError explains which module is missing and possible reasons for it, I’m not sure I care which of MissingPackageError, ToolkitUnavailableException, AntechamberNotFoundError I get.

I think the motivation here is similar to why we’d want to do custom exceptions to begin with; most users will reach for a somewhat broad exception (i.e. one that broadly captures molecule parsing errors) like you describe but some specific cases might require dealing with one or more of the more detailed exceptions. In cases like those, it’s more useful to have a sub(-sub-sub…)classed exception that handles one extremely specific case, even if the contents of the message are similar, just like it’s better to have a ToolkitUnavailableException in addition to ModuleNotFoundError. I would assume that really specific exceptions (maybe one for RDKit’s SMILES parsing failing for reason xyz) are mostly used by developers/powerusers and not of much interest to most users.

I’m almost always writing with one particular erroring line of code in mind

You can still do this with XYZError, but for people who want to except a general class of errors that they know could be raised (e.g. ImportErrors because the toolkit comes with optional dependencies), this requires them to not only dig through the code to locate and correctly import the potential error, but to branch through all possibilities and import all potentially raised errors, which then requires experience and high familiarity with the layout and organization of the code. I would classify anyone who can do this as an OpenFF Toolkit expert who probably wrote some of the code. What I would expect developers to actually do (and what I often do) is run through some test cases in a notebook and just make a long tuple of all exceptions that are eventually raised (except (MissingDependencyError, MissingPackageError, ToolkitUnavailableException, LicenseError)). This also requires a non-trivial amount of work and familiarity. What I expect at least some users to do is a blank try/except Exception, especially if they’re just doing something quick and dirty, or a little prototype, and don’t have the time to mess around with the toolkit code, or remember what an exception is called.

But I had a fiasco a few months ago where it took me several hours to learn that the ValueError I was catching deep in the stack was coming from RDKit and not the line I expected in the OpenFF toolkit.

ValueError is the most generic of the errors – IndexError, KeyError, TypeError, AttributeError all have fairly standard meanings, and often the main Python library depends on them in ways that break if you don’t subclass these main errors (e.g. https://github.com/openforcefield/openff-toolkit/issues/986). One intermediate solution could be to avoid subclassing ValueErrors where possible.

I’d think that downstream coders will get surprised by their excepts catching lines that they didn’t expect, and subsequently needing to dig into our exception hierarchy to figure out how to disentangle just the specific exception that they want to catch.

I personally would look at this from a user perspective. Keeping Toolkit-specific errors but subclassing generic ones still allows for approaches 1 and 2 above – they can still just import whichever specific error they want to catch by commenting out the try/except parts and letting the traceback tell them. However, it might minimise approach 3 (blank except) for the quick scripter by allowing them access to a generic class of “expected-things-going-wrong”, especially if they vaguely know which exception is going to be raised but can’t remember the very long name of the specific exception (e.g. FractionalBondOrderInterpolationMethodUnsupportedError)

After #1021,

* All exceptions are consolidating into a single file `openff/toolkit/utils/exceptions.py`

* There is now a base `OpenForceFieldToolkitException` class which all exceptions inherit from

If there’s going to be custom errors, it probably makes more sense for these to (also) inherit from the Python core library errors. E.g. MissingDependencyError in #1157 sounds a lot like an ImportError

As a smaller task, I think it would be nice to remove some of the assert lines in the non-testing parts of the toolkit. For example,

>>> from openff.toolkit.topology import Molecule
>>> top = Molecule.from_smiles("CCO").to_topology()
>>> top.atom(top.n_topology_atoms + 1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mwt/miniconda3/envs/openff-system/lib/python3.8/site-packages/openff/toolkit/topology/topology.py", line 2934, in atom
    assert 0 <= atom_topology_index < self.n_topology_atoms
AssertionError

this isn’t very useful, and could be replaced by a InvalidAtomIndexError with a descriptive message instead of displaying the internal check and forcing the user to do into what went wrong. (In my case I was just sloppy with atom indexing vs GROMACS, but the point remains)