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

Add Self-Hosted AWS GPU Runner #100

Merged
merged 31 commits into from
Apr 27, 2023
Merged

Add Self-Hosted AWS GPU Runner #100

merged 31 commits into from
Apr 27, 2023

Conversation

mikemhenry
Copy link
Collaborator

No description provided.

environment.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@mikemhenry mikemhenry requested a review from raimis April 20, 2023 10:01
@mikemhenry
Copy link
Collaborator Author

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
Copy link
Collaborator Author

mikemhenry commented Apr 20, 2023

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
Copy link
Contributor

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

@mikemhenry mikemhenry requested a review from RaulPPelaez April 25, 2023 19:57
@mikemhenry
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Collaborator Author

Okay! This should be good to merge now!

Copy link
Contributor

@RaulPPelaez RaulPPelaez left a comment

Choose a reason for hiding this comment

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

This looks great to me. I say lets merge.
BTW, I see this is suffering from #98
Do you have any explanation?
I have not been able to reproduce it. My guess is on some CMake parameter.

Copy link
Contributor

@raimis raimis left a comment

Choose a reason for hiding this comment

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

Looks good!

@raimis raimis merged commit 054d487 into master Apr 27, 2023
@raimis raimis mentioned this pull request Jul 24, 2023
@raimis raimis mentioned this pull request Aug 17, 2023
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.

4 participants