Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions bindings/python/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ dependencies = [
"setproctitle",
"grpcio",
"grpcio-health-checking",
"PyYAML",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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:


🌐 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.

Suggested change
"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.

]

[project.optional-dependencies]
Expand Down
170 changes: 133 additions & 37 deletions bindings/python/src/smg/serve.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ class WorkerLauncher(ABC):
"""Abstract base class for backend worker launchers."""

@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."""
...

Expand All @@ -58,18 +60,38 @@ def gpu_env(self, args: argparse.Namespace, dp_rank: int, env: dict | None = Non
base_gpu = dp_rank * tp_size
gpu_ids = range(base_gpu, base_gpu + tp_size)
env["CUDA_VISIBLE_DEVICES"] = ",".join(str(g) for g in gpu_ids)
env["PYTHONUNBUFFERED"] = "1"
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)

return filtered_backend_args

def launch(
self,
args: argparse.Namespace,
host: str,
port: int,
env: dict | None = None,
self, args: argparse.Namespace, backend_args: list[str], host: str, port: int, env: dict
) -> subprocess.Popen:
"""Launch the worker subprocess."""
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))

return subprocess.Popen(
cmd,
start_new_session=True,
env=env,
stdout=sys.stdout,
stderr=sys.stderr,
)


class SglangWorkerLauncher(WorkerLauncher):
Expand All @@ -78,7 +100,9 @@ class SglangWorkerLauncher(WorkerLauncher):
def _get_tp_size(self, args: argparse.Namespace) -> int:
return getattr(args, "tp_size", 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]:
cmd = [
sys.executable,
"-m",
Expand All @@ -92,6 +116,8 @@ def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[
]
if getattr(args, "connection_mode", "grpc") == "grpc":
cmd.append("--grpc-mode")

cmd.extend(self._filter_backend_args(backend_args, ["--model-path", "--host", "--port"]))
return cmd


Expand All @@ -101,20 +127,29 @@ class VllmWorkerLauncher(WorkerLauncher):
def _get_tp_size(self, args: argparse.Namespace) -> int:
return getattr(args, "tensor_parallel_size", 1)

def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[str]:
if getattr(args, "connection_mode", "grpc") != "grpc":
raise ValueError("vLLM backend only supports grpc connection mode")
return [
def build_command(
self, args: argparse.Namespace, backend_args: list[str], host: str, port: int
) -> list[str]:
vllm_entry_points = (
"vllm.entrypoints.grpc_server"
if getattr(args, "connection_mode", "grpc") == "grpc"
else "vllm.entrypoints.openai.api_server"
)

cmd = [
sys.executable,
"-m",
"vllm.entrypoints.grpc_server",
vllm_entry_points,
"--model",
getattr(args, "model", ""),
"--host",
host,
"--port",
str(port),
]
cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host", "--port"]))

return cmd


class TrtllmWorkerLauncher(WorkerLauncher):
Expand All @@ -125,12 +160,46 @@ class TrtllmWorkerLauncher(WorkerLauncher):
"""

def _get_tp_size(self, args: argparse.Namespace) -> int:
return getattr(args, "tp_size", 1)
"""Get tensor parallel size from args or config file.

Priority: args.tp_size > args.tensor_parallel_size > config file > default(1)
"""
# 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
Comment on lines +167 to +175

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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


# Try reading from config YAML file
config_path = getattr(args, "config", None)
if config_path:
try:
import yaml

with open(config_path) as f:
config = yaml.safe_load(f)
if config and "tensor_parallel_size" in config:
return config["tensor_parallel_size"]
Comment on lines +180 to +186

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

if config and "tp_size" in config:
return config["tp_size"]
except Exception as e:
logger.warning(
"Failed to read tensor_parallel_size from config %s: %s", config_path, e
)

def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[str]:
return 1

