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

Temporarily remove GAFF because of parmchk2 issues #329

Merged
merged 13 commits into from
May 3, 2024
Merged
37 changes: 15 additions & 22 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ jobs:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-latest]
python-version: ["3.9", "3.10"] # Add 3.11 in with AmberTools 23
exclude:
- python-version: "3.10"
os: macos-latest
python-version: ["3.10", "3.11", "3.12"]
amber-conversion: [false]
charmm-conversion: [false]

steps:
- uses: actions/checkout@v4
Expand All @@ -43,14 +42,6 @@ jobs:
create-args: >-
python=${{ matrix.python-version }}

- name: License OpenEye
shell: bash -l {0}
run: |
echo "${SECRET_OE_LICENSE}" > ${OE_LICENSE}
python -c "from openeye import oechem; assert oechem.OEChemIsLicensed()"
env:
SECRET_OE_LICENSE: ${{ secrets.OE_LICENSE }}

Comment on lines -46 to -53
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not needed for direct API access or exclusive functionality, it was just to get charge assignment running faster. This can be re-added at any time provided the org's license is managed, or NAGL could be used as a replacement for quicker charge assignment in tests

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will update the license, but I am thinking that I will (after this PR) add this section in but add some logic to only install openeye/check license if it is not a fork so CI can still be fast sometimes but a PR from an outside contributor doesn't just fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC, that's not necessary if the license is set up as an org secret. OpenFF's setup has worked great for a while; it pulls down the secret in a way that can't be cat'd out from a fork since it's not accessible at all. The slightly annoying thing is CI always fails from a fork, which I think is an okay price to pay for this level of security

https://github.com/openforcefield/openff-toolkit/blob/62b615cb6e3ea2561b869bbd70adaeea90fc04ea/.github/workflows/CI.yml#L87-L92

openforcefield/openff-interchange#962

- name: Install Package
run: |
pip list
Expand All @@ -66,29 +57,31 @@ jobs:

- name: Test Installed Package
run: |
pytest -v --log-cli-level $LOGLEVEL $COV_ARGS --durations=20 \
openmmforcefields/tests/test_amber_import.py \
openmmforcefields/tests/test_template_generators.py \
openmmforcefields/tests/test_system_generator.py \
-k "not TestEspalomaTemplateGenerator"
pytest -v -n logical --log-cli-level $LOGLEVEL $COV_ARGS --durations=20 \
openmmforcefields/tests/

# add --rungaff / --runespaloma to run those tests

env:
COV_ARGS: --cov=openmmforcefields --cov-config=setup.cfg --cov-append --cov-report=xml
LOGLEVEL: "INFO"
KMP_DUPLICATE_LIB_OK: "True"

- name: Test AMBER conversion
if: ${{ matrix.amber-conversion == 'true' }}
run: |
python convert_amber.py --input gaff.yaml --log gaff-tests.csv --verbose
working-directory: ./amber

- name: Test CHARMM conversion
if: ${{ matrix.charmm-conversion == 'true' }}
run: |

# TODO: Uncomment these tests when new ParmEd is released
# TODO: Turn on these tests (in job matrix) when new ParmEd is released
# TODO: Find a way to avoid timing out when running full charmm36.yaml conversion below
# python convert_charmm.py --verbose --in files/waters.yaml && python convert_charmm.py --verbose --in files/charmm36.yaml
# python convert_charmm.py --verbose --in files/waters.yaml
# python convert_charmm.py --verbose
python convert_charmm.py --verbose --in files/waters.yaml && python convert_charmm.py --verbose --in files/charmm36.yaml
python convert_charmm.py --verbose --in files/waters.yaml
python convert_charmm.py --verbose

# TODO: Uncomment this when tests are expected to work
# python test_charmm.py --verbose
Expand All @@ -107,4 +100,4 @@ jobs:
with:
token: ${{ secrets.CODECOV_TOKEN }}
file: ./coverage.xml
fail_ci_if_error: true
fail_ci_if_error: false # I don't want this to cause CI failures when a developer pushes to a fork
Copy link
Collaborator

