Skip to content

fix(trtllm): expose env var to force engine side conversation dp assignment#11705

Merged
peilii merged 13 commits into
mainfrom
gluo/env_overwrite
Jul 16, 2026
Merged

fix(trtllm): expose env var to force engine side conversation dp assignment#11705
peilii merged 13 commits into
mainfrom
gluo/env_overwrite

Conversation

@GuanLuo

@GuanLuo GuanLuo commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Signed-off-by: Guan Luo 41310872+GuanLuo@users.noreply.github.com

Overview

Adds --conversation-affinity / DYN_ENGINE_CONV_AFFINITY to let operators force engine-side conversation-affinity attention-DP routing for TRT-LLM workers, without relying on engine auto-detection. Useful when the engine reports no conversation-affinity capability at handler construction time (e.g. older TRT-LLM, disagg scenarios where the engine is not yet initialized).

Details

  • backend_args.py: registers --conversation-affinity (negatable bool, env DYN_ENGINE_CONV_AFFINITY, default False) and adds conversation_affinity: bool to DynamoTrtllmConfig.
  • llm_worker.py: passes config.conversation_affinity into RequestHandlerConfig.
  • handler_base.py: stores the override as _engine_conversation_affinity_override and ORs it with the engine-detected _conversation_affinity flag when deciding whether to use ConversationParams vs. SchedulingParams.

Where should the reviewer start?

  • components/src/dynamo/trtllm/request_handlers/handler_base.py — the routing decision at conv_affinity = self._conversation_affinity or self._engine_conversation_affinity_override and the conv_kwargs path below it.
  • components/src/dynamo/trtllm/backend_args.py — new flag registration.

Related Issues

🚫 This PR is NOT linked to an issue:

  • Confirmed — no related issue

GuanLuo added 2 commits July 15, 2026 00:50
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
@GuanLuo GuanLuo requested review from a team as code owners July 15, 2026 07:59
@github-actions github-actions Bot added fix backend::trtllm Relates to the trtllm backend labels Jul 15, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

dynamo-review-agent[bot]

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

Conversation affinity routing

Layer / File(s) Summary
Configuration and handler wiring
components/src/dynamo/trtllm/backend_args.py, components/src/dynamo/trtllm/workers/llm_worker.py
Adds the --conversation-affinity setting and passes it into RequestHandlerConfig.
Request handler routing override
components/src/dynamo/trtllm/request_handlers/handler_base.py
Stores the override and enables conversation-based attention-DP parameters when configured or detected.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: exposing a setting to force engine-side conversation DP assignment in TRT-LLM.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
components/src/dynamo/trtllm/request_handlers/handler_base.py (1)

1154-1179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use the combined affinity flag when forwarding conversation_params. In components/src/dynamo/trtllm/request_handlers/handler_base.py:1154-1178, the config override path builds conversation_params but still drops it at the call site because conv_kwargs is gated only by conv_affinity. That leaves override-only requests with neither conversation_params nor scheduling_params. Reuse the combined flag here and add a regression test for override enabled + engine detection disabled.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@components/src/dynamo/trtllm/request_handlers/handler_base.py` around lines
1154 - 1179, The conversation_params forwarding logic near conv_kwargs
incorrectly checks only conv_affinity; gate it with the combined affinity
condition that also includes self._engine_conversation_affinity_override, so
override-only requests receive conversation_params while preserving scheduling
behavior. Add a regression test covering the override-enabled,
engine-detection-disabled case and verify the generated request includes
conversation_params.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@components/src/dynamo/trtllm/request_handlers/handler_base.py`:
- Around line 1154-1179: The conversation_params forwarding logic near
conv_kwargs incorrectly checks only conv_affinity; gate it with the combined
affinity condition that also includes
self._engine_conversation_affinity_override, so override-only requests receive
conversation_params while preserving scheduling behavior. Add a regression test
covering the override-enabled, engine-detection-disabled case and verify the
generated request includes conversation_params.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7b686791-5243-4844-8e88-76c8ba84a149

📥 Commits

