Improve flexibility of auto_tune.sh execution. #23766
Conversation
|
👋 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 You ask your reviewers to trigger select CI tests on top of 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 If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the flexibility of the auto_tune.sh script by allowing benchmark parameters to be passed as environment variables, which is a useful enhancement. The implementation correctly uses parameter expansion for default values. However, I've identified a high-severity issue in the new logging statements where variables are not quoted. This could lead to unexpected shell expansion and globbing, potentially causing incorrect output or errors. I've provided a code suggestion to fix this by properly quoting the variables.
anthonsu
force-pushed
the
main
branch
4 times, most recently
from
5e06902 to
6cc652a
Compare
Signed-off-by: Anthony Su <50185138+anthonsu@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: anthonsu <50185138+anthonsu@users.noreply.github.com> Signed-off-by: Anthony Su <50185138+anthonsu@users.noreply.github.com>
Signed-off-by: anthonsu <50185138+anthonsu@users.noreply.github.com> Signed-off-by: Anthony Su <50185138+anthonsu@users.noreply.github.com>
|
@Chenyaaang @QiliangCui could you take a look? |
Chenyaaang
left a comment
Chenyaaang
left a comment
There was a problem hiding this comment.
LGTM, thank you for the improvement!
yaochengji
left a comment
yaochengji
left a comment
There was a problem hiding this comment.
LGTM, thanks for the improvement!
github-actions
bot
added
the
ready
Signed-off-by: Anthony Su <50185138+anthonsu@users.noreply.github.com> Signed-off-by: anthonsu <50185138+anthonsu@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Anthony Su <50185138+anthonsu@users.noreply.github.com> Signed-off-by: anthonsu <50185138+anthonsu@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Anthony Su <50185138+anthonsu@users.noreply.github.com> Signed-off-by: anthonsu <50185138+anthonsu@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Signed-off-by: Anthony Su <50185138+anthonsu@users.noreply.github.com> Signed-off-by: anthonsu <50185138+anthonsu@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
3 participants
Purpose
Add the flexibility of passing benchmark parameters through environment variables when executing auto_tune.sh script.
Test Plan
Execute and verify script on local benchmark runs.
Test Result
Passed local bash script execution and verified benchmark parameters were successfully passed through via environment settings.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.