fix: preserve user-provided --runner flag in update_engine_config_with_dynamo

[pecord-ent]

Summary

• update_engine_config_with_dynamo() unconditionally set engine_args.runner = "generate", overwriting any user-provided --runner CLI argument
• This broke embedding models (e.g. google/embeddinggemma-300m) that require --runner pooling
• Now only defaults to "generate" when the user didn't explicitly set --runner (i.e. vLLM's default "auto" is still in place)

Root cause

The defaults dict in update_engine_config_with_dynamo() hardcoded "runner": "generate" and the subsequent loop applied all defaults unconditionally via setattr, with no check for whether the user had already set the value.

Fix

Moved the runner default out of the unconditional defaults dict. Before adding it, we check if engine_config.runner is still "auto" (vLLM's default when the user doesn't specify --runner). If the user explicitly provided a value like pooling or draft, it is preserved.

Changelog

• components/src/dynamo/vllm/args.py: Check engine_config.runner before defaulting to "generate"
• components/src/dynamo/vllm/tests/test_vllm_unit.py: Added TestRunnerPreservation class with 5 test cases covering auto→generate default, pooling/generate/draft preservation, and graceful handling when the runner attr is absent (older vLLM)

Test plan

• TestRunnerPreservation::test_runner_defaults_to_generate_when_auto — default behavior unchanged
• TestRunnerPreservation::test_runner_pooling_preserved — embedding model case from update_engine_config_with_dynamo unconditionally overrides --runner, breaking embedding models #7670
• TestRunnerPreservation::test_runner_generate_explicit_preserved — explicit generate still works
• TestRunnerPreservation::test_runner_draft_preserved — draft runner preserved
• TestRunnerPreservation::test_no_runner_attr_skipped_gracefully — backward compat with older vLLM
• Deploy an embedding model with --runner pooling and verify it starts without the ValidationError

Fixes #7670

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Runner configuration is no longer unconditionally overwritten; user-specified values are now preserved, with defaults applied only when auto-configured.

• Tests

  • Added test coverage for runner configuration preservation behavior.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[github-actions]

👋 Hi pecord-ent! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

[coderabbitai]

Walkthrough

Fixes a bug where update_engine_config_with_dynamo() unconditionally overwrote the user-provided --runner CLI argument to "generate", breaking embedding models that require --runner pooling. The fix conditionally applies the default only when the runner is "auto", preserving explicit user values.

Changes

Cohort / File(s)	Summary
Runner Configuration Fix components/src/dynamo/vllm/args.py	Modified update_engine_config_with_dynamo() to conditionally set runner to "generate" only when the existing value is "auto". Preserves user-provided runner values and emits debug logging when user-specified values are retained.
Runner Preservation Tests components/src/dynamo/vllm/tests/test_vllm_unit.py	Added TestRunnerPreservation test class with five test cases covering: (1) "auto" → "generate" conversion, (2) "pooling" preservation, (3) explicit "generate" preservation, (4) "draft" preservation, and (5) handling missing runner attribute gracefully.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: preserving user-provided --runner flag in update_engine_config_with_dynamo, which is the core objective of this PR.
Description check	✅ Passed	The description follows the template with clear Overview/Summary, Details, Root cause, Fix explanation, Changelog, and Test plan sections. All required information is present and well-organized.
Linked Issues check	✅ Passed	The PR fully addresses issue #7670 by conditionally defaulting runner to 'generate' only when engine_config.runner is 'auto', preserving explicit user values like 'pooling' for embedding models.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the runner flag issue: modifications to args.py for conditional defaulting and comprehensive tests in test_vllm_unit.py with no unrelated alterations.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

components/src/dynamo/vllm/args.py (1)

262-268: Consider simplifying redundant getattr after hasattr check.

Since line 262 already confirms runner exists via hasattr(), the getattr(engine_config, "runner", "auto") on line 263 is unnecessarily defensive. Direct attribute access is preferred per coding guidelines.

Suggested simplification

     if hasattr(engine_config, "runner"):
-        if getattr(engine_config, "runner", "auto") == "auto":
+        if engine_config.runner == "auto":
             defaults["runner"] = "generate"
         else:
             logger.debug(
                 f"Preserving user-provided runner: {engine_config.runner}"
             )

