Skip to content
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

Update for OpenMM namespace changes #304

Merged
merged 12 commits into from
May 25, 2021
Merged

Conversation

mikemhenry
Copy link
Contributor

@mikemhenry mikemhenry commented May 17, 2021

Fixes #303: There are changes happening in the openmm namespace, this should allow us to take advantage of the backwards compatible changes in the name space.

Also the README was pointing to the wrong CI badge, fixing that as well in this PR

@mikemhenry
Copy link
Contributor Author

@jchodera Trying to fix the namespace issue but the errors I'm getting a weird, is this
hydrogen = Element.getBySymbol("H") not the same hydrogen as

import simtk.openmm.app.element as element
element.hydrogen

That's the only thing I think that would be causing this that I have changed.

@jchodera
Copy link
Member

I don't want to be the bottleneck in sorting this out---I'd work directly with @peastman and the OpenMM issue tracker to resolve this!

@peastman
Copy link

It should be exactly the same object.

>>> import openmm
>>> import simtk.openmm
Warning: importing 'simtk.openmm' is deprecated.  Import 'openmm' instead.
>>> simtk.openmm.app.element.hydrogen is openmm.app.element.hydrogen
True
>>> simtk.openmm.app.element.hydrogen is openmm.app.element.Element.getBySymbol('H')
True
>>> simtk.openmm.app.element.Element.getBySymbol('H') is openmm.app.element.hydrogen
True

@mikemhenry
Copy link
Contributor Author

Okay so what I was doing isn't the source of the errors now in CI.
When a repo doesn't have activity for 30 days, github stops running the cron CI jobs.

So the last time CI ran and passed was October 29th 2020
Then when I turned the CI cron jobs back on, things failed right away:
https://github.com/choderalab/openmoltools/actions

It looks like the issue is some missing connect records in the pdb file.
Since the code didn't change, the culprit is likely a dependency that updated.

@mikemhenry
Copy link
Contributor Author

So I downgraded a few core packages where I thought this error could happen from

ambertools==20.9
openmm==7.4.0

And thought it worked since tests passed but really what was happening was:

test_amber.test_amber_binary_mixture ... SKIP: Skipping testing of packmol conversion because packmol not found.
test_amber.test_amber_box ... SKIP: Skipping testing of packmol conversion because packmol not found.
test_amber.test_amber_water_mixture ... SKIP: Skipping testing of packmol conversion because packmol not found.

So without the logs from the old CI runs, I have no idea if things were passing because things were working, or these tests were just skipped.

@jchodera
Copy link
Member

Shoot, sorry about this. We just need to stabilize the bleeding here so we can finish ripping openmoltools out of our other projects.

@mikemhenry
Copy link
Contributor Author

Exactly, I'm going to do the minimal amount possible to get nightly openmm to work.
I think there is just one more thing to fix, which is why is parmed thinking the version of openmm installed isn't newer than 6.3 when it is the nightly

   File "/usr/share/miniconda/envs/test/lib/python3.8/site-packages/parmed/utils/decorators.py", line 30, in new_fcn
    raise ImportError('You must have at least OpenMM 6.3 installed')
ImportError: You must have at least OpenMM 6.3 installed

I can see some issues witht this logic:
https://github.com/ParmEd/ParmEd/blob/3.4.1/parmed/utils/decorators.py

So I might need to look at what version of parmed is getting pulled in/install master for testing.

@mikemhenry mikemhenry linked an issue May 25, 2021 that may be closed by this pull request
@mikemhenry
Copy link
Contributor Author

Okay this is ready for review:

  • Fixed the badge in the README pointing to the perses CI
  • Updated imports to work with new openMM name space
  • Removed some errant whitespace
  • Updated CI to use my parmed fork when testing openMM nightly

@mikemhenry mikemhenry requested a review from jchodera May 25, 2021 16:30
@jchodera jchodera changed the title Fix issue 303 Update for OpenMM namespace changes May 25, 2021
Copy link
Member

@jchodera jchodera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@mikemhenry mikemhenry merged commit 135d2e1 into master May 25, 2021
@mikemhenry mikemhenry deleted the feat/fix_openmm_nightly branch May 25, 2021 23:44
@mikemhenry
Copy link
Contributor Author

@jchodera is there any other PRs or issues we should fix before cutting a release?

@mikemhenry mikemhenry restored the feat/fix_openmm_nightly branch May 27, 2021 20:21
@jchodera
Copy link
Member

@mikemhenry : If we're about to cut a new release, it would be good to squeeze in some deprecation warnings about force field generators that have migrated to openmm-forcefields:
#305

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updates for new OpenMM namespace
3 participants