-
Notifications
You must be signed in to change notification settings - Fork 80
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
Changes from all commits
aa267b2
dc57a31
94b2f51
bf5c054
00b750b
db2c5e0
972a75f
e0d14dd
b113048
e135f98
42a2336
6e30ece
ffa7b01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 🤷♂️ |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not the update from (the first) Parsley to (the newest) Sage There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is to catch the way I nuked |
||
_logger.debug(f' {template_generator_cls.__name__} cannot load {small_molecule_forcefield}') | ||
_logger.debug(e) | ||
if self.template_generator is None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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." | ||
) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 securityhttps://github.com/openforcefield/openff-toolkit/blob/62b615cb6e3ea2561b869bbd70adaeea90fc04ea/.github/workflows/CI.yml#L87-L92
openforcefield/openff-interchange#962