-
Notifications
You must be signed in to change notification settings - Fork 51
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
Only support openmm 7.6 #896
Conversation
@ijpulidos I got some nan's in the python 3.9 tests so I will re-run, but assuming things pass this should be good for review and merge! |
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 looks good! I just think not pinning the openmm version should work better.
devtools/conda-envs/test_env.yaml
Outdated
@@ -17,7 +17,7 @@ dependencies: | |||
- numexpr | |||
- autograd | |||
- pymbar | |||
- openmm=7.5.1 | |||
- openmm=7.6 |
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 think we don't want to pin any specific openmm
version. It should use the latest available and it would also not require manually changing it for future openmm releases. Isn't that the case?
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 don't think there is an issue with having to manually change as openmm releases come out since if a new version comes out that breaks things, then we would want to keep testing against the older version while we update.
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'm open to other ideas! Also pinning the "oldest" version we support makes the logic of refining the env a bit easier in the CI, but we could probably refactor it so we don't need to do that.
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.
We should pin openmm >=7.6
, not =
!
Why is it trying to test OpenMM 7.5.1 in the checks? |
In the settings for the repo, those checks are set to "required" so it expects them in the PR. Until we merge this one in, I didn't want to change the repo setting since that would affect other PRs. |
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.
Minor comment, for future releases of openmm. But a non-blocker.
This adjusts our CI to only test 7.6 + nightly, sets the min version of openmm to 7.6 and removes and only env file.