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

ACR, Workflow UX, gh act, azure batch automation #43

Merged
merged 66 commits into from
Sep 27, 2024
Merged

Conversation

jkislin
Copy link
Contributor

@jkislin jkislin commented Sep 18, 2024

This code will not work until we get a github self-hosted runner working in our Azure environment.
ETA on that is End of Week!

@zsusswein @gvegayon

Please feel free to review in the meantime. I'll probably come in with some more commits tomorrow.

@jkislin
Copy link
Contributor Author

jkislin commented Sep 18, 2024

TODO:

  • Makefile update
  • Github Self Hosted Runner for CDCgov (backend in Azure, won't be reflected in code)
  • Pre-commit and NEWS.md

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

📢 Thoughts on this report? Let us know!

@zsusswein
Copy link
Collaborator

zsusswein commented Sep 25, 2024

FYI CI failure in R CMD check is due to an update in DuckDB. I'm going to open a PR to fix.

Edit: fixed in #47

To fix test failures caused by DuckDB v1.1.1 release
@jkislin
Copy link
Contributor Author

jkislin commented Sep 25, 2024

FYI CI failure in R CMD check is due to an update in DuckDB. I'm going to open a PR to fix.

Edit: fixed in #47

I'll pull it in. Thanks! Is the test-coverage failure at all related? I don't think anything I did should've broken that. Appreciate the collab!

@jkislin
Copy link
Contributor Author

jkislin commented Sep 25, 2024

Github S/H Runner complete.
Pre-commit complete.

@jkislin
Copy link
Contributor Author

jkislin commented Sep 25, 2024

All checks now pass. Sprinting toward a batch pool creation MVP.

@jkislin jkislin marked this pull request as ready for review September 26, 2024 03:33
@jkislin
Copy link
Contributor Author

jkislin commented Sep 26, 2024

@zsusswein, @natemcintosh We have reliable pool generation automated as of the latest commit.

Please feel free to suggest changes. This isn't entirely complete, but we're at a stable, testable, and functioning checkpoint.

What's been accomplished as of this PR:

  1. Dockerfiles are now built and pushed to the CFA Azure ACR reliably, tagged with both 'latest' and github shas.
  2. All Github Actions Workflows are now numbered (the number doesn't really matter, it's just an ID) and the workflow run names have been standardized to tell you (a) what workflow has been run as identified by its id# and name, (b) what commit triggered the workflow, and (c) what branch or PR the workflow was launched from. This data is always viewable as is, but by making this the title of the workflow runs, it'll be a lot easier to visually track what's going on at a glance.
  3. I put both dry run and full run nektos/gh-act scripts so we can test GH Actions locally.. in theory. I've never once gotten nektos/gh-act to work, but with some tinkering it might be useful.
  4. Azure Batch Pools are now created programmatically and for every commit, and both pools and jobs are tagged with github commit shas
  5. Jobs within workflows have been named standardly

Here's what we have left - let me know if this PR is useful to you without these, or if we should get these complete before merging:

  1. There are likely parameters buried in the cfa-nnh-pipelines create_expt_pool.py module that I haven't fully ported in my zeal to simplify. I can get that validated tomorrow.
  2. Job submission is very rudimentary and buggy here, and I have it commented out for now. It will take some time to port from cfa-nnh-pipelines.
  3. Curious if you want git long shas or git short shas for versioning pools, containers, and jobs. I've settled on short shas for now for readability.
  4. Take a look at the autoscale formula text file I've put in the top level of the repo. I know we wanted to change things. I'm new to the syntax, so haven't changed anything yet from cfa-nnh-pipelines, but I can dig in tomorrow. There's probably also a way to get this incorporated directly into the Github Actions yaml, but this seems to be a fair approach.
  5. Code Coverage is now failing. I'll give this a look over tomorrow as well.
  6. We need to determine a schedule to run the jobs. Do we still want to go with Wednesday mornings? Apologies if this is obvious!

@zsusswein
Copy link
Collaborator

I think this is feature-complete as-is -- let's focus review on what's here and discuss how to handle your questions in a future PR in #48

Copy link
Collaborator

@natemcintosh natemcintosh left a comment

Choose a reason for hiding this comment

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

I think this is looking good

.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Show resolved Hide resolved
Copy link
Collaborator

@zsusswein zsusswein left a comment

Choose a reason for hiding this comment

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

This is in great shape. I know this was a sprint to get done -- I really appreciate it.

I left some minor nitpicky comments inline, but that's all formatting. I think this has all the functionality we need.

Hold off on merging until @gvegayon can look too, but I'm signed off here.

.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.github/workflows/gh-act/2-full.sh Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Outdated Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Outdated Show resolved Hide resolved
Copy link
Member

@gvegayon gvegayon left a comment

Choose a reason for hiding this comment

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

As a general comment regarding using self-hosted runners. How safe are we about this? I am guessing that it should be OK b/c the running is in an isolated environment, so the biggest benefit is higher comp power, right?

Copy link
Contributor Author

@jkislin jkislin left a comment

Choose a reason for hiding this comment

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

Addressed all comments. Will merge when all checks pass again!

.github/workflows/build-and-push-docker.yaml Outdated Show resolved Hide resolved
.github/workflows/build-and-push-docker.yaml Outdated Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Outdated Show resolved Hide resolved
.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/1-Build-Dependency-Image.yaml Outdated Show resolved Hide resolved
.github/workflows/2-Run-Epinow2-Pipeline.yaml Show resolved Hide resolved
@jkislin jkislin merged commit 7f2712b into main Sep 27, 2024
11 checks passed
@jkislin jkislin deleted the jk-azure-readiness branch September 27, 2024 01:56
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this file is all about running the tests in the final container, the day before the production run?

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