feat(trtllm): publish ForwardPassMetrics via FpmDirectPublisher (non-attention-DP) - #8356
Conversation
Wire the TRT-LLM adapter to publish ForwardPassMetrics so the
planner's SLA-aware scaling loop works with TRT-LLM backends, not
only vLLM. Closes the "add metrics for TrtLLM/SGLang" TODO in
components/src/dynamo/common/forward_pass_metrics.py.
The underlying Rust FpmDirectPublisher (lib/llm/src/fpm_publisher.rs)
already existed for the mocker scheduler. This change exposes it to
Python via a new PyO3 class and hooks it into the TRT-LLM adapter's
existing async stats polling loop. The Rust side owns per-DP-rank
idle heartbeat emission (1 s, matching vLLM); no Python heartbeat
thread is needed.
Changes:
* lib/bindings/python/rust/llm/fpm.rs:
Add FpmDirectPublisher PyO3 class wrapping
dynamo_llm::fpm_publisher::FpmDirectPublisher. Signature is
new(endpoint, worker_id, dp_size) / publish(dp_rank, 9 flat fields,
wall_time_secs) / shutdown(). Variance fields default to 0.0 per
MVP scope.
* lib/bindings/python/rust/lib.rs, dynamo/llm/__init__.py,
dynamo/_core.pyi:
Register + re-export + document the new class.
* components/src/dynamo/trtllm/publisher.py:
Construct FpmDirectPublisher in Publisher.initialize() with
dp_size = engine.get_attention_dp_size() (supports attention DP
from day 1). Extend handle_stat in _publish_stats_task to read
the 9 flat IterationStats fields and attentionDpRank from the
TRT-LLM stats dict and call fpm_publisher.publish(). Existing
ActiveLoad publish and Prometheus gauges are untouched. Defensive
try/except so FPM failures don't break the ActiveLoad pipeline.
Add shutdown hook in cleanup().
* components/src/dynamo/trtllm/tests/test_trtllm_fpm_publisher.py:
Unit tests for the handle_stat mapping (field order, dp_rank
routing, missing-key defaulting, ms→s latency conversion, and
initialize() constructing FpmDirectPublisher with the correct
dp_size).
Requires the companion TRT-LLM change that adds the flat fields to
IterationStats (scheduled_{num_prefill_requests, sum_prefill_tokens,
sum_prefill_kv_tokens, num_decode_requests, sum_decode_kv_tokens} +
queued_{num_prefill_requests, sum_prefill_tokens,
num_decode_requests, sum_decode_kv_tokens}) plus attentionDpRank
injection in TRT-LLM's stats serializer.
Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
…r poison Under enable_attention_dp=True with tp_size>N, only rank 0 receives stats via RPC, but the Rust FpmDirectPublisher spawns N per-rank heartbeat channels. Ranks 1..N-1 would publish permanent zero-heartbeats that the Planner interprets as N-1 idle workers -- bad scaling decisions. This change gates FPM publisher instantiation off in that case: - Publisher.__init__: add `self.fpm_enabled = self.attention_dp_size == 1` alongside the existing `attention_dp_size = engine.get_attention_dp_size()`. get_attention_dp_size() returns tensor_parallel_size iff enable_attention_dp=True AND tp_size>1 (engine.py:118-120), so the condition is False only when effective attention-DP size > 1 -- precisely the planner-poison case. - Publisher.initialize(): wrap FpmDirectPublisher(...) construction in `if self.fpm_enabled:`; else-branch logs once and leaves self.fpm_publisher = None. handle_stat's FPM branch is already wrapped in `if self.fpm_publisher is not None:` so no change needed there. Tests (test_trtllm_fpm_publisher.py): - Rewrite test_publisher_initialize_constructs_fpm_direct_publisher as test_publisher_initialize_constructs_fpm_direct_publisher_when_fpm_enabled (uses attention_dp_size=1, fpm_enabled=True, asserts dp_size==1). - Add test_publisher_does_not_init_fpm_publisher_under_attention_dp (uses attention_dp_size=4, fpm_enabled=False, asserts no construction). - Keep test_handle_stat_routes_per_attention_dp_rank for forward-compat coverage of the Rust multi-rank routing path. Net: 7 cases in the file. Today's attention-DP users already had no FPM visibility (rank-0-only RPC gate predates this change); with the gate they continue to see no FPM messages, avoiding the fake-idle pollution that an ungated publisher would produce. Zero regression. Attention-DP per-rank FPM is tracked as a follow-up; it will flip the gate off and add real per-rank emission (see ~/code/tmp_context/forwardmetric-redesign/20260416/STEPS_14_15_POSTMORTEM.md for the RC2 redesign options). Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
WalkthroughThis pull request introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/src/dynamo/trtllm/publisher.py (1)
395-416: Consider narrowing the blanketexcept Exceptionat initialize/cleanup.The three
except Exceptionsites (initialize at 405,handle_statat 571, cleanup at 815) are all flagged by Ruff BLE001. The hot-path catch at 571 is defensible — you explicitly don't want FPM to poison ActiveLoad/Prometheus publishing on every stat poll — but at init/cleanup the failure modes are narrower and bound to the binding:RuntimeError(fromto_pyerr) is the only realistic failure surface today. Narrowing here would better align with the Python guideline of not using blanketexcept Exceptionwithout re-raising.As per coding guidelines: "avoid blanket
except Exceptionunless you log and re-raise".♻️ Suggested narrowing at init
- except Exception as e: + except RuntimeError as e: logging.warning( f"Failed to initialize FpmDirectPublisher; FPM emission disabled: {e}" ) self.fpm_publisher = NoneApply the same narrowing at line 815 in
cleanup(). Leave line 571 as-is (or guard narrowly aroundself.fpm_publisher.publish(...)) with a comment explaining why it has to stay broad.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/trtllm/publisher.py` around lines 395 - 416, Narrow the blanket except at the FpmDirectPublisher init and cleanup to only catch the realistic binding error: replace "except Exception as e" in the initialization block where FpmDirectPublisher(...) is created (the branch guarded by self.fpm_enabled) with "except RuntimeError as e" and keep the logging and self.fpm_publisher = None behavior; do the same change in the cleanup() method (catch RuntimeError only). Leave the broad catch in handle_stat (or narrowly guard the publish call) with a comment explaining why it must remain broad to avoid poisoning ActiveLoad/Prometheus on stat polls.
🤖 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/trtllm/tests/test_trtllm_fpm_publisher.py`:
- Around line 202-210: Remove the blanket try/except around pub.initialize() so
real exceptions surface, or if a known downstream subsystem raises, catch only
that specific exception and assert its type/message; alternatively stub the
offending subsystem in _build_publisher_stub (similar to how
_init_publish_metrics_thread and _init_publish_kv_cache_events_thread are
already stubbed) so pub.initialize() can run without raising. Ensure
fake_fpm_cls.assert_not_called() remains meaningful by not swallowing unexpected
exceptions from Publisher.initialize().
- Around line 68-237: The tests (e.g., test_handle_stat_maps_fields_single_rank,
test_handle_stat_routes_per_attention_dp_rank,
test_handle_stat_missing_attention_dp_rank_defaults_zero,
test_handle_stat_missing_fpm_fields_are_zero,
test_iter_latency_ms_to_wall_time_secs_conversion,
test_publisher_initialize_constructs_fpm_direct_publisher_when_fpm_enabled,
test_publisher_does_not_init_fpm_publisher_under_attention_dp) are missing
required pytest markers; add a module-level pytestmark = [pytest.mark.pre_merge,
pytest.mark.gpu_0, pytest.mark.unit] at the top of the test file (and import
pytest) OR add those markers to each test function so every test has a
scheduling, GPU, and type marker as per guidelines.
- Around line 142-210: The test mutates global state by rebinding
publisher_mod.FpmDirectPublisher and asyncio.create_task inside
_build_publisher_stub and _run_initialize; change these to use pytest's
monkeypatch: in _build_publisher_stub replace direct assignment of
publisher_mod.FpmDirectPublisher with monkeypatch.setattr(publisher_mod,
"FpmDirectPublisher", fake_fpm_cls) and in _run_initialize use
monkeypatch.setattr(asyncio, "create_task", stub) so the original bindings are
restored automatically; also move the in-function imports (publisher_mod, queue,
threading, asyncio) to module scope and thread the monkeypatch fixture into the
tests that call _build_publisher_stub and _run_initialize so tests remain
hermetic and parallel-safe.
---
Nitpick comments:
In `@components/src/dynamo/trtllm/publisher.py`:
- Around line 395-416: Narrow the blanket except at the FpmDirectPublisher init
and cleanup to only catch the realistic binding error: replace "except Exception
as e" in the initialization block where FpmDirectPublisher(...) is created (the
branch guarded by self.fpm_enabled) with "except RuntimeError as e" and keep the
logging and self.fpm_publisher = None behavior; do the same change in the
cleanup() method (catch RuntimeError only). Leave the broad catch in handle_stat
(or narrowly guard the publish call) with a comment explaining why it must
remain broad to avoid poisoning ActiveLoad/Prometheus on stat polls.
🪄 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: 4b3c8fc4-8a48-44fd-b952-9c2934a87423
📒 Files selected for processing (6)
components/src/dynamo/trtllm/publisher.pycomponents/src/dynamo/trtllm/tests/test_trtllm_fpm_publisher.pylib/bindings/python/rust/lib.rslib/bindings/python/rust/llm/fpm.rslib/bindings/python/src/dynamo/_core.pyilib/bindings/python/src/dynamo/llm/__init__.py
tedzhouhk
left a comment
There was a problem hiding this comment.
approved, for the correctness of the metrics, need to review the trtllm side PR.
… fields Closes the "old TRT-LLM" planner-poison gap left by ai-dynamo#8356. Problem: handle_stat's .publish() call uses stat.get(field, 0) for every required IterationStats field. On a TRT-LLM version that predates NVIDIA/TensorRT-LLM#13199, every field defaults to 0 and the emitted ForwardPassSnapshot is byte-identical to the Rust-side idle heartbeat. The Planner reads that as "worker is idle" and can scale down a worker under real load. Same class of bug as the attention-DP rank 1..N-1 case that ai-dynamo#8356 explicitly gates, but ungated for the old-TRT-LLM path. Fix: one-shot schema probe on the first IterationStats delivered to handle_stat. Strict — all 9 fields added by #13199 must be present, else the probe shuts down the publisher and sets self.fpm_publisher = None. The existing `if self.fpm_publisher is not None:` guard then short- circuits all further FPM emission, matching the attention-DP gate's "zero messages" contract. Scope: - components/src/dynamo/trtllm/publisher.py: * Add _FPM_REQUIRED_STAT_FIELDS (9 fields). * Add self._fpm_schema_checked: bool = False in Publisher.__init__. * Add Publisher._check_fpm_schema(stat) method. * Wire the probe into handle_stat before the existing FPM publish branch. - components/src/dynamo/trtllm/tests/test_trtllm_fpm_publisher.py: * 7 new tests (15 cases via parametrize). Cover: all-fields-present keeps publisher; each of the 9 fields missing individually disables publisher; missing-all-fields (legacy TRT-LLM) disables; noop when fpm_publisher is already None; shutdown exception still disables; probe runs only once; required-fields-tuple-matches-publish guardrail. No Rust changes. No API surface changes. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
NVIDIA/TensorRT-LLM#13199 merged with a different shape than the draft this PR was originally written against: the new IterationStats fields live nested inside `inflightBatchingStats` and use names that don't match the draft's flat top-level keys. Field-extraction changes (publisher.py): - _check_fpm_schema reads the nested `inflightBatchingStats` dict and fails closed if it is missing or any of the 11 required IBS fields is absent. Strict probe protects against silent planner poison when running against a TRT-LLM that predates #13199. - handle_stat reads the 11 IBS fields and composes Dynamo-side fields: scheduled_num_prefill_requests = numContextRequests scheduled_sum_prefill_tokens = numCtxTokens (compute this iter; excludes prefix-cache carryover counted in numCtxKvTokens, to match vLLM's TTFT-prediction semantics) scheduled_sum_prefill_kv_tokens = numCtxKvTokens scheduled_num_decode_requests = numGenRequests scheduled_sum_decode_kv_tokens = numGenKvTokens queued_num_prefill_requests = numQueuedContextRequests queued_sum_prefill_tokens = numQueuedCtxTokens queued_num_decode_requests = numPausedRequests + numQueuedGenRequests queued_sum_decode_kv_tokens = numPausedKvTokens + numQueuedGenKvTokens Paused-decode requests are intentionally double-counted with numScheduledRequests upstream because the planner reads queued_decode as a KV-pressure preemption signal, where paused-decodes carry the same semantic weight as queued-gen-only requests. - FpmDirectPublisher.publish() callsite switched to keyword-only args so adjacent ints with similar units (`scheduled_*` vs `queued_*`, `*_prefill_*` vs `*_decode_*`) cannot be silently transposed. - Init-time and cleanup-time `except Exception` narrowed to `except RuntimeError` (PyO3 surfaces FpmDirectPublisher new/shutdown failures only as PyRuntimeError); hot-path catch in handle_stat stays broad on purpose, with a comment explaining why. Rust binding changes (lib/bindings/python/rust/llm/fpm.rs): - FpmDirectPublisher struct field `_inner` renamed to `inner`. The underscore prefix conventionally signals "intentionally unused" to Rust readers, but the field is load-bearing for Drop-driven shutdown of all per-rank serialization tasks. A future cleanup pass would delete it as dead and leak every spawned task. Documented in a comment. - publish() signature switched to keyword-only via #[pyo3(signature = (*, ...))]; matching update in _core.pyi stub. Test changes (test_trtllm_fpm_publisher.py): - Module-level pytestmark = [unit, trtllm, pre_merge, gpu_0] picked up by CI marker filters. Without these, the tests were silently skipped by the strict-marker config. - _build_fake_stat constructs the nested IBS structure matching the real JSON; _invoke_handler mirrors handle_stat's nested extraction and kwargs publish() exactly. A guardrail test (test_invoke_handler_matches_publisher_keyword_set) catches drift. - Schema-probe parametrize iterates all 11 IBS fields plus the ibs-not-a-dict and missing-ibs-dict edge cases. - New tests for the queued-decode composite under disagg-decode (only-queued-gen) and aggregated (only-paused) engine shapes. - _build_publisher_stub uses the monkeypatch fixture for hermetic state — no module-level rebinds that leak across tests. The blanket try/except in _run_initialize is removed; WorkerMetricsPublisher and KvEventPublisher are stubbed via monkeypatch so initialize() reaches the FPM gate cleanly. Verification (single-GPU local, non-attention-DP): - Unit: 28/28 pass. - Schema probe against real Qwen3-0.6B inside L0_PostMerge build 2697 (commit e903428, first build containing #13199): all 11 required IBS fields present at the nested path, attentionDpRank tagged at the top level. - Sustained load (concurrency 16 vs max_batch_size 4, 45 s): 520 stats sampled, max scheduled_num_decode_requests=4 (saturated batch), max queued_num_prefill_requests=3 (queueing observed), 512/520 iters had non-zero decode signal. Known limitation: TensorRT (legacy) backend leaves IBS at zero-default, so FPM emission against a TRT-engine deployment will be all-zero. The schema probe accepts keys-exist; a value-aware probe is left as follow-up. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
|
@tedzhouhk Heads-up — realigned this PR to the merged TRT-LLM #13199. The merged version landed with a different shape than the draft this PR was reviewed against:
Mapping table is in commit
Also addressed all the inline review feedback in the same commit — replies in each thread below. One small extra not captured in any inline thread: narrowed init/cleanup Mind giving the new diff a look since your earlier approval was conditioned on the trtllm-side review? |
Bind self.fpm_publisher to a local before calling .shutdown() so mypy can narrow Optional[FpmDirectPublisher] -> FpmDirectPublisher inside the method. Caller (_check_fpm_schema) already gates on `is None`, so the new in-method check is a no-op at runtime but satisfies the union-attr check that was failing CI. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
The two doc files referenced .claude/skills/SKILL.md paths, but .claude/ is gitignored and only .agents/skills/* is checked in to the public repo. Lychee on this PR caught the 404s after a cache miss exposed them. Both SKILL.md files exist at the new path, so just fixing the URLs restores the references. Out-of-PR-scope but necessary to unblock CI on this branch. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
…attention-DP) (#8356) Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com> Co-authored-by: Yuewei Na <nv-yna@users.noreply.github.com>
Summary
Close the `TODO: add metrics for TrtLLM/SGLang` in `components/src/dynamo/common/forward_pass_metrics.py`. The Dynamo TRT-LLM adapter now publishes `ForwardPassMetrics` per iteration so the Planner can treat TRT-LLM workers the same way it treats vLLM workers for autoscaling + TTFT/ITL prediction.
Scope: non-attention-DP (single-rank + plain TP). Attention-DP per-rank FPM is a deliberate non-goal for this MVP — the publisher is gated off when `engine.get_attention_dp_size() > 1` to prevent misleading fake-idle heartbeats on ranks 1..N-1. True per-rank FPM emission becomes a follow-up.
Companion TRT-LLM PR: NVIDIA/TensorRT-LLM#13199 — adds the 9 flat `IterationStats` fields that this PR consumes.
Changes
PyO3 `FpmDirectPublisher` (`lib/bindings/python/rust/llm/fpm.rs`): new Python-facing class wrapping the existing Rust `FpmDirectPublisher` struct. Per-dp-rank tokio heartbeat (1s idle interval) is Rust-owned; Python just calls `publish(dp_rank, 9 fields..., wall_time_secs)`. Re-exported from `dynamo.llm`.
TRT-LLM adapter (`components/src/dynamo/trtllm/publisher.py`):
Rationale for the gate: under `enable_attention_dp=True` with `tp_size=N`, `FpmDirectPublisher` allocates N per-rank heartbeat channels but only rank 0 actually receives stats (rank-0-only RPC gate in TRT-LLM's `rpc_worker_mixin.py`). Ranks 1..N-1 would emit permanent all-zero heartbeats that the Planner interprets as N-1 idle workers → bad scaling decisions. Gating the publisher off makes attention-DP workers produce ZERO messages on the `forward-pass-metrics` topic, which is strictly better than fake-idle pollution and matches their pre-MVP state (no FPM).
Unit tests (`components/src/dynamo/trtllm/tests/test_trtllm_fpm_publisher.py`): 7 cases covering `handle_stat` field mapping, dp_rank routing, missing-field defaults, iterLatencyMS → wall_time conversion, gate-positive case (dp_size=1 → FpmDirectPublisher constructed), gate-negative case (dp_size=4 → no construction, `fpm_publisher is None`).
Forward compat
Dynamo-side generic `dp_rank` plumbing is intentionally preserved:
When attention-DP per-rank FPM lands in the follow-up, the gate flips off and real per-rank emission fills in.
Test plan
Follow-up
Summary by CodeRabbit
New Features
Tests