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

Improve : Optimize Dockerfile for Better Build Efficiency #126

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

YashSalunke12
Copy link

What does this PR do?

This pull request optimizes the Dockerfile by reducing the number of image layers, combining instructions, and improving readability. It also adds labels for better documentation. The Dockerfile is now more efficient and follows best practices for building Docker images.

This branch name and pull request heading are clear and descriptive. The branch name indicates that the pull request is related to a feature, specifically the optimization of the Dockerfile. The heading of the pull request provides a concise summary of the changes made in the pull request.
@YashSalunke12
Copy link
Author

@ruomingp Hey, could you please help me with this check? How do I pass this check?

@ruomingp
Copy link
Contributor

@ruomingp Hey, could you please help me with this check? How do I pass this check?

It seems a bug in the Dockerfile?

 => ERROR [ci 1/3] RUN pip install .[dev,gcp,vertexai_tensorboard]         0.6s
------
 > [ci 1/3] RUN pip install .[dev,gcp,vertexai_tensorboard]:
#13 0.554 ERROR: Directory '.[dev,gcp,vertexai_tensorboard]' is not installable. Neither 'setup.py' nor 'pyproject.toml' found.

RUN echo "deb [signed-by=/usr/share/keyrings/cloud.google.gpg] http://packages.cloud.google.com/apt cloud-sdk main" | tee -a /etc/apt/sources.list.d/google-cloud-sdk.list && \
# Install necessary packages in a single layer.
RUN apt-get update && \
apt-get install -y apt-transport-https ca-certificates gnupg curl gcc g++ git jq screen ca-certificates && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like some of these are repeated from above (L10-11)?

Dockerfile Outdated
@@ -54,24 +46,31 @@ RUN ./run_tests.sh "${PYTEST_FILES}"
# Bastion container spec. #
################################################################################

# Bastion container for application deployment.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: These comments are already covered in the comment blocks for each build target, e.g. L46 or L59.

Copy link
Contributor

@markblee markblee left a comment

Choose a reason for hiding this comment

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

Looks like the image is still failing to build. Have you tested building this image locally?

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.

None yet

3 participants