[BLD]: unify Rust Dockerfiles for faster builds #4479
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
| COPY idl/ idl/ | ||
| COPY Cargo.toml Cargo.toml | ||
| COPY Cargo.lock Cargo.lock | ||
| COPY rust/ rust/ |
There was a problem hiding this comment.
[BestPractice]
Consider adding a .dockerignore file to speed up the build process and reduce context size. This is especially important since your Dockerfile copies the entire rust/ directory. A properly configured .dockerignore file would exclude unnecessary files like development artifacts, tests, and documentation from the build context.
|
|
||
| ENV EXCLUDED_PACKAGES="chromadb_rust_bindings chromadb-js-bindings chroma-benchmark " | ||
|
|
||
| RUN --mount=type=cache,sharing=locked,target=/chroma/target/ \ |
There was a problem hiding this comment.
[Documentation]
The builder stage uses --mount=type=cache for caching but this requires BuildKit to be enabled. Consider adding a comment to document this requirement or a fallback mechanism for environments where BuildKit might not be available.
|
Unify Rust Dockerfiles for Faster Builds This PR consolidates 5 separate Rust Dockerfiles into a single unified Dockerfile with multiple targets. The change significantly improves build times (1.3x-2.1x speedup) by leveraging shared build stages and better caching. It also updates Tiltfile and GitHub workflows to use the new Dockerfile. Key Changes: Affected Areas: This summary was automatically generated by @propel-code-bot |
| --mount=type=cache,sharing=locked,target=/usr/local/cargo/registry/ \ | ||
| if [ "$RELEASE_MODE" = "1" ]; then cargo build --workspace $(printf -- '--exclude %s ' $EXCLUDED_PACKAGES) --release; else cargo build --workspace $(printf -- '--exclude %s ' $EXCLUDED_PACKAGES); fi && \ | ||
| # Move CLI binary | ||
| if [ "$RELEASE_MODE" = "1" ]; then mv target/release/chroma ./chroma; else mv target/debug/chroma ./chroma; fi && \ |
There was a problem hiding this comment.
i wish there was a way to reduce the copy paste here.
codetheweb
force-pushed
the
bld-single-rust-dockerfile
branch
from
42c4572 to
eea1f2e
Compare
codetheweb
force-pushed
the
bld-single-rust-dockerfile
branch
from
73e4e5f to
8797f99
Compare
codetheweb
force-pushed
the
bld-single-rust-dockerfile
branch
from
8797f99 to
1395041
Compare
## Description of changes #4479 added auth against DockerHub for higher rate limits when pulling, but I forgot to update the release workflow to properly propogate the secrets down.
This reverts commit 3f2d8b1.
## Description of changes #4479 added auth against DockerHub for higher rate limits when pulling, but I forgot to update the release workflow to properly propogate the secrets down.
Description of changes
This PR unifies the 5 separate Rust Dockerfiles into 1. This results in substantially faster builds as well as being cleaner and easier to maintain.
After this PR, the old Rust Dockerfiles are no longer used in this repo but I'm keeping them around until all downstream consumers are updated to use the new
rust/Dockerfile.Timings
I tested the difference in (local) speed with 3 scenarios. For each one, I measured the time from the start of the build to when all services in Tilt were green.
tilt uprust/types/src/api_types.rsrust/garbage_collector/src/lib.rs