-
Notifications
You must be signed in to change notification settings - Fork 714
ci: Add args to run.sh #1418
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
ci: Add args to run.sh #1418
Conversation
Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com>
Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com>
Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com>
WalkthroughThe script Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
container/run.sh (4)
43-44: Initialize default RUNTIME and WORKDIR variables
The script sets sensible defaults for the Docker runtime and working directory. Consider allowing these to be overridden via environment variables before falling back to defaults or marking them as read-only if immutable.
282-284: ClearRUNTIMEwhen GPUs are disabled
Good addition, though for consistency you could normalize$GPUSonce (e.g.,${GPUS^^}) rather than comparing both cases explicitly.
302-304: Update help text for new options
The help entries are accurate but the description for--runtimecould be more descriptive (e.g. “set Docker runtime plugin: nvidia or none”). Align indentation with other flags.
329-329: Inject--runtimeflag intodocker runinvocation
Using parameter expansion is clean; consider quoting the runtime value (--runtime "$RUNTIME") to avoid issues if the variable ever contains spaces.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
container/run.sh(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
container/run.sh (1)
container/build.sh (1)
missing_requirement(358-360)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: Build and Test - vllm
🔇 Additional comments (3)
container/run.sh (3)
102-109: Correctly parse--runtimeCLI option
The new case branch handles presence and absence of the runtime argument consistently with other options. Good use ofmissing_requirementfor error handling.
118-125: Correctly parse--workdirCLI option
This branch follows the established pattern for options with required values and integrates seamlessly.
336-336: Use configurable working directory
Replacing the hardcoded path with$WORKDIRis correct and properly quoted.
Signed-off-by: Pavithra Vijayakrishnan <160681768+pvijayakrish@users.noreply.github.com>
Overview:
Add --workdir and --runtime flags to run.sh.
Summary by CodeRabbit
--runtimeand--workdircommand-line options for greater flexibility.