feat: add vLLM setup script for modular installation#120
Open
cmagina wants to merge 3 commits intoredhat-et:mainfrom
Open
feat: add vLLM setup script for modular installation#120cmagina wants to merge 3 commits intoredhat-et:mainfrom
cmagina wants to merge 3 commits intoredhat-et:mainfrom
Conversation
cmagina
force-pushed
the
pr-06-vllm-setup-script
branch
from
34ea58e to
59a59ea
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new vLLM installer script and integrates it into images, the Makefile, dev setup, and CI triggers so changes to Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (repo)
participant CI as CI / GitHub Actions
participant Docker as Docker build
participant Container as Dev Container (run_container)
participant DevSetup as devsetup.sh
participant Script as devinstall_vllm.sh
participant Remote as vLLM (git/PyPI)
Dev->>CI: push PR including `scripts/devinstall_vllm.sh`
CI->>Docker: build image (COPY script -> /workspace/bin/devinstall_vllm)
Dev->>Container: run_container (may mount `vllm_path`, set `INSTALL_VLLM`)
Container->>DevSetup: invoke `devsetup.sh` (runs as user)
DevSetup->>Script: call `devinstall_vllm.sh` (if `INSTALL_VLLM` != skip)
Script->>Remote: clone or pip install vLLM (git or wheel)
Remote-->>Script: return source or wheel artifacts
Script-->>Container: install deps, set VLLM_TARGET_DEVICE, configure env
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cmagina
force-pushed
the
pr-06-vllm-setup-script
branch
2 times, most recently
from
0d54217 to
c05ad0f
Compare
cmagina
force-pushed
the
pr-06-vllm-setup-script
branch
from
dba9059 to
f9b8f06
Compare
hinriksnaer
reviewed
Apr 6, 2026
scripts/devinstall_vllm.sh
Outdated
| PIP_INSTALL_ARGS+=("--index-url" "$PIP_VLLM_EXTRA_INDEX_URL") | ||
| elif [ -n "${VLLM_COMMIT:-}" ]; then | ||
| echo "Using the build from commit $VLLM_COMMIT ..." | ||
| PIP_VLLM_EXTRA_INDEX_URL="--extra-index-url ${PIP_VLLM_INDEX_URL_BASE}/${VLLM_COMMIT}" |
Collaborator
There was a problem hiding this comment.
This might be silently ignored in the code path
hinriksnaer
reviewed
Apr 6, 2026
scripts/devinstall_vllm.sh
Outdated
| install_whl | ||
| ;; | ||
| nightly) | ||
| PIP_VLLM_EXTRA_INDEX_URL="--extra-index-url ${PIP_VLLM_INDEX_URL_BASE}/nightly" |
Collaborator
There was a problem hiding this comment.
Also wonder if this is silently ignored
Collaborator
Author
There was a problem hiding this comment.
Its not, its used in the specified index check following this switch case
Add scripts/setup_vllm.sh to support vLLM installation and configuration within containers. This script: - Downloads vLLM source from GitHub when not mounted as a volume - Installs build dependencies for vLLM compilation - Supports installing vLLM wheels from PyPI (release, nightly) - Provides flexible configuration via INSTALL_VLLM environment variable The script supports multiple installation modes: - source: Build from source (with auto-download if not mounted) - release/nightly: Install wheels from PyPI - skip: Skip vLLM installation This is part of the modular script architecture introduced in PR redhat-et#115. Signed-off-by: Craig Magina <cmagina@redhat.com>
cmagina
force-pushed
the
pr-06-vllm-setup-script
branch
from
17b4113 to
ba68bfc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Makefile (1)
153-153: Splitenv_varsinto multiline entries to keep it maintainable.Line 153 is already densely packed and will keep growing with modular installers. A multiline form reduces churn and review risk when adding/removing flags.
♻️ Proposed refactor
- env_vars="-e USERNAME=$(USER) -e USER_UID=`id -u $(USER)` -e USER_GID=`id -g $(USER)` -e TORCH_VERSION=$(torch_version) -e INSTALL_LLVM=$(INSTALL_LLVM) -e INSTALL_TOOLS=$(DEMO_TOOLS) -e INSTALL_JUPYTER=$(INSTALL_JUPYTER) -e NOTEBOOK_PORT=$(NOTEBOOK_PORT) -e INSTALL_HELION=$(INSTALL_HELION) -e INSTALL_TORCH=$(INSTALL_TORCH) -e INSTALL_TRITON=$(INSTALL_TRITON) -e INSTALL_VLLM=$(INSTALL_VLLM) -e USE_CCACHE=$(USE_CCACHE) -e MAX_JOBS=$(MAX_JOBS)"; \ + env_vars="\ + -e USERNAME=$(USER) \ + -e USER_UID=`id -u $(USER)` \ + -e USER_GID=`id -g $(USER)` \ + -e TORCH_VERSION=$(torch_version) \ + -e INSTALL_LLVM=$(INSTALL_LLVM) \ + -e INSTALL_TOOLS=$(DEMO_TOOLS) \ + -e INSTALL_JUPYTER=$(INSTALL_JUPYTER) \ + -e NOTEBOOK_PORT=$(NOTEBOOK_PORT) \ + -e INSTALL_HELION=$(INSTALL_HELION) \ + -e INSTALL_TORCH=$(INSTALL_TORCH) \ + -e INSTALL_TRITON=$(INSTALL_TRITON) \ + -e INSTALL_VLLM=$(INSTALL_VLLM) \ + -e USE_CCACHE=$(USE_CCACHE) \ + -e MAX_JOBS=$(MAX_JOBS)"; \As per coding guidelines "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 153, The env_vars shell variable is a long single-line string that's hard to maintain; modify the Makefile so the env_vars assignment (symbol: env_vars) is written as a multiline, concatenated string with one -e flag per line (using shell continuation backslashes or Makefile line-continuation) so each environment entry like USERNAME=$(USER), USER_UID=`id -u $(USER)`, TORCH_VERSION=$(torch_version), INSTALL_LLVM=$(INSTALL_LLVM), etc., is on its own line; ensure the final variable expansion and trailing semicolon/continuation are preserved and that any backticks or subshells (e.g., `id -u $(USER)`) remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 112-114: Check that vllm_path exists before adding the mount: in
the Makefile block that references vllm_path and updates volume_arg (using
SELINUXFLAG), add an existence test (e.g., [ -e "$(vllm_path)" ] or [ -d
"$(vllm_path)" ]) when vllm_path is non-empty; if the path does not exist, print
a clear error message and exit non‑zero so the build fails fast instead of
appending a bad -v mount. Ensure you update only the branch that handles
vllm_path and keep volume_arg assembly unchanged when the path is valid.
In `@scripts/devinstall_vllm.sh`:
- Around line 77-87: The INSTALL_TORCH handling in scripts/devinstall_vllm.sh
incorrectly treats "skip" as a version and calls devinstall_torch, which fails;
update the conditional around the branch that currently checks [
"${INSTALL_TORCH:-}" != "source" ] / [ -n "${INSTALL_TORCH:-}" ] to also ensure
INSTALL_TORCH != "skip" (i.e., add && [ "${INSTALL_TORCH}" != "skip" ]), so that
when INSTALL_TORCH=skip the script does not call devinstall_torch; adjust the
branches around the INSTALL_TORCH checks (the block invoking devinstall_torch
and the echo messages) accordingly to skip installation when INSTALL_TORCH is
"skip".
- Around line 156-159: The pip_install_args array is being appended with a
single string combining option and value when VLLM_COMMIT is set, which produces
a malformed single argument when later expanded; update the append for
pip_install_args so it adds two separate array elements—the option token
(--extra-index-url) and the URL value constructed from pip_vllm_index_url_base
and VLLM_COMMIT—mirroring the correct pattern used in the other branch, so that
expanding "${pip_install_args[@]}" yields two separate arguments for pip/uv.
- Around line 127-133: The script currently expands VLLM_TARGET_DEVICE
unconditionally which triggers nounset errors under set -u; change the code to
avoid expanding an unset variable by either providing a safe default or
exporting only when set: use parameter expansion like export
VLLM_TARGET_DEVICE="${VLLM_TARGET_DEVICE:-}" (or a sensible default such as
"cpu") or wrap the tee block in an if [ -n "${VLLM_TARGET_DEVICE:-}" ]; then ...
fi so the VLLM_TARGET_DEVICE variable is not referenced when undefined.
- Around line 95-96: The pip install calls currently pass the uv-specific flag
"--prerelease=allow" to the generic pip fallback; update the pip_install logic
so it uses the proper flag depending on the installer: when invoking the 'uv'
wrapper (uv pip install) pass "--prerelease=allow", but when falling back to the
system 'pip' command use "--pre" instead. Modify the code paths that call
pip_install (referenced as pip_install in the script and the uv invocation path)
at the locations noted (the branches that install requirements/cuda.txt and the
other three occurrences) to conditionally set the flag based on whether uv is
available or not, ensuring uv gets "--prerelease=allow" and pip gets "--pre".
---
Nitpick comments:
In `@Makefile`:
- Line 153: The env_vars shell variable is a long single-line string that's hard
to maintain; modify the Makefile so the env_vars assignment (symbol: env_vars)
is written as a multiline, concatenated string with one -e flag per line (using
shell continuation backslashes or Makefile line-continuation) so each
environment entry like USERNAME=$(USER), USER_UID=`id -u $(USER)`,
TORCH_VERSION=$(torch_version), INSTALL_LLVM=$(INSTALL_LLVM), etc., is on its
own line; ensure the final variable expansion and trailing
semicolon/continuation are preserved and that any backticks or subshells (e.g.,
`id -u $(USER)`) remain intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04571f60-4249-47e7-a1e3-f83f7fa713dd
📒 Files selected for processing (9)
.github/workflows/amd-image.yml.github/workflows/cpu-image.yml.github/workflows/nvidia-image.ymlMakefiledockerfiles/Dockerfile.tritondockerfiles/Dockerfile.triton-amddockerfiles/Dockerfile.triton-cpuscripts/devinstall_vllm.shscripts/devsetup.sh
Signed-off-by: Craig Magina <cmagina@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Makefile (1)
112-114:⚠️ Potential issue | 🟠 MajorFail fast when
vllm_pathis set but invalid.Line 112 currently ignores a non-empty but nonexistent
vllm_path, which hides misconfiguration and makes source-mode behavior hard to debug.🔧 Proposed fix
- if [ -n "$(vllm_path)" ] && [ -d "$(vllm_path)" ]; then \ - volume_arg+=" -v $(vllm_path):/workspace/vllm$(SELINUXFLAG)"; \ - fi; \ + if [ -n "$(vllm_path)" ] && [ -d "$(vllm_path)" ]; then \ + volume_arg+=" -v $(vllm_path):/workspace/vllm$(SELINUXFLAG)"; \ + elif [ -n "$(vllm_path)" ]; then \ + echo "!! ERROR: vllm_path does not exist: $(vllm_path)" >&2; \ + exit 1; \ + fi; \As per coding guidelines "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 112 - 114, The Makefile currently ignores a non-empty but nonexistent vllm_path, hiding misconfiguration; update the conditional around vllm_path/volume_arg so that if [ -n "$(vllm_path)" ] && [ -d "$(vllm_path)" ] then append to volume_arg as before, but add an explicit branch that if [ -n "$(vllm_path)" ] && [ ! -d "$(vllm_path)" ] then print a clear error message referencing vllm_path and exit 1 (so the build fails fast); keep SELINUXFLAG usage unchanged when appending.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Makefile`:
- Around line 112-114: The Makefile currently ignores a non-empty but
nonexistent vllm_path, hiding misconfiguration; update the conditional around
vllm_path/volume_arg so that if [ -n "$(vllm_path)" ] && [ -d "$(vllm_path)" ]
then append to volume_arg as before, but add an explicit branch that if [ -n
"$(vllm_path)" ] && [ ! -d "$(vllm_path)" ] then print a clear error message
referencing vllm_path and exit 1 (so the build fails fast); keep SELINUXFLAG
usage unchanged when appending.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 765caee7-156b-490b-b59f-e0382249ccbd
📒 Files selected for processing (2)
Makefilescripts/devinstall_vllm.sh
Signed-off-by: Craig Magina <cmagina@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is part 6 of 11 in the rework modernization effort. It adds the vLLM setup script as part of the modular framework installation architecture.
Changes:
scripts/setup_vllm.shfor vLLM installation and configurationFeatures:
INSTALL_VLLMenvironment variable:source: Build from source (with auto-download if not mounted)release,nightly: Install wheels from PyPIskip: Skip vLLM installationWhy this change?
vLLM is an important framework supported by this repository. This dedicated setup script:
Testing:
Dependencies:
Related PRs:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores