Repository navigation
pass through backend args - #441
gongwei-130 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughPropagates backend-specific CLI args through serve parsing and launcher flow: Changes
Sequence DiagramsequenceDiagram
participant CLI as User / CLI
participant Parser as parse_serve_args()
participant Orch as ServeOrchestrator
participant Launcher as WorkerLauncher
participant Builder as build_command() / _filter_backend_args()
participant Worker as Backend Process
CLI->>Parser: invoke serve with argv
Parser-->>CLI: returns (backend, args, backend_args)
CLI->>Orch: ServeOrchestrator(backend, args, backend_args)
Orch->>Launcher: launch(args, backend_args, host, port, env)
Launcher->>Builder: build_command(args, backend_args, host, port)
Builder->>Builder: _filter_backend_args(backend_args, allowed_keys)
Builder-->>Launcher: command + filtered backend args
Launcher->>Worker: Popen(command, env with PYTHONUNBUFFERED, stdout/stderr redirected)
Worker-->>Launcher: process handle
Launcher-->>Orch: subprocess.Popen returned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
|
Hi @gongwei-130, the branch Please use one of the following formats:
Allowed types:
|
|
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:
|
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 enhances the flexibility and configurability of the serving infrastructure by enabling the passing of backend-specific arguments to worker processes. It introduces argument filtering to ensure compatibility and proper configuration for different backends, particularly focusing on TensorRT-LLM integration by adding specific argument parsing and configuration options. Highlights
Changelog
Activity
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
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
bindings/python/src/smg/serve.py (4)
33-36:⚠️ Potential issue | 🟠 MajorABC
build_commandsignature is missing thebackend_argsparameter.All three implementations (
SglangWorkerLauncher,VllmWorkerLauncher,TrtllmWorkerLauncher) acceptbackend_args: list[str], but the abstract method signature on line 34 was not updated. This breaks the Liskov contract and will be flagged by type checkers.Proposed fix
`@abstractmethod` - def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[str]: + def build_command(self, args: argparse.Namespace, backend_args: list[str], host: str, port: int) -> list[str]: """Build the CLI command list to launch a worker.""" ...🤖 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 33 - 36, The abstract method build_command in the base class is missing the backend_args parameter; update its signature to include backend_args: list[str] and adjust its return type to match implementations, and update the docstring accordingly so it aligns with SglangWorkerLauncher.build_command, VllmWorkerLauncher.build_command, and TrtllmWorkerLauncher.build_command; ensure the abstractmethod declaration and any type hints/imports reflect list[str] for backend_args to restore Liskov substitution and satisfy type checkers.
441-473:⚠️ Potential issue | 🟠 MajorReturn type annotation is wrong — function returns a 3-tuple but annotation says 2-tuple.
Line 443 declares
-> tuple[str, argparse.Namespace]but line 473 returns(backend, args, backend_args). The docstring (lines 449-450) is also stale — it doesn't mentionbackend_args.Proposed fix
def parse_serve_args( argv: list[str] | None = None, -) -> tuple[str, argparse.Namespace]: +) -> tuple[str, argparse.Namespace, list[str]]: """Two-pass argument parsing for serve command. Pass 1: Extract --backend with parse_known_args (no backend imports). Pass 2: Build full parser with backend-specific + router args. Returns: - Tuple of (backend_name, parsed_namespace). + Tuple of (backend_name, parsed_namespace, 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 441 - 473, Update parse_serve_args to reflect that it returns three values: change the return type annotation from -> tuple[str, argparse.Namespace] to -> tuple[str, argparse.Namespace, list[str]] (or a more specific type for backend_args) and update the docstring to describe the third returned value backend_args; ensure the return statement (backend, args, backend_args) matches the new annotation and that callers expect/handle the three-tuple. Reference: parse_serve_args, backend_args, add_serve_args, RouterArgs.add_cli_args, and _import_backend_args.
456-473: 🧹 Nitpick | 🔵 Trivial
backend_argsfrom pass 1 may overlap with args already parsed in pass 2.
parse_known_argsin pass 1 (line 459) captures everything unrecognized by (serve + router) intobackend_args. Pass 2 then parses the full originalargvincluding backend-specific args intoargs. So arguments like--modelappear in bothargs(parsed) andbackend_args(raw). The_filter_backend_argshelpers mitigate this for a known set of keys, but any new backend argument that's also explicitly set inbuild_commandfromargswould be duplicated on the command line unless its filter list is kept in sync.Consider re-filtering
backend_argsafter pass 2, or documenting this coupling clearly so future maintainers know to update the filter lists when adding new explicit 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 456 - 473, The first-pass parse_known_args collects backend_args from argv but the second pass fully parses argv into args so flags like --model can end up both parsed in args and still present in backend_args; after parser.parse_args(argv) re-filter backend_args against the final parsed args (call the existing helper like _filter_backend_args or a new function) to remove any keys already represented in args (or those used by build_command), ensuring backend_args contains only true backend-only flags; reference parse_known_args, parser.parse_args, backend_args, args, _filter_backend_args, and build_command when implementing the re-filtering or documenting the coupling.
195-216:⚠️ Potential issue | 🟠 Major
TrtllmWorkerLauncher.build_commandacceptsbackend_argsbut never uses them.Unlike the sglang and vLLM launchers, the TRT-LLM launcher silently drops all extra backend arguments. If this is intentional, add a comment explaining why. Otherwise, implement
_filter_backend_argsand extend the command, consistent with the other launchers.🤖 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 195 - 216, The build_command method in TrtllmWorkerLauncher/serve.py currently ignores the backend_args parameter; implement a _filter_backend_args method (matching sglang/vLLM launchers) to sanitize/allowlist backend_args and then extend the cmd list with the filtered args before returning; update build_command to call self._filter_backend_args(backend_args) and cmd.extend(filtered_args) (or, if dropping backend_args was intentional, add a clear comment inside build_command explaining why backend_args are ignored).
🤖 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 84-86: The docstring for _filter_backend_args incorrectly
references "vLLM's grpc_server" due to copy-paste; update the docstring to
accurately describe that this method filters backend_args for the sglang
launcher (or the sglang backend) and mention the specific flags it keeps (e.g.,
"--model-path", "--host", "--port") so the doc correctly reflects the behavior
of _filter_backend_args and the filtered_args variable.
- Line 355: The CLI argument "--tp-size" is declared with type=str but later
treated as an integer in _get_tp_size and used in arithmetic (dp_rank * tp_size)
inside gpu_env; change the argparse declaration in
group.add_argument("--tp-size", ...) to use type=int so argparse stores an int,
and ensure any callers (_get_tp_size, gpu_env) continue to use the integer value
dp_rank and tp_size without casting.
- Around line 184-189: The YAML loading block returns
config["tensor_parallel_size"] without type validation; update the logic (around
the yaml.safe_load call using config_path in serve.py) to explicitly validate
and cast tensor_parallel_size to an int before returning (e.g., ensure the value
exists, attempt int(value), and raise or fallback if conversion fails), and
propagate the integer into gpu_env usage so downstream arithmetic receives an
integer rather than a string or other type.
- Around line 72-75: The code in launch() uses env before ensuring it's set,
causing a TypeError; fix by resolving env early (e.g., env = env or
os.environ.copy()) before writing env["PYTHONUNBUFFERED"]="1", then call
self.build_command(...) and pass that resolved env into subprocess.Popen
(referencing build_command and the subprocess.Popen(...) call) so you never
index into a None env.
---
Outside diff comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 33-36: The abstract method build_command in the base class is
missing the backend_args parameter; update its signature to include
backend_args: list[str] and adjust its return type to match implementations, and
update the docstring accordingly so it aligns with
SglangWorkerLauncher.build_command, VllmWorkerLauncher.build_command, and
TrtllmWorkerLauncher.build_command; ensure the abstractmethod declaration and
any type hints/imports reflect list[str] for backend_args to restore Liskov
substitution and satisfy type checkers.
- Around line 441-473: Update parse_serve_args to reflect that it returns three
values: change the return type annotation from -> tuple[str, argparse.Namespace]
to -> tuple[str, argparse.Namespace, list[str]] (or a more specific type for
backend_args) and update the docstring to describe the third returned value
backend_args; ensure the return statement (backend, args, backend_args) matches
the new annotation and that callers expect/handle the three-tuple. Reference:
parse_serve_args, backend_args, add_serve_args, RouterArgs.add_cli_args, and
_import_backend_args.
- Around line 456-473: The first-pass parse_known_args collects backend_args
from argv but the second pass fully parses argv into args so flags like --model
can end up both parsed in args and still present in backend_args; after
parser.parse_args(argv) re-filter backend_args against the final parsed args
(call the existing helper like _filter_backend_args or a new function) to remove
any keys already represented in args (or those used by build_command), ensuring
backend_args contains only true backend-only flags; reference parse_known_args,
parser.parse_args, backend_args, args, _filter_backend_args, and build_command
when implementing the re-filtering or documenting the coupling.
- Around line 195-216: The build_command method in TrtllmWorkerLauncher/serve.py
currently ignores the backend_args parameter; implement a _filter_backend_args
method (matching sglang/vLLM launchers) to sanitize/allowlist backend_args and
then extend the cmd list with the filtered args before returning; update
build_command to call self._filter_backend_args(backend_args) and
cmd.extend(filtered_args) (or, if dropping backend_args was intentional, add a
clear comment inside build_command explaining why backend_args are ignored).
| cmd = self.build_command(args, backend_args, host, port) | ||
| logger.info("Launching worker with command: %s", " ".join(cmd)) | ||
| env["PYTHONUNBUFFERED"] = "1" | ||
| return subprocess.Popen(cmd, start_new_session=True, env=env or os.environ.copy(), stdout=sys.stdout, stderr=sys.stderr) |
There was a problem hiding this comment.
env may be None when accessed on line 74, causing TypeError.
env defaults to None (line 69), but line 74 indexes into it before the env or os.environ.copy() fallback on line 75. If launch() is ever called without an explicit env, this crashes.
🐛 Proposed fix: resolve env before use
def launch(
self,
args: argparse.Namespace,
backend_args: list[str],
host: str,
port: int,
env: dict | None = None,
) -> subprocess.Popen:
"""Launch the worker subprocess."""
cmd = self.build_command(args, backend_args, host, port)
logger.info("Launching worker with command: %s", " ".join(cmd))
+ if env is None:
+ env = os.environ.copy()
env["PYTHONUNBUFFERED"] = "1"
- return subprocess.Popen(cmd, start_new_session=True, env=env or os.environ.copy(), stdout=sys.stdout, stderr=sys.stderr)
+ return subprocess.Popen(cmd, start_new_session=True, env=env, stdout=sys.stdout, stderr=sys.stderr)📝 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.
| cmd = self.build_command(args, backend_args, host, port) | |
| logger.info("Launching worker with command: %s", " ".join(cmd)) | |
| env["PYTHONUNBUFFERED"] = "1" | |
| return subprocess.Popen(cmd, start_new_session=True, env=env or os.environ.copy(), stdout=sys.stdout, stderr=sys.stderr) | |
| cmd = self.build_command(args, backend_args, host, port) | |
| logger.info("Launching worker with command: %s", " ".join(cmd)) | |
| if env is None: | |
| env = os.environ.copy() | |
| env["PYTHONUNBUFFERED"] = "1" | |
| return subprocess.Popen(cmd, start_new_session=True, env=env, stdout=sys.stdout, stderr=sys.stderr) |
🤖 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 72 - 75, The code in launch()
uses env before ensuring it's set, causing a TypeError; fix by resolving env
early (e.g., env = env or os.environ.copy()) before writing
env["PYTHONUNBUFFERED"]="1", then call self.build_command(...) and pass that
resolved env into subprocess.Popen (referencing build_command and the
subprocess.Popen(...) call) so you never index into a None env.
| def _filter_backend_args(self, backend_args: list[str]) -> list[str]: | ||
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | ||
| filtered_args = ["--model-path", "--host", "--port"] |
There was a problem hiding this comment.
Copy-paste error in docstring: says "vLLM's grpc_server" but this is the sglang launcher.
def _filter_backend_args(self, backend_args: list[str]) -> list[str]:
- """Filter backend_args to only include those relevant for vLLM's grpc_server."""
+ """Filter backend_args to only include those relevant for sglang."""📝 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]) -> list[str]: | |
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | |
| filtered_args = ["--model-path", "--host", "--port"] | |
| def _filter_backend_args(self, backend_args: list[str]) -> list[str]: | |
| """Filter backend_args to only include those relevant for sglang.""" | |
| filtered_args = ["--model-path", "--host", "--port"] |
🤖 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 84 - 86, The docstring for
_filter_backend_args incorrectly references "vLLM's grpc_server" due to
copy-paste; update the docstring to accurately describe that this method filters
backend_args for the sglang launcher (or the sglang backend) and mention the
specific flags it keeps (e.g., "--model-path", "--host", "--port") so the doc
correctly reflects the behavior of _filter_backend_args and the filtered_args
variable.
| try: | ||
| import yaml | ||
| with open(config_path, "r") as f: | ||
| config = yaml.safe_load(f) | ||
| if config and "tensor_parallel_size" in config: | ||
| return config["tensor_parallel_size"] |
There was a problem hiding this comment.
No type validation on tensor_parallel_size read from YAML config.
yaml.safe_load could return a string or other type for tensor_parallel_size. Since this value feeds into arithmetic in gpu_env, consider casting to int explicitly.
Proposed fix
if config and "tensor_parallel_size" in config:
- return config["tensor_parallel_size"]
+ return int(config["tensor_parallel_size"])🤖 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 184 - 189, The YAML loading
block returns config["tensor_parallel_size"] without type validation; update the
logic (around the yaml.safe_load call using config_path in serve.py) to
explicitly validate and cast tensor_parallel_size to an int before returning
(e.g., ensure the value exists, attempt int(value), and raise or fallback if
conversion fails), and propagate the integer into gpu_env usage so downstream
arithmetic receives an integer rather than a string or other type.
| """ | ||
| 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=str, help="Tensor parallel size (overrides config file)") |
There was a problem hiding this comment.
--tp-size declared as type=str but used as int in _get_tp_size.
_get_tp_size (line 172-174) returns the value directly, and it's subsequently used in arithmetic (dp_rank * tp_size in gpu_env). With type=str, argparse stores the string "4" instead of the integer 4, causing TypeError in GPU assignment math.
🐛 Fix: change type to int
- group.add_argument("--tp-size", type=str, help="Tensor parallel size (overrides config file)")
+ group.add_argument("--tp-size", type=int, help="Tensor parallel size (overrides config file)")📝 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.
| group.add_argument("--tp-size", type=str, help="Tensor parallel size (overrides config file)") | |
| group.add_argument("--tp-size", type=int, help="Tensor parallel size (overrides config file)") |
🤖 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 355, The CLI argument "--tp-size"
is declared with type=str but later treated as an integer in _get_tp_size and
used in arithmetic (dp_rank * tp_size) inside gpu_env; change the argparse
declaration in group.add_argument("--tp-size", ...) to use type=int so argparse
stores an int, and ensure any callers (_get_tp_size, gpu_env) continue to use
the integer value dp_rank and tp_size without casting.
![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 useful feature to pass through arbitrary arguments to the backend workers. The implementation is on the right track, but there are a few key issues to address. The argument filtering logic is not robust and is duplicated across launchers. Additionally, the feature appears to be incompletely implemented for the TensorRT-LLM launcher. I've left specific comments with suggestions to fix these issues and improve the code's maintainability.
| def _filter_backend_args(self, backend_args: list[str]) -> list[str]: | ||
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | ||
| filtered_args = ["--model-path", "--host", "--port"] | ||
| filtered_backend_args = [] | ||
| skip_next = False | ||
| for arg in backend_args: | ||
| if skip_next: | ||
| skip_next = False | ||
| continue | ||
| if arg in filtered_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.
This filtering logic has a bug: it won't correctly filter arguments passed in the --arg=value format. For example, if backend_args contains "--model-path=my-model", it will not be filtered out, potentially causing conflicts.
Additionally, this method is nearly identical to the one in VllmWorkerLauncher, leading to code duplication. Consider moving this logic to the WorkerLauncher base class and having subclasses define the arguments to be filtered.
I've suggested a more robust implementation using argparse.parse_known_args, which handles various argument formats correctly. I also fixed a minor copy-paste error in the docstring.
def _filter_backend_args(self, backend_args: list[str]) -> list[str]:
"""Filter backend_args to remove arguments controlled by the sglang launcher."""
# Using argparse is more robust for handling both "--arg value" and "--arg=value" formats.
parser = argparse.ArgumentParser(add_help=False)
parser.add_argument("--model-path", type=str)
parser.add_argument("--host", type=str)
parser.add_argument("--port", type=str)
_, remaining_args = parser.parse_known_args(backend_args)
return remaining_args| def _filter_backend_args(self, backend_args: list[str]) -> list[str]: | ||
| """Filter backend_args to only include those relevant for vLLM's grpc_server.""" | ||
| filtered_args = ["--model", "--host", "--port"] | ||
| filtered_backend_args = [] | ||
| skip_next = False | ||
| for arg in backend_args: | ||
| if skip_next: | ||
| skip_next = False | ||
| continue | ||
| if arg in filtered_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.
This method has the same bug as the one in SglangWorkerLauncher: it doesn't handle arguments in the --arg=value format. It's also a near-duplicate, which makes the code harder to maintain.
As mentioned in my other comment, this logic should be moved to the WorkerLauncher base class to avoid duplication. For now, here is a more robust implementation for this specific class using argparse.
def _filter_backend_args(self, backend_args: list[str]) -> list[str]:
"""Filter backend_args to only include those relevant for vLLM's grpc_server."""
# Using argparse is more robust for handling both "--arg value" and "--arg=value" formats.
parser = argparse.ArgumentParser(add_help=False)
parser.add_argument("--model", type=str)
parser.add_argument("--host", type=str)
parser.add_argument("--port", type=str)
_, remaining_args = parser.parse_known_args(backend_args)
return remaining_args| # Try --tp-size argument first | ||
| tp_size = getattr(args, "tp_size", None) | ||
| if tp_size is not None: | ||
| return tp_size | ||
|
|
||
| # Try --tensor-parallel-size (vLLM-style naming) | ||
| tp_size = getattr(args, "tensor_parallel_size", None) | ||
| if tp_size is not None: | ||
| return tp_size |
There was a problem hiding this comment.
The logic to determine the tensor parallel size can be made more concise. The current implementation with multiple if statements is clear but a bit verbose. I've suggested a more compact version that achieves the same result.
# Try --tp-size, then --tensor-parallel-size
tp_size = getattr(args, "tp_size", getattr(args, "tensor_parallel_size", None))
if tp_size is not None:
return tp_size|
|
||
| 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. |
There was a problem hiding this comment.
This comment in the docstring is no longer accurate. The code now adds a --tp-size command-line argument on line 355, which can override the value from the config file. The comment should be updated to reflect this.
TP size can be provided via --tp-size, or read from the config file.
| """Coordinate worker launch, health checking, router startup, and shutdown.""" | ||
|
|
||
| def __init__(self, backend: str, args: argparse.Namespace): | ||
| def __init__(self, backend: str, args: argparse.Namespace, backend_args: list[str]): |
There was a problem hiding this comment.
please update unit test as we have changed the signature
| return 1 | ||
|
|
||
| def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[str]: | ||
| def build_command(self, args: argparse.Namespace, backend_args: list[str], host: str, port: int) -> list[str]: |
There was a problem hiding this comment.
same here
please update unit tests
|
|
||
| def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[str]: | ||
|
|
||
| def _filter_backend_args(self, backend_args: list[str]) -> list[str]: |
There was a problem hiding this comment.
this is the same code copied in another launcher
please extract this out
3136b21 to
3bb3108
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bindings/python/src/smg/serve.py (2)
440-472:⚠️ Potential issue | 🟡 MinorReturn type annotation and docstring not updated to reflect the new 3-tuple return.
The function now returns
(backend, args, backend_args)but the annotation still declares-> tuple[str, argparse.Namespace]and the docstring says "Tuple of (backend_name, parsed_namespace)". Type-checkers and IDEs will report incorrect types for all call sites.📝 Proposed fix
def parse_serve_args( argv: list[str] | None = None, -) -> tuple[str, argparse.Namespace]: +) -> tuple[str, argparse.Namespace, list[str]]: """Two-pass argument parsing for serve command. Pass 1: Extract --backend with parse_known_args (no backend imports). Pass 2: Build full parser with backend-specific + router args. Returns: - Tuple of (backend_name, parsed_namespace). + Tuple of (backend_name, parsed_namespace, 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 440 - 472, Update parse_serve_args to reflect the new 3-tuple return: change the return type annotation from -> tuple[str, argparse.Namespace] to -> tuple[str, argparse.Namespace, list[str]] (or a more precise type for backend_args if known) and update the docstring "Returns:" section to describe Tuple of (backend_name, parsed_namespace, backend_args). Ensure any references in the body still return (backend, args, backend_args) and keep the function signature and docstring consistent with the new return value.
33-36:⚠️ Potential issue | 🟠 MajorAbstract
build_commandsignature is stale — does not match any concrete implementation.The abstract declaration still uses the old three-argument form
(args, host, port), but every subclass andlaunch()(line 87) now pass(args, backend_args, host, port). Python's ABC machinery only checks that the method is overridden, not that signatures match, so there is no runtime crash — but any futureWorkerLaunchersubclass following the abstract signature will produce incorrect commands whenlaunch()calls it.🐛 Proposed fix
`@abstractmethod` - def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[str]: + def build_command(self, args: argparse.Namespace, backend_args: list[str], host: str, port: int) -> list[str]: """Build the CLI command list to launch a worker.""" ...🤖 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 33 - 36, The abstract method signature for build_command on WorkerLauncher is outdated; update its declaration to accept four parameters (args: argparse.Namespace, backend_args: dict | argparse.Namespace, host: str, port: int) and return list[str] so it matches all concrete implementations and the launch() caller; locate the abstractmethod build_command in class WorkerLauncher and change its parameter list to include backend_args and update the docstring to reflect the new parameters.bindings/python/tests/test_serve.py (1)
36-38:⚠️ Potential issue | 🟡 MinorTest is fragile — fails if
SMG_DEFAULT_BACKENDis set in the environment.
DEFAULT_BACKENDreads fromos.getenv("SMG_DEFAULT_BACKEND", "sglang"). The assertion should either patch the env or importDEFAULT_BACKENDinside a context that controls the env:with patch.dict(os.environ, {}, clear=True): from importlib import reload import smg.serve as s assert s.DEFAULT_BACKEND == "sglang"🤖 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 36 - 38, The test test_default_backend_is_sglang is fragile because DEFAULT_BACKEND is read from the environment at import time; update the test to control the env before importing/reloading the module: use unittest.mock.patch.dict(os.environ, {}, clear=True) to clear SMG_DEFAULT_BACKEND (and any env) and then import or importlib.reload the smg.serve module, and assert smg.serve.DEFAULT_BACKEND == "sglang"; reference DEFAULT_BACKEND and the test function name to locate and replace the current direct assertion.
🤖 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/tests/test_serve.py`:
- Around line 631-632: Several orchestrator tests construct backend_args as a
bare string which causes _filter_backend_args (and launchers) to iterate
characters; update every test that sets backend_args = "--model-path /tmp/model"
(used when instantiating ServeOrchestrator) to pass a list of strings instead,
e.g. backend_args = ["--model-path", "/tmp/model"], so ServeOrchestrator,
_filter_backend_args and any real launch() invocation receive list[str] not str;
apply the same replacement to all occurrences in the test file where
backend_args is currently a single string.
- Line 486: Remove the stray debug print statement left in the test by deleting
the print(cmd) call; locate the print(cmd) invocation in
bindings/python/tests/test_serve.py (inside the failing test body) and remove it
so tests no longer emit debug output (no replacement logging is necessary unless
you intentionally want persistent test logs).
---
Outside diff comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 440-472: Update parse_serve_args to reflect the new 3-tuple
return: change the return type annotation from -> tuple[str, argparse.Namespace]
to -> tuple[str, argparse.Namespace, list[str]] (or a more precise type for
backend_args if known) and update the docstring "Returns:" section to describe
Tuple of (backend_name, parsed_namespace, backend_args). Ensure any references
in the body still return (backend, args, backend_args) and keep the function
signature and docstring consistent with the new return value.
- Around line 33-36: The abstract method signature for build_command on
WorkerLauncher is outdated; update its declaration to accept four parameters
(args: argparse.Namespace, backend_args: dict | argparse.Namespace, host: str,
port: int) and return list[str] so it matches all concrete implementations and
the launch() caller; locate the abstractmethod build_command in class
WorkerLauncher and change its parameter list to include backend_args and update
the docstring to reflect the new parameters.
In `@bindings/python/tests/test_serve.py`:
- Around line 36-38: The test test_default_backend_is_sglang is fragile because
DEFAULT_BACKEND is read from the environment at import time; update the test to
control the env before importing/reloading the module: use
unittest.mock.patch.dict(os.environ, {}, clear=True) to clear
SMG_DEFAULT_BACKEND (and any env) and then import or importlib.reload the
smg.serve module, and assert smg.serve.DEFAULT_BACKEND == "sglang"; reference
DEFAULT_BACKEND and the test function name to locate and replace the current
direct assertion.
---
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 63-64: The docstring for _filter_backend_args incorrectly
references "vLLM's grpc_server"; update it to a neutral description reflecting
that this is a shared base-class helper (e.g., "Filter backend_args to only
include arguments relevant for the backend gRPC server" or "Filter backend_args
to only include arguments relevant for the server component"), preserving
mention of the parameters backend_args and filter_args and the return type
semantics so callers of _filter_backend_args still understand its purpose.
- Around line 347-362: The CLI arg --tp-size is defined as type=str in
_add_trtllm_stub_args but used numerically later (via _get_tp_size feeding into
gpu_env where dp_rank * tp_size occurs), causing TypeError; change the --tp-size
argument to type=int (or parse/convert it to int in _get_tp_size) so tp_size is
an integer at use sites, update any docstring/help text accordingly, and ensure
_get_tp_size and gpu_env consistently expect and handle an int tp_size.
- Around line 182-194: The YAML-loaded tensor_parallel_size must be converted to
an integer to avoid TypeError during arithmetic in gpu_env; update the code that
reads config["tensor_parallel_size"] (the block using config_path and
yaml.safe_load) to coerce the value with int(), e.g. attempt tp_size =
int(config["tensor_parallel_size"]) and return tp_size, and fall back to logging
a warning and returning 1 if the cast fails (catch ValueError/TypeError) so
gpu_env's dp_rank * tp_size arithmetic always uses an int.
- Around line 88-96: The code dereferences env before the fallback, causing a
TypeError; change the logic so you compute a working environment first (e.g.,
local_env = env or os.environ.copy()), set local_env["PYTHONUNBUFFERED"] = "1",
then pass local_env to subprocess.Popen; update the block around the
logger.info/return subprocess.Popen call to use local_env instead of env to
avoid the crash.
| assert "0.0.0.0" in cmd | ||
| assert "--port" in cmd | ||
| assert "50051" in cmd | ||
| print(cmd) |
There was a problem hiding this comment.
Debug print(cmd) left in production test body — remove before merge.
📝 Proposed fix
- print(cmd)
for arg in backend_args:
assert arg in cmd📝 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: | |
| assert arg in cmd |
🤖 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 486, Remove the stray debug
print statement left in the test by deleting the print(cmd) call; locate the
print(cmd) invocation in bindings/python/tests/test_serve.py (inside the failing
test body) and remove it so tests no longer emit debug output (no replacement
logging is necessary unless you intentionally want persistent test logs).
There was a problem hiding this comment.
Actionable comments posted: 2
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)
37-38: 🧹 Nitpick | 🔵 TrivialThis assertion is fragile if
SMG_DEFAULT_BACKENDis set in the environment.Since
DEFAULT_BACKEND = os.getenv("SMG_DEFAULT_BACKEND", "sglang")is evaluated at import time, this test will fail whenever the env var is set. Consider usingmonkeypatchto ensure a clean env or asserting only the fallback behavior.♻️ Proposed fix
- def test_default_backend_is_sglang(self): - assert DEFAULT_BACKEND == "sglang" + def test_default_backend_is_sglang(self, monkeypatch): + monkeypatch.delenv("SMG_DEFAULT_BACKEND", raising=False) + # Re-import to pick up the cleared env + import importlib + import smg.serve as serve_mod + importlib.reload(serve_mod) + assert serve_mod.DEFAULT_BACKEND == "sglang"🤖 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 37 - 38, The test test_default_backend_is_sglang is fragile because DEFAULT_BACKEND is set from the environment variable SMG_DEFAULT_BACKEND at import time; update the test to isolate environment by using monkeypatch (or otherwise ensure SMG_DEFAULT_BACKEND is unset) before importing or reloading the module so DEFAULT_BACKEND picks the fallback "sglang", or change the assertion to explicitly verify the fallback logic rather than the imported value; target the DEFAULT_BACKEND symbol and the SMG_DEFAULT_BACKEND env var when making this change.
🤖 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 375: DEFAULT_BACKEND is evaluated at import time which makes tests
brittle; replace the module-level constant with a small accessor function so the
env var is read at use-time. Implement get_default_backend() that returns
os.getenv("SMG_DEFAULT_BACKEND", "sglang"), replace uses of DEFAULT_BACKEND with
calls to get_default_backend(), and update the test that asserts DEFAULT_BACKEND
== "sglang" to either call get_default_backend() or set SMG_DEFAULT_BACKEND via
monkeypatch/os.environ before importing the module. Ensure the new function name
get_default_backend is used consistently where the backend default is needed.
In `@bindings/python/tests/test_serve.py`:
- Around line 758-759: Tests pass a bare string to backend_args which should be
list[str]; update each occurrence where backend_args = "--model-path /tmp/model"
(and its variants) to be a list of tokens like ["--model-path", "/tmp/model"] so
ServeOrchestrator("sglang", args, backend_args) receives a list; ensure all
listed test sites (the backend_args variables used before constructing
ServeOrchestrator) are changed to lists so _filter_backend_args / launch()
iterate arg tokens rather than characters.
---
Outside diff comments:
In `@bindings/python/tests/test_serve.py`:
- Around line 37-38: The test test_default_backend_is_sglang is fragile because
DEFAULT_BACKEND is set from the environment variable SMG_DEFAULT_BACKEND at
import time; update the test to isolate environment by using monkeypatch (or
otherwise ensure SMG_DEFAULT_BACKEND is unset) before importing or reloading the
module so DEFAULT_BACKEND picks the fallback "sglang", or change the assertion
to explicitly verify the fallback logic rather than the imported value; target
the DEFAULT_BACKEND symbol and the SMG_DEFAULT_BACKEND env var when making this
change.
---
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 64-65: The docstring for WorkerLauncher._filter_backend_args is
incorrect/too specific to vLLM; update the docstring for the function
_filter_backend_args (in class WorkerLauncher) to describe that it filters
backend_args to include only backend-relevant CLI args for any backend (not
vLLM-specific), e.g., "Filter backend_args to only include those relevant for
the backend's server/launcher," and ensure wording reflects that this helper is
shared across all backends rather than referencing "vLLM's grpc_server."
- Around line 183-191: The YAML-loaded TP size may be a string so update the
code that reads config_path and returns config["tensor_parallel_size"] or
config["tp_size"] to defensively cast the value to int before returning; locate
where config is read (check the block referencing config_path and the keys
"tensor_parallel_size" and "tp_size") and wrap the returned value with int(...)
or validate/convert non-int types so downstream arithmetic in gpu_env receives
an integer.
- Line 359: The bug is that group.add_argument("--tp-size", type=str, ...)
registers args.tp_size as a string while _get_tp_size and gpu_env perform
integer arithmetic (dp_rank * tp_size); change the argument to an integer by
updating group.add_argument("--tp-size", type=int, help=...) so
args.tp_size/_get_tp_size return an int (or alternatively ensure _get_tp_size
casts args.tp_size to int before returning) so dp_rank * tp_size no longer
raises a TypeError.
- Around line 64-77: The _filter_backend_args function currently assumes every
filtered flag consumes the next token which causes boolean/store-true flags to
accidentally drop the following arg; update _filter_backend_args to only skip
the next token when the matched filter arg actually has an attached value
(handle both separate-token values and --arg=value forms): when you see an arg
in filter_args, if the arg contains '=' skip nothing further; else peek at the
next token and only set skip_next if that next token exists and does NOT start
with '--'; otherwise do not skip the next token. Keep references to
_filter_backend_args, filtered_backend_args and skip_next when making the
change.
In `@bindings/python/tests/test_serve.py`:
- Line 488: Remove the leftover debug statement "print(cmd)" from the test body
(the stray print of the variable cmd); simply delete that line so the test no
longer prints during runs, then run the test suite to confirm no behavioral
changes.
|
|
||
| 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 is resolved at import time — test that asserts DEFAULT_BACKEND == "sglang" is fragile.
DEFAULT_BACKEND reads SMG_DEFAULT_BACKEND at module-import time. If the env var is set in CI or a developer's shell, both the default behavior and the test on line 38 of test_serve.py will silently break. This is a minor correctness concern but worth noting.
🤖 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 375, DEFAULT_BACKEND is evaluated
at import time which makes tests brittle; replace the module-level constant with
a small accessor function so the env var is read at use-time. Implement
get_default_backend() that returns os.getenv("SMG_DEFAULT_BACKEND", "sglang"),
replace uses of DEFAULT_BACKEND with calls to get_default_backend(), and update
the test that asserts DEFAULT_BACKEND == "sglang" to either call
get_default_backend() or set SMG_DEFAULT_BACKEND via monkeypatch/os.environ
before importing the module. Ensure the new function name get_default_backend is
used consistently where the backend default is needed.
| backend_args = "--model-path /tmp/model" | ||
| orch = ServeOrchestrator("sglang", args, backend_args) |
There was a problem hiding this comment.
backend_args still passed as a bare string in multiple orchestrator tests.
ServeOrchestrator.__init__ type-hints backend_args: list[str]. Passing a bare string works accidentally because these tests mock launch(), but iterating a string yields individual characters, not arg tokens. If any test path stops mocking or if _filter_backend_args is exercised, this will produce garbage commands.
For example, on line 758:
backend_args = "--model-path /tmp/model" # str, not list[str]All affected sites: lines 758, 779, 798, 815, 830, 847, 855, 867, 878.
🐛 Proposed fix (representative — apply to all sites)
- backend_args = "--model-path /tmp/model"
+ backend_args = ["--model-path", "/tmp/model"]And for the trtllm test at line 878:
- backend_args = "--config /tmp/config.yml"
+ backend_args = ["--config", "/tmp/config.yml"]Also applies to: 779-780, 798-799, 815-816, 830-831, 847-848, 855-856, 867-868, 878-879
🤖 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 758 - 759, Tests pass a
bare string to backend_args which should be list[str]; update each occurrence
where backend_args = "--model-path /tmp/model" (and its variants) to be a list
of tokens like ["--model-path", "/tmp/model"] so ServeOrchestrator("sglang",
args, backend_args) receives a list; ensure all listed test sites (the
backend_args variables used before constructing ServeOrchestrator) are changed
to lists so _filter_backend_args / launch() iterate arg tokens rather than
characters.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pyproject.toml`:
- Line 33: Update the PyYAML dependency declaration in pyproject.toml to pin the
minimum version to 6.0.1 to ensure Python 3.12 compatibility: replace the
current "PyYAML" entry with a version-constrained requirement (e.g.,
"PyYAML>=6.0.1") so that the package resolver will install a release that
includes cp312 wheels; this change relates to the dependency list where "PyYAML"
is declared and to the existing requires-python = ">=3.12" constraint.
In `@bindings/python/src/smg/serve.py`:
- Around line 214-215: The inline comment is incorrect: replace the misleading
"Add optional config file" with a precise comment describing what's happening
(e.g. "Add filtered backend args (--model, --host, --port)") or, if the original
intent was to add only a config file, change the code around
cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host",
"--port"])) to only append the config-file argument; locate this behavior in
serve.py around the call to _filter_backend_args, backend_args, and cmd.extend
and update the comment or code accordingly.
---
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 162-194: The config-loaded tensor-parallel values may be strings,
so in _get_tp_size cast the YAML-read values to int before returning: when
reading config in serve._get_tp_size, replace returns of
config["tensor_parallel_size"] and config["tp_size"] with
int(config["tensor_parallel_size"]) / int(config["tp_size"]) (and optionally
guard with try/ValueError to log and fall back to default), ensuring the
function always returns an int for downstream arithmetic like gpu_env dp_rank *
tp_size.
- Around line 354-357: The "--tp-size" CLI arg is declared with type=str but
later used as an integer (see parser.add_argument("--tp-size") and its consumers
_get_tp_size and gpu_env which do math with dp_rank * tp_size and use tp_size in
range()), causing string semantics and TypeError; fix by changing the argparse
declaration for "--tp-size" to type=int (and validate/convert in _get_tp_size if
needed) so tp_size is an int everywhere it's used (ensure any downstream uses
like gpu_env, dp_rank * tp_size, and range(tp_size) receive an int).
- Around line 66-79: Docstring and argument-skipping logic in
_filter_backend_args are wrong: update the docstring to describe this as a
base-class helper (not "vLLM's grpc_server"), and change the skip_next logic so
it doesn't assume every filtered option takes a value. Specifically, in
_filter_backend_args, treat filter_args as option names; when you encounter an
arg that matches a filter option, handle three cases: a long form with '='
(e.g., "--opt=val") — drop it only, a standalone option followed by a next token
that starts with '-' (i.e., another option) — treat it as a boolean flag and do
not skip the next token, and a standalone option followed by a non-option token
— skip the next token as its value. Ensure you reference _filter_backend_args in
the change and keep behavior for non-filtered args unchanged.
In `@bindings/python/tests/test_serve.py`:
- Around line 757-879: Several tests pass backend_args as a bare string (which
iterates by character) but ServeOrchestrator.__init__ expects backend_args:
list[str]; update each test to pass a list of argument tokens instead of a
single string. Specifically, in test_build_router_args_injects_worker_urls_grpc,
test_build_router_args_http_mode, test_build_router_args_vllm_grpc_urls,
test_cleanup_workers_handles_already_dead_process,
test_cleanup_workers_sigkill_on_timeout, test_cleanup_workers_empty_list,
test_signal_handler_sets_guard_flag, test_signal_handler_guard_prevents_reentry,
and test_trtllm_orchestrator_launches_grpc_workers replace backend_args =
"--model-path /tmp/model" (and variants like "--model /tmp/m" or "--config
/tmp/config.yml") with tokenized lists (e.g., ["--model-path", "/tmp/model"],
["--model", "/tmp/m"], ["--config", "/tmp/config.yml"]) so downstream code like
ServeOrchestrator._filter_backend_args and launch() receive proper argv tokens.
- Line 488: Remove the stray debugging statement "print(cmd)" left in the test
body: delete the "print(cmd)" line (or replace it with an appropriate assertion
or logger if output is required) in the test that constructs the "cmd" variable
so the test no longer prints during runs; ensure no other leftover debug prints
referencing "cmd" remain and re-run tests.
| "setproctitle", | ||
| "grpcio", | ||
| "grpcio-health-checking", | ||
| "PyYAML", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PyYAML version history Python 3.12 support minimum version
💡 Result:
Minimum PyYAML version with Python 3.12 support: 6.0.1 (it ships CPython 3.12 wheels, e.g., cp312 builds). [1]
Notes from version history (PyPI release dates):
- 6.0 — Oct 13, 2021 [2] (users reported install/build failures on Python 3.12) [3]
- 6.0.1 — Jul 17, 2023 [1] (first release with cp312 wheels ⇒ practical 3.12 support)
- 6.0.2 — Aug 6, 2024 [2]
- 6.0.3 — Sep 25, 2025 [4]
Answer: For Python 3.12, use PyYAML >= 6.0.1. [1]
Sources:
[1] (pypi.org)
[2] (pypi.org)
[3] (github.com)
[4] (pypi.org)
Citations:
- 1: https://pypi.org/project/PyYAML/6.0.1/?utm_source=openai
- 2: https://pypi.org/project/PyYAML/6.0b1/?utm_source=openai
- 3: Python 3.12 failed to install pyYaML=6.0.0 yaml/pyyaml#756
- 4: https://pypi.org/project/pyyaml/?utm_source=openai
🌐 Web query:
PyYAML latest version 2025 changelog Python 3.12
💡 Result:
- Latest PyYAML release in 2025: 6.0.3, released Sep 25, 2025. [1][2]
- Changelog (6.0.3): “Support for Python 3.14 and free-threading (experimental).” (Full diff:
6.0.2...6.0.3.) [1] - Python 3.12 compatibility: PyYAML 6.0.3 ships CPython 3.12 wheels on PyPI (so it installs cleanly on Python 3.12 without needing a local build toolchain in most environments). [2]
- Related prior release (useful context for 3.12+): 6.0.2 (Aug 6, 2024) added “Support for Cython 3.x and Python 3.13.” [1][3]
Sources: GitHub releases page for PyYAML [1], PyPI release history/files for PyYAML 6.0.3 [2], PyYAML CHANGES file (notes up through 6.0.2) [3].
Pin PyYAML to version 6.0.1 or later for Python 3.12 support.
PyYAML 6.0 (released Oct 2021) does not include Python 3.12 wheels and users reported install/build failures on Python 3.12. PyYAML 6.0.1 (released Jul 2023) was the first version to ship cp312 wheels and provide practical Python 3.12 support. Given requires-python = ">=3.12", use PyYAML>=6.0.1.
📦 Proposed fix
- "PyYAML",
+ "PyYAML>=6.0.1",📝 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.
| "PyYAML", | |
| "PyYAML>=6.0.1", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindings/python/pyproject.toml` at line 33, Update the PyYAML dependency
declaration in pyproject.toml to pin the minimum version to 6.0.1 to ensure
Python 3.12 compatibility: replace the current "PyYAML" entry with a
version-constrained requirement (e.g., "PyYAML>=6.0.1") so that the package
resolver will install a release that includes cp312 wheels; this change relates
to the dependency list where "PyYAML" is declared and to the existing
requires-python = ">=3.12" constraint.
| # 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: says "Add optional config file" but applies all filtered backend_args.
- # Add optional config file
+ # Pass through remaining backend args (e.g. --config)
cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host", "--port"]))📝 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.
| # Add optional config file | |
| cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host", "--port"])) | |
| # Pass through remaining backend args (e.g. --config) | |
| cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host", "--port"])) |
🤖 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 214 - 215, The inline comment
is incorrect: replace the misleading "Add optional config file" with a precise
comment describing what's happening (e.g. "Add filtered backend args (--model,
--host, --port)") or, if the original intent was to add only a config file,
change the code around cmd.extend(self._filter_backend_args(backend_args,
["--model", "--host", "--port"])) to only append the config-file argument;
locate this behavior in serve.py around the call to _filter_backend_args,
backend_args, and cmd.extend and update the comment or code accordingly.
|
Closing this PR because the branch name To fix this, please rename your branch locally, push the new branch, and open a new PR: git branch -m <new-branch-name>
git push origin -u <new-branch-name> |
|
Hi @gongwei-130, the branch Please use one of the following formats:
Allowed types:
|
2 participants
Description
Problem
Solution
Changes
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Improvements
Tests