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.

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

Jump to bottom

[CI] image build logic enhancement #199

Merged
merged 1 commit into from
Dec 9, 2023
Merged

[CI] image build logic enhancement #199

merged 1 commit into from
Dec 9, 2023

Conversation

SamYuan1990
Copy link
Collaborator

  • use latest GHA to manage container image build instead of make script.
  • update logic between base image and image itself
  • add arm64 and s390 image support

@sunya-ch
Copy link
Contributor

sunya-ch commented Dec 4, 2023

@SamYuan1990 The changes mostly look good to me. Is it possible to have the base image rebuild only when the relevant files are changed (Dockerfile.base and requirements.txt)?
https://how.wtf/run-workflow-step-or-job-based-on-file-changes-github-actions.html

@SamYuan1990
Copy link
Collaborator Author

@SamYuan1990 The changes mostly look good to me. Is it possible to have the base image rebuild only when the relevant files are changed (Dockerfile.base and requirements.txt)? https://how.wtf/run-workflow-step-or-job-based-on-file-changes-github-actions.html

I will try, there two items.

  1. if the base image updates, we need to build from base image.
  2. if there no changes on base image, we need to build model server image any way.

but, I am curious that it seems before this PR. if we update (for example, requirement.txt) and I didn't find a line in code limit the model server image build after base image?

@SamYuan1990 SamYuan1990 force-pushed the updateContainerAction branch from f1784ce to 1afde94 Compare December 4, 2023 11:53
@SamYuan1990 SamYuan1990 marked this pull request as draft December 4, 2023 11:58
@SamYuan1990 SamYuan1990 force-pushed the updateContainerAction branch 3 times, most recently from 0ec3c6d to 0a2353a Compare December 4, 2023 12:12
@SamYuan1990
Copy link
Collaborator Author

as tested locally, s390x is not supported, let's make multiple platform support later. https://github.com/SamYuan1990/kepler-model-server/actions/runs/7086334125/job/19284397824#step:6:509 + @huoqifeng in loop for information

@SamYuan1990
Copy link
Collaborator Author

ERROR: failed to solve: quay.io/sustainable_computing_io/kepler_model_server_base:v0.7: pulling from host quay.io failed with status code [manifests v0.7]: 502 Bad Gateway @sunya-ch could you please help rerun the test case? https://github.com/sustainable-computing-io/kepler-model-server/actions/runs/7086391503/job/19284563239?pr=199

@SamYuan1990 SamYuan1990 marked this pull request as ready for review December 4, 2023 12:16
@SamYuan1990
Copy link
Collaborator Author

@sunya-ch , could you please help review this PR?

sunya-ch
sunya-ch approved these changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@sunya-ch sunya-ch left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/lgtm Thank you. --> Sorry, need to change the review.

sunya-ch
sunya-ch reviewed Dec 8, 2023
View reviewed changes
.github/workflows/build-push.yml Outdated
runs-on: ubuntu-latest
outputs:
src: ${{ steps.changes.outputs.src }}
infra: ${{ steps.changes.outputs.infra }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see infra map below.
I think we need to remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated.

sunya-ch
sunya-ch requested changes Dec 8, 2023
View reviewed changes
Copy link
Contributor

@sunya-ch sunya-ch left a comment

Choose a reason for hiding this comment

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

I don't think we need infra in outputs. Other parts are good to go.

@SamYuan1990 SamYuan1990 force-pushed the updateContainerAction branch from 0a2353a to 7050463 Compare December 9, 2023 11:58
sunya-ch
sunya-ch approved these changes Dec 9, 2023
View reviewed changes
Copy link
Contributor

@sunya-ch sunya-ch left a comment

Choose a reason for hiding this comment

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

Thank you!

@sunya-ch sunya-ch merged commit 10091b8 into sustainable-computing-io:main Dec 9, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunya-ch sunya-ch sunya-ch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SamYuan1990 @sunya-ch