Skip to content

Conversation

@wzshiming
Copy link
Contributor

@wzshiming wzshiming commented Dec 15, 2025

Fixed #23588

Purpose

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Copilot AI review requested due to automatic review settings December 15, 2025 09:36
@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.

@wzshiming wzshiming force-pushed the fix/docker-build-cache branch from 40a7895 to 4440751 Compare December 15, 2025 09:36
@mergify mergify bot added the ci/build label Dec 15, 2025
@wzshiming wzshiming force-pushed the fix/docker-build-cache branch from 4440751 to 87334ea Compare December 15, 2025 09:37
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to improve Docker build caching by excluding the .git directory from the COPY command and making the wheel build step conditional. While the change to COPY is correct, the logic for the conditional wheel build is flawed. It introduces a critical bug where no wheel is built by default, and it contains redundant code. I've provided a suggestion to fix the logic and remove the redundancy.

@wzshiming wzshiming mentioned this pull request Dec 15, 2025
5 tasks
@chaunceyjiang chaunceyjiang added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 15, 2025
@wzshiming wzshiming force-pushed the fix/docker-build-cache branch 2 times, most recently from 21b229a to 47fe12b Compare December 15, 2025 10:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to fix Docker build cache issues by excluding the .git directory from the COPY command and introducing a USE_GIT_VERSION build argument to conditionally control version generation behavior.

Key changes:

  • Excludes .git directory from being copied into the build context to prevent cache invalidation
  • Introduces USE_GIT_VERSION argument to control whether git-derived or static versioning is used
  • Adds docker/ to .dockerignore to reduce build context size

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
docker/Dockerfile Adds --exclude=.git to COPY command and introduces conditional logic for wheel building based on USE_GIT_VERSION flag
.dockerignore Excludes docker/ directory from build context

Critical Issue Found: The conditional logic for the two RUN blocks has a bug where both conditions check for the same case (USE_GIT_VERSION != "0"), meaning the fallback case is never executed.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wzshiming wzshiming force-pushed the fix/docker-build-cache branch from 47fe12b to 72141c8 Compare December 15, 2025 11:08
Signed-off-by: Shiming Zhang <[email protected]>
@wzshiming wzshiming force-pushed the fix/docker-build-cache branch from 72141c8 to ee5c214 Compare December 15, 2025 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/build ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI]: Reduce docker build time with caching

2 participants