Exception rework part 1

[mattwthompson]

A step toward resolving #771, specifically #771 (comment)

The process was

• search through the big files and find custom exceptions that were defined alongside functional code; move them to openff/toolkit/utils/exceptions.py
• %s/MessageException/OpenFFToolkitException
• Fix imports in tests
• Fix some definitions in __all__s
• Wrestled with multiple ParseError definitions

1. API isn't broken (with a small caveat and exception below) since any submodule that uses an exception that was moved from it to exceptions.py should be importing it anyway.
2. Actually implementing a deprecation warning is tricky; putting it in the body of the class will cause it to be triggered when anything from the module is imported (unworkable IMO - running the README example will trigger this despite not importing it) but tucking it away in the __init__ means it will be triggered only if MessageException is raised, but not when it's imported. I went with the second option since I couldn't find a way to throw the warning when it's imported but not when other exceptions are.
3. It turns out there are two ParseError in the code! One is for failing to parse SMIRNOFF source, the other is for when a wrapped toolkit fails to parse a SMILES string. I've renamed each to be more descriptive, deprecated the SMIRNOFF one in a way that shouldn't break existing code, and removed the SMILES one as I think it's less likely that somebody is capturing that specific exception.
4. A subtlety with the imports is that, as a side effect of using star imports in openff/toolkit/typing/engines/smirnoff/__init__.py, the exceptions imported into parameters.py and forcefield.py must be defined in the __all__ of those files for i.e. from openff.toolkit.typing.engines.smirnoff import UnassignedValenceParameterException to work, since that import path goes through the star import (from .parameters import *). from openff.toolkit.typing.engines.smirnoff.parameters import UnassignedValenceParameterException works no matter what's in the __all__, since that was imported into that module. This is a side effect of using star imports.

• Tag issue being addressed
• Add tests
• Update docstrings/documentation, if applicable
• Lint codebase
• Update changelog

[codecov]

Codecov Report

Attention: Patch coverage is 94.01709% with 7 lines in your changes missing coverage. Please review.

Project coverage is 86.82%. Comparing base (f417161) to head (c7a2f7c).
Report is 98 commits behind head on master.

Additional details and impacted files

[lilyminium]

Actually implementing a deprecation warning is tricky; putting it in the body of the class will cause it to be triggered when anything from the module is imported (unworkable IMO - running the README example will trigger this despite not importing it) but tucking it away in the init means it will be triggered only if MessageException is raised, but not when it's imported. I went with the second option since I couldn't find a way to throw the warning when it's imported but not when other exceptions are.

Which Python versions are you aiming for? The return of the module __getattr__ is super handy for this, and an example is even given in the rationale. It's 3.7+

https://www.python.org/dev/peps/pep-0562/#id8

[mattwthompson]

Wow - that's exactly what isneeded here. Thanks @lilyminium!

Now I get pretty much the behavior I want - a relevant warning emitted when a deprecated exception is imported, but nothing when not:

$ python -c "from openff.toolkit.utils.exceptions import AntechamberNotFoundError"  # No warnings!
$ python -c "from openff.toolkit.utils.exceptions import MessageException" 
/Users/mwt/software/openforcefield/openff/toolkit/utils/exceptions.py:16: DeprecationWarning: MessageException is DEPRECATED and will be removed in a future release of the OpenFF Toolkit. All custom exceptions now inherit from OpenFFToolkitException, which should be used as a replacement for MessageException. Import it via `from openff.toolkit.utils.exceptions import OpenFFToolkitException`.
  warnings.warn(warning_msg, DeprecationWarning)

One small downside of "deprecating" the ParseError and not just removing it is that, because I inherit SMIRNOFFParseError from it, that gets spat out when ForceField is loaded. Not sure if that inheritance shouldn't happen - I did it so that existing code can catch ParseError in a deprecation period.

