-
Notifications
You must be signed in to change notification settings - Fork 763
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
Updated base image to Debian image and changed install commands compatible with Debian image #2528
base: master
Are you sure you want to change the base?
Conversation
…tible with Debian image Signed-off-by: Debabrata47 <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
From #2312 (comment).
@mani1soni Thank you for this!
Please can you also update the model initializer script and verify locally that this images is working with HF snapshot download API ?
@andreyvelich Do you still want to perform manually checking? Currently, I guess we can verify the behavior by our notebook E2E tests, right?
cmd/initializer/dataset/Dockerfile
Outdated
@@ -10,7 +10,7 @@ COPY pkg/initializer pkg/initializer | |||
RUN pip install -r requirements.txt | |||
|
|||
# Git is needed for the Kubeflow SDK to download JobSet Python models. | |||
RUN apk update && apk add --no-cache git | |||
RUN apt update && apt install -y git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RUN apt update && apt install -y git | |
RUN apt-get update && apt-get install -y git | |
RUN apt-get clean && rm -rf /var/lib/apt/lists/* |
We want to clean up source lists since we want to make images as minimal as possible.
Additionally, apt
is an interactive command; in script, apt-get
is more stable and appropriate.
cmd/initializer/model/Dockerfile
Outdated
@@ -10,7 +10,7 @@ COPY pkg/initializer pkg/initializer | |||
RUN pip install -r requirements.txt | |||
|
|||
# Git is needed for the Kubeflow SDK to download JobSet Python models. | |||
RUN apk update && apk add --no-cache git | |||
RUN apt update && apt install -y git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RUN apt update && apt install -y git | |
RUN apt-get update && apt-get install -y git | |
RUN apt-get clean && rm -rf /var/lib/apt/lists/* |
ditto
Signed-off-by: Debabrata47 <[email protected]>
@tenzen-y Thanks for the feedback, have commited the suggested changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks,
/lgtm
I will leave final approval on @andreyvelich
@@ -1,4 +1,4 @@ | |||
FROM python:3.11-alpine | |||
FROM python:3.11-slim-bookworm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Debabrata47 Please can you try to build this image locally, and verify that it can download dataset from HF since we don't have e2e tests in our test infra yet?
What this PR does / why we need it: This PR replaces the docker image from "python:3.11-alpine" to "python:3.11-slim-bookworm" and install command which are compatible with Debian image. As per the comment #2303.
Which issue(s) this PR fixes This PR Fixes #2311 issue and #2312 issue