Skip to content

ensure test jobs always run on PRs #708

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.

Sign up for GitHub

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

Merged
rapids-bot merged 5 commits into from
Aug 20, 2024
Merged

ensure test jobs always run on PRs #708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
229c43a
ensure test jobs always run on PRs
jameslamb Aug 14, 2024
b6c21ac
just put pr-builder alongside other checks
jameslamb Aug 14, 2024
11d05f7
pass architecture-specific tag to test job
jameslamb Aug 14, 2024
5566721
quotes
jameslamb Aug 14, 2024
ab85538
remove setup-python, just always run build-multiarch-manifest, ensure…
jameslamb Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 26 additions & 6 deletions .github/workflows/build-test-publish-images.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,26 @@ permissions:
statuses: none

jobs:
pr-builder:
if: ${{ !cancelled() && inputs.build_type == 'pull-request' }}
needs:
- checks
- compute-matrix
- build
Copy link
Copy Markdown
Contributor

@bdice bdice Aug 15, 2024

Choose a reason for hiding this comment

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

Are we missing build-multiarch-manifest here?

Copy link
Copy Markdown
Member Author

@jameslamb jameslamb Aug 15, 2024

Choose a reason for hiding this comment

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

No, it (I think intentionally) does not run on PRs: #708 (comment)

Copy link
Copy Markdown
Member Author

@jameslamb jameslamb Aug 15, 2024

Choose a reason for hiding this comment

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

@AyodeAwe what was the reason for not running build-multiarch-manifest on PRs any more as of #702?

I made some assumptions here but maybe those assumptions are wrong.

Copy link
Copy Markdown
Contributor

@AyodeAwe AyodeAwe Aug 15, 2024

Choose a reason for hiding this comment

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

build-multiarch-manifest was never usually run in PRs:

docker/.github/workflows/build-test-publish-images.yml

Line 194 in 5cea518

if: inputs.build_type == 'branch'

Copy link
Copy Markdown
Member Author

@jameslamb jameslamb Aug 15, 2024

Choose a reason for hiding this comment

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

Ah! Ok great, thanks for checking my understanding there.

Copy link
Copy Markdown
Member Author

@jameslamb jameslamb Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just pushed a commit making build-multiarch-manifest run on PRs (and adding it to pr-builder here).

I think it just makes everything simpler...:

  • it can now be required by the pr-builder job (to be sure it isn't accidentally skipped)
  • issues with it can be detected on PRs, instead of only on branch builds
  • it can be unconditionally included in the needs: block for delete-temp-images

Those jobs tend to take like 5-30 seconds each and don't use GPUs. I think that's a pretty small price to pay in exchange for the benefits I listed above.

- build-multiarch-manifest
- test
- delete-temp-images
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/pr-builder.yaml@branch-24.10
checks:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
- name: Run pre-commit
run: |
pip install pre-commit
pre-commit run --all-files
compute-matrix:
runs-on: ubuntu-latest
container:
Expand Down Expand Up @@ -119,7 +139,7 @@ jobs:

echo "TEST_MATRIX=$(yq -n -o json 'env(TEST_MATRIX)' | jq -c '{include: .}')" | tee --append "${GITHUB_OUTPUT}"
build:
needs: compute-matrix
needs: [checks, compute-matrix]
strategy:
matrix: ${{ fromJSON(needs.compute-matrix.outputs.MATRIX) }}
fail-fast: false
Expand Down Expand Up @@ -169,7 +189,6 @@ jobs:
${{ needs.compute-matrix.outputs.ALPHA_TAG }}-\
py${{ matrix.PYTHON_VER }}"
build-multiarch-manifest:
if: ${{ !cancelled() && inputs.build_type == 'branch' }}
needs: [build, compute-matrix]
strategy:
matrix: ${{ fromJSON(needs.compute-matrix.outputs.MATRIX) }}
Expand Down Expand Up @@ -208,7 +227,7 @@ jobs:
ARCHES: ${{ toJSON(matrix.ARCHES) }}
run: ci/create-multiarch-manifest.sh
test:
needs: [compute-matrix, build, build-multiarch-manifest]
needs: [compute-matrix, build]
if: inputs.run_tests
strategy:
matrix: ${{ fromJSON(needs.compute-matrix.outputs.TEST_MATRIX) }}
Expand All @@ -228,10 +247,11 @@ jobs:
${{ needs.compute-matrix.outputs.RAPIDS_VER }}\
${{ needs.compute-matrix.outputs.ALPHA_TAG }}-\
cuda${{ matrix.CUDA_VER }}-\
py${{ matrix.PYTHON_VER }}"
py${{ matrix.PYTHON_VER }}-\
${{ matrix.ARCH }}"
delete-temp-images:
if: ${{ !cancelled() && needs.test.result == 'success' }}
needs: [compute-matrix, build-multiarch-manifest, test]
if: ${{ !cancelled() && (needs.test.result == 'success' || needs.test.result == 'skipped') }}
needs: [compute-matrix, build, build-multiarch-manifest, test]
strategy:
matrix: ${{ fromJSON(needs.compute-matrix.outputs.MATRIX) }}
fail-fast: false
Expand Down
19 changes: 0 additions & 19 deletions .github/workflows/pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,7 @@ concurrency:
cancel-in-progress: true

jobs:
pr-builder:
needs:
- checks
- docker
secrets: inherit
uses: rapidsai/shared-workflows/.github/workflows/pr-builder.yaml@branch-24.10
checks:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
- uses: actions/setup-python@v5
with:
python-version: "3.10"
- name: Run pre-commit
run: |
pip install pre-commit
pre-commit run --all-files
Comment thread
jakirkham marked this conversation as resolved.
docker:
needs: [checks]
uses: ./.github/workflows/build-test-publish-images.yml
with:
build_type: pull-request
Expand Down