$ python -c "from openff.toolkit.utils.exceptions import ParseError" 
/Users/mwt/software/openforcefield/openff/toolkit/utils/exceptions.py:16: DeprecationWarning: ParseError is DEPRECATED and will be removed in a future release of the OpenFF Toolkit.
  warnings.warn(warning_msg, DeprecationWarning)
$ python -c "from openff.toolkit.typing.engines.smirnoff import ForceField"
/Users/mwt/software/openforcefield/openff/toolkit/utils/exceptions.py:16: DeprecationWarning: ParseError is DEPRECATED and will be removed in a future release of the OpenFF Toolkit.
  warnings.warn(warning_msg, DeprecationWarning)

[mattwthompson]

It's 3.7+

Oh, and we're pretty much 3.7+ at this point. We'll likely support 3.7 for a while, so 3.8+ features that aren't backported by a third party are more or less off limits. Our 3.6 status is not officially supported but unofficially still exists (conda-forge/openff-toolkit-feedstock#11). Nothing's broken that we're aware of, and we're mostly not testing it anymore.

[lilyminium]

Yay!

One small downside of "deprecating" the ParseError and not just removing it is that, because I inherit SMIRNOFFParseError from it, that gets spat out when ForceField is loaded.

Yeah, that's a real downside of star imports -- I'm guessing you're still import ParseErroring because users expect it in all sorts of namespaces and import __all__? Otherwise you could try propagating the __getattr__, although I can't currently think of a way to do it non-repetitively. You can dynamically set __getattr__ on module imports in __init__ though.

Defined namespaces would make this much easier: from openff.toolkit.utils import exceptions; raise exceptions.ParseError()

Oh, and we're pretty much 3.7+ at this point.

Thanks, good to know. That's pretty on-pace with numpy.

[mattwthompson]

I'm guessing you're still import ParseErroring because users expect it in all sorts of namespaces and import __all__?

Pretty much - i.e. this notebook expects it to be there

from openff.toolkit.typing.engines.smirnoff import (ForceField, 
UnassignedValenceParameterException, BondHandler, AngleHandler,
ProperTorsionHandler, ImproperTorsionHandler,
vdWHandler)

In a perfect world I wouldn't want code like this to exist after a change, but as things stand right now it's valuable to keep kinda-old code working when there's not a large feature associated with the API break.

[adalke]

The ParseError for SMILES was my doing. It was added a few weeks ago in #1006 . Previously, parse errors were ignored. I didn't realize there was a SMIRNOFF ParseError and didn't think to check because it wasn't in exceptions.py! So, definitely needed. :)

In any case, there shouldn't be anyone using that SMILES exception yet.

[lilyminium]

I'm curious what additional functionality this adds to Exception? Is it having the .msg attribute?

>>> exc = ValueError("my message")
>>> str(exc)
'my message'
>>> exc.args
('my message',)

[mattwthompson]

Pretty much, yeah. I could go either way on this; I recall seeing/using some code that relies on .msg existing, but I couldn't find it with a quick search. I might prefer the aesthetic of clearly having a message attribute over hoping that calling str() on it. I think that should always work, but I vaguely recall being bitten by cases in which that isn't true.

[lilyminium]

It'll give you functional differences if msg ever happens to not be a string, I'm not sure if that'd affect scripts and things. OFFTKExc.msg would just naively return 3, I assume.

>>> exc = ValueError(3)
>>> str(exc)
'3'
>>> exc.args
(3,)

Normal exceptions also take in multiple arguments.

>>> exc2 = ValueError("This is probably good for complex errors", "but I've never seen anyone use it", 3)
>>> str(exc2)
'(\'This is probably good for complex errors\', "but I\'ve never seen anyone use it", 3)'
>>> exc2.args
('This is probably good for complex errors', "but I've never seen anyone use it", 3)
>>> raise exc2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: ('This is probably good for complex errors', "but I've never seen anyone use it", 3)

[mattwthompson]

