-
Notifications
You must be signed in to change notification settings - Fork 21
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
Support HIP #951
base: main
Are you sure you want to change the base?
Support HIP #951
Conversation
Will keep adding to this PR as needed, will also expand the news entry when the dust settles |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #951 +/- ##
==========================================
- Coverage 94.59% 91.64% -2.95%
==========================================
Files 134 134
Lines 9935 9935
==========================================
- Hits 9398 9105 -293
- Misses 537 830 +293
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@mikemhenry do you want to add the allowable platforms list as a validator to the OpenmmEngineSettings class? I feel like we should document what we support somewhere, and it's probably as good a place to go "well you can do this but know that OpenCL and HIP aren't thoroughly validated. |
@IAlibay that is a good point! I will add that and also add a note somewhere in the docs, maybe we need a page that is like "Using OpenFE in Production" where we list what we have heavily validated and also list tips for using it, like generating all the partial charges before running transformations. |
@@ -50,7 +50,7 @@ def get_openmm_platform(platform_name=None): | |||
platform.setPropertyDefaultValue( | |||
'DeterministicForces', 'true') | |||
|
|||
if name != 'CUDA': |
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.
Do we need to set DeterministicForces or Percision for HIP?
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.
Good catch, I was able to do that with the OpenMM API, but I need to test on a real GPU to see if it works (as in, setting those properties worked on my openMM HIP build but it could crash as soon as I try and run a MD step on the GPU)
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.
Is there an easy way to test this out? I.e. would spinning up an AWS runner that uses an AMD card work?
So we don't currently have any validators: A literal would be my preference -- users can deal with errors of using |
I personally would prefer it if it was a validator - mainly you get things like "OpenCL" | "opencl". |
Will use this PR to track what we need to support HIP. Related PRs and Issues:
#826
choderalab/openmmtools#753
Checklist
news
entryDevelopers certificate of origin