[CI/Build] setuptools-scm fixes #8900
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 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:
🚀 |
dtrifiro
force-pushed
the
setuptools-scm-fixes
branch
2 times, most recently
from
134c137
to
4dc7106
Compare
| @@ -1,4 +1,31 @@ | |||
| vllm/*.so | |||
| /.venv | |||
| /build | |||
| dist | |||
|
|
|||
There was a problem hiding this comment.
These additions should be safe and should make sure that COPY . . in the Dockerfiles does not copy any additional files that shouldn't go in a build.
I'm unsure on whether there's anything else we might need to add here to avoid copying temporary from the host into the container build (mostly thinking about vllm/vllm_flash_attn right now)
dtrifiro
force-pushed
the
setuptools-scm-fixes
branch
from
4dc7106
to
b1f72da
Compare
| @@ -9,16 +9,7 @@ RUN apt-get update -y && \ | |||
| ffmpeg libsm6 libxext6 libgl1 | |||
| WORKDIR /workspace | |||
|
|
|||
| # copy requirements | |||
There was a problem hiding this comment.
I think we copy files separately to avoid docker build cache invalidation.
There was a problem hiding this comment.
Is this a problem? The next step builds vllm, and that relies sccache anyway, which greatly speeds up the process.
dtrifiro
force-pushed
the
setuptools-scm-fixes
branch
from
b1f72da
to
4a811b2
Compare
- expand .dockerignore - use `COPY . .` instructions to copy full repo before building the image (Dockerfile/Dockerfile.openvino) - add setuptools_scm section in pyproject.toml to silence warning - get rid of `buildkite_commit` build arg for release pipelines (replaced with setuptools-scm) - install build requirements in `build.sh` - fix get_vllm_version in `collect_env.py`
| from vllm import __version__, __version_tuple__ | ||
|
|
||
| if __version__ == "dev": | ||
| return "N/A (dev)" |
There was a problem hiding this comment.
can we use 1.0.0-dev instead of N/A
There was a problem hiding this comment.
__version__ will be set to dev only if setuptools-scm doesn't write vllm/_version.py (or something else bad happens with imports, see here, meaning we cannot infer what version this is (.git is not present ).
In this case, I think we have no reason for setting this to 1.0.0-dev, we can just acknowledge that this is an unknown development build.
| if __version__ == "dev": | ||
| return "N/A (dev)" | ||
|
|
||
| if len(__version_tuple__) == 4: # dev build |
There was a problem hiding this comment.
why is this 4 particularly?
There was a problem hiding this comment.
can I get some more context on __version_tuple__?
There was a problem hiding this comment.
You can find the setuptools-scm docs here: https://setuptools-scm.readthedocs.io/en/v8.0.1/usage/
For dev builds, version tuple is going to look like this:
(0, 6, 3, 'dev76', 'g78a09a757')
for exact tags, version tuple is going to look like this:
(0, 6, 3)
i.e. you have an extra field for development builds which includes the git sha
|
Testing this out today. |
Signed-off-by: charlifu <[email protected]>
Signed-off-by: Alvant <[email protected]>
Signed-off-by: Amit Garg <[email protected]>
Signed-off-by: Sumit Dubey <[email protected]>
COPY . .instructions to copy full repo before building the image (Dockerfile/Dockerfile.openvino). This prevents the repo from being "dirty" in the container build.buildkite_commitbuild arg for release pipelines (replaced with setuptools-scm)requirements-build.txtas part ofbuild.shSee #4738 (comment) for context