Choose a reason for hiding this comment

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

      if: ${{ github.repository == 'openmm/openmmforcefields'
              && github.event != 'schedule' }}

I thought this logic would prevent that, but if it wasn't working then this change is great for the same reasons we got rid of the openeye license

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also expect this to skip it from being run from a fork 🤷‍♂️

19 changes: 0 additions & 19 deletions .github/workflows/dependabot_automerge.yml

This file was deleted.

66 changes: 0 additions & 66 deletions .github/workflows/espaloma_ci.yaml

This file was deleted.

7 changes: 2 additions & 5 deletions devtools/conda-envs/test_env.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ name: openmmforcefields-test

channels:
- conda-forge
- openeye

dependencies:
# Base depends
Expand All @@ -22,19 +21,17 @@ dependencies:

# Requirements for conversion tools
- pyyaml
- ambertools <23
- ambertools >=22,<24
- lxml
- networkx

# JSON caching of residue templates
- tinydb
# OpenEye toolkits are only used to speed up testing; they are not required for use
- openeye-toolkits

# Validating URLs
- validators

#
# Espaloma requirements below
#
- espaloma >=0.3.1
# espaloma >=0.3.1
4 changes: 2 additions & 2 deletions openmmforcefields/generators/system_generators.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class SystemGenerator:
postprocess_system : method
If not None, this method will be called as ``system = postprocess_system(system)`` to post-process the System object for create_system(topology) before it is returned.
"""
def __init__(self, forcefields=None, small_molecule_forcefield='openff-1.0.0', forcefield_kwargs=None, nonperiodic_forcefield_kwargs=None, periodic_forcefield_kwargs=None, template_generator_kwargs=None, barostat=None, molecules=None, cache=None, postprocess_system=None):
def __init__(self, forcefields=None, small_molecule_forcefield='openff-2.2.0', forcefield_kwargs=None, nonperiodic_forcefield_kwargs=None, periodic_forcefield_kwargs=None, template_generator_kwargs=None, barostat=None, molecules=None, cache=None, postprocess_system=None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not the update from (the first) Parsley to (the newest) Sage

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea! We will want to make sure we mention that in the changelog/release notes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea - I'm adding a couple of notes to the changelog section of the README

"""
This is a utility class to generate OpenMM Systems from Open Force Field Topology objects using AMBER
protein force fields and GAFF small molecule force fields.
Expand Down Expand Up @@ -206,7 +206,7 @@ def __init__(self, forcefields=None, small_molecule_forcefield='openff-1.0.0', f
_logger.debug(f'Trying {template_generator_cls.__name__} to load {small_molecule_forcefield}')
self.template_generator = template_generator_cls(forcefield=small_molecule_forcefield, cache=cache, template_generator_kwargs=self.template_generator_kwargs)
break
except ValueError as e:
except (ValueError, NotImplementedError) as e:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to catch the way I nuked GAFFTemplateGenerator, probably something that should be reverted alongside adding it back

_logger.debug(f' {template_generator_cls.__name__} cannot load {small_molecule_forcefield}')
_logger.debug(e)
if self.template_generator is None:
Expand Down
6 changes: 6 additions & 0 deletions openmmforcefields/generators/template_generators.py
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,12 @@ def __init__(self, molecules=None, forcefield=None, cache=None, **kwargs):

Newly parameterized molecules will be written to the cache, saving time next time!
"""
raise NotImplementedError(
"This release (0.13.x) of openmmforcefields temporarily drops GAFF support and "
"thereby the GAFFTemplateGenerator class. Support will be re-introduced in "
"future releases (0.14.x). To use this class, install version 0.12.0 or older."
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am happy with this error message

# Initialize molecules and cache
super().__init__(molecules=molecules, cache=cache)

Expand Down
23 changes: 21 additions & 2 deletions openmmforcefields/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,17 @@

def pytest_addoption(parser):
parser.addoption(
"--runespaloma", action="store_true", default=False, help="run espaloma tests"
"--runespaloma",
action="store_true",
default=False,
help="run espaloma tests",
)

parser.addoption(
"--rungaff",
action="store_true",
default=False,
help="run gaff tests",
)


Expand All @@ -18,4 +28,13 @@ def pytest_collection_modifyitems(config, items):
if not config.getoption("--runespaloma"):
for item in items:
if "espaloma" in item.keywords:
item.add_marker(skip_slow)
item.add_marker(skip_slow)

del skip_slow

skip_slow = pytest.mark.skip(reason="need --rungaff option to run")

if not config.getoption("--rungaff"):
for item in items:
if "gaff" in item.keywords:
item.add_marker(skip_slow)
23 changes: 13 additions & 10 deletions openmmforcefields/tests/test_system_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ def test_barostat(self):
assert force.getFrequency() == frequency

@pytest.mark.parametrize("small_molecule_forcefield", [
'gaff-2.11',
pytest.param('gaff-2.11', marks=pytest.mark.gaff),
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma)])
def test_create_with_template_generator(self, small_molecule_forcefield):
Expand All @@ -198,7 +198,7 @@ def test_create_with_template_generator(self, small_molecule_forcefield):
del generator

@pytest.mark.parametrize("small_molecule_forcefield", [
'gaff-2.11',
pytest.param('gaff-2.11', marks=pytest.mark.gaff),
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma)])
def test_forcefield_default_kwargs(self, small_molecule_forcefield, test_systems):
Expand Down Expand Up @@ -234,7 +234,7 @@ def test_forcefield_default_kwargs(self, small_molecule_forcefield, test_systems
assert forces['NonbondedForce'].getNonbondedMethod() == openmm.NonbondedForce.PME, "Expected LJPME, got {forces['NonbondedForce'].getNonbondedMethod()}"

@pytest.mark.parametrize("small_molecule_forcefield", [
'gaff-2.11',
pytest.param('gaff-2.11', marks=pytest.mark.gaff),
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma)])
def test_forcefield_kwargs(self, small_molecule_forcefield, test_systems):
Expand Down Expand Up @@ -280,7 +280,7 @@ def test_forcefield_kwargs(self, small_molecule_forcefield, test_systems):
'NonbondedForce'].getNonbondedMethod() == openmm.NonbondedForce.LJPME, "Expected LJPME, got {forces['NonbondedForce'].getNonbondedMethod()}"

@pytest.mark.parametrize("small_molecule_forcefield", [
'gaff-2.11',
pytest.param('gaff-2.11', marks=pytest.mark.gaff),
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma)])
def test_parameterize_molecules_from_creation(self, test_systems, small_molecule_forcefield):
Expand All @@ -307,7 +307,7 @@ def test_parameterize_molecules_from_creation(self, test_systems, small_molecule
assert (t2.interval() < t1.interval())

@pytest.mark.parametrize("small_molecule_forcefield", [
'gaff-2.11',
pytest.param('gaff-2.11', marks=pytest.mark.gaff),
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma)])
def test_parameterize_molecules_specified_during_create_system(self, test_systems, small_molecule_forcefield):
Expand All @@ -325,10 +325,13 @@ def test_parameterize_molecules_specified_during_create_system(self, test_system
# Specify molecules during system creation
system = generator.create_system(openmm_topology, molecules=molecules)

@pytest.mark.parametrize("small_molecule_forcefield", [
'gaff-2.11',
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma)])
@pytest.mark.parametrize(
"small_molecule_forcefield", [
pytest.param('gaff-2.11', marks=pytest.mark.gaff),
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma),
]
)
def test_add_molecules(self, test_systems, small_molecule_forcefield):
"""Test that Molecules can be added to SystemGenerator later"""
# Create a SystemGenerator for this force field
Expand All @@ -355,7 +358,7 @@ def test_add_molecules(self, test_systems, small_molecule_forcefield):
assert (t2.interval() < t1.interval())

@pytest.mark.parametrize("small_molecule_forcefield", [
'gaff-2.11',
pytest.param('gaff-2.11', marks=pytest.mark.gaff),
'openff-2.0.0',
pytest.param('espaloma-0.3.2', marks=pytest.mark.espaloma)])
def test_cache(self, test_systems, small_molecule_forcefield):
Expand Down
Loading
Loading