Conversation
github-actions
Bot
added
feat
backend::vllm
WalkthroughThe changes introduce a modular configuration system for Dynamo by creating reusable configuration classes and argument groups, replacing flat argument definitions. An Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/src/dynamo/sglang/register.py (1)
146-162:⚠️ Potential issue | 🟡 MinorStale docstring: function no longer returns
None.The docstring on line 157-158 says "or None if extraction fails", but all code paths now return
runtime_config. Update to reflect the actual behavior.Suggested fix
Returns: - ModelRuntimeConfig with extracted values, or None if extraction fails. + ModelRuntimeConfig with extracted values (may be partially populated if scheduler info is unavailable).components/src/dynamo/sglang/main.py (1)
284-288:⚠️ Potential issue | 🟡 MinorWarning message references obsolete flag name
--dyn-endpoint-types.Line 286 mentions
--dyn-endpoint-typesbut the primary flag is now--endpoint-types(with--dyn-endpoint-typesas an obsolete alias). Consider updating the warning to reference the current flag name.
🤖 Fix all issues with AI agents
In `@components/src/dynamo/sglang/args.py`:
- Around line 263-269: The code that replaces the config flag currently only
looks for the exact token "--config" (using unknown.index) and misses the
"--config=path" form, causing a duplicate config flag; update the logic in the
function/class handling the unknown list (referenced by unknown and
temp_config_file in args.py, likely inside ConfigArgumentMerger or the argument
merging routine) to detect either an exact "--config" token or any token that
startswith "--config="; if you find "--config" replace the next element with
temp_config_file, and if you find "--config=..." replace that single token with
"--config" and temp_config_file (or replace it in-place to use
"--config=temp_config_file") so only one config specification remains.
In `@components/src/dynamo/sglang/backend_args.py`:
- Around line 84-92: The default for the --image-diffusion-base-url flag uses a
second env var ("DYN_IMAGE_DIFFUSION_BASE_URL") at import time which conflicts
with the add_argument env_var ("DYN_SGL_IMAGE_DIFFUSION_BASE_URL"); fix by
consolidating to a single env var—update the add_argument call that defines
flag_name "--image-diffusion-base-url" (and uses env_var
"DYN_SGL_IMAGE_DIFFUSION_BASE_URL") to remove the
os.environ.get("DYN_IMAGE_DIFFUSION_BASE_URL", ...) lookup and either use a
plain hardcoded default "http://localhost:8008/" or, if legacy support is
required, replace it with os.environ.get("DYN_SGL_IMAGE_DIFFUSION_BASE_URL",
os.environ.get("DYN_IMAGE_DIFFUSION_BASE_URL", "http://localhost:8008/"))) so
precedence is clear and not duplicated.
🧹 Nitpick comments (3)
components/src/dynamo/sglang/args.py (3)
180-186: Usage of private argparse internals (_StoreTrueAction,_StoreFalseAction).These are CPython implementation details and may break on alternative interpreters or future Python versions. This is a common pragmatic choice, but worth noting.
240-247: Accessing private_group_actionsto splice SGLang flags into help output.
sg._group_actions.append(action)relies on an internal argparse attribute. This is a pragmatic workaround for grouping help text, but it's fragile. Consider adding a comment noting this is intentional and why a cleaner API isn't available.
150-162: Temp file cleanup relies on caller — considerNamedTemporaryFile(delete=False)or a context manager.The temp file is created on line 152 and cleaned up on lines 299-303. If an exception occurs between these points (e.g., during
_apply_disagg_config_defaultsorconfig_merger.merge_config_with_args), the temp file leaks. Atry/finallywrapper around the full block using the temp file would be more robust.
|
Please review https://app.devin.ai/review/ai-dynamo/dynamo/pull/6280 |
Signed-off-by: jh-nv <jihao@nvidia.com>
Signed-off-by: jh-nv <jihao@nvidia.com>
3 participants
Overview:
This PR migrates the SGLang backend configuration path to Dynamo’s shared configuration framework and separates Dynamo wrapper arguments from native SGLang engine arguments.
Details:
Where should the reviewer start?
components/src/dynamo/sglang/backend_args.py
components/src/dynamo/sglang/args.py
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Release Notes
New Features
Refactor