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

[nightly] Create Nightly Pipeline, make docker-nightly-publish.yml & integration.yml more modular #2628

Merged
merged 65 commits into from
Dec 19, 2024

Conversation

HappyAmazonian
Copy link
Contributor

@HappyAmazonian HappyAmazonian commented Dec 9, 2024

Description

This PR includes the following changes

  • docker-nightly-publish.yml now pushes image to ECR only

  • docker-nightly-publish.yml add arch arg

  • integration.yml add workflow_call for reusing in future

  • integration.yml add tag-suffix & envs to allow the tests to fetch the image from ECR

  • tests/integration/tests.py now it uses ECR image to perform the tests

  • docker_publish.yml syncs images in the temp ECR to the staging ECR repo and dockerhub

  • nightly.yml introduce a pipeline of build,integtest,publish nightly images


  • If this change is a backward incompatible change, why must this change be made?
  • docker-nightly-publish now only pushes to ECR.
    nightly.yml will handles build, integ test, and push to the staging ECR/dockerhub.

  • you can use docker-nightly-publish to build the image and the built image will be in the temp ECR repo.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing

I have created one run here
integ test: https://github.com/deepjavalibrary/djl-serving/actions/runs/12360671952/job/34496566447
nightly build: https://github.com/deepjavalibrary/djl-serving/actions/runs/12401201241/job/34620194153
docker publish: https://github.com/deepjavalibrary/djl-serving/actions/runs/12400140451

nightly pipeline run: https://github.com/deepjavalibrary/djl-serving/actions/runs/12399112958/job/34622823684

@HappyAmazonian HappyAmazonian requested review from zachgk and a team as code owners December 9, 2024 20:44
@Lokiiiiii
Copy link
Member

Lokiiiiii commented Dec 10, 2024

Pushing and Pulling the built container to transfer it between hosts in itself takes >10 mins. I would like to avoid this if possible.

I added a persistent FSX cache volume to all the self hosted runners. Can you try to use the cache to persist data between runners instead of using ECR ? Maybe this can save us some time. I assume we would run into the same issue with PR sanity tests.

.github/workflows/integration.yml Outdated Show resolved Hide resolved
.github/workflows/docker-nightly-publish.yml Outdated Show resolved Hide resolved
@HappyAmazonian HappyAmazonian changed the title [nightly] Trigger Integ test before pushing to dockerhub [nightly] nightly yml now push to ECR repo & integ test can be launched with ECR image Dec 16, 2024
.github/workflows/docker-nightly-publish.yml Outdated Show resolved Hide resolved
./gradlew --refresh-dependencies :serving:dockerDeb -Psnapshot
- name: Build and push nightly docker image
if: ${{ inputs.mode == '' || inputs.mode == 'nightly' }}
- name: Build release docker image
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Let's call this "Build release candidate docker image"

- name: Build serving package for nightly
if: ${{ inputs.mode == '' || inputs.mode == 'nightly' }}
run: |
./gradlew --refresh-dependencies :serving:dockerDeb -Psnapshot
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move this as a command under Build temp docker image? I don't think we need a separate step for this only

Comment on lines 106 to 107
tempRunIdTag="${{ env.AWS_ECR_REPO }}:${{ matrix.arch }}-${{ inputs.mode }}-${GITHUB_RUN_ID}"
tempCommitTag="${{ env.AWS_ECR_REPO }}:${{ matrix.arch }}-${{ inputs.mode }}-${GITHUB_SHA}"
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make a slight change here where if inputs.mode == 'release, we use the DJL_VERSION value instead of release?

or maybe for both release/nightly/tmp we always just add the djl version number to the tag

tests/integration/tests.py Show resolved Hide resolved
@HappyAmazonian HappyAmazonian changed the title [nightly] nightly yml now push to ECR repo & integ test can be launched with ECR image [nightly] Create Nightly Pipeline, make docker-nightly-publish.yml & integration.yml more modular Dec 18, 2024
@siddvenk siddvenk dismissed Lokiiiiii’s stale review December 19, 2024 19:18

changes were addressed as requested

@siddvenk siddvenk merged commit 3aebeb5 into master Dec 19, 2024
9 checks passed
@siddvenk siddvenk deleted the nightly-integ branch December 19, 2024 19:18
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