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

build: remove grpcio-tools build time dependency #230

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dtrifiro
Copy link
Contributor

@dtrifiro dtrifiro commented Feb 18, 2025

grpcio-tools is not available as a wheel for some platforms, making building vllm-tgis-adapter require building grpcio-tools from scratch.

Since this is not always possible (e.g. bazel not available on rhel), and is quite time consuming, we just include the generated code in the repo.

Note: this could (should?) also be added as a pre-commit hook, although this might slow down development a bit, so I left this as a gha step, for now.

@dtrifiro dtrifiro force-pushed the remove-grpcio-tools-dependency branch 2 times, most recently from 745f0a7 to 0a106b0 Compare February 18, 2025 17:59
@dtrifiro dtrifiro marked this pull request as draft February 18, 2025 18:03
@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.46%. Comparing base (178d4ba) to head (3a03dd5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #230   +/-   ##
=======================================
  Coverage   63.46%   63.46%           
=======================================
  Files          28       28           
  Lines        1768     1768           
  Branches      219      219           
=======================================
  Hits         1122     1122           
  Misses        541      541           
  Partials      105      105           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dtrifiro dtrifiro force-pushed the remove-grpcio-tools-dependency branch from 0a106b0 to a442404 Compare February 18, 2025 18:08
@R3hankhan123
Copy link

@dtrifiro , if we build grpcio-tools from scratch it will take alot of time as suggested in the pr, but for s390x we install the package with one env variable. Using this variable installation will take max 5 mins
GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1 python3 -m pip install grpcio-tools --no-cache-dir

Screenshot 2025-02-20 at 12 01 05 PM

Note: grpcio-tools took only 1-2 mins to build rest of time is taken by grpcio

@R3hankhan123
Copy link

after this vlllm-tgis-adapter is getting installed successfully after on s390x

@R3hankhan123
Copy link

@dtrifiro one more observation I have made for s390x is that during pip install vllm_tgis_adapter it takes the wheel that is already present on pypi and builds only grpcio and hftransfer

@R3hankhan123
Copy link

These packages are being installed

STEP 5/8: RUN --mount=type=cache,target=/root/.cache/pip     --mount=type=cache,target=/root/.cache/uv     --mount=type=bind,from=build,source=/workspace/vllm,target=/workspace/vllm     HOME=/root uv pip install vllm-tgis-adapter==0.6.2
Using Python 3.12.5 environment at: /opt/vllm
Resolved 117 packages in 248ms
Uninstalled 2 packages in 3ms
warning: Failed to hardlink files; falling back to full copy. This may lead to degraded performance.
         If the cache and target directories are on different filesystems, hardlinking may not be supported.
         If this is intentional, set `export UV_LINK_MODE=copy` or use `--link-mode=copy` to suppress this warning.
Installed 21 packages in 358ms
 + accelerate==1.3.0
 + cachetools==5.5.2
 + deprecated==1.2.18
 + googleapis-common-protos==1.68.0
 + grpcio==1.70.0
 + grpcio-health-checking==1.70.0
 + grpcio-reflection==1.70.0
 + hf-transfer==0.1.8
 - importlib-metadata==8.6.1
 + importlib-metadata==8.5.0
 + opentelemetry-api==1.30.0
 + opentelemetry-exporter-otlp==1.30.0
 + opentelemetry-exporter-otlp-proto-common==1.30.0
 + opentelemetry-exporter-otlp-proto-grpc==1.30.0
 + opentelemetry-exporter-otlp-proto-http==1.30.0
 + opentelemetry-proto==1.30.0
 + opentelemetry-sdk==1.30.0
 + opentelemetry-semantic-conventions==0.51b0
 + opentelemetry-semantic-conventions-ai==0.4.2
 - prometheus-client==0.21.1
 + prometheus-client==0.21.0
 + vllm-tgis-adapter==0.6.2

setup.py

`grpcio-tools` is not available as a wheel for some platforms, making
building vllm-tgis-adapter require building `grpcio-tools` from scratch.

Since this is not always possible, and is quite time consuming, we just
include the generated code in the repo.
@dtrifiro dtrifiro force-pushed the remove-grpcio-tools-dependency branch from a442404 to 3a03dd5 Compare February 24, 2025 10:06
@dtrifiro dtrifiro requested a review from joerunde February 24, 2025 13:21
@dtrifiro dtrifiro marked this pull request as ready for review February 24, 2025 13:21
@dtrifiro
Copy link
Contributor Author

dtrifiro commented Feb 24, 2025

@joerunde could you take a look? I don't see any potential issues with doing this apart from the annoyance of having to commit the generated .pyi in the repo and having to re-generate those every time generation.proto is updated.

Note: I'm not a big fan of introducing generated code in the repo and/or manual steps in the build process, so the alternative of using GRPC_PYTHON_BUILD_SYSTEM_OPENSSL=1 to build this package for s390x, might be good enough

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.

3 participants