Add GAFF test (and GAFF skips) to CI

mikemhenry commented 2 months ago

Checklist

[ ] Added a news entry

Developers certificate of origin

[ ] I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

mikemhenry commented 2 months ago

=========================== short test summary info ============================
FAILED openfe/tests/protocols/test_openmm_plain_md_protocols.py::test_dry_run_gaff_vacuum - ValueError: No registered small molecule template generators could load force field 'gaff-2.11'
Available installed force fields are:
  GAFFTemplateGenerator: ['gaff-1.4', 'gaff-1.8', 'gaff-1.81', 'gaff-2.1', 'gaff-2.11']
  SMIRNOFFTemplateGenerator: ['openff-2.1.0', 'opc3-1.0.1', 'openff-1.1.0', 'openff-2.0.0-rc.1', 'tip3p_fb-1.1.1', 'openff-2.1.1', 'openff-1.3.0', 'openff-1.0.0-RC2', 'tip5p', 'tip5p-1.0.0', 'spce', 'tip3p_fb-1.0.0', 'tip4p_ew', 'tip4p_fb-1.0.0', 'opc-1.0.0', 'opc-1.0.1', 'opc-1.0.2', 'tip3p-1.0.0', 'openff-2.0.0', 'openff-1.2.1', 'openff-1.0.0-RC1', 'openff-1.3.1-alpha.1', 'tip3p-1.0.1', 'tip4p_fb', 'tip3p_fb', 'openff-2.2.0', 'openff-1.0.1', 'openff-1.3.1', 'openff-1.1.1', 'openff-2.0.0-rc.2', 'opc3', 'opc', 'tip4p_ew-1.0.0', 'spce-1.0.0', 'tip3p', 'tip3p_fb-1.1.0', 'opc3-1.0.0', 'openff-1.0.0', 'openff-2.1.0-rc.1', 'openff-2.2.0-rc1', 'openff-1.2.0', 'tip4p_fb-1.0.1', 'ff14sb_off_impropers_0.0.1', 'ff14sb_off_impropers_0.0.3', 'ff14sb_off_impropers_0.0.4', 'ff14sb_off_impropers_0.0.2', 'smirnoff99Frosst-1.0.2', 'smirnoff99Frosst-1.0.9', 'smirnoff99Frosst-1.1.0', 'smirnoff99Frosst-1.0.5', 'smirnoff99Frosst-1.0.8', 'smirnoff99Frosst-1.0.0', 'smirnoff99Frosst-1.0.6', 'smirnoff99Frosst-1.0.3', 'smirnoff99Frosst-1.0.4', 'smirnoff99Frosst-1.0.1', 'smirnoff99Frosst-1.0.7']
  EspalomaTemplateGenerator: ['espaloma-0.3.2']
FAILED openfe/tests/protocols/test_openmm_equil_rfe_protocols.py::test_dry_run_gaff_vacuum - ValueError: No registered small molecule template generators could load force field 'gaff-2.11'
Available installed force fields are:
  GAFFTemplateGenerator: ['gaff-1.4', 'gaff-1.8', 'gaff-1.81', 'gaff-2.1', 'gaff-2.11']
  SMIRNOFFTemplateGenerator: ['openff-2.1.0', 'opc3-1.0.1', 'openff-1.1.0', 'openff-2.0.0-rc.1', 'tip3p_fb-1.1.1', 'openff-2.1.1', 'openff-1.3.0', 'openff-1.0.0-RC2', 'tip5p', 'tip5p-1.0.0', 'spce', 'tip3p_fb-1.0.0', 'tip4p_ew', 'tip4p_fb-1.0.0', 'opc-1.0.0', 'opc-1.0.1', 'opc-1.0.2', 'tip3p-1.0.0', 'openff-2.0.0', 'openff-1.2.1', 'openff-1.0.0-RC1', 'openff-1.3.1-alpha.1', 'tip3p-1.0.1', 'tip4p_fb', 'tip3p_fb', 'openff-2.2.0', 'openff-1.0.1', 'openff-1.3.1', 'openff-1.1.1', 'openff-2.0.0-rc.2', 'opc3', 'opc', 'tip4p_ew-1.0.0', 'spce-1.0.0', 'tip3p', 'tip3p_fb-1.1.0', 'opc3-1.0.0', 'openff-1.0.0', 'openff-2.1.0-rc.1', 'openff-2.2.0-rc1', 'openff-1.2.0', 'tip4p_fb-1.0.1', 'ff14sb_off_impropers_0.0.1', 'ff14sb_off_impropers_0.0.3', 'ff14sb_off_impropers_0.0.4', 'ff14sb_off_impropers_0.0.2', 'smirnoff99Frosst-1.0.2', 'smirnoff99Frosst-1.0.9', 'smirnoff99Frosst-1.1.0', 'smirnoff99Frosst-1.0.5', 'smirnoff99Frosst-1.0.8', 'smirnoff99Frosst-1.0.0', 'smirnoff99Frosst-1.0.6', 'smirnoff99Frosst-1.0.3', 'smirnoff99Frosst-1.0.4', 'smirnoff99Frosst-1.0.1', 'smirnoff99Frosst-1.0.7']
  EspalomaTemplateGenerator: ['espaloma-0.3.2']
