fix(vllm): preserve user-specified --runner flag in update_engine_config_with_dynamo - #7918
fix(vllm): preserve user-specified --runner flag in update_engine_config_with_dynamo#7918MatejKosec wants to merge 1 commit into
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🧹 Nitpick comments (2)
components/src/dynamo/vllm/args.py (1)
237-244: Simplify thehasattr/getattrcombination to direct attribute access.After
hasattr(engine_config, "runner")confirms the attribute exists, usinggetattr(engine_config, "runner")is redundant. Per coding guidelines, prefer direct attribute access on known fields. This also addresses the Ruff B009 warning.✨ Suggested simplification
# Set runner default only when the user did not explicitly pass --runner. # vLLM 0.13+ renamed 'task' to 'runner'; None means the user did not # specify a value, so we can safely apply the 'generate' default. # Embedding models require --runner embed, which must not be overridden. - if hasattr(engine_config, "runner") and getattr(engine_config, "runner") is None: + if hasattr(engine_config, "runner") and engine_config.runner is None: engine_config.runner = "generate" logger.debug(" engine_args.runner = generate (default)")As per coding guidelines: "prefer direct attribute access over defensive getattr on known fields".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/vllm/args.py` around lines 237 - 244, The code uses hasattr(engine_config, "runner") followed by getattr(engine_config, "runner") which is redundant and triggers Ruff B009; change the conditional to check the attribute and access it directly (e.g., if hasattr(engine_config, "runner") and engine_config.runner is None:) and then set engine_config.runner = "generate" and call logger.debug(" engine_args.runner = generate (default)"); update the block around engine_config.runner and logger.debug accordingly.components/src/dynamo/vllm/tests/test_vllm_unit.py (1)
732-744: Remove redundant in-function import.
DisaggregationModeis already imported at module level (line 27). The in-function import on line 734 is unnecessary and violates the coding guideline to keep imports at the top of the file.✨ Suggested fix
def _make_dynamo_config_stub(): """Minimal dynamo config stub for update_engine_config_with_dynamo tests.""" - from dynamo.vllm.constants import DisaggregationMode - stub = SimpleNamespace( disaggregation_mode=DisaggregationMode.AGGREGATED, multimodal_worker=False,As per coding guidelines: "keep imports at the top of the file (no in-function/class imports)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/vllm/tests/test_vllm_unit.py` around lines 732 - 744, The _make_dynamo_config_stub function contains an unnecessary in-function import of DisaggregationMode; remove the line "from dynamo.vllm.constants import DisaggregationMode" and use the module-level DisaggregationMode import (already present at top of the file) inside _make_dynamo_config_stub so imports are kept at the top and the function simply references DisaggregationMode when constructing the SimpleNamespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/src/dynamo/vllm/tests/test_vllm_unit.py`:
- Around line 812-815: The multi-line assertion for engine_cfg.runner is failing
formatting; collapse the assertion message into a single-line expression or
assign the f-string to a temporary variable and use that in the assert so
Black/ruff will accept it (e.g., assert engine_cfg.runner == "pooling",
f"Expected runner='pooling' to be preserved, but got
runner='{engine_cfg.runner}'."), then run pre-commit hooks (pre-commit run
--all-files or ruff format) to auto-apply formatting fixes.
---
Nitpick comments:
In `@components/src/dynamo/vllm/args.py`:
- Around line 237-244: The code uses hasattr(engine_config, "runner") followed
by getattr(engine_config, "runner") which is redundant and triggers Ruff B009;
change the conditional to check the attribute and access it directly (e.g., if
hasattr(engine_config, "runner") and engine_config.runner is None:) and then set
engine_config.runner = "generate" and call logger.debug(" engine_args.runner =
generate (default)"); update the block around engine_config.runner and
logger.debug accordingly.
In `@components/src/dynamo/vllm/tests/test_vllm_unit.py`:
- Around line 732-744: The _make_dynamo_config_stub function contains an
unnecessary in-function import of DisaggregationMode; remove the line "from
dynamo.vllm.constants import DisaggregationMode" and use the module-level
DisaggregationMode import (already present at top of the file) inside
_make_dynamo_config_stub so imports are kept at the top and the function simply
references DisaggregationMode when constructing the SimpleNamespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f24a1c05-18d7-42d2-a204-5ff12e9151db
📒 Files selected for processing (2)
components/src/dynamo/vllm/args.pycomponents/src/dynamo/vllm/tests/test_vllm_unit.py
|
|
||
| assert engine_cfg.runner == "pooling", ( | ||
| f"Expected runner='pooling' to be preserved, but got runner='{engine_cfg.runner}'." | ||
| ) |
There was a problem hiding this comment.
Fix formatting to pass pre-commit hooks.
The pipeline failure indicates that black reformatted this file. The multi-line assertion message likely needs adjustment to satisfy the formatter.
Run pre-commit run --all-files or ruff format locally to auto-fix the formatting before merging.
🧰 Tools
🪛 GitHub Actions: Pre Merge
[error] 812-814: pre-commit failed: black hook re-formatted files (1 file modified). Reproduce with pre-commit run --all-files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/src/dynamo/vllm/tests/test_vllm_unit.py` around lines 812 - 815,
The multi-line assertion for engine_cfg.runner is failing formatting; collapse
the assertion message into a single-line expression or assign the f-string to a
temporary variable and use that in the assert so Black/ruff will accept it
(e.g., assert engine_cfg.runner == "pooling", f"Expected runner='pooling' to be
preserved, but got runner='{engine_cfg.runner}'."), then run pre-commit hooks
(pre-commit run --all-files or ruff format) to auto-apply formatting fixes.
github-actions
Bot
added
deployment::k8s
6dfe973 to
88e95f9
Compare
…rate' default update_engine_config_with_dynamo placed "runner": "generate" in the `defaults` dict and then unconditionally applied every entry via setattr, silently overriding any value the user passed on the CLI (e.g. --runner embed required for embedding models). Move the runner default out of the blanket loop and guard it with an explicit None check: only set runner="generate" when the field is None, meaning the user did not specify --runner at all. Also adds TestRunnerDefaultNotOverridden unit tests that verify: - runner defaults to "generate" when unset - runner="embed" is preserved (embedding model use case) - runner="pooling" is preserved Fixes #7670 Signed-off-by: Matej Kosec <mkosec@4u2g-0421.ipp3a2.colossus.nvidia.com> Signed-off-by: Matej Kosec <mkosec@nvidia.com>
88e95f9 to
7856cb2
Compare
|
Closing — #7680 addresses the same issue. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| def _make_dynamo_config_stub(): | ||
| """Minimal dynamo config stub for update_engine_config_with_dynamo tests.""" | ||
| from dynamo.vllm.constants import DisaggregationMode |
There was a problem hiding this comment.
🟡 Redundant import inside function body violates "Keep imports at the top of the file" critical rule
_make_dynamo_config_stub() at line 734 imports from dynamo.vllm.constants import DisaggregationMode inside the function body, but this exact import already exists at module level on components/src/dynamo/vllm/tests/test_vllm_unit.py:27. Per .ai/python-guidelines.md critical rule "Keep imports at the top of the file": "Always flag any import statement that appears inside a function body." The module-level import is sufficient; the in-function import is redundant and hides the dependency.
| from dynamo.vllm.constants import DisaggregationMode |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Fixes #7670
Problem
update_engine_config_with_dynamo()unconditionally setsrunner: "generate"in the defaults dict, overriding any user-specified--runnerflag. This breaks embedding models which require--runner embed.Fix
Removed
runnerfrom the unconditional defaults. Only setsrunner = "generate"whenengine_config.runner is None(user did not specify).Tests
3 new unit tests in
test_vllm_unit.py(TestRunnerDefaultNotOverridden) — all pass.Fixes #7670
Summary by CodeRabbit