Skip to content

Make CUDA the default compute platform - #1576

Merged
IAlibay merged 12 commits into
mainfrom
default-cuda
Oct 20, 2025
Merged

Make CUDA the default compute platform#1576
IAlibay merged 12 commits into
mainfrom
default-cuda

Conversation

@IAlibay

@IAlibay IAlibay commented Oct 14, 2025

Copy link
Copy Markdown
Member

TODO:

  • Fix tests for limiting the compute platforms
  • Add news entry

Checklist

  • Added a news entry

Developers certificate of origin

@mikemhenry

Copy link
Copy Markdown
Contributor

I know we like to use .lower() to make things easier for users, but do we think that breaking roundtrip serialization is worth it? Looks like we need to update a fixture so that our tests work. While its nice for testing and can be done with a fixture, do we want to expose a get_test_settings() method that just runs a little like these tests or do you think it depends too much on what you are trying to do?

@IAlibay

IAlibay commented Oct 14, 2025

Copy link
Copy Markdown
Member Author

but do we think that breaking roundtrip serialization is worth it

I might be missing something, we're storing the string as-is, unless you mean something else?

@mikemhenry

Copy link
Copy Markdown
Contributor

I see what you are saying, we use .lower() on the check but don't enforce any regularization on serialization, so if a user sets the default platform to CuDA that is what will be written in the transformation + result json.

@IAlibay

IAlibay commented Oct 17, 2025

Copy link
Copy Markdown
Member Author

I see what you are saying, we use .lower() on the check but don't enforce any regularization on serialization, so if a user sets the default platform to CuDA that is what will be written in the transformation + result json.

That's the behaviour we've always had - if you want to change this, I would suggest we do this in a separate PR. I'm only scoping this as "changing the default from None to cuda".


@field_validator('compute_platform')
def supported_sampler(cls, v):
supported = ['cpu', 'opencl', 'cuda']

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just adding a link here so if we ever decide we need HIP support, I can find this easily

#951

@mikemhenry mikemhenry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Really just wondering if it makes sense to set s.protocol_repeats = 1 in the testing fixtures instead of in most of the tests that use the fixture

Comment thread openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py
Comment thread openfe/tests/protocols/openmm_ahfe/test_ahfe_protocol.py Outdated
Comment thread openfe/tests/protocols/openmm_rfe/test_hybrid_top_protocol.py Outdated
Comment thread openfe/tests/protocols/openmm_septop/test_septop_protocol.py
Comment thread openfe/tests/protocols/openmm_septop/test_septop_protocol.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure if any tests here actually need more than one repeat

Comment thread openfe/tests/protocols/openmm_septop/test_septop_protocol.py Outdated
Comment thread openfe/tests/protocols/openmm_septop/test_septop_protocol.py Outdated
Comment thread openfe/tests/protocols/openmm_septop/test_septop_protocol.py Outdated
Comment thread openfe/tests/protocols/openmm_septop/test_septop_protocol.py Outdated
@IAlibay

IAlibay commented Oct 17, 2025

Copy link
Copy Markdown
Member Author

Really just wondering if it makes sense to set s.protocol_repeats = 1 in the testing fixtures instead of in most of the tests that use the fixture

Thanks, I will manually check and update - apologies in advance, for my own sanity I might not merge your suggestions from the github interface (so I can keep track of what has and hasn't been updated).

@codecov

codecov Bot commented Oct 17, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.56604% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.42%. Comparing base (ec229cc) to head (063efe8).

Files with missing lines Patch % Lines
...s/protocols/openmm_rfe/test_hybrid_top_protocol.py 75.00% 14 Missing ⚠️
...ests/protocols/openmm_md/test_plain_md_protocol.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1576      +/-   ##
==========================================
- Coverage   95.45%   93.42%   -2.03%     
==========================================
  Files         174      174              
  Lines       14552    14534      -18     
==========================================
- Hits        13891    13579     -312     
- Misses        661      955     +294     
Flag Coverage Δ
fast-tests 93.42% <90.56%> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mikemhenry

mikemhenry commented Oct 17, 2025

Copy link
Copy Markdown
Contributor

Really just wondering if it makes sense to set s.protocol_repeats = 1 in the testing fixtures instead of in most of the tests that use the fixture

Thanks, I will manually check and update - apologies in advance, for my own sanity I might not merge your suggestions from the github interface (so I can keep track of what has and hasn't been updated).

That is fine! I haven't been able to get the "add suggestion to batch" to work with the new PR UI anyway so it would have been a separate commit for each one. I mostly did it so I could keep track of how many there were set to one versus how many were set to the default.

@IAlibay

IAlibay commented Oct 17, 2025

Copy link
Copy Markdown
Member Author

Ok that's all of the protocol repeats dealt with @mikemhenry !

@mikemhenry

Copy link
Copy Markdown
Contributor

I'm happy with it!

@github-actions

Copy link
Copy Markdown

No API break detected ✅

@IAlibay

IAlibay commented Oct 20, 2025

Copy link
Copy Markdown
Member Author

Failure is unrelated to this PR - merging so we can fix other things elsewhere.

@IAlibay
IAlibay merged commit 7f93af7 into main Oct 20, 2025
4 of 12 checks passed
@IAlibay
IAlibay deleted the default-cuda branch October 20, 2025 13:10
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.

2 participants