def build_command(
self, args: argparse.Namespace, backend_args: list[str], host: str, port: int
) -> list[str]:
if getattr(args, "connection_mode", "grpc") != "grpc":
raise ValueError("TensorRT-LLM backend only supports grpc connection mode")
return [

cmd = [
sys.executable,
"-m",
"tensorrt_llm.commands.serve",
Expand All @@ -142,6 +211,11 @@ def build_command(self, args: argparse.Namespace, host: str, port: int) -> list[
str(port),
]

# Add optional config file
cmd.extend(self._filter_backend_args(backend_args, ["--model", "--host", "--port"]))
Comment on lines +214 to +215

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
# 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.


return cmd


BACKEND_LAUNCHERS: dict[str, type[WorkerLauncher]] = {
"sglang": SglangWorkerLauncher,
Expand Down Expand Up @@ -271,9 +345,21 @@ def _add_vllm_args(parser: argparse.ArgumentParser) -> None:


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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

"""
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)")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

--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.

Suggested change
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.

group.add_argument(
"--config",
type=str,
required=False,
help="Config file path (YAML, optional - must contain tensor_parallel_size if provided)",
)


BACKEND_ARG_ADDERS = {
Expand All @@ -283,7 +369,7 @@ def _add_trtllm_stub_args(parser: argparse.ArgumentParser) -> None:
}

BACKEND_CHOICES = list(BACKEND_ARG_ADDERS.keys())
DEFAULT_BACKEND = "sglang"
DEFAULT_BACKEND = os.getenv("SMG_DEFAULT_BACKEND", "sglang")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.



# ---------------------------------------------------------------------------
Expand All @@ -304,7 +390,7 @@ def add_serve_args(parser: argparse.ArgumentParser) -> None:
"--connection-mode",
default="grpc",
choices=["grpc", "http"],
help="Connection mode for workers (default: grpc). Note: vllm and trtllm only support grpc",
help="Connection mode for workers (default: grpc). Note: trtllm only support grpc",
)
# Router host/port - may be overridden by backend (e.g. sglang)
group.add_argument(
Expand Down Expand Up @@ -353,26 +439,31 @@ def _import_backend_args(backend: str, parser: argparse.ArgumentParser) -> None:

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.
Pass 1: Parse with only serve + router args (parse_known_args). We get
--backend and known flags; any remaining argv tokens are left as
backend_args (no backend-specific parser loaded yet).
Pass 2: Build full parser with serve + backend-specific + router args,
then parse_args(argv) to get the full namespace. backend_args from pass 1
are returned unchanged for the launcher to pass through to worker commands.

Returns:
Tuple of (backend_name, parsed_namespace).
Tuple of (backend_name, parsed_namespace, backend_args).
backend_args: argv tokens not recognized in pass 1 (e.g. --config path).
"""
if argv is None:
argv = []

# Pass 1: extract --backend (lightweight, no backend imports)
# Pass 1: serve + router args only; unknown tokens become backend_args
pre_parser = argparse.ArgumentParser(add_help=False)
pre_parser.add_argument("--backend", default=DEFAULT_BACKEND, choices=BACKEND_CHOICES)
pre_args, _ = pre_parser.parse_known_args(argv)
backend = pre_args.backend
add_serve_args(pre_parser)
RouterArgs.add_cli_args(pre_parser, use_router_prefix=True, exclude_host_port=True)
serve_router_args, backend_args = pre_parser.parse_known_args(argv)
backend = serve_router_args.backend

# Pass 2: build full parser with backend-specific args
# Use conflict_handler='resolve' to let backend args override serve defaults
# Pass 2: full parser with backend-specific args; resolve so backend can override
parser = argparse.ArgumentParser(
description=f"Launch {backend} worker(s) + gateway router",
conflict_handler="resolve",
Expand All @@ -382,7 +473,7 @@ def parse_serve_args(
RouterArgs.add_cli_args(parser, use_router_prefix=True, exclude_host_port=True)

args = parser.parse_args(argv)
return backend, args
return backend, args, backend_args


# ---------------------------------------------------------------------------
Expand All @@ -395,9 +486,10 @@ def parse_serve_args(
class ServeOrchestrator:
"""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]):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update unit test as we have changed the signature

self.backend = backend
self.args = args
self.backend_args = backend_args
self.launcher: WorkerLauncher = BACKEND_LAUNCHERS[backend]()
self.workers: list[tuple[subprocess.Popen, int]] = []
self._shutting_down = False
Expand All @@ -424,7 +516,7 @@ def _launch_workers(self) -> None:
host = self.args.worker_host
for dp_rank, port in enumerate(ports):
env = self.launcher.gpu_env(self.args, dp_rank)
proc = self.launcher.launch(self.args, host, port, env)
proc = self.launcher.launch(self.args, self.backend_args, host, port, env)
self.workers.append((proc, port))
logger.info(
"Launched %s worker on %s:%d (pid %d, GPUs: %s)",
Expand Down Expand Up @@ -500,6 +592,10 @@ def _cleanup_workers(self) -> None:

def serve_main(argv: list[str] | None = None) -> None:
"""Parse serve args, create orchestrator, and run."""
backend, args = parse_serve_args(argv)
orchestrator = ServeOrchestrator(backend, args)
logging.basicConfig(
level=logging.INFO,
format="%(asctime)s - %(name)s - %(levelname)s - %(message)s",
)
backend, args, backend_args = parse_serve_args(argv)
orchestrator = ServeOrchestrator(backend, args, backend_args)
orchestrator.run()
Loading
Loading