Skip to content

[CI/Build] Update causal-conv1d and lm-eval #22141

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

Closed
wants to merge 2 commits into from

Conversation

DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Aug 3, 2025

Essential Elements of an Effective PR Description Checklist

  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

Purpose

Test Plan

Test Result

(Optional) Documentation Update

@DarkLight1337 DarkLight1337 requested a review from simon-mo August 3, 2025 07:39
@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 3, 2025
Copy link

github-actions bot commented Aug 3, 2025

👋 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 documentation Improvements or additions to documentation ci/build rocm Related to AMD ROCm labels Aug 3, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates two key dependencies, causal-conv1d and lm-eval. The update to causal-conv1d to version 1.5.2 is a positive change that simplifies the build process by allowing it to be managed through pip-compile. However, the update to lm-eval pins the dependency to a specific git commit hash. While this may be necessary to resolve an immediate compatibility issue, it introduces potential maintenance and security risks. I've added comments suggesting the addition of explanatory notes in the code to clarify the reason for this pinning and to facilitate future updates to a stable release.

@@ -71,7 +71,7 @@ COPY --from=build_vllm ${COMMON_WORKDIR}/vllm /vllm-workspace
RUN cd /vllm-workspace \
&& rm -rf vllm \
&& python3 -m pip install -e tests/vllm_test_utils \
&& python3 -m pip install lm-eval[api]==0.4.4 \
&& python3 -m pip install git+https://github.com/EleutherAI/lm-evaluation-harness.git@bc811365ef3917a15da6aac9b7feafb4ba0fb42f#egg=lm-eval[api] \
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Pinning a dependency to a specific commit hash can introduce maintenance and security risks. It makes the build less reproducible if the commit history is changed (e.g., force-push) and bypasses the security vetting of official releases. If this specific commit is necessary to unblock a temporary issue, please add a comment explaining the reason for this pin and which issue/PR it relates to. This will help with future maintenance and make it clear when this can be reverted to a stable, versioned release.

mamba_ssm==2.2.5 # required for plamo2 test
matplotlib # required for qwen-vl test
mistral_common[image,audio] >= 1.8.2 # required for voxtral test
num2words # required for smolvlm test
open_clip_torch==2.32.0 # Required for nemotron_vl test
opencv-python-headless >= 4.11.0 # required for video test
datamodel_code_generator # required for minicpm3 test
lm-eval[api]==0.4.8 # required for model evaluation test
lm-eval[api] @ git+https://github.com/EleutherAI/lm-evaluation-harness.git@bc811365ef3917a15da6aac9b7feafb4ba0fb42f # required for model evaluation test
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Pinning a dependency to a specific commit hash, as done here for lm-eval, can be risky for long-term maintenance and security. It makes it difficult to track updates and can lead to build instability if the commit is ever removed or the repository history is altered. It's generally better to use a versioned release. If this pin is a temporary measure to resolve a specific compatibility issue, it would be very helpful to add a comment here explaining the context, linking to the relevant issue or PR, and outlining the conditions under which this can be updated to a released version.

@DarkLight1337 DarkLight1337 requested a review from mgoin August 3, 2025 07:44
@DarkLight1337
Copy link
Member Author

Hmm seems pip-compile still doesn't work well for CPU machines

@DarkLight1337
Copy link
Member Author

DarkLight1337 commented Aug 3, 2025

I'll just update lm-eval in the other PR then. Meanwhile we can see if the tests still pass with the updated versions

@DarkLight1337
Copy link
Member Author

Prefer #22409, closing

@DarkLight1337 DarkLight1337 deleted the update-deps branch August 7, 2025 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build documentation Improvements or additions to documentation ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant