fix(trtllm): decouple KV events from performance metrics - #12162
Conversation
WalkthroughChangesTensorRT-LLM metrics separation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/src/dynamo/trtllm/tests/test_trtllm_unit.py (1)
427-453: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the explicit-enablement path too.
This test only proves that KV-event publishing does not add performance flags. Add cases using
return_perf_metricsandenable_iter_perf_statsoverrides that assert those flags remain inengine_args; otherwise an implementation that disables metrics entirely would still pass.🤖 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/tests/test_trtllm_unit.py` around lines 427 - 453, Add explicit-enablement cases to the test around init_llm_worker, using return_perf_metrics and enable_iter_perf_stats overrides. Assert each explicitly enabled flag remains present with its configured value in EngineArgsCaptured.engine_args, while preserving the existing publish-kv-events assertions that these flags are absent by default.
🤖 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.
Inline comments:
In `@docs/backends/trtllm/trtllm-observability.md`:
- Line 145: Update the TensorRT-LLM observability description to state that
request-level metrics are enabled by either return_perf_metrics or
enable_iter_perf_stats, while only KV-transfer metrics additionally require
RequestPerfMetrics.timing_metrics. Replace the current “and” wording without
changing the surrounding metric or configuration guidance.
---
Nitpick comments:
In `@components/src/dynamo/trtllm/tests/test_trtllm_unit.py`:
- Around line 427-453: Add explicit-enablement cases to the test around
init_llm_worker, using return_perf_metrics and enable_iter_perf_stats overrides.
Assert each explicitly enabled flag remains present with its configured value in
EngineArgsCaptured.engine_args, while preserving the existing publish-kv-events
assertions that these flags are absent by default.
🪄 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: Enterprise
Run ID: 473fcbf2-05bf-41c7-8120-386b98d95190
📒 Files selected for processing (5)
components/src/dynamo/trtllm/backend_args.pycomponents/src/dynamo/trtllm/tests/test_trtllm_unit.pycomponents/src/dynamo/trtllm/workers/llm_worker.pydocs/backends/trtllm/trtllm-dp-rank-routing.mddocs/backends/trtllm/trtllm-observability.md
|
bf9051b to
157e828
Compare
…-kv-event-metrics Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com> # Conflicts: # components/src/dynamo/profiler/templates/dgd/trtllm/agg.yaml # components/src/dynamo/profiler/templates/dgd/trtllm/disagg.yaml # components/src/dynamo/trtllm/workers/llm_worker.py # docs/fern/backends/trtllm/trtllm-dp-rank-routing.md # docs/fern/pages/developer-guide/knowledge-base/modular-components/backends/tensorrt-llm/observability.md # examples/backends/trtllm/deploy/agg_router.yaml # examples/backends/trtllm/deploy/disagg_router.yaml
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Signed-off-by: Guan Luo <41310872+GuanLuo@users.noreply.github.com>
Summary
--publish-kv-eventsfrom implicitly setting TensorRT-LLM'sreturn_perf_metricsandenable_iter_perf_statsWhy
The legacy internal
publish_events_and_metricsboolean controlled both KV-event publication and TensorRT-LLM performance statistics. This made a KV-event A/B also enable unrelated iteration and request metric work, contaminating performance measurements—particularly the DeepSeek-V4 KV-event benchmark that motivated this change.Users can still enable performance collection explicitly through
--extra-engine-argsor--override-engine-argswith:Validation
git diff --check: passedisort,black,flake8,codespell,ruff, and repository file checks: passedpytest-marker-reporthook: skipped for the commit after it failed during unrelated test collection because/tmp/pytest_port_allocations.lockis owned bynobodyand not writableSummary by CodeRabbit
Improvements
Documentation