Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helper function to signify optional dependencies, soften serialization deps #794

Merged
merged 8 commits into from Jan 5, 2021

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Dec 18, 2020

Resolves #772 - but might be best to hold off merging until the conda-forge migration is complete and the dust has settled.

  • Reach agreement on which packages should be considered optional (I think all of below should be)
    • smirnoff99frosst
    • mdtraj
    • nglview
    • toml
    • yaml
    • bson
    • xmltodict
    • msgpack
    • openmmtools
  • Tag issue being addressed
  • Add tests
  • Update docstrings/documentation, if applicable
  • Lint codebase
  • Update changelog

Heres's what it looks like at the moment (verbose but instructive; will wrap these into tests):

>>> from openforcefield.utils.utils import MissingDependencyError, requires_package
>>> raise MissingDependencyError('mypy')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
openforcefield.utils.utils.MissingDependencyError: Missing dependency mypy. Try installing it with

$ conda install mypy -c conda-forge
>>> raise MissingDependencyError('openforcefields')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
openforcefield.utils.utils.MissingDependencyError: Missing dependency openforcefields. Try installing it with

$ conda install openforcefields -c conda-forge -c omnia
>>> @requires_package('foobar')
... def fn():
...     pass
...
>>> fn()
Traceback (most recent call last):
  File "/Users/mwt/software/openforcefield/openforcefield/utils/utils.py", line 101, in wrapper
    importlib.import_module(package_name)
  File "/Users/mwt/miniconda3/envs/openff-dev/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 973, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'foobar'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mwt/software/openforcefield/openforcefield/utils/utils.py", line 103, in wrapper
    raise MissingDependencyError(package_name)
openforcefield.utils.utils.MissingDependencyError: Missing dependency foobar. Try installing it with

$ conda install foobar -c conda-forge
>>> from openforcefield.topology import Molecule
/Users/mwt/software/openforcefield/openforcefield/topology/molecule.py:4120: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  lambda frame: f'{self.name if self.name is not "" else self.hill_formula}{frame}\n'
/Users/mwt/software/openforcefield/openforcefield/topology/molecule.py:4125: SyntaxWarning: "is not" with a literal. Did you mean "!="?
  lambda frame: f'{self.name if self.name is not "" else self.hill_formula} Frame {frame}\n'
>>> mol = Molecule.from_smiles("CC")
>>> mol.to_qcschema()
Traceback (most recent call last):
  File "/Users/mwt/software/openforcefield/openforcefield/utils/utils.py", line 101, in wrapper
    importlib.import_module(package_name)
  File "/Users/mwt/miniconda3/envs/openff-dev/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 973, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'qcelemental'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mwt/software/openforcefield/openforcefield/utils/utils.py", line 103, in wrapper
    raise MissingDependencyError(package_name)
openforcefield.utils.utils.MissingDependencyError: Missing dependency qcelemental. Try installing it with

$ conda install qcelemental -c conda-forge

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #794 (64288d2) into master (a148678) will increase coverage by 0.03%.
The diff coverage is 88.23%.

@mattwthompson mattwthompson marked this pull request as ready for review December 18, 2020 16:13
Copy link
Member

@j-wags j-wags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and works well on a technical level. I think that mdtraj and xmltodict shouldn't be decorated using the new decorator, so if my proposed changes about undecorating those are good, this is ready for merge. Please add this as a "new feature" in the release history.

openforcefield/topology/topology.py Outdated Show resolved Hide resolved
openforcefield/utils/serialization.py Outdated Show resolved Hide resolved
openforcefield/utils/serialization.py Outdated Show resolved Hide resolved
Comment on lines +86 to +88
if package_name in ["openforcefields", "smirnoff99frosst"]:
self.msg += " -c omnia"
super().__init__(self.msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@lgtm-com
Copy link

lgtm-com bot commented Jan 5, 2021

This pull request introduces 1 alert when merging d0197fe into a148678 - view on LGTM.com

new alerts:

  • 1 for Unused import

@mattwthompson mattwthompson merged commit 988c6d6 into master Jan 5, 2021
@mattwthompson mattwthompson deleted the serialization-deps branch May 25, 2021 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make non-standard serialization dependencies optional
2 participants