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

[CI/Build] remove .github from .dockerignore, add dirty repo check #9375

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Oct 15, 2024

Including .github in .dockerignore results having a dirty repo when copying the source while building the image, causing the version string setuptools-scm not to match tags exactly.

Also include tools/check_repo.sh to check repo status before building.

See context here

including this in .dockerignore results having a dirty repo
when copying the source while building the image.
Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@simon-mo
Copy link
Collaborator

Can we add a clause in Dockerfile to assert the git repo is in non dirty state (gated by a flag off by default, and turned on by release, similar to wheel size check, although wheel size check is on by default)

@simon-mo
Copy link
Collaborator

Actually in all our ci it shouldn't be dirty

@dtrifiro
Copy link
Contributor Author

Can we add a clause in Dockerfile to assert the git repo is in non dirty state (gated by a flag off by default, and turned on by release, similar to wheel size check, although wheel size check is on by default)

Added a clause to check for git repo status (dirty and presence of tags) under the --build-arg GIT_REPO_CHECK=1

Copy link
Collaborator

@simon-mo simon-mo left a comment

Choose a reason for hiding this comment

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

Let's revert the non default Dockerfile changes? I believe given those don't get triggered in release pipeline we don't need to do that.

@dtrifiro dtrifiro changed the title [CI/Build] remove .github from .dockerignore [CI/Build] remove .github from .dockerignore, add dirty repo check Oct 17, 2024
@simon-mo
Copy link
Collaborator

[2024-10-17T17:13:54Z] #18 [build 4/8] RUN --mount=type=bind,source=.git,target=.git if [ "1" != 0 ]; then bash tools/check_repo.sh ; fi

  | [2024-10-17T17:13:55Z] #18 0.638 v0.6.3-10-gb007d160
  | [2024-10-17T17:13:55Z] #18 DONE 0.7s

Build pass here https://buildkite.com/vllm/release/builds/1409#01929b76-e6c7-479c-9b7c-f6e25653eec9

@simon-mo simon-mo merged commit a2c71c5 into vllm-project:main Oct 17, 2024
20 of 22 checks passed
@dtrifiro dtrifiro deleted the fix-release branch October 17, 2024 17:26
charlifu pushed a commit to charlifu/vllm that referenced this pull request Oct 23, 2024
vrdn-23 pushed a commit to vrdn-23/vllm that referenced this pull request Oct 23, 2024
Alvant pushed a commit to compressa-ai/vllm that referenced this pull request Oct 26, 2024
garg-amit pushed a commit to garg-amit/vllm that referenced this pull request Oct 28, 2024
FerdinandZhong pushed a commit to FerdinandZhong/vllm that referenced this pull request Oct 29, 2024
mfournioux pushed a commit to mfournioux/vllm that referenced this pull request Nov 20, 2024
tlrmchlsmth pushed a commit to neuralmagic/vllm that referenced this pull request Nov 23, 2024
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.

2 participants