-
Notifications
You must be signed in to change notification settings - Fork 46
feat(vllm): add Gemma 4 models, image, and ROCm serving recipes #144
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
Open
coketaste
wants to merge
8
commits into
ROCm:develop
Choose a base branch
from
coketaste:coketaste/gemma4
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
4e188f6
feat(vllm): add Gemma 4 models, image, and ROCm serving recipes
coketaste 912aebc
Merge branch 'ROCm:develop' into coketaste/gemma4
coketaste b4de892
chore(vllm): bump base image to vllm-openai-rocm v0.20.0, pin transfo…
coketaste 77f0161
feat(vllm): consolidate Gemma 4 models into standard pyt_vllm stack
coketaste 76bc276
docs(vllm): update README — Gemma 4 now uses standard pyt_vllm image
coketaste b455096
fix(vllm): apply shlex.quote to all non-bool extra_args values (was p…
coketaste 11d6bd8
fix(vllm): address PR review feedback and enable full concurrency
coketaste cac3f03
fix(vllm): address Copilot review — ENTRYPOINT, bool flags, testable …
coketaste File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -487,6 +487,44 @@ | |
| "args": | ||
| "--model_repo Qwen/Qwen3-8B --config configs/extended.yaml" | ||
| }, | ||
| { | ||
| "name": "pyt_vllm_gemma-4-26b-a4b-it", | ||
| "data": "huggingface", | ||
| "dockerfile": "docker/pyt_vllm", | ||
| "scripts": "scripts/vllm/run.sh", | ||
| "n_gpus": "-1", | ||
| "owner": "[email protected]", | ||
| "training_precision": "", | ||
| "multiple_results": "perf_gemma-4-26B-A4B-it.csv", | ||
| "tags": [ | ||
| "pyt", | ||
| "vllm", | ||
| "vllm_extended", | ||
| "inference" | ||
| ], | ||
| "timeout": -1, | ||
| "args": | ||
| "--model_repo google/gemma-4-26B-A4B-it --config configs/default.yaml" | ||
|
coketaste marked this conversation as resolved.
|
||
| }, | ||
| { | ||
| "name": "pyt_vllm_gemma-4-31b-it", | ||
| "data": "huggingface", | ||
| "dockerfile": "docker/pyt_vllm", | ||
| "scripts": "scripts/vllm/run.sh", | ||
| "n_gpus": "-1", | ||
| "owner": "[email protected]", | ||
| "training_precision": "", | ||
| "multiple_results": "perf_gemma-4-31B-it.csv", | ||
| "tags": [ | ||
| "pyt", | ||
| "vllm", | ||
| "vllm_extended", | ||
| "inference" | ||
| ], | ||
| "timeout": -1, | ||
| "args": | ||
| "--model_repo google/gemma-4-31B-it --config configs/default.yaml" | ||
| }, | ||
| { | ||
| "name": "pyt_vllm_qwen3-32b", | ||
| "data": "huggingface", | ||
|
|
||
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
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
Empty file.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| """Tests for extra_args quoting in run_vllm.py.""" | ||
| import sys | ||
| import os | ||
| import shlex | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
|
coketaste marked this conversation as resolved.
Outdated
|
||
| def build_extra_args_str_old(extra_args: dict) -> str: | ||
| """Replicates the OLD selective-quoting logic from run_vllm.py (pre-fix).""" | ||
| extra_args_str = "" | ||
| for k, v in extra_args.items(): | ||
| if isinstance(v, bool): | ||
| extra_args_str += f" {k}" | ||
| else: | ||
| s = str(v) | ||
| st = s.strip() | ||
| if ( | ||
| k == "--limit-mm-per-prompt" | ||
| or (st[:1] in "{[") | ||
| or any(ch.isspace() for ch in s) | ||
| ): | ||
| extra_args_str += f" {k} {shlex.quote(s)}" | ||
| else: | ||
| extra_args_str += f" {k} {s}" | ||
| return extra_args_str.strip() | ||
|
|
||
|
|
||
| def build_extra_args_str_new(extra_args: dict) -> str: | ||
| """Replicates the NEW universal-quoting logic (post-fix).""" | ||
| extra_args_str = "" | ||
| for k, v in extra_args.items(): | ||
| if isinstance(v, bool): | ||
| extra_args_str += f" {k}" | ||
| else: | ||
| extra_args_str += f" {k} {shlex.quote(str(v))}" | ||
| return extra_args_str.strip() | ||
|
coketaste marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| # --- Tests that FAIL with the old logic, PASS with the new --- | ||
|
|
||
| def test_shell_metachar_no_space_is_quoted_by_new(): | ||
| """Values with shell metacharacters but no spaces are NOT quoted by old code. | ||
|
|
||
| The old code only quotes when there's whitespace, a JSON-like prefix, or the | ||
| --limit-mm-per-prompt key. A value like 'foo;bar' (no space) slips through | ||
| unquoted, allowing shell injection. The new code always quotes. | ||
| """ | ||
| args = {"--some-arg": "foo;bar"} | ||
| old = build_extra_args_str_old(args) | ||
| new = build_extra_args_str_new(args) | ||
| # Old code: no whitespace -> not quoted, semicolon is a live shell metachar | ||
| assert old == "--some-arg foo;bar", f"unexpected old output: {old!r}" | ||
| # New code: shlex.quote wraps the value in single quotes | ||
| assert new == "--some-arg 'foo;bar'", f"unexpected new output: {new!r}" | ||
| assert old != new | ||
|
|
||
|
|
||
| def test_plain_string_with_metachar_is_unquoted_by_old(): | ||
| """Old code leaves plain strings with $ unquoted (variable expansion risk).""" | ||
| args = {"--trust-remote-code": "yes$HOME"} | ||
| old = build_extra_args_str_old(args) | ||
| new = build_extra_args_str_new(args) | ||
| # Old code: no whitespace, no JSON prefix -> raw string passed to shell | ||
| assert old == "--trust-remote-code yes$HOME", f"unexpected old output: {old!r}" | ||
| # New code: always quoted | ||
| assert new == "--trust-remote-code 'yes$HOME'", f"unexpected new output: {new!r}" | ||
|
|
||
|
|
||
| # --- Tests that PASS with BOTH old and new logic --- | ||
|
|
||
| def test_json_value_is_quoted(): | ||
| args = {"--limit-mm-per-prompt": '{"image":0,"audio":0}'} | ||
| result = build_extra_args_str_new(args) | ||
| assert result == """--limit-mm-per-prompt '{"image":0,"audio":0}'""" | ||
|
|
||
|
|
||
| def test_bool_flag_has_no_value(): | ||
| args = {"--async-scheduling": True} | ||
| result = build_extra_args_str_new(args) | ||
| assert result == "--async-scheduling" | ||
|
|
||
|
|
||
| def test_string_with_space_is_quoted(): | ||
| args = {"--served-model-name": "my model"} | ||
| result = build_extra_args_str_new(args) | ||
| assert result == "--served-model-name 'my model'" | ||
|
|
||
|
|
||
| def test_plain_safe_scalar_passthrough(): | ||
| """shlex.quote does not add quotes to safe alphanumeric values.""" | ||
| args = {"--max-model-len": 32768} | ||
| result = build_extra_args_str_new(args) | ||
| # shlex.quote('32768') == '32768' (no shell quoting needed for pure digits) | ||
| assert result == "--max-model-len 32768" | ||
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.
Uh oh!
There was an error while loading. Please reload this page.