= 2 failed, 805 passed, 61 skipped, 1 xfailed, 2 xpassed, 1557 warnings in 338.71s (0:05:38) =

mikemhenry commented 2 months ago

Okay so it works, as in this test is asking for gaff, so we expect it to fail, but we should be getting this error message:

        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."
        )

mikemhenry commented 2 months ago

so need to do some work in openmmff, once we get the error we expect I will mark test tests as xfail

mikemhenry commented 2 months ago

FAILED openfe/tests/protocols/test_openmm_plain_md_protocols.py::test_dry_run_gaff_vacuum - openmmforcefields.generators.template_generators.GAFFNotSupportedError: 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.
FAILED openfe/tests/protocols/test_openmm_equil_rfe_protocols.py::test_dry_run_gaff_vacuum - openmmforcefields.generators.template_generators.GAFFNotSupportedError: 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.

wooo it works!

mikemhenry commented 1 month ago

I will use this PR to fix the CI so we do test gaff, but skip it on envs where we can't run the gaff tests

mikemhenry commented 1 month ago

Okay as part of the big openmmforcefeilds (OMMFF) dust up of 2024, we only can use GAFF with ambertools 22 on python 3.10 for reasons so what we need to do is skip the GAFF tests if we are dealing with non-ambertools 22, since only with ambertools 22 do we support GAFF, because of how our packaging works, OMMFF=0.12.0 is the version we need. So the plan is for python 3.10 tests, we test OMMFF=0.12.0, which should let us test GAFF, and on the other python versions we pull in OMFF=0.13.0

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.28%. Comparing base (e8492ea) to head (2ad523f).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #847 +/- ## ========================================== - Coverage 93.09% 92.28% -0.81% ========================================== Files 134 134 Lines 9708 9804 +96 ========================================== + Hits 9038 9048 +10 - Misses 670 756 +86 ``` | [Flag](https://app.codecov.io/gh/OpenFreeEnergy/openfe/pull/847/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy) | Coverage Δ | | |---|---|---| | [fast-tests](https://app.codecov.io/gh/OpenFreeEnergy/openfe/pull/847/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy) | `92.28% <100.00%> (?)` | | | [slow-tests](https://app.codecov.io/gh/OpenFreeEnergy/openfe/pull/847/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=OpenFreeEnergy#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mikemhenry commented 1 month ago

This is the note I am adding:

mikemhenry commented 1 month ago

Also dropping python 3.9 from test matrix

mikemhenry commented 1 month ago

also adding an osx arm test

mikemhenry commented 1 month ago

Okay so osx-arm will have to go until I can get an arm build for citeproc-py and mypy needs a mypy --install-types I think in the CI to fix missing stubs

I don't want to push any changes until the other tests get a chance to run

mikemhenry commented 1 month ago

Hmm, getting openfe/tests/protocols/test_openmm_equil_rfe_protocols.py::test_dry_run_gaff_vacuum - KeyError: 'system' for ommff=0.12.0 leg of the matrix

>               system = unit.run(dry=True)["debug"]["system"]
E               KeyError: 'system'

If I print out unit.run(dry=True)["debug"] for the openfe/tests/protocols/test_openmm_equil_rfe_protocols.py test I get {'sampler': <instance of HybridRepexSampler>} and if I print unit.run(dry=True)["debug"] for the openfe/tests/protocols/test_openmm_plain_md_protocols.py test I get {'debug': {'system': <openmm.openmm.System; proxy of <Swig Object of type 'OpenMM::System *' at 0x7a602b6207e0> >}}

Any ideas @IAlibay or @hannahbaumann ?

mikemhenry commented 1 month ago

Good idea @hannahbaumann for re-naming that variable

mikemhenry commented 1 month ago

Once this gets merged in we can add osx-arm to the tests https://github.com/conda-forge/citeproc-py-feedstock/pull/9

mikemhenry commented 1 month ago

Should I name the gaff test something different? I am worried we might get confused later

hannahbaumann commented 1 month ago

Maybe? I'm not sure what would be better. Would we also need the GAFF test for the afe protocol or do you think it's not necessary there?

mikemhenry commented 1 month ago

Maybe? I'm not sure what would be better.

Would we also need the GAFF test for the afe protocol or do you think it's not necessary there?

I didn't get an error so the test might not be doing what we think it does, can you link it?

pep8speaks commented 1 month ago

Hello @mikemhenry! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file openfe/tests/protocols/test_openmm_equil_rfe_protocols.py:

Line 253:5: E303 too many blank lines (2)

Comment last updated at 2024-05-10 22:03:51 UTC

mikemhenry commented 1 month ago

I can't easily add a custom name without adding something to all the CI strings, I vote until it is an issue, we just leave the 2 python 3.10 named tests and we can fix it when it becomes a problem

OpenFreeEnergy / openfe