Reviewing files that changed from the base of the PR and between b0b7786 and 266ab73.

📒 Files selected for processing (3)
  • components/src/dynamo/trtllm/backend_args.py
  • components/src/dynamo/trtllm/request_handlers/handler_base.py
  • components/src/dynamo/trtllm/workers/llm_worker.py

@datadog-official

datadog-official Bot commented Jul 15, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 95.65%
Overall Coverage: 29.85% (-17.96%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: cee306a | Docs | Give us feedback!

Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
@peilii peilii self-assigned this Jul 15, 2026
peilii added 2 commits July 15, 2026 12:44
…TestConversationAffinity

MagicMock auto-creates truthy attributes; config.conversation_affinity
was returning a MagicMock object instead of False, making
_engine_conversation_affinity_override truthy and causing the
affinity-off test cases to take the conversation_params branch
instead of the scheduling_params branch.

Refs: #11705
Raise RuntimeError at startup when --conversation-affinity /
DYN_ENGINE_CONV_AFFINITY is set on a TRT-LLM build that lacks
ConversationParams (<=1.3.0rc20). The existing guard only covered
the auto-detection path.

Refs: #11705
@peilii peilii enabled auto-merge (squash) July 15, 2026 20:03
peilii added 2 commits July 15, 2026 14:10
The __init__ guard fires for any HandlerBase constructed with a bare
MagicMock() config (truthy conversation_affinity attribute), breaking
existing tests that don't pin it. CONVERSATION_PARAMS_AVAILABLE is also
False in the CPU test environment, compounding the breakage.

Refs: #11705
Raise RuntimeError on first request when DYN_ENGINE_CONV_AFFINITY is
set on a TRT-LLM build that lacks ConversationParams (<=1.3.0rc20).
The existing guard only covered the engine auto-detection path.

Guard goes in _generate_locally_impl (not __init__) because
CONVERSATION_PARAMS_AVAILABLE is False in the CPU test environment.
Pin config.conversation_affinity=False in all MagicMock-based handler
helpers so the guard does not fire spuriously in tests.

Refs: #11705
@pull-request-size pull-request-size Bot added size/M and removed size/S labels Jul 15, 2026
peilii added 2 commits July 15, 2026 15:51
Three tests in test_trtllm_unit.py verify the env var and CLI flag are
read correctly and default to False.

Two tests in TestConversationAffinity cover the override path behavior:
- override=True suppresses dp_rank and forwards conversation_params
  (regression case for the conv_kwargs gate)
- override=True raises RuntimeError when ConversationParams API is
  missing (guard added in _generate_locally_impl)

Refs: #11705
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Jul 15, 2026
Comment thread components/src/dynamo/trtllm/request_handlers/handler_base.py
Per review: consolidate the two runtime checks into the existing
`if _conversation_affinity is None` block so both the override guard
and the engine-detection guard fire exactly once (on first request),
then simplify `conv_affinity = self._conversation_affinity`.

Update the two override tests to reset `_conversation_affinity = None`
before driving the handler so the lazy init path is exercised.

Refs: DYN-3570
nv-yna and others added 2 commits July 15, 2026 16:33
…ath (#11738)

Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
Co-authored-by: Yuewei Na <nv-yna@users.noreply.github.com>
test_trtllm_trace_propagation: _make_engine bypasses __init__ but
_generate_started now reads _engine_conversation_affinity_override
unconditionally — add the missing default (False) alongside the existing
_conversation_affinity = False.

test_trtllm_engine_conversation_affinity: the override test patches
ConversationParams but not CONVERSATION_PARAMS_AVAILABLE in llm_engine,
so the guard fires in a real TRT-LLM 1.3.0rc20 environment. Patch
llm_engine.CONVERSATION_PARAMS_AVAILABLE = True to match the intent.

Refs: DYN-3570
@peilii peilii merged commit 1590dc6 into main Jul 16, 2026
95 checks passed
@peilii peilii deleted the gluo/env_overwrite branch July 16, 2026 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::trtllm Relates to the trtllm backend fix size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants