Temporarily remove GAFF because of parmchk2 issues (#329)

* Temporarily remove GAFF because of parmchk2 issues * Drop OpenEye from tests * Drop automerge action * Update Python and AmberTools versions, run tests in parallel * Remove Espaloma from test environment * Remove Espaloma CI * Refactor some tests to enable skipping GAFF * Skip SPC/E test when iterating over small molecule force fields * Update default force field in `SystemGenerator` * Streamline testing * Update testing setup * Mask CodeCov upload failures * Add release notes
openmm · May 3, 2024 · b685637 · b685637
1 parent 7149bc0
commit b685637
Show file tree

Hide file tree

Showing 10 changed files with 213 additions and 263 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -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
@@ -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 }}
-
     - name: Install Package
       run: |
         pip list
@@ -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
@@ -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
diff --git a/.github/workflows/dependabot_automerge.yml b/.github/workflows/dependabot_automerge.yml
diff --git a/.github/workflows/espaloma_ci.yaml b/.github/workflows/espaloma_ci.yaml
diff --git a/README.md b/README.md
@@ -337,6 +337,17 @@ See the corresponding directories for information on how to use the provided con
 
 # [Changelog](https://github.com/openmm/openmmforcefields/releases)
 
+## 0.13.0 Temporarily remove GAFFTemplateGenerator
+
+This release temporarily removes GAFFTemplateGenerator because of packaging incompatibilities with
+AmberTools 23. This functionality is planned to be re-introduced in 0.14.0.
+
+This release is expected to work with Python 3.10-3.12.
+
+Other changes include
+* The default force field of `SystemGenerator` was updated from `openff-1.0.0` (code name Parsley) to
+  `openff-2.0.0` (code name Sage).
+
 ## 0.12.0 Updates for espaloma and support a offxml string in SystemGenerator
 See our [0.12.0 release page](https://github.com/openmm/openmmforcefields/releases/tag/0.12.0) for more details.
 

diff --git a/devtools/conda-envs/test_env.yaml b/devtools/conda-envs/test_env.yaml
@@ -2,7 +2,6 @@ name: openmmforcefields-test
 
 channels:
   - conda-forge
-  - openeye
 
 dependencies:
     # Base depends
@@ -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
diff --git a/openmmforcefields/generators/system_generators.py b/openmmforcefields/generators/system_generators.py
@@ -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):
         """
         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.
@@ -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:
                     _logger.debug(f'  {template_generator_cls.__name__} cannot load {small_molecule_forcefield}')
                     _logger.debug(e)
             if self.template_generator is None:

diff --git a/openmmforcefields/generators/template_generators.py b/openmmforcefields/generators/template_generators.py
@@ -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."
+        )
+
         # Initialize molecules and cache
         super().__init__(molecules=molecules, cache=cache)
 

diff --git a/openmmforcefields/tests/conftest.py b/openmmforcefields/tests/conftest.py
@@ -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",
     )
 
 
@@ -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)
diff --git a/openmmforcefields/tests/test_system_generator.py b/openmmforcefields/tests/test_system_generator.py
@@ -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):
@@ -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):
@@ -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):
@@ -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):
@@ -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):
@@ -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
@@ -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):