Repository navigation
feat: smg serve pass through engine args to engine - #460
Conversation
Summary of ChangesHello @gongwei-130, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the configurability and robustness of backend worker management. By introducing a flexible argument passthrough system and a refined two-pass argument parsing strategy, it allows for greater control over backend-specific settings, particularly for TensorRT-LLM and vLLM. The changes also enhance logging and process handling, making the system more observable and maintainable, while new tests ensure the reliability of these critical updates. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements two-pass CLI parsing to collect backend-specific args, adds PyYAML dependency, and extends WorkerLauncher/ServeOrchestrator APIs to accept and forward Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (parse_serve_args)
participant Orch as ServeOrchestrator
participant Launcher as WorkerLauncher
participant Worker as Backend Worker
CLI->>CLI: two-pass parse -> (backend, args, backend_args)
CLI->>Orch: start(backend, args, backend_args)
Orch->>Launcher: launch(worker_id, args, backend_args)
Launcher->>Launcher: build_command(args, backend_args, host, port)
Launcher->>Worker: spawn subprocess(command, env)
Worker-->>Launcher: stdout/stderr -> forwarded/logged
Worker-->>Orch: health/status (via health checks)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![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 introduces a valuable feature by allowing backend-specific arguments to be passed through the smg serve command, improving argument parsing robustness and extensibility. However, this change also introduces a security risk where sensitive information (like API keys) passed as arguments can be leaked into application logs. It is recommended to implement a redaction mechanism for command-line logging or advise users on more secure methods for passing secrets. Additionally, there's a potential typing bug, a suggestion for more idiomatic code, and a minor test cleanup.
| vLLM's EngineArgs. We manually add the most commonly used arguments. | ||
| TP size is read from the config file, not passed as CLI argument. | ||
| """ | ||
| group = parser.add_argument_group("TensorRT-LLM Options") |
There was a problem hiding this comment.
The --tp-size argument is defined with type=str. However, the _get_tp_size method, which uses this argument's value, is type-hinted to return an int. This will cause a type inconsistency, as getattr(args, "tp_size") will return a string, which is then returned by _get_tp_size. This could lead to runtime errors later when the value is used in arithmetic operations (e.g., in gpu_env). Please change the type to int.
| group = parser.add_argument_group("TensorRT-LLM Options") | |
| group.add_argument("--tp-size", type=int, help="Tensor parallel size (overrides config file)") |
| return env | ||
|
|
||
| def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]: | ||
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | ||
| filtered_backend_args = [] | ||
| skip_next = False | ||
| for arg in backend_args: | ||
| if skip_next: | ||
| skip_next = False | ||
| continue | ||
| if arg in filter_args: | ||
| skip_next = True # Assume all valid args take a value | ||
| continue | ||
| filtered_backend_args.append(arg) |
There was a problem hiding this comment.
The current implementation of _filter_backend_args using a skip_next flag is functional but can be a bit error-prone and less idiomatic in Python. A more robust and readable approach would be to use an iterator over the backend_args list. This avoids manual index management or state flags.
| return env | |
| def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]: | |
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | |
| filtered_backend_args = [] | |
| skip_next = False | |
| for arg in backend_args: | |
| if skip_next: | |
| skip_next = False | |
| continue | |
| if arg in filter_args: | |
| skip_next = True # Assume all valid args take a value | |
| continue | |
| filtered_backend_args.append(arg) | |
| def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]: | |
| """Filter backend_args to remove arguments that are explicitly handled by the launcher.""" | |
| filtered_backend_args = [] | |
| args_iter = iter(backend_args) | |
| filter_set = set(filter_args) | |
| for arg in args_iter: | |
| if arg in filter_set: | |
| # Assume filtered args take a value. Skip the arg and its value. | |
| try: | |
| next(args_iter) | |
| except StopIteration: | |
| pass # Malformed CLI args, but we can ignore. | |
| else: | |
| filtered_backend_args.append(arg) | |
| return filtered_backend_args |
| assert "0.0.0.0" in cmd | ||
| assert "--port" in cmd | ||
| assert "50051" in cmd | ||
| print(cmd) |
There was a problem hiding this comment.
There appears to be a leftover print(cmd) statement from debugging. This should be removed.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 216-217: Update the misleading comment above the cmd.extend call:
instead of "Add optional config file" clarify that this line appends all
remaining backend-specific args filtered by _filter_backend_args; reference the
call site (cmd.extend(self._filter_backend_args(backend_args, ["--model",
"--host", "--port"]))) and change the comment to something like "Append
remaining backend-specific args" so it accurately reflects the behavior.
- Around line 68-81: The _filter_backend_args method currently has a misleading
docstring about "vLLM's grpc_server" and only filters space-separated flags;
update the docstring to state it generically filters backend_args for any
backend, and modify the loop in _filter_backend_args to also recognize and skip
equals-separated forms (e.g., "--model=/path") by checking if an arg starts with
any filter arg plus "=" (or splitting on "=") in addition to the existing exact
match logic; keep using filtered_backend_args, skip_next, backend_args, and
filter_args to identify and drop both "--key value" and "--key=value"
occurrences so duplicates are not passed through.
- Line 358: The --tp-size CLI arg is declared as type=str but later used as an
int in arithmetic (see group.add_argument("--tp-size"), _get_tp_size, and
gpu_env where dp_rank * tp_size and range(base_gpu, base_gpu + tp_size) are
computed); change the argument to parse as an int (type=int) and ensure
_get_tp_size returns an int (or casts the value to int and validates it is
positive) so gpu_env receives an integer tp_size and avoids TypeError.
In `@bindings/python/tests/test_serve.py`:
- Around line 759-760: Tests pass backend_args as a plain string to
ServeOrchestrator, but ServeOrchestrator.__init__ expects backend_args:
list[str] and _filter_backend_args iterates elements; change all test
invocations (the helper calls creating ServeOrchestrator) to pass backend_args
as a list of argument strings (e.g., ["--model-path", "/tmp/model"] or
["--model-path", "/tmp/model"] depending on how your args are tokenized) instead
of a single string so iteration yields whole arguments, and update every
occurrence referenced (lines creating orch = ServeOrchestrator("sglang", args,
backend_args) at the listed test sites).
- Line 488: Remove the leftover debug print by deleting the `print(cmd)`
statement that prints the `cmd` variable in the test (remove the line containing
`print(cmd)` in bindings/python/tests/test_serve.py) so the test output remains
clean; ensure no other debug prints remain around `cmd` or in the same test
function.
| def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]: | ||
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | ||
| filtered_backend_args = [] | ||
| skip_next = False | ||
| for arg in backend_args: | ||
| if skip_next: | ||
| skip_next = False | ||
| continue | ||
| if arg in filter_args: | ||
| skip_next = True # Assume all valid args take a value | ||
| continue | ||
| filtered_backend_args.append(arg) | ||
|
|
||
| return filtered_backend_args |
There was a problem hiding this comment.
Misleading docstring and missing --key=value handling.
Two issues:
-
The docstring references "vLLM's grpc_server" but this is a generic base-class method used by all backends. Update to match actual intent.
-
The filter only handles
--key value(space-separated) form. If a user passes--model=/path/to/model(equals-separated), it won't be filtered and will be duplicated in the command. Consider handling the=form as well.
Proposed fix
- def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]:
- """Filter backend_args to only include those relevant for vLLM's grpc_server."""
+ def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]:
+ """Remove args in *filter_args* (and their values) from *backend_args*."""
filtered_backend_args = []
skip_next = False
for arg in backend_args:
if skip_next:
skip_next = False
continue
- if arg in filter_args:
+ # Handle both `--key value` and `--key=value` forms
+ if arg in filter_args or any(arg.startswith(f"{fa}=") for fa in filter_args):
skip_next = True # Assume all valid args take a value
continue
filtered_backend_args.append(arg)
-
return filtered_backend_args📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]: | |
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | |
| filtered_backend_args = [] | |
| skip_next = False | |
| for arg in backend_args: | |
| if skip_next: | |
| skip_next = False | |
| continue | |
| if arg in filter_args: | |
| skip_next = True # Assume all valid args take a value | |
| continue | |
| filtered_backend_args.append(arg) | |
| return filtered_backend_args | |
| def _filter_backend_args(self, backend_args: list[str], filter_args: list[str]) -> list[str]: | |
| """Remove args in *filter_args* (and their values) from *backend_args*.""" | |
| filtered_backend_args = [] | |
| skip_next = False | |
| for arg in backend_args: | |
| if skip_next: | |
| skip_next = False | |
| continue | |
| # Handle both `--key value` and `--key=value` forms | |
| if arg in filter_args or any(arg.startswith(f"{fa}=") for fa in filter_args): | |
| skip_next = True # Assume all valid args take a value | |
| continue | |
| filtered_backend_args.append(arg) | |
| return filtered_backend_args |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 68 - 81, The
_filter_backend_args method currently has a misleading docstring about "vLLM's
grpc_server" and only filters space-separated flags; update the docstring to
state it generically filters backend_args for any backend, and modify the loop
in _filter_backend_args to also recognize and skip equals-separated forms (e.g.,
"--model=/path") by checking if an arg starts with any filter arg plus "=" (or
splitting on "=") in addition to the existing exact match logic; keep using
filtered_backend_args, skip_next, backend_args, and filter_args to identify and
drop both "--key value" and "--key=value" occurrences so duplicates are not
passed through.
| # Add optional config file | ||
| cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host", "--port"])) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Misleading comment.
The comment says "Add optional config file" but _filter_backend_args passes through all unfiltered backend args, not just config. Consider updating to something like "Append remaining backend-specific args".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 216 - 217, Update the
misleading comment above the cmd.extend call: instead of "Add optional config
file" clarify that this line appends all remaining backend-specific args
filtered by _filter_backend_args; reference the call site
(cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host",
"--port"]))) and change the comment to something like "Append remaining
backend-specific args" so it accurately reflects the behavior.
| assert "0.0.0.0" in cmd | ||
| assert "--port" in cmd | ||
| assert "50051" in cmd | ||
| print(cmd) |
There was a problem hiding this comment.
Remove leftover debug print statement.
print(cmd) appears to be a debug artifact. Remove it to keep test output clean.
Proposed fix
- print(cmd)
for arg in backend_args:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(cmd) | |
| for arg in backend_args: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/tests/test_serve.py` at line 488, Remove the leftover debug
print by deleting the `print(cmd)` statement that prints the `cmd` variable in
the test (remove the line containing `print(cmd)` in
bindings/python/tests/test_serve.py) so the test output remains clean; ensure no
other debug prints remain around `cmd` or in the same test function.
github-actions
Bot
added
python-bindings
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9a2331fe3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| BACKEND_CHOICES = list(BACKEND_ARG_ADDERS.keys()) | ||
| DEFAULT_BACKEND = "sglang" | ||
| DEFAULT_BACKEND = os.getenv("SMG_DEFAULT_BACKEND", "sglang") |
There was a problem hiding this comment.
This default now comes directly from SMG_DEFAULT_BACKEND, but argparse does not enforce choices for default values; if the env var is misspelled (for example sglangg), parsing succeeds and _import_backend_args later indexes BACKEND_ARG_ADDERS[backend] with an unknown key, causing a runtime KeyError traceback instead of a user-facing CLI validation error.
Useful? React with 👍 / 👎.
|
Hi @gongwei-130, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
Signed-off-by: gongwei-130 <weigong28@gmail.com>
f9a2331 to
bc2fbca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 395-396: Fix the grammar in the user-facing help string for the
connection mode argument (the add_argument call that sets help for
"--connection-mode" / "Connection mode for workers"). Update the help text to
read something like "Connection mode for workers (default: grpc). Note: trtllm
only supports grpc." (or "gRPC") so it uses the correct verb and consistent
casing.
---
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 349-364: The CLI declares --tp-size as type=str but it's used
numerically downstream (see _get_tp_size and gpu_env where dp_rank * tp_size and
range(base_gpu, base_gpu + tp_size) are performed); change the argument
definition in _add_trtllm_stub_args so --tp-size is parsed as an integer
(type=int) and add minimal validation (positive int) or coerce/validate in
_get_tp_size to ensure it always returns an int before being used with dp_rank,
base_gpu, and range; update references to the flag name only (no other API
changes).
- Around line 216-217: The comment "Add optional config file" is misleading
because the code actually appends the remaining backend-specific args (after
filtering out --model, --host, --port) to the command; update the comment above
cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host",
"--port"])) to accurately state that it appends the remaining backend args or
filter out those specific flags, and ensure the reference to
_filter_backend_args, backend_args, and cmd.extend is clear so future readers
understand the actual behavior.
- Around line 68-81: The _filter_backend_args method has a misleading docstring
and fails to strip equals-form arguments (e.g., "--model=/path") so filtered
keys can leak through; update the docstring to remove the vLLM-specific
reference and make the logic treat filter_args entries as keys/prefixes (e.g.,
"--model") so it removes both "--key value" pairs and "--key=value" forms and
avoids duplicates. Concretely, in _filter_backend_args look at each arg and if
it equals any filter key or startswith any filter key + "=" then skip it (and
for the separate "--key" token set skip_next to drop the following value);
otherwise append; keep the same parameters (backend_args, filter_args) and
symbols to locate the change.
In `@bindings/python/tests/test_serve.py`:
- Line 488: Remove the leftover debug print by deleting the `print(cmd)`
statement in the test (the debug print referencing the local variable `cmd`) so
test output stays clean; simply remove that line from the test function where
`cmd` is constructed and re-run the tests to confirm no behavior change.
- Around line 759-760: Tests instantiate ServeOrchestrator with backend_args as
a plain string which should be a list[str]; update each test case that sets
backend_args (e.g., the instances around the current diff) to pass a list, e.g.
backend_args = ["--model-path", "/tmp/model"], so _filter_backend_args and
ServeOrchestrator.__init__ receive a list and iteration is over arguments not
characters; ensure all occurrences in the test file that currently use a string
are changed to the equivalent list form (refer to ServeOrchestrator and
_filter_backend_args to locate the affected code).
| help="Connection mode for workers (default: grpc). Note: trtllm only support grpc", | ||
| ) |
There was a problem hiding this comment.
Minor grammar fix in user-facing help text.
- help="Connection mode for workers (default: grpc). Note: trtllm only support grpc",
+ help="Connection mode for workers (default: grpc). Note: trtllm only supports grpc",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| help="Connection mode for workers (default: grpc). Note: trtllm only support grpc", | |
| ) | |
| help="Connection mode for workers (default: grpc). Note: trtllm only supports grpc", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 395 - 396, Fix the grammar in
the user-facing help string for the connection mode argument (the add_argument
call that sets help for "--connection-mode" / "Connection mode for workers").
Update the help text to read something like "Connection mode for workers
(default: grpc). Note: trtllm only supports grpc." (or "gRPC") so it uses the
correct verb and consistent casing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc2fbcad1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cmd = self.build_command(args, host, port) | ||
| return subprocess.Popen(cmd, start_new_session=True, env=env or os.environ.copy()) | ||
| cmd = self.build_command(args, backend_args, host, port) | ||
| logger.info("Launching worker with command: %s", " ".join(cmd)) |
There was a problem hiding this comment.
WorkerLauncher.launch now logs the full worker command at INFO level, which includes raw backend_args; if operators pass secrets such as API keys or model hub tokens via backend flags, those credentials are written to application/CI logs in plaintext. This is a security regression introduced by the new pass-through behavior because previously these backend arguments were not emitted.
Useful? React with 👍 / 👎.
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
Can you update the PR title and fill in the PR descriptions? @gongwei-130
gongwei-130
changed the title
gongwei-130
changed the title
|
Signed-off-by: gongwei-130 <56567052+gongwei-130@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8d6d27677
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stdout=sys.stdout, | ||
| stderr=sys.stderr, |
There was a problem hiding this comment.
WorkerLauncher.launch now passes stdout=sys.stdout and stderr=sys.stderr into subprocess.Popen, which breaks smg serve when it is invoked from contexts that replace stdio streams (for example redirect_stdout, notebook kernels, or some test harnesses) because those stream objects often do not provide a usable fileno(). In those environments worker startup fails before health checks run, while the previous Popen behavior (omitting stdout/stderr) safely inherited parent descriptors.
Useful? React with 👍 / 👎.
| if arg in filter_args: | ||
| skip_next = True # Assume all valid args take a value |
There was a problem hiding this comment.
--flag=value tokens before backend-arg filtering
_filter_backend_args only removes exact matches like --model/--model-path, so equivalent CLI forms such as --model=/path are not filtered and get forwarded to worker commands even when SMG already injected those fields. This defeats the deduplication path and can produce conflicting or unsupported backend invocations (notably for TensorRT-LLM where model is already passed positionally), depending on how users/scripts spell arguments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/python/tests/test_serve.py (1)
898-902: 🧹 Nitpick | 🔵 TrivialRename opaque
capture_launchparametersa,btoargs,backend_args.The positional captures
aandbsilently consumeargsandbackend_argsfromWorkerLauncher.launch. Naming them explicitly makes the intent obvious even though the values are unused in the body.♻️ Proposed fix
- def capture_launch(a, b, host, port, env): + def capture_launch(args, backend_args, host, port, env):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/python/tests/test_serve.py` around lines 898 - 902, Rename the opaque positional parameters in the test helper capture_launch from `a` and `b` to descriptive names `args` and `backend_args` so it's clear they correspond to the positional `args` and `backend_args` passed by WorkerLauncher.launch; update the capture_launch signature to def capture_launch(args, backend_args, host, port, env): and keep the body unchanged (still appending env and returning the MagicMock) so behavior is identical but intent is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/src/smg/serve.py`:
- Line 374: DEFAULT_BACKEND is read from os.getenv without validating it against
BACKEND_CHOICES, which can cause a KeyError later when
BACKEND_LAUNCHERS[backend] is used; after setting DEFAULT_BACKEND (and before
argparse uses it), check if DEFAULT_BACKEND is in BACKEND_CHOICES and if not
either reset it to a safe default (e.g., "sglang") or raise a clear error;
ensure args.backend will only ever be a member of BACKEND_CHOICES by validating
DEFAULT_BACKEND and updating DEFAULT_BACKEND or raising so
BACKEND_LAUNCHERS[args.backend] cannot KeyError.
---
Outside diff comments:
In `@bindings/python/tests/test_serve.py`:
- Around line 898-902: Rename the opaque positional parameters in the test
helper capture_launch from `a` and `b` to descriptive names `args` and
`backend_args` so it's clear they correspond to the positional `args` and
`backend_args` passed by WorkerLauncher.launch; update the capture_launch
signature to def capture_launch(args, backend_args, host, port, env): and keep
the body unchanged (still appending env and returning the MagicMock) so behavior
is identical but intent is explicit.
---
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Line 395: Fix the grammar in the argparse help string for the connection mode
option: locate the add_argument call that defines the "Connection mode for
workers" help text (the help= parameter in serve.py where it mentions trtllm)
and change "only support grpc" to "only supports grpc" so the sentence is
grammatically correct.
- Around line 216-217: The comment above the cmd.extend call is misleading: it
says "Add optional config file" but the code
cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host",
"--port"])) actually forwards all remaining backend args except the filtered
ones; update the comment to accurately describe the behaviour (e.g., "Forward
remaining backend args, excluding --model/--host/--port") or change the call to
only append a config-file-specific arg if the intent was to add a config file;
reference _filter_backend_args and the cmd.extend(...) invocation when making
the change.
- Around line 68-81: _docstring for _filter_backend_args is misleading and the
function incorrectly assumes all filter args take a separate value and thus
drops only the next token; update the docstring to say it removes filter args
whether they are passed as separate tokens or in --key=value form, and modify
the loop in _filter_backend_args to treat an arg as a filtered item if it
exactly matches an entry in filter_args or startswith f"{filter_arg}=" for any
filter_arg (so it skips both "--key value" and "--key=value" forms) and only set
skip_next when an exact match without '=' is found; refer to variables/function
names _filter_backend_args, backend_args, filter_args, filtered_backend_args,
and skip_next when making the change.
In `@bindings/python/tests/test_serve.py`:
- Around line 488-490: Remove the leftover debug print by deleting the
standalone print(cmd) call in the test so it no longer emits runtime output;
leave the subsequent assertion loop that checks each arg in backend_args against
cmd (the variables cmd and backend_args are the ones to keep intact).
|
|
||
| BACKEND_CHOICES = list(BACKEND_ARG_ADDERS.keys()) | ||
| DEFAULT_BACKEND = "sglang" | ||
| DEFAULT_BACKEND = os.getenv("SMG_DEFAULT_BACKEND", "sglang") |
There was a problem hiding this comment.
DEFAULT_BACKEND from env var is not validated against BACKEND_CHOICES, leading to a KeyError at runtime.
argparse does not check defaults against choices — validation only applies to values explicitly supplied on the command line. If SMG_DEFAULT_BACKEND is set to an unrecognised backend string, args.backend will silently hold that invalid value and BACKEND_LAUNCHERS[backend]() will raise a KeyError.
🛡️ Proposed fix — validate immediately after reading the env var
-DEFAULT_BACKEND = os.getenv("SMG_DEFAULT_BACKEND", "sglang")
+DEFAULT_BACKEND = os.getenv("SMG_DEFAULT_BACKEND", "sglang")
+if DEFAULT_BACKEND not in BACKEND_CHOICES:
+ raise ValueError(
+ f"SMG_DEFAULT_BACKEND={DEFAULT_BACKEND!r} is not a valid backend; "
+ f"choose from {BACKEND_CHOICES}"
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` at line 374, DEFAULT_BACKEND is read from
os.getenv without validating it against BACKEND_CHOICES, which can cause a
KeyError later when BACKEND_LAUNCHERS[backend] is used; after setting
DEFAULT_BACKEND (and before argparse uses it), check if DEFAULT_BACKEND is in
BACKEND_CHOICES and if not either reset it to a safe default (e.g., "sglang") or
raise a clear error; ensure args.backend will only ever be a member of
BACKEND_CHOICES by validating DEFAULT_BACKEND and updating DEFAULT_BACKEND or
raising so BACKEND_LAUNCHERS[args.backend] cannot KeyError.
|
@CatherineSue comments addressed and all tests passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27b10bfd8d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e76e39be5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if getattr(args, "connection_mode", "grpc") == "grpc": | ||
| cmd.append("--grpc-mode") | ||
|
|
||
| cmd.extend(self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"])) |
There was a problem hiding this comment.
--grpc-mode from passthrough args in HTTP mode
SglangWorkerLauncher.build_command now appends raw backend_args after deriving transport from --connection-mode, but it only filters --model-path/--host/--port. If a user runs with --connection-mode http and also passes backend flags that include --grpc-mode (e.g., reused sglang launch args), the worker is launched in gRPC mode while health_check() and worker_url() still follow HTTP from args.connection_mode, so startup can fail with repeated unhealthy checks and the router gets mismatched http:// URLs. This conflict was introduced by the new passthrough behavior and should be filtered or reconciled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 119-122: The code unconditionally appends "--grpc-mode" when
args.connection_mode == "grpc" (in SglangWorkerLauncher/serve.py) but then calls
self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"])
which does not include "--grpc-mode", causing duplicate flags if a user supplied
it in backend_args; update the filter list passed to _filter_backend_args to
include "--grpc-mode" so the user-supplied flag is filtered out and only the
single appended "--grpc-mode" remains, ensuring no duplicate is emitted.
- Around line 135-139: The variable vllm_entry_points is misleadingly plural
while it holds a single string; rename it to vllm_entry_point throughout the
code (declaration in serve.py and every usage site) to reflect singular value,
update any references in surrounding logic (including getattr(args,
"connection_mode", "grpc") branch) and adjust any docs/comments or tests that
reference vllm_entry_points to use vllm_entry_point instead.
- Around line 182-194: The current broad except Exception around the import and
file parsing hides ImportError from "import yaml" and misreports it as a config
read failure; update the block to handle ImportError separately (e.g., except
ImportError as ie: logger.warning("PyYAML not installed, cannot read %s: %s",
config_path, ie)) and keep a narrower except Exception as e for file I/O or YAML
parse errors (logger.warning("Failed to read tensor_parallel_size from config
%s: %s", config_path, e)); reference the import yaml line, the config_path
variable, and the logger when making the change.
- Around line 349-365: The docstring for _add_trtllm_stub_args incorrectly
states that TP size is read from the config file and not passed via CLI; update
the docstring to reflect that a --tp-size CLI argument is provided (and may
override config), or remove the sentence entirely. Locate the
_add_trtllm_stub_args function and modify its docstring text to accurately
describe that --tp-size is available as a CLI flag and that the config file
option still exists and can be overridden by the CLI.
- Around line 458-459: In serve_main(argv: list[str] | None = None) the current
guard replaces argv None with an empty list which causes
pre_parser.parse_known_args(argv) and parser.parse_args(argv) to parse nothing;
remove the `if argv is None: argv = []` fallback so that argparse receives None
and falls back to sys.argv as intended, leaving the rest of the code (calls to
pre_parser.parse_known_args(argv) and parser.parse_args(argv)) unchanged.
---
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 216-217: The comment "# Add optional config file" is misleading
for the cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host",
"--port"])) call; update the comment to accurately describe that this line
extends the command with filtered backend arguments (specifically --model,
--host, and --port) or rename it to something like "# Add filtered backend args
(model, host, port)" so the intent in serve.py next to cmd.extend(...) is clear
and accurate.
- Around line 395-396: Fix the grammar in the help string that currently reads
"trtllm only support grpc" to "trtllm only supports grpc" where the argument's
help is defined (the help parameter passed to add_argument containing
"Connection mode for workers (default: grpc). Note: trtllm only support grpc").
Update that literal to use "supports" so the full help becomes: "Connection mode
for workers (default: grpc). Note: trtllm only supports grpc".
- Line 374: DEFAULT_BACKEND is taken from SMG_DEFAULT_BACKEND but never
validated against BACKEND_CHOICES, so an invalid env value can become
args.backend and trigger a KeyError in BACKEND_LAUNCHERS[backend](); fix by
validating the env value at import/initialization: check if DEFAULT_BACKEND is
in BACKEND_CHOICES and if not either set DEFAULT_BACKEND to a safe fallback
(e.g., "sglang") or raise a clear ValueError, and ensure the argparse default
for the backend uses this validated DEFAULT_BACKEND so args.backend is always a
known choice.
- Around line 68-81: The _filter_backend_args function incorrectly assumes every
filtered token takes a separate value and also doesn't handle "--key=value"
forms; update the docstring to remove the vLLM-specific reference and describe
handling of both "--flag" and "--flag=value" forms, then change the loop logic
in _filter_backend_args so that when a token matches a filter entry you do one
of three things: (1) if the token contains '=' (e.g., "--model=/path"), drop it
and do not set skip_next; (2) if the token exactly equals a known value-bearing
flag (use filter_args membership or a separate set of value-bearing names) then
set skip_next=True to skip the next token; (3) if the token exactly equals a
boolean flag (a filter entry that is known to be store_true), drop it but do not
set skip_next; use the existing names backend_args, filter_args, and skip_next
to implement this behavior so only real value tokens are skipped and
"--key=value" tokens are handled correctly.
| if getattr(args, "connection_mode", "grpc") == "grpc": | ||
| cmd.append("--grpc-mode") | ||
|
|
||
| cmd.extend(self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"])) |
There was a problem hiding this comment.
--grpc-mode missing from SglangWorkerLauncher's filter list — silent duplication possible.
When connection_mode == "grpc", --grpc-mode is unconditionally appended to the command at line 120. Because "--grpc-mode" is absent from the _filter_backend_args call's filter list on line 122, any user who also passes --grpc-mode as a raw backend arg (a native sglang flag) will have it emitted twice in the subprocess command, which sglang's parser may reject or behave unexpectedly on.
🐛 Proposed fix
- cmd.extend(self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"]))
+ cmd.extend(
+ self._filter_backend_args(
+ backend_args,
+ ["--model-path", "--host", "--port"],
+ bool_filter_args=["--grpc-mode"],
+ )
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if getattr(args, "connection_mode", "grpc") == "grpc": | |
| cmd.append("--grpc-mode") | |
| cmd.extend(self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"])) | |
| if getattr(args, "connection_mode", "grpc") == "grpc": | |
| cmd.append("--grpc-mode") | |
| cmd.extend(self._filter_backend_args(backend_args, ["--model-path", "--host", "--port", "--grpc-mode"])) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 119 - 122, The code
unconditionally appends "--grpc-mode" when args.connection_mode == "grpc" (in
SglangWorkerLauncher/serve.py) but then calls
self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"])
which does not include "--grpc-mode", causing duplicate flags if a user supplied
it in backend_args; update the filter list passed to _filter_backend_args to
include "--grpc-mode" so the user-supplied flag is filtered out and only the
single appended "--grpc-mode" remains, ensuring no duplicate is emitted.
| vllm_entry_points = ( | ||
| "vllm.entrypoints.grpc_server" | ||
| if getattr(args, "connection_mode", "grpc") == "grpc" | ||
| else "vllm.entrypoints.openai.api_server" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: vllm_entry_points variable name is misleadingly plural.
The variable holds a single string. Rename to vllm_entry_point (singular) to avoid confusion.
♻️ Proposed rename
- vllm_entry_points = (
+ vllm_entry_point = (
"vllm.entrypoints.grpc_server"
if getattr(args, "connection_mode", "grpc") == "grpc"
else "vllm.entrypoints.openai.api_server"
)
cmd = [
sys.executable,
"-m",
- vllm_entry_points,
+ vllm_entry_point,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vllm_entry_points = ( | |
| "vllm.entrypoints.grpc_server" | |
| if getattr(args, "connection_mode", "grpc") == "grpc" | |
| else "vllm.entrypoints.openai.api_server" | |
| ) | |
| vllm_entry_point = ( | |
| "vllm.entrypoints.grpc_server" | |
| if getattr(args, "connection_mode", "grpc") == "grpc" | |
| else "vllm.entrypoints.openai.api_server" | |
| ) | |
| cmd = [ | |
| sys.executable, | |
| "-m", | |
| vllm_entry_point, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 135 - 139, The variable
vllm_entry_points is misleadingly plural while it holds a single string; rename
it to vllm_entry_point throughout the code (declaration in serve.py and every
usage site) to reflect singular value, update any references in surrounding
logic (including getattr(args, "connection_mode", "grpc") branch) and adjust any
docs/comments or tests that reference vllm_entry_points to use vllm_entry_point
instead.
| try: | ||
| import yaml | ||
|
|
||
| with open(config_path) as f: | ||
| config = yaml.safe_load(f) | ||
| if config and "tensor_parallel_size" in config: | ||
| return int(config["tensor_parallel_size"]) | ||
| if config and "tp_size" in config: | ||
| return int(config["tp_size"]) | ||
| except Exception as e: | ||
| logger.warning( | ||
| "Failed to read tensor_parallel_size from config %s: %s", config_path, e | ||
| ) |
There was a problem hiding this comment.
Bare except Exception swallows ImportError from import yaml with a misleading log.
If PyYAML is not installed, import yaml at line 183 raises ImportError, which is caught by the broad except Exception and logged as "Failed to read tensor_parallel_size from config %s: %s". The user sees a config-read error when the real problem is a missing package, making diagnosis much harder.
🐛 Proposed fix
- try:
- import yaml
-
- with open(config_path) as f:
- config = yaml.safe_load(f)
- if config and "tensor_parallel_size" in config:
- return int(config["tensor_parallel_size"])
- if config and "tp_size" in config:
- return int(config["tp_size"])
- except Exception as e:
- logger.warning(
- "Failed to read tensor_parallel_size from config %s: %s", config_path, e
- )
+ try:
+ import yaml
+ except ImportError:
+ logger.warning(
+ "PyYAML is not installed; cannot read tensor_parallel_size from config %s. "
+ "Install it with: pip install pyyaml",
+ config_path,
+ )
+ else:
+ try:
+ with open(config_path) as f:
+ config = yaml.safe_load(f)
+ if config and "tensor_parallel_size" in config:
+ return int(config["tensor_parallel_size"])
+ if config and "tp_size" in config:
+ return int(config["tp_size"])
+ except Exception as e:
+ logger.warning(
+ "Failed to read tensor_parallel_size from config %s: %s", config_path, e
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 182 - 194, The current broad
except Exception around the import and file parsing hides ImportError from
"import yaml" and misreports it as a config read failure; update the block to
handle ImportError separately (e.g., except ImportError as ie:
logger.warning("PyYAML not installed, cannot read %s: %s", config_path, ie)) and
keep a narrower except Exception as e for file I/O or YAML parse errors
(logger.warning("Failed to read tensor_parallel_size from config %s: %s",
config_path, e)); reference the import yaml line, the config_path variable, and
the logger when making the change.
| def _add_trtllm_stub_args(parser: argparse.ArgumentParser) -> None: | ||
| """Stub for TRT-LLM args until full integration.""" | ||
| group = parser.add_argument_group("TRT-LLM Options (stub)") | ||
| group.add_argument("--model", type=str, help="Model path") | ||
| """Add TensorRT-LLM specific arguments. | ||
|
|
||
| Note: TensorRT-LLM doesn't provide a centralized argument manager like | ||
| vLLM's EngineArgs. We manually add the most commonly used arguments. | ||
| TP size is read from the config file, not passed as CLI argument. | ||
| """ | ||
| group = parser.add_argument_group("TensorRT-LLM Options") | ||
| group.add_argument("--model", type=str, help="Model path (HuggingFace ID or local path)") | ||
| group.add_argument("--tp-size", type=int, help="Tensor parallel size (overrides config file)") | ||
| group.add_argument( | ||
| "--config", | ||
| type=str, | ||
| required=False, | ||
| help="Config file path (YAML, optional - must contain tensor_parallel_size if provided)", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Stale docstring contradicts the code directly below it.
Line 354 says "TP size is read from the config file, not passed as CLI argument" but --tp-size is explicitly added as a CLI argument at line 358.
📝 Proposed fix
- Note: TensorRT-LLM doesn't provide a centralized argument manager like
- vLLM's EngineArgs. We manually add the most commonly used arguments.
- TP size is read from the config file, not passed as CLI argument.
+ Note: TensorRT-LLM doesn't provide a centralized argument manager like
+ vLLM's EngineArgs. We manually add the most commonly used arguments.
+ TP size can be provided via --tp-size CLI arg or via the YAML config file
+ (CLI arg takes precedence).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 349 - 365, The docstring for
_add_trtllm_stub_args incorrectly states that TP size is read from the config
file and not passed via CLI; update the docstring to reflect that a --tp-size
CLI argument is provided (and may override config), or remove the sentence
entirely. Locate the _add_trtllm_stub_args function and modify its docstring
text to accurately describe that --tp-size is available as a CLI flag and that
the config file option still exists and can be overridden by the CLI.
| if argv is None: | ||
| argv = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find entry-point declarations for serve_main
echo "=== Searching for entry-point declarations ==="
fd -e toml -e cfg -e py | head -20 | xargs grep -l "entry" 2>/dev/null | head -5
# Look for pyproject.toml specifically
echo -e "\n=== Checking pyproject.toml ==="
find . -name "pyproject.toml" | head -3 | xargs cat 2>/dev/null
# Look for setup files
echo -e "\n=== Checking setup.py/setup.cfg ==="
find . -name "setup.py" -o -name "setup.cfg" | head -3 | xargs cat 2>/dev/nullRepository: lightseekorg/smg
Length of output: 8414
🏁 Script executed:
#!/bin/bash
# Examine the serve.py file around lines 458-459
echo "=== Code at lines 450-470 in serve.py ==="
sed -n '450,470p' bindings/python/src/smg/serve.py
# Check the function signature of serve_main
echo -e "\n=== serve_main function signature ==="
rg -n "def serve_main" -A 5 bindings/python/src/smg/serve.pyRepository: lightseekorg/smg
Length of output: 1404
🏁 Script executed:
#!/bin/bash
# Search for where serve_main is called or used
echo "=== Usages of serve_main ==="
rg -n "serve_main" --type py
# Check if there's a __main__ or cli entry point
echo -e "\n=== Check for __main__ block ==="
rg -n "__main__" -A 10 bindings/python/src/smg/serve.py | head -20Repository: lightseekorg/smg
Length of output: 353
🏁 Script executed:
#!/bin/bash
# Examine cli.py to see how serve_main is called and what 'unknown' is
echo "=== cli.py around the serve_main call (lines 100-120) ==="
sed -n '100,130p' bindings/python/src/smg/cli.py
# Look at the broader context of the main function
echo -e "\n=== cli.py main function (first 50 lines) ==="
sed -n '1,150p' bindings/python/src/smg/cli.py | head -80Repository: lightseekorg/smg
Length of output: 3170
🏁 Script executed:
#!/bin/bash
# Check the entire cli.py to understand the argument flow
echo "=== Full cli.py ==="
cat bindings/python/src/smg/cli.pyRepository: lightseekorg/smg
Length of output: 3795
🏁 Script executed:
#!/bin/bash
# Search for direct calls to serve_main to verify if it's ever called with argv=None
echo "=== All references to serve_main ==="
rg -n "serve_main" --type py
# Check test files
echo -e "\n=== serve_main in test files ==="
fd -e py -path "*/tests/*" | xargs grep -n "serve_main" 2>/dev/null
# Check if serve_main is documented or used as a library function
echo -e "\n=== Check serve.py for any direct test or example usage ==="
sed -n '1,50p' bindings/python/src/smg/serve.pyRepository: lightseekorg/smg
Length of output: 2199
🏁 Script executed:
#!/bin/bash
# Verify argparse behavior with None vs []
python3 << 'EOF'
import argparse
import sys
# Test 1: What happens with argv=None
parser1 = argparse.ArgumentParser()
parser1.add_argument('--test', default='default_value')
# Simulate the serve.py code: if argv is None: argv = []
print("=== Test: argv=None converted to [] ===")
argv = None
if argv is None:
argv = []
args = parser1.parse_args(argv)
print(f"With argv=[]: --test value = {args.test}")
# Test 2: What should happen - let argparse handle None
print("\n=== Test: argv=None passed directly to argparse ===")
sys.argv = ['prog', '--test', 'cli_value']
parser2 = argparse.ArgumentParser()
parser2.add_argument('--test', default='default_value')
args = parser2.parse_args(None)
print(f"With argv=None: --test value = {args.test}")
# Test 3: parse_known_args behavior
print("\n=== Test: parse_known_args with [] ===")
sys.argv = ['prog', '--test', 'cli_value']
parser3 = argparse.ArgumentParser()
parser3.add_argument('--test', default='default_value')
args, unknown = parser3.parse_known_args([])
print(f"With argv=[]: --test value = {args.test}, unknown = {unknown}")
print("\n=== Test: parse_known_args with None ===")
sys.argv = ['prog', '--test', 'cli_value']
parser4 = argparse.ArgumentParser()
parser4.add_argument('--test', default='default_value')
args, unknown = parser4.parse_known_args(None)
print(f"With argv=None: --test value = {args.test}, unknown = {unknown}")
EOFRepository: lightseekorg/smg
Length of output: 430
Critical: argv = [] when argv=None silently discards all CLI arguments.
argparse.parse_args() when passed None falls through to sys.argv[1:]; when passed [] it parses an empty list. By converting None → [] at line 459, both pre_parser.parse_known_args(argv) and parser.parse_args(argv) receive an empty list. Although the current CLI always calls serve_main(unknown) with a list, the function signature serve_main(argv: list[str] | None = None) explicitly permits None (e.g., for testing or library usage). If argv=None is ever passed, every argument the user types on the command line is silently discarded and only defaults are applied, completely breaking the command.
Remove the guard to let argparse handle None natively:
🐛 Proposed fix
- if argv is None:
- argv = []
-
- # Pass 1: serve + router args only; unknown tokens become backend_args
+ # Pass 1: serve + router args only; unknown tokens become backend_args
+ # argv=None → argparse reads sys.argv[1:] in both passes (intended behaviour)
pre_parser = argparse.ArgumentParser(add_help=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/src/smg/serve.py` around lines 458 - 459, In serve_main(argv:
list[str] | None = None) the current guard replaces argv None with an empty list
which causes pre_parser.parse_known_args(argv) and parser.parse_args(argv) to
parse nothing; remove the `if argv is None: argv = []` fallback so that argparse
receives None and falls back to sys.argv as intended, leaving the rest of the
code (calls to pre_parser.parse_known_args(argv) and parser.parse_args(argv))
unchanged.
Signed-off-by: gongwei-130 <56567052+gongwei-130@users.noreply.github.com>
2e76e39 to
dcff2c6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcff2c65ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| group.add_argument("--model", type=str, help="Model path (HuggingFace ID or local path)") | ||
| group.add_argument("--tp-size", type=int, help="Tensor parallel size (overrides config file)") | ||
| group.add_argument( | ||
| "--config", |
There was a problem hiding this comment.
smg serve now advertises backend-arg pass-through, but TRT-LLM still only registers --model, --tp-size, and --config in _add_trtllm_stub_args, while parse_serve_args() does a strict parse_args() pass; any other valid TRT-LLM CLI flag is rejected before launch instead of being forwarded. This means common TensorRT-LLM tuning/runtime options outside this stub cannot be used through smg serve --backend trtllm, so the new pass-through path is effectively broken for that backend.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 119-122: The code unconditionally appends "--grpc-mode" when
getattr(args, "connection_mode", "grpc") == "grpc" which can duplicate the flag
if the user also passed "--grpc-mode" in backend_args; update the call to
self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"]) to
also exclude "--grpc-mode" and pass it via the bool_filter_args parameter (e.g.
include "--grpc-mode" in the boolean filters) so the flag is only added once to
cmd; adjust the invocation around cmd.append("--grpc-mode") and
_filter_backend_args to use the bool_filter_args option accordingly.
- Line 374: The DEFAULT_BACKEND value pulled from the environment is not
validated, so an unrecognized SMG_DEFAULT_BACKEND will later cause a KeyError in
ServeOrchestrator.__init__ when calling BACKEND_LAUNCHERS[backend](); update the
code that sets DEFAULT_BACKEND to validate against the keys of BACKEND_LAUNCHERS
(or a canonical list of allowed backends) and if the env value is invalid either
fall back to a safe default like "sglang" or raise a clear ValueError indicating
the invalid SMG_DEFAULT_BACKEND and listing valid choices; ensure
ServeOrchestrator.__init__ uses the validated backend variable.
- Around line 349-365: The docstring in _add_trtllm_stub_args is stale: it
states "TP size is read from the config file, not passed as CLI argument" while
the function actually registers a --tp-size CLI option; update the docstring to
accurately reflect behavior (e.g., mention that TP size may be provided via
--tp-size or via the config file) or remove the contradictory sentence so the
docstring matches the added group.add_argument("--tp-size", ...) behavior.
- Around line 68-81: _update the docstring of _filter_backend_args to remove
vLLM-specific wording and state it's a generic backend arg filter; change the
loop logic to treat filter_args as pairs or metadata indicating whether an
option expects a value (or accept a convention: if a filter arg is present in
the form "--name" then treat ":=" or explicit value forms), handle "--key=value"
by matching the prefix before '=' against filter_args and skip only that token
(not the next), and only set skip_next=True when the matched filter arg is known
to accept a separate value (so boolean flags like "--grpc-mode" are matched but
do not consume the following token); ensure filtered_backend_args appends
untouched tokens otherwise so duplicates are avoided. Reference symbols:
function _filter_backend_args, parameter backend_args, parameter filter_args,
and the skip_next logic.
- Around line 458-459: The code forcibly changes argv None to an empty list,
which breaks CLI behavior by preventing argparse from falling back to sys.argv;
in the function where argv is accepted (the parse/CLI entry in serve.py that
calls argparse.parse_known_args / parse_args), remove the assignment that
converts None to [] and allow argv to remain None so argparse can use
sys.argv[1:], or explicitly pass None into parse_args/parse_known_args instead
of an empty list (identify the block that currently sets "if argv is None: argv
= []" and delete or replace it).
- Around line 182-194: The current try/except around "import yaml" and the file
parse in serve.py swallows ImportError and reports a misleading "Failed to read
tensor_parallel_size..." message; split the import and file parsing so
ImportError is handled separately: attempt "import yaml" first and on
ImportError log a clear message (using logger.warning or logger.error) that
PyYAML is not installed, then proceed to a narrower try/except around opening
config_path and yaml.safe_load to handle file/parse errors and convert found
keys ("tensor_parallel_size" / "tp_size") to int; reference the existing logger
and the code block that reads config_path and checks those keys to localize
changes.
- Around line 198-219: The build_command method is forwarding SMG-only flags
(--config and --tp-size) from backend_args into the tensorrt_llm.commands.serve
subprocess via cmd.extend(self._filter_backend_args(...)), causing parse errors;
update TrtllmWorkerLauncher.build_command to (1) filter out SMG-only flags
(--config, --tp-size) or translate them to TRT-LLM equivalents before extending
cmd (map --config -> --extra_llm_api_options <path> and --tp-size -> --tp_size
<value>), using the existing _filter_backend_args helper to locate allowed
flags, and (2) replace the stale comment "# Add optional config file" with a
description like "Append filtered backend args, translating SMG-only flags to
TRT-LLM flags" so the intent is clear. Ensure mapping handles presence/absence
and preserves ordering when building the final cmd list.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 429ddad038
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| group = parser.add_argument_group("TensorRT-LLM Options") | ||
| group.add_argument("--model", type=str, help="Model path (HuggingFace ID or local path)") | ||
| group.add_argument("--tp-size", type=int, help="Tensor parallel size (overrides config file)") |
There was a problem hiding this comment.
The new --tp-size serve option is forwarded verbatim to the TensorRT-LLM worker command, but the TensorRT-LLM invocation documented in this repo uses --tp_size (underscore) for that CLI (docs/getting-started/index.md, TensorRT-LLM gRPC example). In the current flow, smg serve --backend trtllm --tp-size 2 will append --tp-size 2 to tensorrt_llm.commands.serve, which can cause the worker process to exit on unrecognized arguments before health checks succeed.
Useful? React with 👍 / 👎.
|
Hi @slin1237 @CatherineSue may you help check, anything else needs to be done, thanks! |
4 participants
Description
pass through engine args to engine when launching workers.
Problem
SMG doesn't populate engine args to engine when launching workers.
Solution
Treat all un-recognized args from SMG/router side as engine args and pass down to engine. Filtered some parameters out which are populated by SMG.
Changes
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Improvements
Tests
Dependencies