Repository navigation
fix parameters pass through for trtllm - #509
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 resolves an issue with parameter passing for the TensorRT-LLM backend, which lacked a centralized argument manager. The core change involves adapting the argument parsing strategy to use 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
|
📝 WalkthroughWalkthroughUpdated CLI parsing in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 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:
|
![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
The pull request addresses the challenge of passing parameters to TensorRT-LLM by switching to parse_known_args for the trtllm backend. This allows the system to recognize known arguments and ignore unknown ones, which is necessary because TensorRT-LLM lacks a centralized argument manager like vLLM's EngineArgs. Additionally, the --config argument has been removed from _add_trtllm_stub_args as it was not being used directly as a CLI argument for trtllm and the --tp-size argument was renamed to --tp_size for consistency. These changes improve the flexibility and robustness of argument parsing for the trtllm backend. All comments are valid and align with the provided rules.
![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: 30ea60920d
ℹ️ 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".
| required=False, | ||
| help="Config file path (YAML, optional - must contain tensor_parallel_size if provided)", | ||
| ) | ||
| group.add_argument("--tp_size", type=int, help="Tensor parallel size (overrides config file)") |
There was a problem hiding this comment.
--tp-size flag spelling for TensorRT-LLM
Changing the stub flag from --tp-size to --tp_size breaks existing CLI usage and silently drops TP size in the orchestrator path: users passing --tp-size now leave args.tp_size unset, so TrtllmWorkerLauncher._get_tp_size() falls back to defaults while the raw token is only forwarded in backend_args. In multi-worker runs this mis-sizes CUDA_VISIBLE_DEVICES and can cause incorrect GPU allocation or worker startup failures.
Useful? React with 👍 / 👎.
| if backend == "trtllm": | ||
| args, _ = parser.parse_known_args(argv) |
There was a problem hiding this comment.
--config in parsed args before TP sizing
Using parse_known_args for trtllm while no longer defining --config means config is discarded from args, but _get_tp_size() depends on args.config to read tensor parallel size from YAML. As a result, launches that specify TP only via --config now default to TP=1 for GPU env assignment, which can overlap GPUs across data-parallel workers and break startup.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/src/smg/serve.py (1)
357-367:⚠️ Potential issue | 🟠 Major
--configis never registered — the YAML tp_size fallback in_get_tp_sizeis unreachable dead code
TrtllmWorkerLauncher._get_tp_sizereadsgetattr(args, "config", None)at line 189 to find the YAML config path, but--configis never added to any parser (_add_trtllm_stub_args,add_serve_args, orRouterArgs). Even after switching toparse_known_argsin pass 2,--configis still not in the trtllm parser, so argparse silently drops it into_andargs.configremainsNone. The entireif config_path:block (lines 190–202) is therefore permanently unreachable.The downstream impact is silent: a user who passes
--config model.yamlexpecting the YAML'stensor_parallel_sizeto driveCUDA_VISIBLE_DEVICESwill instead get tp_size=1, leading to wrong GPU assignment for multi-GPU workers.Fix: add
--configto the stub so it is parsed intoargs.config.🐛 Proposed fix
def _add_trtllm_stub_args(parser: argparse.ArgumentParser) -> None: 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, + default=None, + help="Path to TensorRT-LLM YAML config file (used to read 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 357 - 367, The trt-llm CLI never registers --config so TrtllmWorkerLauncher._get_tp_size never sees args.config; update _add_trtllm_stub_args to register the config option by adding a group.add_argument("--config", type=str, help="Path to YAML config file") (alongside existing --model and --tp_size) so argparse will populate args.config and allow _get_tp_size to read tensor_parallel_size from the YAML.
🤖 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 479-483: When backend == "trtllm" the call to
parser.parse_known_args(argv) drops unknown tokens into the discard variable `_`
without feedback; update the branch handling parse_known_args to detect if `_`
is non-empty and emit a debug/warn log listing those discarded tokens (include
context like the backend and argv), so callers see misspelled flags; keep
existing behavior of returning backend, args, backend_args and do not change the
parse logic otherwise (refer to parser.parse_known_args, the `_` discard
variable, args, and the backend == "trtllm" branch).
---
Outside diff comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 357-367: The trt-llm CLI never registers --config so
TrtllmWorkerLauncher._get_tp_size never sees args.config; update
_add_trtllm_stub_args to register the config option by adding a
group.add_argument("--config", type=str, help="Path to YAML config file")
(alongside existing --model and --tp_size) so argparse will populate args.config
and allow _get_tp_size to read tensor_parallel_size from the YAML.
| if backend == "trtllm": | ||
| args, _ = parser.parse_known_args(argv) | ||
| else: | ||
| args = parser.parse_args(argv) | ||
| return backend, args, backend_args |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Log the discarded tokens from parse_known_args to aid diagnosis
Unknown args in pass 2 are silently dropped into _. A user who misspells a serve-level flag (e.g., --tp-size instead of --tp_size) will get no feedback: pass 2 silently ignores it, args.tp_size stays None, and GPU assignment defaults to 1. Adding a debug log of the discarded tokens makes this easier to catch.
♻️ Proposed change
if backend == "trtllm":
- args, _ = parser.parse_known_args(argv)
+ args, unknown = parser.parse_known_args(argv)
+ if unknown:
+ logger.debug(
+ "trtllm: ignoring unrecognized args in pass-2 parse (will be forwarded via backend_args): %s",
+ unknown,
+ )
else:
args = parser.parse_args(argv)🤖 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 479 - 483, When backend ==
"trtllm" the call to parser.parse_known_args(argv) drops unknown tokens into the
discard variable `_` without feedback; update the branch handling
parse_known_args to detect if `_` is non-empty and emit a debug/warn log listing
those discarded tokens (include context like the backend and argv), so callers
see misspelled flags; keep existing behavior of returning backend, args,
backend_args and do not change the parse logic otherwise (refer to
parser.parse_known_args, the `_` discard variable, args, and the backend ==
"trtllm" branch).
Signed-off-by: gongwei-130 <weigong28@gmail.com>
30ea609 to
2ebedd3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ebedd33fd
ℹ️ 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".
|
|
||
| args = parser.parse_args(argv) | ||
| if backend == "trtllm": | ||
| args, _ = parser.parse_known_args(argv) |
There was a problem hiding this comment.
Using parse_known_args here and dropping the unknown list makes typos in SMG-owned CLI flags silently bypass validation for trtllm. In that case the mistyped option falls back to defaults in args (for example, a misspelled worker port flag keeps worker_base_port=31000) and is forwarded as backend passthrough, which can break worker startup if TensorRT-LLM does not recognize it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bindings/python/src/smg/serve.py (1)
357-367:⚠️ Potential issue | 🟠 MajorRemoving
--configfrom stub args breaks the config-basedtp_sizefallback
_add_trtllm_stub_argsno longer registers--config, and pass 2 now usesparse_known_argswhich drops--configinto_. Consequentlyargs.configis alwaysNonein the CLI flow, making the config-YAML path in_get_tp_size(Lines 189–202) permanently unreachable. A user relying on the config file for tp_size will silently gettp_size=1instead of the correct value — wrongCUDA_VISIBLE_DEVICESassignment and potential misallocation across workers.The docstring on Line 362 ("TP size is read from the config file, not passed as CLI argument") is now incorrect on both counts:
--tp_sizeis a CLI argument, and the config path is dead.The docstring on Line 171 ("Priority: args.tp_size > args.tensor_parallel_size > config file > default(1)") should also be updated.
Simplest fix: re-add
--configto the stub args so pass 2 populatesargs.configwhilebackend_args(from pass 1) still carries it through to the worker command:🐛 Proposed fix
def _add_trtllm_stub_args(parser: argparse.ArgumentParser) -> None: """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. + --tp_size overrides the value derived from --config; both can coexist. """ 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, help="Path to TensorRT-LLM config YAML")Also fix the stale comment on Line 177:
- # Try --tp-size argument first + # Try --tp_size argument first🤖 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 357 - 367, The _add_trtllm_stub_args function removed registration of --config which causes parse_known_args to drop the flag and makes _get_tp_size never read config; re-add a --config (str) argument to the TensorRT-LLM argument group so args.config is populated in the CLI flow, ensure _add_trtllm_stub_args still registers --tp_size, and update the function docstring and the priority comment referenced by _get_tp_size to reflect that --tp_size is a CLI argument and that priority is args.tp_size > args.tensor_parallel_size > config file > default(1); keep parse_known_args usage but ensure backend_args still forwards config to workers.bindings/python/tests/test_serve.py (1)
543-598: 🛠️ Refactor suggestion | 🟠 MajorConfig-based
tp_sizetests bypass the CLI integration, masking the broken flowAll tests in this block call
_get_tp_sizewith a hand-craftedargparse.Namespace(config=str(config_file)). They pass regardless of whether the CLI flow actually populatesargs.config— which it does not (see the issue raised on serve.py Lines 357–367). Consider adding an integration-level assertion inTestParseServeArgs.test_trtllm_basicto make this gap visible:# Currently args.config is None because --config is not in stub args; # if config-based tp_size inference is intended, this assertion should hold: assert args.config == "/tmp/config.yml" # will fail until --config is re-added to stub 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` around lines 543 - 598, Tests call TrtllmWorkerLauncher._get_tp_size directly with a synthetic argparse.Namespace(config=...), bypassing the CLI parsing and hiding that the CLI stub in TestParseServeArgs.test_trtllm_basic does not populate args.config; add an integration-level check in TestParseServeArgs.test_trtllm_basic (or update the CLI stub used there) to assert args.config is set to the expected config path so the suite will fail if --config isn’t wired up, or alternatively update the CLI stub to include the --config argument so _get_tp_size behavior through the real parse flow is exercised (refer to _get_tp_size and TestParseServeArgs.test_trtllm_basic to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 357-367: The _add_trtllm_stub_args function removed registration
of --config which causes parse_known_args to drop the flag and makes
_get_tp_size never read config; re-add a --config (str) argument to the
TensorRT-LLM argument group so args.config is populated in the CLI flow, ensure
_add_trtllm_stub_args still registers --tp_size, and update the function
docstring and the priority comment referenced by _get_tp_size to reflect that
--tp_size is a CLI argument and that priority is args.tp_size >
args.tensor_parallel_size > config file > default(1); keep parse_known_args
usage but ensure backend_args still forwards config to workers.
In `@bindings/python/tests/test_serve.py`:
- Around line 543-598: Tests call TrtllmWorkerLauncher._get_tp_size directly
with a synthetic argparse.Namespace(config=...), bypassing the CLI parsing and
hiding that the CLI stub in TestParseServeArgs.test_trtllm_basic does not
populate args.config; add an integration-level check in
TestParseServeArgs.test_trtllm_basic (or update the CLI stub used there) to
assert args.config is set to the expected config path so the suite will fail if
--config isn’t wired up, or alternatively update the CLI stub to include the
--config argument so _get_tp_size behavior through the real parse flow is
exercised (refer to _get_tp_size and TestParseServeArgs.test_trtllm_basic to
locate the code).
---
Duplicate comments:
In `@bindings/python/src/smg/serve.py`:
- Around line 479-481: The parse_known_args call discards unrecognized tokens
(second return value) which should be logged for observability: change the line
using parser.parse_known_args(argv) in the branch where backend == "trtllm" to
capture the discarded tokens (e.g., args, discarded =
parser.parse_known_args(argv)), and if discarded is non-empty, emit a log (use
the existing logger in this module, e.g., logger.warning or process_logger.warn)
that includes the discarded tokens and contextual info (like backend and argv)
so operators can see which args were ignored.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b866556de9
ℹ️ 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 backend == "trtllm": | ||
| args, _ = parser.parse_known_args(argv) |
There was a problem hiding this comment.
--config in parsed trtllm args
Using parse_known_args for the trtllm pass-2 parse here means options that are no longer declared (notably --config after this change) are silently dropped from args. TrtllmWorkerLauncher._get_tp_size() relies on args.config to read tensor-parallel size from YAML before computing CUDA_VISIBLE_DEVICES, so runs that set TP only via config now fall back to tp=1, which can assign overlapping GPUs and fail multi-worker startup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
bindings/python/tests/test_serve.py
🤖 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 307-310: The test currently exits during backend import (via
_import_backend_args) instead of during pass-2 parsing; fix by stubbing/mocking
_import_backend_args in test_unknown_arg_rejected_in_pass2 so it does nothing
(e.g., monkeypatch _import_backend_args to a no-op or return None), call
parse_serve_args(["--backend", "sglang", "--totally-unknown-flag"]), and assert
it raises SystemExit from the full parser; keep the test name but ensure it no
longer duplicates test_sglang_explicit_exits by verifying the unknown-flag
rejection rather than import failure.
- Around line 158-166: Add a test that verifies the trtllm stub parser accepts
and returns the --tp_size flag: update or add a test in TestAddTrtllmStubArgs
that calls _add_trtllm_stub_args (or uses _import_backend_args for "trtllm") to
build an argparse.Parser, then call parser.parse_known_args with
["--model","/path/to/model","--tp_size","4"] (or similar) and assert args.model
and that the parsed value or backend_args reflect the tp_size as expected;
reference _add_trtllm_stub_args, TestAddTrtllmStubArgs,
TrtllmWorkerLauncher._get_tp_size and parser.parse_known_args to locate the
relevant code to modify.
| def test_trtllm_adds_model_arg(self): | ||
| parser = argparse.ArgumentParser() | ||
| _import_backend_args("trtllm", parser) | ||
| args = parser.parse_args(["--model", "/path/to/model", "--config", "/path/to/config.yml"]) | ||
| args, backend_args = parser.parse_known_args( | ||
| ["--model", "/path/to/model", "--config", "/path/to/config.yml"] | ||
| ) | ||
| assert args.model == "/path/to/model" | ||
| assert args.config == "/path/to/config.yml" | ||
| assert "--config" in backend_args | ||
| assert "/path/to/config.yml" in backend_args |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the serve.py file and examine the implementations
echo "=== Looking for serve.py ==="
fd -t f "serve.py" bindings/python
echo ""
echo "=== Finding _add_trtllm_stub_args ==="
rg -A 20 "def _add_trtllm_stub_args" bindings/python
echo ""
echo "=== Finding _get_tp_size ==="
rg -A 10 "def _get_tp_size" bindings/pythonRepository: lightseekorg/smg
Length of output: 5081
🏁 Script executed:
#!/bin/bash
# Get the full TrtllmWorkerLauncher._get_tp_size implementation
echo "=== Full TrtllmWorkerLauncher._get_tp_size ==="
rg -A 30 "class TrtllmWorkerLauncher" bindings/python/src/smg/serve.py | grep -A 30 "_get_tp_size"
echo ""
echo "=== Check if args.config is referenced in TrtllmWorkerLauncher._get_tp_size ==="
sed -n '/class TrtllmWorkerLauncher/,/^class /p' bindings/python/src/smg/serve.py | grep -A 25 "def _get_tp_size" | head -30Repository: lightseekorg/smg
Length of output: 2147
🏁 Script executed:
#!/bin/bash
# Search for tp_size coverage in tests
echo "=== Searching for tp_size in test file ==="
rg "tp_size" bindings/python/tests/test_serve.py
echo ""
echo "=== Full TestAddTrtllmStubArgs test ==="
sed -n '/class TestAddTrtllmStubArgs/,/^class /p' bindings/python/tests/test_serve.py | head -30Repository: lightseekorg/smg
Length of output: 2537
Correctly reflects --config removal from stub; --tp_size stub argument lacks direct test coverage.
The switch to parse_known_args and the backend_args assertions accurately exercise the new behavior where --config is no longer a stub argument. However, _add_trtllm_stub_args does add --tp_size as a new argument, yet TestAddTrtllmStubArgs (lines 183-196) lacks a test for it—only testing --model. The _get_tp_size() method is thoroughly tested elsewhere, but the stub argument parser itself should verify that --tp_size can be parsed correctly, following the existing pattern of test_adds_model_arg.
Note: TrtllmWorkerLauncher._get_tp_size safely accesses args.config via getattr(args, "config", None), so the removal of --config from stub args does not cause breakage; it simply falls back to the default.
🤖 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 158 - 166, Add a test that
verifies the trtllm stub parser accepts and returns the --tp_size flag: update
or add a test in TestAddTrtllmStubArgs that calls _add_trtllm_stub_args (or uses
_import_backend_args for "trtllm") to build an argparse.Parser, then call
parser.parse_known_args with ["--model","/path/to/model","--tp_size","4"] (or
similar) and assert args.model and that the parsed value or backend_args reflect
the tp_size as expected; reference _add_trtllm_stub_args, TestAddTrtllmStubArgs,
TrtllmWorkerLauncher._get_tp_size and parser.parse_known_args to locate the
relevant code to modify.
| def test_unknown_arg_rejected_in_pass2(self): | ||
| """Unknown args should be rejected by the full parser in pass 2.""" | ||
| with pytest.raises(SystemExit): | ||
| parse_serve_args(["--backend", "trtllm", "--totally-unknown-flag"]) | ||
| parse_serve_args(["--backend", "sglang", "--totally-unknown-flag"]) |
There was a problem hiding this comment.
test_unknown_arg_rejected_in_pass2 exits for the wrong reason and duplicates test_sglang_explicit_exits.
Using "sglang" as the backend causes _import_backend_args("sglang", parser) to call parser.error(...) (import failure) and raise SystemExit(2) before pass-2 parsing runs at all. The --totally-unknown-flag token is never evaluated, so the test does not verify what its docstring claims. It is also functionally identical to test_sglang_explicit_exits (lines 267-270), adding no incremental coverage.
The previous "trtllm" backend was also wrong post-PR: trtllm now uses parse_known_args, so the unknown flag would be silently swallowed, not rejected.
To genuinely test pass-2 unknown-arg rejection, mock _import_backend_args so a non-trtllm backend does not error on import:
🛠️ Suggested fix
def test_unknown_arg_rejected_in_pass2(self):
"""Unknown args should be rejected by the full parser in pass 2."""
- with pytest.raises(SystemExit):
- parse_serve_args(["--backend", "sglang", "--totally-unknown-flag"])
+ # Patch _import_backend_args so the backend loads successfully, letting
+ # pass-2 parse_args be the one to reject the unknown flag.
+ with patch("smg.serve._import_backend_args"):
+ with pytest.raises(SystemExit):
+ parse_serve_args(["--backend", "sglang", "--totally-unknown-flag"])🤖 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 307 - 310, The test
currently exits during backend import (via _import_backend_args) instead of
during pass-2 parsing; fix by stubbing/mocking _import_backend_args in
test_unknown_arg_rejected_in_pass2 so it does nothing (e.g., monkeypatch
_import_backend_args to a no-op or return None), call
parse_serve_args(["--backend", "sglang", "--totally-unknown-flag"]), and assert
it raises SystemExit from the full parser; keep the test name but ensure it no
longer duplicates test_sglang_explicit_exits by verifying the unknown-flag
rejection rather than import failure.
2 participants
Description
Problem
TensorRT-LLM doesn't provide a centralized argument manager like vLLM's EngineArgs. It is not feasible to add them all manually.
https://github.com/lightseekorg/smg/blob/main/bindings/python/src/smg/serve.py#L469-L486
The are two pass to passing args
Pass 1: serve + router args only; unknown tokens become backend_args
Pass 2: full parser with backend-specific args;
pass 2 would fail with trtllm since trtllm doesn't provide a centralized argument manager.
Fix:
At pass 2, change to use
parse_known_args, best effort to recognize args, if not, just ignore it for trtllm.Solution
Changes
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
--tp-sizeto--tp_size.