chore: add preference logic for using tool-call and reasoning parsers for sglang #2922
Conversation
Signed-off-by: ayushag <ayushag@nvidia.com>
WalkthroughAdds runtime Dynamo logging configuration and introduces a helper to reconcile parser values from sglang and dynamo sources. Updates parse_args to compute tool_call_parser and reasoning_parser via the helper, validate values, warn on conflicts, and pass resolved values into DynamoArgs. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI User
participant Mod as args.py (module)
participant Log as Dynamo Logging
participant Parser as _set_parser()
participant CFG as DynamoArgs
Note over Mod: Module import
Mod->>Log: configure_dynamo_logging()
Note right of Log: Logging configured
CLI->>Mod: parse_args(argv)
Mod->>Parser: resolve tool_call_parser (sglang vs dyn)
Parser-->>Mod: tool_call_parser (validated)
Mod->>Parser: resolve reasoning_parser (sglang vs dyn)
Parser-->>Mod: reasoning_parser (validated)
Mod->>CFG: Construct with resolved parser values
CFG-->>CLI: Parsed/validated config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks (1 passed, 2 warnings)❌ Failed Checks (2 warnings)
✅ Passed Checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/backends/sglang/src/dynamo/sglang/args.py (3)
109-118: Prefer argparse error over raising ValueError; reference the actual CLI flagsUse parser.error for a clean CLI message/exit (no stack trace) and reference --tool-call-parser/--reasoning-parser (hyphenated) to match user input. Also clears the Ruff TRY003 warning.
- # Disable sglang based parser arguments - if parsed_args.tool_call_parser is not None: - raise ValueError( - "tool_call_parser is not supported for SGLang backend. Please use --dyn-tool-call-parser instead to enable tool calling." - ) - if parsed_args.reasoning_parser is not None: - raise ValueError( - "reasoning_parser is not supported for SGLang backend. Please use --dyn-reasoning-parser instead to enable reasoning." - ) + # Disable SGLang-built-in parser arguments (use Dynamo flags instead) + if parsed_args.tool_call_parser is not None: + parser.error("--tool-call-parser is not supported for the SGLang backend. Use --dyn-tool-call-parser instead.") + if parsed_args.reasoning_parser is not None: + parser.error("--reasoning-parser is not supported for the SGLang backend. Use --dyn-reasoning-parser instead.")
109-118: Guardrail: fail-fast if both legacy and dyn flags are providedIf users pass both forms, the first parser.error will trigger, but the message won’t show the combined misuse. Consider a single check to produce a consolidated hint.
+ # Optional: consolidated check for clearer UX if both are supplied + if (parsed_args.tool_call_parser is not None) and (parsed_args.dyn_tool_call_parser is not None): + parser.error("Do not mix --tool-call-parser with --dyn-tool-call-parser. Use only --dyn-tool-call-parser.") + if (parsed_args.reasoning_parser is not None) and (parsed_args.dyn_reasoning_parser is not None): + parser.error("Do not mix --reasoning-parser with --dyn-reasoning-parser. Use only --dyn-reasoning-parser.")
173-195: Port selection race: “find-free-port” can collidereserve_free_port releases the socket before ServerArgs binds, so another process can take the port. Prefer passing 0 to let the OS choose, or hold the socket until bind time if SGLang supports FD passing.
Would you like a follow-up PR to switch to an OS-assigned port (0) if ServerArgs supports it?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/backends/sglang/src/dynamo/sglang/args.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/backends/sglang/src/dynamo/sglang/args.py (1)
lib/bindings/python/rust/llm/local_model.rs (2)
tool_call_parser(71-73)reasoning_parser(76-78)
🪛 Ruff (0.12.2)
components/backends/sglang/src/dynamo/sglang/args.py
111-113: Avoid specifying long messages outside the exception class
(TRY003)
115-117: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
components/backends/sglang/src/dynamo/sglang/args.py (2)
151-158: LGTM: dyn flags correctly wired into DynamoArgsThe mapping to dyn_tool_call_parser/dyn_reasoning_parser matches the new flags and enforces the intended behavior.
83-171: No lingering legacy flags found. Verified that--tool-call-parserand--reasoning-parserdo not appear anywhere in code, scripts, or documentation—safe to finalize removal.
ishandhanani
left a comment
ishandhanani
left a comment
There was a problem hiding this comment.
As per slack convo - lets map args instead of disabling
Signed-off-by: ayushag <ayushag@nvidia.com>
ayushag-nv
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
components/backends/sglang/src/dynamo/sglang/args.py (3)
18-21: Avoid configuring root logging at import timeCalling configure_dynamo_logging() on import removes existing root handlers, which can clobber host logging when this module is imported as a library. Defer to runtime (e.g., at the start of parse_args) or guard behind an env flag.
Apply this diff here and initialize in parse_args:
-from dynamo.runtime.logging import configure_dynamo_logging +from dynamo.runtime.logging import configure_dynamo_logging @@ -configure_dynamo_logging() +# Defer logging initialization to runtime to avoid import-time side effects.Then near the top of parse_args():
# initialize logging once args are being processed configure_dynamo_logging()
86-114: Make _set_parser generic, log consistently, and fix Ruff TRY003
- Pass allowed choices instead of branching on arg_name.
- Include the provided invalid value in the error.
- Downgrade to info if both sources match; warn only on conflict.
- Use a module logger instead of root.
Apply this diff:
-def _set_parser( - sglang_str: Optional[str], - dynamo_str: Optional[str], - arg_name: str = "tool-call-parser", -) -> Optional[str]: - # If both are present, give preference to dynamo_str - if sglang_str is not None and dynamo_str is not None: - logging.warning( - f"--dyn-{arg_name} and --{arg_name} are both set. Giving preference to --dyn-{arg_name}" - ) - return dynamo_str - # If dynamo_str is not set, use try to use sglang_str if it matches with the allowed parsers - elif sglang_str is not None: - logging.warning(f"--dyn-{arg_name} is not set. Using --{arg_name}.") - if arg_name == "tool-call-parser" and sglang_str not in get_tool_parser_names(): - raise ValueError( - f"--{arg_name} is not a valid tool call parser. Valid parsers are: {get_tool_parser_names()}" - ) - elif ( - arg_name == "reasoning-parser" - and sglang_str not in get_reasoning_parser_names() - ): - raise ValueError( - f"--{arg_name} is not a valid reasoning parser. Valid parsers are: {get_reasoning_parser_names()}" - ) - return sglang_str - else: - return dynamo_str +_logger = logging.getLogger(__name__) + +def _set_parser( + sglang_str: Optional[str], + dynamo_str: Optional[str], + *, + arg_flag: str, + allowed: set[str], +) -> Optional[str]: + # If both are present, prefer dynamo_str but avoid noisy logs when identical. + if sglang_str is not None and dynamo_str is not None: + if sglang_str == dynamo_str: + _logger.info("%s specified in both places with the same value: %s", arg_flag, dynamo_str) + else: + _logger.warning("%s specified in both places; preferring --dyn-%s: %s -> %s", + arg_flag, arg_flag.lstrip("-"), sglang_str, dynamo_str) + return dynamo_str + if sglang_str is not None: + if sglang_str not in allowed: + allowed_str = ", ".join(sorted(allowed)) + raise ValueError(f"Invalid {arg_flag} '{sglang_str}'. Valid values: {allowed_str}") + _logger.warning("--dyn-%s not set; using %s=%s", + arg_flag.lstrip("-"), arg_flag, sglang_str) + return sglang_str + return dynamo_strOptionally cache allowed sets once at module import:
TOOL_PARSERS = set(get_tool_parser_names()) REASONING_PARSERS = set(get_reasoning_parser_names())This also addresses Ruff TRY003 on Lines 101-103 and 108-110 by shortening exception messages.
174-184: Propagate resolved parser values back into parsed_args to keep ServerArgs consistentWithout syncing, ServerArgs may see different values than DynamoArgs. Update parsed_args after resolution.
Apply this diff (assumes refactored _set_parser signature from earlier comment):
- tool_call_parser = _set_parser( - parsed_args.tool_call_parser, - parsed_args.dyn_tool_call_parser, - "tool-call-parser", - ) - reasoning_parser = _set_parser( - parsed_args.reasoning_parser, - parsed_args.dyn_reasoning_parser, - "reasoning-parser", - ) + tool_call_parser = _set_parser( + parsed_args.tool_call_parser, + parsed_args.dyn_tool_call_parser, + arg_flag="--tool-call-parser", + allowed=TOOL_PARSERS if 'TOOL_PARSERS' in globals() else set(get_tool_parser_names()), + ) + reasoning_parser = _set_parser( + parsed_args.reasoning_parser, + parsed_args.dyn_reasoning_parser, + arg_flag="--reasoning-parser", + allowed=REASONING_PARSERS if 'REASONING_PARSERS' in globals() else set(get_reasoning_parser_names()), + ) + + # Keep parsed_args in sync for downstream consumers (e.g., ServerArgs). + if tool_call_parser is not None or reasoning_parser is not None: + args_dict = vars(parsed_args) + if tool_call_parser is not None: + args_dict["tool_call_parser"] = tool_call_parser + args_dict["dyn_tool_call_parser"] = tool_call_parser + if reasoning_parser is not None: + args_dict["reasoning_parser"] = reasoning_parser + args_dict["dyn_reasoning_parser"] = reasoning_parser + parsed_args = Namespace(**args_dict)If ServerArgs does not consume these fields, feel free to skip syncing; otherwise this prevents subtle mismatches.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/backends/sglang/src/dynamo/sglang/args.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/backends/sglang/src/dynamo/sglang/args.py (2)
lib/bindings/python/src/dynamo/runtime/logging.py (1)
configure_dynamo_logging(77-107)lib/bindings/python/rust/parsers.rs (2)
get_tool_parser_names(10-12)get_reasoning_parser_names(16-18)
🪛 Ruff (0.12.2)
components/backends/sglang/src/dynamo/sglang/args.py
101-103: Avoid specifying long messages outside the exception class
(TRY003)
108-110: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
components/backends/sglang/src/dynamo/sglang/args.py (1)
190-191: LGTM: resolved values plumbed into DynamoArgsPassing the reconciled tool_call_parser and reasoning_parser here is correct.
… for sglang (#2922) Signed-off-by: ayushag <ayushag@nvidia.com> Signed-off-by: Indrajit Bhosale <iamindrajitb@gmail.com>
… for sglang (#2922) Signed-off-by: ayushag <ayushag@nvidia.com>
… for sglang (#2922) Signed-off-by: ayushag <ayushag@nvidia.com> Signed-off-by: hongkuanz <hongkuanz@nvidia.com>
Overview:
As we are currently using --dyn-tool-call-parser and --dyn-reasoning-parser for tool calling and reasoning, this PR adds preference logic to use both sglang defaults and dynamo based args
vLLM is safe as we are not using its frontendArgs class in our code base. This is a temporary solution for now.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
New Features
Bug Fixes