diff --git a/news/disable_oechem_nagl.rst b/news/disable_oechem_nagl.rst new file mode 100644 index 000000000..fc73b3e82 --- /dev/null +++ b/news/disable_oechem_nagl.rst @@ -0,0 +1,26 @@ +**Added:** + +* + +**Changed:** + +* + +**Deprecated:** + +* + +**Removed:** + +* + +**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:** + +* diff --git a/openfe/protocols/openmm_utils/charge_generation.py b/openfe/protocols/openmm_utils/charge_generation.py index 2dcdd0edd..ad3697ff2 100644 --- a/openfe/protocols/openmm_utils/charge_generation.py +++ b/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 diff --git a/openfe/tests/protocols/openmm_abfe/test_abfe_slow.py b/openfe/tests/protocols/openmm_abfe/test_abfe_slow.py index aa714766e..c4c64bd76 100644 --- a/openfe/tests/protocols/openmm_abfe/test_abfe_slow.py +++ b/openfe/tests/protocols/openmm_abfe/test_abfe_slow.py @@ -13,7 +13,7 @@ @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", ) diff --git a/openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py b/openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py index 206ff98c7..cf471130a 100644 --- a/openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py +++ b/openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py @@ -537,8 +537,8 @@ def assign_fictitious_charges(offmol): "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( diff --git a/openfe/tests/protocols/openmm_md/test_plain_md_protocol.py b/openfe/tests/protocols/openmm_md/test_plain_md_protocol.py index f7dddfb03..60c7e8c47 100644 --- a/openfe/tests/protocols/openmm_md/test_plain_md_protocol.py +++ b/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( diff --git a/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py b/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py index 283104e52..60045bcc0 100644 --- a/openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py +++ b/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( diff --git a/openfe/tests/protocols/openmm_rfe/test_hybrid_top_slow.py b/openfe/tests/protocols/openmm_rfe/test_hybrid_top_slow.py index a8875142d..0cd31b08c 100644 --- a/openfe/tests/protocols/openmm_rfe/test_hybrid_top_slow.py +++ b/openfe/tests/protocols/openmm_rfe/test_hybrid_top_slow.py @@ -6,10 +6,11 @@ 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( diff --git a/openfe/tests/protocols/test_openmmutils.py b/openfe/tests/protocols/test_openmmutils.py index 15310c6b6..9511f2bcd 100644 --- a/openfe/tests/protocols/test_openmmutils.py +++ b/openfe/tests/protocols/test_openmmutils.py @@ -826,7 +826,9 @@ def test_am1bcc_conformer_nochange(self, eg5_ligands): # 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 @@ def test_latest_production_nagl(self, uncharged_mol): ) 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 @@ def test_no_production_nagl(self, uncharged_mol): "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 @@ def test_no_production_nagl(self, uncharged_mol): "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 @@ def test_am1bcc_reference( 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"], diff --git a/openfecli/tests/commands/test_charge_generation.py b/openfecli/tests/commands/test_charge_generation.py index db843c305..ae481cf44 100644 --- a/openfecli/tests/commands/test_charge_generation.py +++ b/openfecli/tests/commands/test_charge_generation.py @@ -9,6 +9,10 @@ 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" diff --git a/openfecli/tests/commands/test_plan_rbfe_network.py b/openfecli/tests/commands/test_plan_rbfe_network.py index dda23dac2..ed3805123 100644 --- a/openfecli/tests/commands/test_plan_rbfe_network.py +++ b/openfecli/tests/commands/test_plan_rbfe_network.py @@ -10,7 +10,10 @@ 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 @@ def yaml_nagl_settings(): """ -@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 @@ def lomap_yaml_settings(): """ -@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 @@ def custom_yaml_radial(): """ -@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" diff --git a/openfecli/tests/commands/test_plan_rhfe_network.py b/openfecli/tests/commands/test_plan_rhfe_network.py index e7de92629..a3780d79c 100644 --- a/openfecli/tests/commands/test_plan_rhfe_network.py +++ b/openfecli/tests/commands/test_plan_rhfe_network.py @@ -10,6 +10,10 @@ 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 @@ def yaml_nagl_settings(): """ -@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 @@ def custom_yaml_settings(): """ -@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