Temporarily remove GAFF because of parmchk2 issues #329
Conversation
| - 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 }} | ||
|
|
There was a problem hiding this comment.
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.
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.
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 security
| 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.
Not the update from (the first) Parsley to (the newest) Sage
There was a problem hiding this comment.
Good idea! We will want to make sure we mention that in the changelog/release notes
There was a problem hiding this comment.
Good idea - I'm adding a couple of notes to the changelog section of the README
| except ValueError as e: | ||
| except (ValueError, NotImplementedError) as e: |
There was a problem hiding this comment.
This is to catch the way I nuked GAFFTemplateGenerator, probably something that should be reverted alongside adding it back
| class TestGAFFTemplateGenerator(unittest.TestCase): | ||
| TEMPLATE_GENERATOR = GAFFTemplateGenerator | ||
|
|
||
| amber_forcefields = ['amber/protein.ff14SB.xml', 'amber/tip3p_standard.xml', 'amber/tip3p_HFE_multivalent.xml'] | ||
| class TemplateGeneratorBaseCase(unittest.TestCase): |
There was a problem hiding this comment.
The diff here is large, but the basic change is refactoring out the non-GAFF stuff from TestGAFFTemplateGenerator into a new TemplateGeneratorBaseCase; previously both SMIRNOFF and Espaloma tests subclassed from it, which make it impossible to run while skipping GAFF tests
| @@ -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.
if: ${{ github.repository == 'openmm/openmmforcefields'
&& github.event != 'schedule' }}
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.
I also expect this to skip it from being run from a fork 🤷♂️
| "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.
I am happy with this error message
|
🤌 those release notes 🔥 |
|
I like to keep release notes brief (the changes of me introducing a typo scale linearly with number of words) |
No description provided.