Disable NAGL as a partial charge backend when OEToolkit is installed but not chosen as the registry backend. (#1762)

* NAGL can no longer be used with oechem installed if you are using the rdkit backend.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Alyssa Travitz <31974495+atravitz@users.noreply.github.com>

news/disable_oechem_nagl.rst

@@ -0,0 +1,26 @@
+**Added:**
+
+* <news item>
+
+**Changed:**
+
+* <news item>
+
+**Deprecated:**
+
+* <news item>
+
+**Removed:**
+
+* <news item>
+
+**Fixed:**
+
+* Due to issues with OpenFF's handling of toolkit registries
+  with NAGL, the use of NAGL models (e.g. AshGC) when OpenEye
+  is installed but not requested as the charge backend has been
+  disabled (Issue #1760).
+
+**Security:**
+
+* <news item>

openfe/protocols/openmm_utils/charge_generation.py

@@ -403,6 +403,12 @@ def assign_offmol_partial_charges(
         errmsg = "OpenEye is not available and cannot be selected as a backend"
         raise ImportError(errmsg)
 
+    # Issue 1760
+    if HAS_OPENEYE and method.lower() == "nagl":
+        if toolkit_backend.lower() != "openeye":
+            errmsg = "OpenEye toolkit is installed but not used in the OpenFF toolkit registry backend. This is not possible with NAGL charges."
+            raise ValueError(errmsg)
+
     toolkits = ToolkitRegistry([i() for i in BACKEND_OPTIONS[toolkit_backend.lower()]])
 
     # We make a copy of the molecule since we're going to modify conformers

openfe/tests/protocols/openmm_abfe/test_abfe_slow.py

@@ -13,7 +13,7 @@ from openfe.protocols.openmm_utils.charge_generation import HAS_NAGL, HAS_OPENEY
 @pytest.mark.integration
 @pytest.mark.flaky(reruns=3)  # pytest-rerunfailures; we can get bad minimisation
 @pytest.mark.skipif(not HAS_NAGL, reason="need NAGL")
-@pytest.mark.xfail(
+@pytest.mark.skipif(
     HAS_OPENEYE and HAS_NAGL,
     reason="NAGL/openeye incompatibility. See https://github.com/openforcefield/openff-nagl/issues/177",
 )

openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py

@@ -537,8 +537,8 @@ def test_dry_run_solv_user_charges_benzene(benzene_modifications, protocol_dry_s
             "rdkit",
             "nagl",
             marks=pytest.mark.skipif(
-                not HAS_NAGL or sys.platform.startswith("darwin"),
-                reason="needs NAGL and/or on macos",
+                not HAS_NAGL or HAS_OPENEYE or sys.platform.startswith("darwin"),
+                reason="needs NAGL (without oechem) and/or on macos",
             ),
         ),
         pytest.param(

openfe/tests/protocols/openmm_md/test_plain_md_protocol.py

@@ -243,8 +243,8 @@ def test_dry_run_espaloma_vacuum_user_charges(benzene_modifications, vac_setting
             "rdkit",
             "nagl",
             marks=pytest.mark.skipif(
-                not HAS_NAGL or sys.platform.startswith("darwin"),
-                reason="needs NAGL and/or on macos",
+                not HAS_NAGL or HAS_OPENEYE or sys.platform.startswith("darwin"),
+                reason="needs NAGL (without oechem) and/or on macos",
             ),
         ),
         pytest.param(

openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py

@@ -634,8 +634,8 @@ def test_dry_run_ligand_system_cutoff(
             "rdkit",
             "nagl",
             marks=pytest.mark.skipif(
-                not HAS_NAGL or sys.platform.startswith("darwin"),
-                reason="needs NAGL and/or on macos",
+                not HAS_NAGL or HAS_OPENEYE or sys.platform.startswith("darwin"),
+                reason="needs NAGL (without oechem) and/or on macos",
             ),
         ),
         pytest.param(

openfe/tests/protocols/openmm_rfe/test_hybrid_top_slow.py

@@ -6,10 +6,11 @@ import numpy as np
 import pytest
 from gufe.protocols import execute_DAG
 from numpy.testing import assert_allclose
-from openff.units import unit
+from openff.units import unit as offunit
 
 import openfe
 from openfe.protocols import openmm_rfe
+from openfe.protocols.openmm_utils.charge_generation import HAS_NAGL, HAS_OPENEYE
 
 
 @pytest.mark.slow
@@ -28,14 +29,14 @@ def test_openmm_run_engine(
     # these settings are a small self to self sim, that has enough eq that
     # it doesn't occasionally crash
     s = openfe.protocols.openmm_rfe.RelativeHybridTopologyProtocol.default_settings()
-    s.simulation_settings.equilibration_length = 0.1 * unit.picosecond
-    s.simulation_settings.production_length = 0.1 * unit.picosecond
-    s.simulation_settings.time_per_iteration = 20 * unit.femtosecond
+    s.simulation_settings.equilibration_length = 0.1 * offunit.picosecond
+    s.simulation_settings.production_length = 0.1 * offunit.picosecond
+    s.simulation_settings.time_per_iteration = 20 * offunit.femtosecond
     s.forcefield_settings.nonbonded_method = "nocutoff"
     s.protocol_repeats = 1
     s.engine_settings.compute_platform = platform
-    s.output_settings.checkpoint_interval = 20 * unit.femtosecond
-    s.output_settings.positions_write_frequency = 20 * unit.femtosecond
+    s.output_settings.checkpoint_interval = 20 * offunit.femtosecond
+    s.output_settings.positions_write_frequency = 20 * offunit.femtosecond
 
     p = openmm_rfe.RelativeHybridTopologyProtocol(s)
 
@@ -110,6 +111,11 @@ def test_openmm_run_engine(
 
 @pytest.mark.integration  # takes ~7 minutes to run
 @pytest.mark.flaky(reruns=3)
+@pytest.mark.skipif(not HAS_NAGL, reason="need NAGL")
+@pytest.mark.skipif(
+    HAS_OPENEYE and HAS_NAGL,
+    reason="NAGL/openeye incompatibility. See https://github.com/openforcefield/openff-nagl/issues/177",
+)
 def test_run_eg5_sim(eg5_protein, eg5_ligands, eg5_cofactor, tmpdir):
     # this runs a very short eg5 complex leg
     # different to previous test:
@@ -118,11 +124,14 @@ def test_run_eg5_sim(eg5_protein, eg5_ligands, eg5_cofactor, tmpdir):
     # - runs in solvated protein
     # if this passes 99.9% chance of a good time
     s = openfe.protocols.openmm_rfe.RelativeHybridTopologyProtocol.default_settings()
-    s.simulation_settings.equilibration_length = 0.1 * unit.picosecond
-    s.simulation_settings.production_length = 0.1 * unit.picosecond
-    s.simulation_settings.time_per_iteration = 20 * unit.femtosecond
+    s.simulation_settings.equilibration_length = 0.1 * offunit.picosecond
+    s.simulation_settings.production_length = 0.1 * offunit.picosecond
+    s.simulation_settings.time_per_iteration = 20 * offunit.femtosecond
+    s.forcefield_settings.nonbonded_cutoff = 0.8 * offunit.nanometer
+    s.partial_charge_settings.partial_charge_method = "nagl"
+    s.partial_charge_settings.nagl_model = "openff-gnn-am1bcc-0.1.0-rc.3.pt"
     s.protocol_repeats = 1
-    s.output_settings.checkpoint_interval = 20 * unit.femtosecond
+    s.output_settings.checkpoint_interval = 20 * offunit.femtosecond
 
     p = openmm_rfe.RelativeHybridTopologyProtocol(s)
 
@@ -158,13 +167,13 @@ def test_run_dodecahedron_sim(benzene_system, toluene_system, benzene_to_toluene
     Test that we can run a ligand in solvent RFE with a non-cubic box
     """
     settings = openmm_rfe.RelativeHybridTopologyProtocol.default_settings()
-    settings.solvation_settings.solvent_padding = 1.5 * unit.nanometer
+    settings.solvation_settings.solvent_padding = 1.5 * offunit.nanometer
     settings.solvation_settings.box_shape = "dodecahedron"
     settings.protocol_repeats = 1
-    settings.simulation_settings.equilibration_length = 0.1 * unit.picosecond
-    settings.simulation_settings.production_length = 0.1 * unit.picosecond
-    settings.simulation_settings.time_per_iteration = 20 * unit.femtosecond
-    settings.output_settings.checkpoint_interval = 20 * unit.femtosecond
+    settings.simulation_settings.equilibration_length = 0.1 * offunit.picosecond
+    settings.simulation_settings.production_length = 0.1 * offunit.picosecond
+    settings.simulation_settings.time_per_iteration = 20 * offunit.femtosecond
+    settings.output_settings.checkpoint_interval = 20 * offunit.femtosecond
     protocol = openmm_rfe.RelativeHybridTopologyProtocol(settings=settings)
 
     dag = protocol.create(

openfe/tests/protocols/test_openmmutils.py

@@ -826,7 +826,9 @@ class TestOFFPartialCharge:
         # but the charges should have
         assert not np.allclose(charges, lig.partial_charges)
 
-    @pytest.mark.skipif(not HAS_NAGL, reason="NAGL is not available")
+    @pytest.mark.skipif(
+        not HAS_NAGL or HAS_OPENEYE, reason="NAGL is not available or oechem is installed"
+    )
     def test_latest_production_nagl(self, uncharged_mol):
         """We expect to find a NAGL model and be able to generate partial charges with it."""
         charge_generation.assign_offmol_partial_charges(
@@ -839,7 +841,9 @@ class TestOFFPartialCharge:
         )
         assert uncharged_mol.partial_charges.units == "elementary_charge"
 
-    @pytest.mark.skipif(not HAS_NAGL, reason="NAGL is not available")
+    @pytest.mark.skipif(
+        not HAS_NAGL or HAS_OPENEYE, reason="NAGL is not available or oechem is installed"
+    )
     def test_no_production_nagl(self, uncharged_mol):
         """Cleanly handle the case where a NAGL model isn't found."""
         with mock.patch(
@@ -887,8 +891,8 @@ class TestOFFPartialCharge:
                 "nagl",
                 None,
                 marks=pytest.mark.skipif(
-                    not HAS_NAGL or sys.platform.startswith("darwin"),
-                    reason="needs NAGL and/or on macos",
+                    not HAS_NAGL or HAS_OPENEYE or sys.platform.startswith("darwin"),
+                    reason="needs NAGL (without oechem) and/or on macos",
                 ),
             ),
             pytest.param(
@@ -897,8 +901,8 @@ class TestOFFPartialCharge:
                 "nagl",
                 None,
                 marks=pytest.mark.skipif(
-                    not HAS_NAGL or sys.platform.startswith("darwin"),
-                    reason="needs NAGL and/or on macos",
+                    not HAS_NAGL or HAS_OPENEYE or sys.platform.startswith("darwin"),
+                    reason="needs NAGL (without oechem) and/or on macos",
                 ),
             ),
             pytest.param(
@@ -961,6 +965,22 @@ class TestOFFPartialCharge:
             rtol=1e-4,
         )
 
+    @pytest.mark.skipif(not HAS_OPENEYE, reason="OEToolkit is not available")
+    def test_nagl_oechem_not_openeye_error(self, uncharged_mol):
+        errmsg = "OpenEye toolkit is installed but not used in the OpenFF toolkit registry."
+        with pytest.raises(ValueError, match=errmsg):
+            charge_generation.assign_offmol_partial_charges(
+                uncharged_mol,
+                overwrite=False,
+                method="nagl",
+                toolkit_backend="rdkit",
+                generate_n_conformers=None,
+                nagl_model=None,
+            )
+
+    @pytest.mark.skipif(
+        HAS_OPENEYE, reason="NAGL does not work with OpenEye when using the rdkit backend"
+    )
     def test_nagl_import_error(self, monkeypatch, uncharged_mol):
         monkeypatch.setattr(
             sys.modules["openfe.protocols.openmm_utils.charge_generation"],

openfecli/tests/commands/test_charge_generation.py

@@ -9,6 +9,10 @@ from openff.toolkit import Molecule
 from openff.units import unit
 from openff.utilities.testing import skip_if_missing
 
+from openfe.protocols.openmm_utils.charge_generation import (
+    HAS_NAGL,
+    HAS_OPENEYE,
+)
 from openfecli.commands.generate_partial_charges import charge_molecules
 
 
@@ -122,8 +126,13 @@ def test_charge_molecules_overwrite(
         pytest.param(2, id="2"),
     ],
 )
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_charge_settings(methane, tmpdir, caplog, yaml_nagl_settings, ncores):
     runner = CliRunner()
     mol_path = tmpdir / "methane.sdf"

openfecli/tests/commands/test_plan_rbfe_network.py

@@ -10,7 +10,10 @@ from gufe import AlchemicalNetwork, SmallMoleculeComponent
 from openff.units import unit
 from openff.utilities import skip_if_missing
 
-from openfe.protocols.openmm_utils.charge_generation import HAS_OPENEYE
+from openfe.protocols.openmm_utils.charge_generation import (
+    HAS_NAGL,
+    HAS_OPENEYE,
+)
 from openfecli.commands.plan_rbfe_network import (
     plan_rbfe_network,
     plan_rbfe_network_main,
@@ -70,8 +73,13 @@ def validate_charges(smc):
     assert len(off_mol.partial_charges) == off_mol.n_atoms
 
 
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_plan_rbfe_network_main():
     from gufe import (
         ProteinComponent,
@@ -137,8 +145,13 @@ partial_charge:
 """
 
 
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_plan_rbfe_network(mol_dir_args, protein_args, tmpdir, yaml_nagl_settings):
     """
     smoke test
@@ -218,8 +231,13 @@ def test_plan_rbfe_network_n_repeats(mol_dir_args, protein_args, input_n_repeat,
         pytest.param(False, id="No overwrite"),
     ],
 )
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_plan_rbfe_network_charge_overwrite(dummy_charge_dir_args, protein_args, tmpdir, yaml_nagl_settings, overwrite):  # fmt: skip
     # make sure the dummy charges are overwritten when requested
 
@@ -268,9 +286,13 @@ def eg5_files():
         yield pdb_path, lig_path, cof_path
 
 
-@pytest.mark.xfail(HAS_OPENEYE, reason="openff-nagl#177")
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_plan_rbfe_network_cofactors(eg5_files, tmpdir, yaml_nagl_settings):
     # use nagl charges for CI speed!
     settings_path = tmpdir / "settings.yaml"
@@ -372,8 +394,13 @@ partial_charge:
 """
 
 
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_lomap_yaml_plan_rbfe_smoke_test(lomap_yaml_settings, cdk8_files, tmpdir):
     protein, ligand = cdk8_files
     settings_path = tmpdir / "settings.yaml"
@@ -413,9 +440,13 @@ partial_charge:
 """
 
 
-@pytest.mark.xfail(HAS_OPENEYE, reason="openff-nagl#177")
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_custom_yaml_plan_radial_smoke_test(custom_yaml_radial, eg5_files, tmpdir):
     protein, ligand, cofactor = eg5_files
     settings_path = tmpdir / "settings.yaml"

openfecli/tests/commands/test_plan_rhfe_network.py

@@ -10,6 +10,10 @@ from gufe import AlchemicalNetwork, SmallMoleculeComponent, SolventComponent
 from gufe.tokenization import JSON_HANDLER
 from openff.utilities.testing import skip_if_missing
 
+from openfe.protocols.openmm_utils.charge_generation import (
+    HAS_NAGL,
+    HAS_OPENEYE,
+)
 from openfecli.commands.plan_rhfe_network import (
     plan_rhfe_network,
     plan_rhfe_network_main,
@@ -54,8 +58,13 @@ def validate_charges(smc):
     assert len(off_mol.partial_charges) == off_mol.n_atoms
 
 
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_plan_rhfe_network_main():
     from openfe.protocols.openmm_utils.omm_settings import OpenFFPartialChargeSettings
     from openfe.setup import (
@@ -102,8 +111,13 @@ partial_charge:
 """
 
 
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_plan_rhfe_network(mol_dir_args, tmpdir, yaml_nagl_settings):
     """
     smoke test
@@ -175,8 +189,13 @@ partial_charge:
 """
 
 
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_custom_yaml_plan_rhfe_smoke_test(custom_yaml_settings, mol_dir_args, tmpdir):
     settings_path = tmpdir / "settings.yaml"
     with open(settings_path, "w") as f:
@@ -201,8 +220,13 @@ def test_custom_yaml_plan_rhfe_smoke_test(custom_yaml_settings, mol_dir_args, tm
         pytest.param(False, id="No overwrite"),
     ],
 )
-@skip_if_missing("openff.nagl")
-@skip_if_missing("openff.nagl_models")
+@pytest.mark.skipif(
+    not HAS_NAGL,
+    reason="needs NAGL",
+)
+@pytest.mark.skipif(
+    HAS_OPENEYE, reason="cannot use NAGL with rdkit backend when OpenEye is installed"
+)
 def test_plan_rhfe_network_charge_overwrite(dummy_charge_dir_args, tmpdir, yaml_nagl_settings, overwrite):  # fmt: skip
     # make sure the dummy charges are overwritten when requested