Hmm, I don't know why I dropped the self compared to the same code in MessageException; it looks out of place, but I probably did it at some point to get an ephemeral block of code to do what I wanted. The inside of OpenFFToolkitException is otherwise copied from MessageException, I didn't put care into thinking if that was wise.

What I'm gathering from this code is that ... overriding __init__ and __str__ is unnecessary? Whether writing tests or being a user of the toolkit, I'd be ok with patterns like

try:
    do_toolkit_thing(...)
except SomeError as e:
    if "foo" in str(e):
        ...
    elif "bar" in str(e):
        ...

which is functionally the same as if "foo" in e.msg but doesn't override anything while subclassing out of the stdlib

[lilyminium]

Yeah, as a user I would assume that your exceptions work the same as the standard ones and hop straight to str(e) anyway. Do you publically advertise e.msg?

[mattwthompson]

In any case, there shouldn't be anyone using that SMILES exception yet.

I opened #1023 to hopefully make that change before the next release, which may be v0.10.0. (The rest of this PR is aimed at a something later).

I realize this adds an extra moving piece, I think it's worthwhile.

[mattwthompson]

I've merged #1023, which should resolve the duplicate ParseError issue.

There should be a flurry of upstream changes in the next couple days while 0.10.0 is prepped; I'm going to slow down or hold off on moving this PR forward until everything has settled down; hopefully in just a couple of days.

[mattwthompson]

Thanks for the review @lilyminium! Sorry about all the typos

[lgtm-com]

This pull request introduces 2 alerts and fixes 1 when merging e7d5edf into d78912b - view on LGTM.com

new alerts:

• 1 for Unused import
• 1 for Missing call to `__init__` during object initialization

fixed alerts:

• 1 for Unused import

[lilyminium]

The comment was more about the []() linking, where the link isn't a real URL but a qualified path name. Does that work in rendered docs anywhere?

[lgtm-com]

This pull request introduces 2 alerts and fixes 1 when merging a477843 into d78912b - view on LGTM.com

new alerts:

• 1 for Unused import
• 1 for Missing call to `__init__` during object initialization

fixed alerts:

• 1 for Unused import

[mattwthompson]

Left to do:

• Merge master in
• SMIRNOFFParsingError -> SMIRNOFFParseError / Ensure consistency of above (particularly in docstrings)
• Make all exceptions import from OpenFFToolkitException, or have a really good reason for not doing so
• Make OpenFFToolkitException import from _DeprecatedMessageException, which allows for current code to emit a warning instead of breaking
• Make notes about deprecation plan (done)
• Do a once-over on API breaks before merging

[j-wags]

LGTM pending the to-dos in @mattwthompson's message above.

[mattwthompson]

I noticed that I broke the API in one place that could cause issues for users who bend the rules. Around cb5dfc0 the behavior is

>>> from openff.toolkit.utils.toolkits import OpenEyeToolkitWrapper
>>> from openff.toolkit.utils.toolkits import MessageException
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name 'MessageException' from 'openff.toolkit.utils.toolkits' (/Users/mwt/software/openforcefield/openff/toolkit/utils/toolkits.py)

which is not ideal.

I think 85a00ba fixes this, without emitting the warning for safer imports:

>>> from openff.toolkit.utils.toolkits import OpenEyeToolkitWrapper
>>> from openff.toolkit.utils.toolkits import MessageException
/Users/mwt/software/openforcefield/openff/toolkit/utils/exceptions.py:18: DeprecationWarning: MessageException is DEPRECATED and will be removed in a future release of the OpenFF Toolkit. All custom exceptions now inherit from OpenFFToolkitException, which should be used as a replacement for MessageException. Import it via `from openff.toolkit.utils.exceptions import OpenFFToolkitException`.
  warnings.warn(warning_msg, DeprecationWarning)

[lgtm-com]

This pull request fixes 1 alert when merging c7a2f7c into f417161 - view on LGTM.com

fixed alerts:

• 1 for Unused import

[j-wags]

Good catch, and nice fix @mattwthompson. Thanks for the attention to detail!