As per coding guidelines: "Avoid 'defensive' getattr(obj, 'attr', default) on known types/fields—prefer direct attribute access so contract changes fail loudly."

🤖 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 262 - 268, The code
redundantly uses getattr(engine_config, "runner", "auto") after confirming the
attribute exists with hasattr(engine_config, "runner"); replace the defensive
getattr with direct attribute access (use engine_config.runner) in the
conditional so it becomes if engine_config.runner == "auto":
defaults["runner"]="generate" else logger.debug(f"Preserving user-provided
runner: {engine_config.runner}"), keeping the same behavior and touching the
symbols engine_config, defaults, and logger.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@components/src/dynamo/vllm/args.py`:
- Around line 262-268: The code redundantly uses getattr(engine_config,
"runner", "auto") after confirming the attribute exists with
hasattr(engine_config, "runner"); replace the defensive getattr with direct
attribute access (use engine_config.runner) in the conditional so it becomes if
engine_config.runner == "auto": defaults["runner"]="generate" else
logger.debug(f"Preserving user-provided runner: {engine_config.runner}"),
keeping the same behavior and touching the symbols engine_config, defaults, and
logger.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 12c6450a-8e72-4674-b48a-2589a0896c42

📥 Commits

Reviewing files that changed from the base of the PR and between 98d0ce9 and 16d2f65.

📒 Files selected for processing (2)

• components/src/dynamo/vllm/args.py
• components/src/dynamo/vllm/tests/test_vllm_unit.py

[rmccorm4]

/ok to test 0088066

[rmccorm4]

Hi @pecord-ent, thanks for the contribution! Seems like some of the new tests fail with same issue along these lines:

=================================== FAILURES ===================================
______ TestRunnerPreservation.test_runner_defaults_to_generate_when_auto _______
components/src/dynamo/vllm/tests/test_vllm_unit.py:768: in test_runner_defaults_to_generate_when_auto
    update_engine_config_with_dynamo(dynamo_cfg, engine_cfg)
        dynamo_cfg = namespace(disaggregation_mode=<DisaggregationMode.AGGREGATED: 'agg'>, use_kv_events=False, enable_local_indexer=True)
        engine_cfg = namespace(runner='auto', enable_prefix_caching=True, block_size=16, skip_tokenizer_init=True, enable_log_requests=True, disable_log_stats=True, kv_events_config=None, kv_transfer_config=None)
        self       = <test_vllm_unit.TestRunnerPreservation object at 0x7fccbd58cec0>
/opt/dynamo/venv/lib/python3.12/site-packages/dynamo/vllm/args.py:286: in update_engine_config_with_dynamo
    if dynamo_config.benchmark_mode is not None:
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E   AttributeError: 'types.SimpleNamespace' object has no attribute 'benchmark_mode'
        defaults   = {'disable_log_stats': False, 'enable_log_requests': False, 'kv_events_config': None, 'runner': 'generate', ...}
        dynamo_config = namespace(disaggregation_mode=<DisaggregationMode.AGGREGATED: 'agg'>, use_kv_events=False, enable_local_indexer=True)
        engine_config = namespace(runner='auto', enable_prefix_caching=True, block_size=16, skip_tokenizer_init=True, enable_log_requests=True, disable_log_stats=True, kv_events_config=None, kv_transfer_config=None)
        kv_cfg     = None

Can you take a look?

[pecord-ent]

Good catch @rmccorm4, thanks! The _make_dynamo_config() test helper was missing benchmark_mode — update_engine_config_with_dynamo() accesses dynamo_config.benchmark_mode directly (not via getattr), so the SimpleNamespace blew up with AttributeError.

Fixed in 5b38db7 — added benchmark_mode=None to the helper defaults so the benchmark block is cleanly skipped. Should be green now.

[pecord-ent]

To clarify — the fix doesn't skip any test or code path. benchmark_mode=None is the normal "benchmarking not enabled" state (matching the Config dataclass default). The function still runs end-to-end; it just doesn't enter the benchmark-specific branch since there's no benchmark config to apply — same as any real non-benchmark invocation.

The test helper was simply an incomplete mock of DynamoConfig.