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

Patch VLLM when building Docker #92

Closed
wants to merge 1 commit into from

Conversation

ppetrovicTT
Copy link
Collaborator

No description provided.

@ppetrovicTT ppetrovicTT requested a review from tstescoTT February 3, 2025 16:35
@ppetrovicTT ppetrovicTT self-assigned this Feb 3, 2025
@@ -115,6 +115,10 @@ COPY --chown=${CONTAINER_APP_USERNAME}:${CONTAINER_APP_USERNAME} "locust" "${APP
RUN /bin/bash -c "source ${PYTHON_ENV_DIR}/bin/activate \
&& pip install --default-timeout=240 --no-cache-dir -r requirements.txt"

# apply patch to remove best-of argument from vllm
WORKDIR ${vllm_dir}
RUN git apply ${APP_DIR}/benchmarking/benchmark_serving.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

The patch file is meant to be temporary, if we're going to be using it longer then we should just add the patch to our vLLM fork, or obviate it with proper support for the options it removes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this with the intention to streamline the user experience, as it's currently one manual step in order to run the benchmarks. I thought since it's in this repository, that it's purpose was to workaround VLLM change.

All good! I'll reject this PR.
I created an issue as well: #93

@ppetrovicTT
Copy link
Collaborator Author

Created an issue for a proper fix:
#93

@ppetrovicTT ppetrovicTT closed this Feb 3, 2025
@ppetrovicTT ppetrovicTT deleted the ppetrovic/bench-auto-patch branch February 3, 2025 17:34
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