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 testing #580

Merged
merged 4 commits into from
May 6, 2022
Merged

Add self hosted aws gpu testing #580

merged 4 commits into from
May 6, 2022

Conversation

mikemhenry
Copy link
Contributor

@mikemhenry mikemhenry commented Apr 22, 2022

Description

Resolves: #560

I'm sure some secrets will be missing, which we can make org secrets instead of repo secrets so we can use this in other repos

Todos

  • Implement feature / fix bug
  • Add tests
  • Update documentation as needed
  • Update changelogNotable points that this PR has either accomplished or will accomplish.

Status

  • Ready to go

@mikemhenry mikemhenry requested review from ijpulidos and mjw99 April 22, 2022 19:00
@mikemhenry
Copy link
Contributor Author

@mjw99 I thought you would want to take a look at this one, please as any questions you have about how this works! I'd like to start using this as a template to adding GPU testing to other repos, so I'd like to add comments explaining what is going on in the file.

@mikemhenry
Copy link
Contributor Author

Also, in order to really test this and see what fails we will need to merge it, but I'd like to wait to do that until we get this file documented!

@mjw99
Copy link
Collaborator

mjw99 commented Apr 25, 2022

Hi Mike,

Generally looks good.

I would pin the version of Ubuntu at 20.04 since 22.04 has just arrived; you probably do not want this to latently change whilst you're initially testing this. Also actions/checkout is now at version 3. Also, one super minor thing in devtools/conda-envs/test_env.yaml , it seems to have cython quoted twice.

Copy link
Collaborator

@mjw99 mjw99 left a comment

Choose a reason for hiding this comment

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

Hi Mike,

Generally looks good.

I would pin the version of Ubuntu at 20.04 since 22.04 has just arrived; you probably do not want this to latently change whilst you're initially testing this. Also actions/checkout is now at version 3. Also, one super minor thing in devtools/conda-envs/test_env.yaml , it seems to have cython quoted twice

@mikemhenry
Copy link
Contributor Author

Good idea! Github is kinda slow when rolling these updates out, but I rather not worry about it actions/runner-images#5490

@mikemhenry mikemhenry requested a review from mjw99 May 6, 2022 09:10
@mikemhenry
Copy link
Contributor Author

Also I know this makes the diff messy, but I sorted the packages in each section so it is easier to catch dupes

Copy link
Collaborator

@mjw99 mjw99 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikemhenry
Copy link
Contributor Author

Sweet! I expect something to fail post merge, I might have to add the PAT to this repo, but I think it's better to see what fails and fix it iteratively

@mjw99 mjw99 merged commit 0771ddc into main May 6, 2022
@mjw99 mjw99 deleted the feat/add_gpu_ci branch May 6, 2022 10:09
@mjw99
Copy link
Collaborator

mjw99 commented May 6, 2022

Hmmm on merge; I'm seeing:
Error: Input required and not supplied: aws-region

Are the relevant AWS secrets set OK on the master?

@mikemhenry
Copy link
Contributor Author

mikemhenry commented May 6, 2022

Not sure what you mean, it looks like I will need to add secrets.AWS_ACCESS_KEY_ID secrets.AWS_SECRET_ACCESS_KEY secrets.AWS_REGION and secrets.GH_PERSONAL_ACCESS_TOKEN

They are set correctly in the repo that is currently using this yaml file https://github.com/choderalab/perses/
I thought that maybe I had set them on the org level, but per-repo level is nice if we ever need/want to use different aws accounts for security reasons.

@mikemhenry
Copy link
Contributor Author

I think I know what you mean, no these are not set OK on master so it will fail until I add them, which I will do right now.

@ijpulidos ijpulidos added this to the 0.21.4 milestone Jun 1, 2022
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.

Set up GPU CI
3 participants