search

openmm / NNPOps

High-performance operations for neural network potentials
Other
81 stars 17 forks source link

Add Self-Hosted AWS GPU Runner #100

Closed mikemhenry closed 1 year ago

mikemhenry commented 1 year ago

Ready for review @RaulPPelaez and @raimis

I only tested the latest version and before we merge this in we will want to decide how often to run the scheduled test and we want to remove the part the triggers the run in this branch.

mikemhenry commented 1 year ago

Also one more thing I want to do before merge is to use variables for the package versions so it is easier to change in the future

RaulPPelaez commented 1 year ago

This is a really cool functionality @mikemhenry, thanks! Its just some minor comments from me.

mikemhenry commented 1 year ago

Okay this is ready for review @raimis and @RaulPPelaez The only thing left is to decide how frequent you want to run it. It costs @jchodera $0.526 per hour, and it looks like it runs for ~ 11 minutes so it is pretty cheap. I think running it on every commit on a PR though is wasteful. I recommend running it nightly, merge commits to master, and manually on a branch that can be done on-demand (useful to do before merging).

peastman commented 1 year ago

If we want to keep the cost down, running it only on merge commits to master is probably sufficient. Running it nightly on code that's already been merged and tested doesn't tell us much more.

RaulPPelaez commented 1 year ago

I agree with @peastman, nightly is too much. I say we run this just before committing to master. Other than that the ci looks great to me, if we want to play it safe get the timeout down to 25 mins. Thanks @mikemhenry

mikemhenry commented 1 year ago

Okay! This should be good to merge now!