Skip to content

chore: move go mod download before copy source in Dockerfile #3460

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

Merged
merged 1 commit into from
Apr 25, 2025

Conversation

fscnick
Copy link
Contributor

@fscnick fscnick commented Apr 22, 2025

Why are these changes needed?

To avoid downloading dependences after changing the source file, move go mod download up before copying source files.

In docker build, the cache depends on the previous layer. Which means the upper layer has changed, the following layers are subject to re-build.

Usually, the modification of deps is less frequent than the modification of source code. In the origin Dockerfile,

# Copy the go source
COPY proto/ proto/
COPY ray-operator/ ray-operator/
COPY apiserver/ apiserver/
WORKDIR /workspace/apiserver
# cache deps before building and copying source so that we don't need to re-download as much
# and so that source changes don't invalidate our downloaded layer
RUN go mod download

go mod download is after copying the go source. Once source files has changed. the docker build will re-run go mod download without leveraging the cache.

Thus, move go mod download before the copying the go source.

Related issue number

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@fscnick fscnick marked this pull request as ready for review April 22, 2025 16:10
@dentiny
Copy link
Contributor

dentiny commented Apr 23, 2025

To avoid downloading dependences after changing the source file

Sorry I'm not that familiar with golang, just for my own curiosity, could you please elaborate a little bit on the problem you've seen? thank you!

@fscnick
Copy link
Contributor Author

fscnick commented Apr 23, 2025

To avoid downloading dependences after changing the source file

Sorry I'm not that familiar with golang, just for my own curiosity, could you please elaborate a little bit on the problem you've seen? thank you!

  • go.mod and go.sum are the deps file in Go. And go mod download fetch the deps.
  • In docker build, the cache depends on the previous layer. Which means the upper layer has changed, the following layers are subject to re-build.

Usually, the modification of deps is less frequent than the modification of source code. In the origin Dockerfile,

# Copy the go source
COPY proto/ proto/
COPY ray-operator/ ray-operator/
COPY apiserver/ apiserver/
WORKDIR /workspace/apiserver
# cache deps before building and copying source so that we don't need to re-download as much
# and so that source changes don't invalidate our downloaded layer
RUN go mod download

go mod download is after copying the go source. Once source files has changed. the docker build will re-run go mod download without leveraging the cache.

Thus, move go mod download before the copying the go source. In ray-operator, it is right after copy deps.

COPY go.mod go.mod
COPY go.sum go.sum
# cache deps before building and copying source so that we don't need to re-download as much
# and so that source changes don't invalidate our downloaded layer
RUN go mod download

In addition, it copies ray-operator's and proto's go.mod and go.sum in the dockerfile because apiserver/go.mod replaces the module with local directory.

kuberay/apiserver/go.mod

Lines 96 to 99 in 37cf2ac

replace (
github.com/ray-project/kuberay/proto => ../proto
github.com/ray-project/kuberay/ray-operator => ../ray-operator
)

Kindly let me know if there is something unclear or wrong.

Copy link
Contributor

@dentiny dentiny left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation! I think it makes sense a lot.
Could you please at least add the docker layer part into PR description? I didn't get it at first sight. Thank you a ton!

COPY ray-operator/go.sum ray-operator/go.sum

# cache deps before building and copying source so that we don't need to re-download as much
# and so that source changes don't invalidate our downloaded layer
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it's a recommended practice, to download all dependencies right after we figure out the go.mod`.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

The change makes sense to me, but I'm curious about the difference between (1) the build time for updating the KubeRay API server source code without this PR, and (2) the build time for updating the KubeRay API server source code with this PR.

@fscnick
Copy link
Contributor Author

fscnick commented Apr 24, 2025

The change makes sense to me, but I'm curious about the difference between (1) the build time for updating the KubeRay API server source code without this PR, and (2) the build time for updating the KubeRay API server source code with this PR.

Without this PR: (95.9s)
without PR

With this PR: (85.4s)
with PR

Copy link
Member

@kevin85421 kevin85421 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!

@kevin85421 kevin85421 merged commit 7638040 into ray-project:master Apr 25, 2025
22 checks passed
@fscnick fscnick deleted the move-go-mod-download-earlier branch April 25, 2025 13:19
win5923 pushed a commit to win5923/kuberay that referenced this pull request Apr 27, 2025
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