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

GHA release workflow, refactor PR and Daily workflows #1968

Merged
merged 52 commits into from
Feb 22, 2023

Conversation

bandish-shah
Copy link
Contributor

@bandish-shah bandish-shah commented Feb 14, 2023

What does this PR do?

  • Adds a release.yaml pipeline which can be triggered by the creation of a properly formatted tag (beginning with a 'v') or manually by repo admins. The pipeline does the following:

    1. Runs code quality checks
    2. Builds source and wheel distributions
    3. Publishes PyPi production packages if triggered by by the push of a tag. Otherwise builds and pushes packages to Test PyPi.
    4. Builds and pushes production Docker images if triggered by the push of a tag, skipped otherwise. Production Docker build and push can be performed independently by triggering the release-docker pipeline manually.
  • Adds a release-docker.yaml pipeline which builds and pushes production Docker images. This flow can be triggered manually by repo admins

  • Adds coverage, pytest-cpu, pytest-gpu and docker-configure-build-push reusable workflows

  • Adds/refactors PR pr-docker, pr-cpu, pr-gpu pipelines to use reusable workflows

  • Refacxtors pytest-daily to daily pipeline and uses reusable workflows

Future Work (addressed by future PRs)

  • Update daily to build Docker images and run tests on staged images
  • Update Composer Docker builds to be gated by PyTorch images

What issue(s) does this change relate to?

Closes CO-1186

Testing

All workflows were tested manually by adding the pull_request trigger to all workflows and running in GHA. This trigger has been removed from the daily, release-docker and release workflows. The pr-gpu workflow has been changed to use the pull_request_target workflow since it needs to provide secrets to PR's from forked repos. Links to passing GHA runs for these workflows can be found below.

pr-gpu: https://github.com/mosaicml/composer/actions/runs/4199439168

daily: https://github.com/mosaicml/composer/actions/runs/4208510415

release-docker: https://github.com/mosaicml/composer/actions/runs/4208510424

release: https://github.com/mosaicml/composer/actions/runs/4210173449

Before submitting

  • Have you read the contributor guidelines?
  • Did you update any related tests and add any new tests related to your change? (see testing)
  • Did you run pre-commit on your change? (see the pre-commit section of prerequisites)

@bandish-shah bandish-shah changed the title [DO NOT COMMIT] Generalize Docker build flow [DO NOT MERGE] Generalize Docker build flow Feb 14, 2023
@bandish-shah bandish-shah changed the title [DO NOT MERGE] Generalize Docker build flow GHA release workflow, refactor PR and Daily workflows Feb 20, 2023
@bandish-shah bandish-shah marked this pull request as ready for review February 21, 2023 18:16
@bandish-shah bandish-shah requested a review from a team as a code owner February 21, 2023 18:16
Copy link
Contributor

@dakinggg dakinggg left a comment

Choose a reason for hiding this comment

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

Blocking because of my question about secrets, otherwise LGTM and we can ship and fix

.github/workflows/pr-docker.yaml Outdated Show resolved Hide resolved
.github/workflows/pr-gpu.yaml Show resolved Hide resolved
.github/workflows/pr-gpu.yaml Show resolved Hide resolved
.github/workflows/pr-cpu.yaml Outdated Show resolved Hide resolved
.github/workflows/pytest-cpu.yaml Outdated Show resolved Hide resolved
.github/workflows/release.yaml Outdated Show resolved Hide resolved
.github/workflows/release-docker.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Left 1 minor comment, and also confused by Daniel's points on python 3.9 and the push: false. Once this is approved, we can:

  1. update settings to remove old required tests
  2. merge this in
  3. update required tests

OR

  1. force merge this in
  2. update required tests

Former is has a small window of tests are not required so it's not ideal, but latter uses force merging which is kinda nasty. Will let you pick whatever you want

.github/workflows/release-docker.yaml Outdated Show resolved Hide resolved
@mvpatel2000 mvpatel2000 self-requested a review February 22, 2023 01:47
Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

LGTM. There's enough moving pieces though that I would not merge until @dakinggg also approves

Copy link
Contributor

@dakinggg dakinggg left a comment

Choose a reason for hiding this comment

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

LGTM, lets see if it works!

@bandish-shah bandish-shah merged commit 2d9053b into dev Feb 22, 2023
@bandish-shah bandish-shah deleted the bandish/ci_release branch February 22, 2023 17:49
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.

3 participants