Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
qin-ctx
reviewed
Mar 13, 2026
| @@ -24,7 +32,9 @@ ENV UV_NO_DEV=1 | |||
| WORKDIR /app | |||
|
|
|||
| # Copy source required for setup.py artifact builds and native extension build. | |||
Collaborator
There was a problem hiding this comment.
[Bug] COPY Cargo.toml Cargo.lock ./ 放在 COPY pyproject.toml ... 之前，Cargo.lock 是一个 3232 行的文件，任何变更都会使后续所有层的 Docker 缓存失效，包括 uv sync 这个最耗时的步骤。建议将 Cargo 文件的 COPY 与 Python 依赖安装分离，或调整层顺序以优化缓存命中率。
| ENV PATH="/usr/local/go/bin:${PATH}" | ||
| # Reuse Rust toolchain from stage 2 so setup.py can compile ov CLI in-place. | ||
| COPY --from=rust-toolchain /usr/local/cargo /usr/local/cargo | ||
| COPY --from=rust-toolchain /usr/local/rustup /usr/local/rustup |
Collaborator
There was a problem hiding this comment.
[Suggestion] 将完整的 cargo + rustup 工具链（通常 500MB~800MB）复制到 py-builder 阶段，但实际上 setup.py 只需要最终编译出的 ov 二进制文件（几 MB）。
建议改为在 rust-toolchain 阶段直接编译，只复制产物：
# Stage 2: build Rust CLI
FROM rust:1.88-trixie AS rust-builder
WORKDIR /build
COPY Cargo.toml Cargo.lock ./
COPY crates/ crates/
RUN --mount=type=cache,target=/usr/local/cargo/registry \
--mount=type=cache,target=/build/target \
cargo build --release --package ov_cli
# Stage 3: py-builder
FROM ghcr.io/astral-sh/uv:python3.13-trixie-slim AS py-builder
COPY --from=rust-builder /build/target/release/ov /app/openviking/bin/ovsetup.py 已支持 OV_SKIP_OV_BUILD=1 和 OV_PREBUILT_BIN_DIR 环境变量来跳过 Rust 构建，所以这个方案可行。
| # Copy source required for setup.py artifact builds and native extension build. | ||
| COPY Cargo.toml Cargo.lock ./ | ||
| COPY pyproject.toml uv.lock setup.py README.md ./ | ||
| COPY crates/ crates/ |
Collaborator
There was a problem hiding this comment.
[Suggestion] 当前 uv sync 已使用 --mount=type=cache，但 Cargo 的依赖下载和编译缓存未被挂载。每次构建都会重新下载和编译所有 Rust 依赖。
建议添加 Cargo cache mount，例如：
RUN --mount=type=cache,target=/root/.cache/uv \
--mount=type=cache,target=/usr/local/cargo/registry \
--mount=type=cache,target=/app/target \
uv sync --no-editable
qin-ctx
approved these changes
Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issue
Type of Change
Changes Made
Testing
Checklist
Screenshots (if applicable)
Additional Notes