Skip to content

Conversation

@yma11
Copy link
Contributor

@yma11 yma11 commented Feb 19, 2025

No description provided.

@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

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 either: Add ready label to the PR or enable auto-merge.

🚀

@mergify mergify bot added the ci/build label Feb 19, 2025
@yma11
Copy link
Contributor Author

yma11 commented Feb 19, 2025

@youkaichao we run into some issues caused by setuptools version in our internal jenkins but not sure whether upstream CI affected. It's better to have this in case. Can you take a look?

@youkaichao
Copy link
Member

cc @dtrifiro can you take a look and check if we need to pin setuptools directly in requirements-common.txt ?

@youkaichao
Copy link
Member

@youkaichao we run into some issues caused by setuptools version in our internal jenkins but not sure whether upstream CI affected. It's better to have this in case. Can you take a look?

@yma11 can you provide more details, e.g. versions of setuptools used, what is the problem?

@dtrifiro
Copy link
Contributor

@yma11 can you elaborate on why this pin is needed? Also, build time dependencies shouldn't be added to requirements-xpu.txt, setuptools-scm should actually be removed.

@yma11
Copy link
Contributor Author

yma11 commented Feb 19, 2025

@youkaichao we run into some issues caused by setuptools version in our internal jenkins but not sure whether upstream CI affected. It's better to have this in case. Can you take a look?

@yma11 can you provide more details, e.g. versions of setuptools used, what is the problem?

We are using python3.10. So based on setuptools related conf in requirements-common (setuptools>=74.1.1; python_version > '3.11') and in requirement-xpu(setuptools-scm>=8), we eventually have such stacks:
image
This may also relates with base image XPU uses. In this condition, vllm can't be installed properly during docker build with error message like:

#15 2.146 /usr/local/lib/python3.10/dist-packages/setuptools_scm/_integration/setuptools.py:31: RuntimeWarning: 
#15 2.146 ERROR: setuptools==59.6.0 is used in combination with setuptools_scm>=8.x
......
#15 3.371 creating build/bdist.linux-x86_64/egg/EGG-INFO
#15 3.372 copying UNKNOWN.egg-info/PKG-INFO -> build/bdist.linux-x86_64/egg/EGG-INFO
#15 3.372 copying UNKNOWN.egg-info/SOURCES.txt -> build/bdist.linux-x86_64/egg/EGG-INFO
#15 3.372 copying UNKNOWN.egg-info/dependency_links.txt -> build/bdist.linux-x86_64/egg/EGG-INFO
#15 3.373 copying UNKNOWN.egg-info/requires.txt -> build/bdist.linux-x86_64/egg/EGG-INFO
#15 3.373 copying UNKNOWN.egg-info/top_level.txt -> build/bdist.linux-x86_64/egg/EGG-INFO
#15 3.373 zip_safe flag not set; analyzing archive contents...
#15 3.373 creating dist
#15 3.373 creating 'dist/UNKNOWN-0.7.3.dev216+g4c82229898.d20250218.xpu-py3.10.egg' and adding 'build/bdist.linux-x86_64/egg' to it
#15 3.376 removing 'build/bdist.linux-x86_64/egg' (and everything under it)
#15 3.377 Processing UNKNOWN-0.7.3.dev216+g4c82229898.d20250218.xpu-py3.10.egg
#15 3.378 Copying UNKNOWN-0.7.3.dev216+g4c82229898.d20250218.xpu-py3.10.egg to /usr/local/lib/python3.10/dist-packages

IMO, seems this is env specific.
cc @dtrifiro

Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing the python_version marker in requirements-common.txt instead?

diff --git a/requirements-common.txt b/requirements-common.txt
index cfa020256..681c3179e 100644
--- a/requirements-common.txt
+++ b/requirements-common.txt
@@ -32,7 +32,7 @@ importlib_metadata
 mistral_common[opencv] >= 1.5.0
 pyyaml
 six>=1.16.0; python_version > '3.11' # transitive dependency of pandas that needs to be the latest version for python 3.12
-setuptools>=74.1.1; python_version > '3.11' # Setuptools is used by triton, we need to ensure a modern version is installed for 3.12+ so that it does not try to import distutils, which was removed in 3.12
+setuptools>=74.1.1; # Setuptools is used by triton, we need to ensure a modern version is installed for 3.12+ so that it does not try to import distutils, which was removed in 3.12
 einops # Required for Qwen2-VL.
 compressed-tensors == 0.9.1 # required for compressed-tensors
 depyf==0.18.0 # required for profiling and debugging with compilation config

@youkaichao do you see any problem with this?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have enough context to tell. maybe you can ask the one who added this line.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) February 22, 2025 16:05
@simon-mo simon-mo merged commit b56155e into vllm-project:main Feb 22, 2025
11 of 19 checks passed
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Feb 22, 2025
Akshat-Tripathi pushed a commit to krai/vllm that referenced this pull request Mar 3, 2025
lulmer pushed a commit to lulmer/vllm that referenced this pull request Apr 7, 2025
shreyankg pushed a commit to shreyankg/vllm that referenced this pull request May 3, 2025
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.

5 participants