Skip to content

Conversation

@dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Aug 8, 2024

  • use requirements-build.txt to install build requirements
  • use cache bind mounts to speed up builds
  • use ccache
  • build wheel and install it instead of using setup.py install

- use `requirements-build.txt` to install build requirements
- use cache bind mounts to speed up builds
- use ccache
- build wheel and install it instead of using `setup.py install`
@github-actions
Copy link

github-actions bot commented Aug 8, 2024

👋 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 consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

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

🚀

@dtrifiro
Copy link
Contributor Author

dtrifiro commented Aug 8, 2024

/ready

@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 8, 2024
ENV CCACHE_DIR=/root/.cache/ccache
RUN --mount=type=cache,target=/root/.cache/pip \
--mount=type=cache,target=/root/.cache/ccache \
VLLM_TARGET_DEVICE=cpu python3 setup.py bdist_wheel && \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A significant issue here is that the torch version installed by requirements-cpu.txt will override whatever is defined in requirements-cpu.txt.

@mgoin
Copy link
Member

mgoin commented Aug 8, 2024

@bigPYJ1151 @zhouyuan does this look okay by you?

RUN --mount=type=cache,target=/root/.cache/pip \
--mount=type=bind,src=requirements-build.txt,target=requirements-build.txt \
pip install --upgrade pip && \
pip install -r requirements-build.txt
Copy link
Member

Choose a reason for hiding this comment

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

Overall LGTM, thanks for the improvement!

The only thing I concerned is using requirements-build.txt will install torch with many CUDA dependencies, which looks a bit redundant and increases download time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not: PIP_EXTRA_INDEX_URL is set before the RUN command, resulting in the following dependencies being installed:

Package           Version
----------------- ---------
cmake             3.30.2
filelock          3.15.4
fsspec            2024.6.1
Jinja2            3.1.4
MarkupSafe        2.1.5
mpmath            1.3.0
networkx          3.3
ninja             1.11.1.1
packaging         24.1
pip               24.1.2
setuptools        70.3.0
sympy             1.13.1
torch             2.4.0+cpu
typing_extensions 4.12.2
wheel             0.43.0

One possible issue might come up with some package managers such as uv:

export UV_EXTRA_INDEX_URL=https://download.pytorch.org/whl/cpu
uv pip install -r requirements-build.txt

will fail due to the way uv resolves local version identifiers: in order for this to succeed, the +cpu specifier should be explicitly added (torch==2.40+cpu).

Copy link
Contributor Author

@dtrifiro dtrifiro Aug 8, 2024

Choose a reason for hiding this comment

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

Note that this PIP_EXTRA_INDEX url also works for PEP517 style builds, so running

export PIP_EXTRA_INDEX_URL=https://download.pytorch.org/whl/cpu 
export VLLM_TARGET_DEVICE=cpu 
pip install git+https://github.com/vllm-project/vllm

will also install the cpu version when creating the isolated build venv.

@DarkLight1337 DarkLight1337 added the x86-cpu Related to Intel & AMD CPU label Aug 8, 2024
Copy link
Contributor

@maxdebayser maxdebayser left a comment

Choose a reason for hiding this comment

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

Overall the changes look good to me. There's only one detail I asked for more clarification about.

--mount=type=cache,target=/root/.cache/ccache \
VLLM_TARGET_DEVICE=cpu python3 setup.py bdist_wheel && \
pip install dist/*.whl

Copy link
Contributor

Choose a reason for hiding this comment

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

Do the files in dist stick around in the container image? Can they be removed with && rm -r dist or is there some dependency on these files after pip install?

Copy link
Contributor Author

@dtrifiro dtrifiro Aug 8, 2024

Choose a reason for hiding this comment

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

Yeah, that's correct, good catch. I did not bother too much about cleaning the image as I think this is clear this is mostly a CI image.

If there's an interest in making this production ready, we can set up multi-stage builds and have a final deployment stage without dev/build dependencies.

I can remove this if others think this is worth removing in this PR

Copy link
Member

Choose a reason for hiding this comment

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

This isn't a big overhead, we can remove later

@mgoin mgoin merged commit e904576 into vllm-project:main Aug 8, 2024
@dtrifiro dtrifiro deleted the fix-cpu-dockerfile branch August 9, 2024 08:11
Alvant pushed a commit to compressa-ai/vllm that referenced this pull request Oct 26, 2024
LeiWang1999 pushed a commit to LeiWang1999/vllm-bitblas that referenced this pull request Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed x86-cpu Related to Intel & AMD CPU

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants