diff --git a/docs/config-reference.md b/docs/config-reference.md index 59ee0b182..89d0d6d82 100644 --- a/docs/config-reference.md +++ b/docs/config-reference.md @@ -639,9 +639,43 @@ backend: | `prefill_environment` | dict | {} | Environment variables for prefill | | `decode_environment` | dict | {} | Environment variables for decode | | `trtllm_config` | object | null | TRTLLM CLI configuration per mode | +| `publish_metrics` | bool | true | Pass `--publish-metrics` to Dynamo TRT-LLM workers; does not enable KV events | +| `publish_events_and_metrics` | bool or null | unset | `false`: disable both publication flags; `true`: enable the combined flag; unset/null: inherit defaults | + +With `frontend.type: dynamo`, prefill, decode, and aggregated workers publish engine metrics +by default using `--publish-metrics`, regardless of whether observability is enabled. Without +observability, this does not enable KV events. `observability.enabled: true` retains its existing +superset behavior: it additionally enables `--publish-events-and-metrics` when the combined +setting is omitted or null. Explicitly requesting the combined flag also works without +observability. + +**An explicit `backend.publish_events_and_metrics: false` is a master opt-out:** neither +publication flag is passed, even if `publish_metrics` is true or observability is enabled. +This differs from omitting the combined setting, which keeps metrics on by default. Unset +values remain null through config serialization so a saved config does not acquire an opt-out. + +| `publish_events_and_metrics` | Observability | Default publication flags | +| --- | --- | --- | +| omitted / null | disabled / omitted | `--publish-metrics` | +| omitted / null | enabled | `--publish-metrics --publish-events-and-metrics` | +| `false` | either | none | +| `true` | either | `--publish-metrics --publish-events-and-metrics` | + +**Compatibility:** the metrics-only flag requires a Dynamo build containing +[ai-dynamo/dynamo#12162](https://github.com/ai-dynamo/dynamo/pull/12162) or equivalent support. +Older builds (including Dynamo v1.4.2) reject the flag. Set `backend.publish_metrics: false` +to omit only the new flag, including when observability is enabled; this does not disable a +combined flag enabled by observability or the recipe. Set `backend.publish_events_and_metrics: false` +to omit **both** flags. srt-slurm does not substitute the combined flag as an automatic +compatibility fallback, because that would enable KV events. Omitting the flag does +not override metrics-related environment variables supplied by the user. Metrics collection +adds engine telemetry work; metrics-only does not mean zero overhead. + +These options do not change native `trtllm_serve` or sidecar worker commands. `srtctl dry-run` +shows the publication flag selected for Dynamo TRT-LLM workers. **Key differences from SGLang backend**: -- No aggregated mode support (prefill/decode only) +- Supports prefill, decode, and aggregated workers - Uses MPI-style launching (one srun per endpoint with all nodes) - Uses `trtllm-llmapi-launch` for distributed launching - Automatically sets `TRTLLM_EPLB_SHM_NAME` with unique UUID per endpoint @@ -739,9 +773,14 @@ their URLs are appended to `AIPERF_SERVER_METRICS_URLS` after the logical worker Two caveats for `AIPERF_SERVER_METRICS_URLS`: -- **TRT-LLM worker URLs are omitted when the workers publish no metrics.** A Dynamo TRT-LLM worker - launched without `--publish-events-and-metrics` (the default; `observability.enabled` turns it - on) serves nothing on its sys-port `/metrics`, so those URLs are not advertised. With +- **Dynamo TRT-LLM worker URLs are advertised when engine metrics are enabled.** This is the + default via `backend.publish_metrics: true` (`--publish-metrics`) when the combined setting + is omitted; `backend.publish_events_and_metrics: true` also enables them. Explicit + `backend.publish_events_and_metrics: false` suppresses both publication flags and worker + URLs, regardless of the metrics-only setting. URLs are also omitted when no flag is enabled + (for example, metrics-only false and the combined setting omitted without observability). + This applies to built-in AIPerf and custom benchmarks, excluding sidecars, whose behavior is + unchanged. Runtime-only metrics may still exist but do not constitute an engine-metrics capture. With `frontend.type: trtllm_serve` the gate is the worker's own engine config instead: its `/prometheus/metrics` URL is advertised when that mode's `return_perf_metrics` is true (the srtctl default for trtllm_serve recipes; an explicit `false` drops the URL). KVBM URLs are diff --git a/src/srtctl/backends/trtllm.py b/src/srtctl/backends/trtllm.py index 2a8725cdd..3842860da 100644 --- a/src/srtctl/backends/trtllm.py +++ b/src/srtctl/backends/trtllm.py @@ -105,12 +105,16 @@ class TRTLLMProtocol: # engine does not recognise. served_model_name: str | None = None - # Whether dynamo.trtllm workers pass `--publish-events-and-metrics`. - # Enables the worker to publish KV-cache events (add/evict) + metrics, which - # the dynamo frontend consumes for KV-cache-aware routing (router-mode: kv). - # This may impact performance so should be disabled if exact KV aware routing - # is not needed. - publish_events_and_metrics: bool = False + # Publish TRT-LLM engine metrics without enabling KV-cache events. + # Requires a Dynamo build supporting --publish-metrics; set False to omit + # the flag for older builds. Native trtllm-serve and sidecars are unaffected. + publish_metrics: bool = True + + # None means unspecified: metrics default on, events off (observability + # promotes this to True). Explicit False is a master opt-out of BOTH + # publication flags, even when publish_metrics is True. Preserve None in + # schema round-trips so an omitted value never becomes an explicit opt-out. + publish_events_and_metrics: bool | None = None # Controls batched startup of workers that share the same node. # 0 = start all workers in parallel (no constraint). @@ -145,6 +149,18 @@ class TRTLLMProtocol: Schema: ClassVar[builtins.type[Schema]] = Schema + @property + def dynamo_metrics_flags(self) -> tuple[str, ...]: + """Effective publication flags, preserving the explicit legacy opt-out.""" + if self.publish_events_and_metrics is False: + return () + flags = [] + if self.publish_metrics: + flags.append("--publish-metrics") + if self.publish_events_and_metrics: + flags.append("--publish-events-and-metrics") + return tuple(flags) + # ========================================================================= # BackendProtocol Implementation # ========================================================================= @@ -373,8 +389,7 @@ def build_worker_command( ] ) - if self.publish_events_and_metrics: - cmd.append("--publish-events-and-metrics") + cmd.extend(self.dynamo_metrics_flags) return self._wrap_with_numa_cpu_bind(cmd) diff --git a/src/srtctl/cli/mixins/benchmark_stage.py b/src/srtctl/cli/mixins/benchmark_stage.py index 0dd05c516..04a3ba49f 100644 --- a/src/srtctl/cli/mixins/benchmark_stage.py +++ b/src/srtctl/cli/mixins/benchmark_stage.py @@ -638,6 +638,14 @@ def _get_aiperf_server_metrics_env( ranks are not advertised as separate engines. """ urls: list[str] = [] + dynamo_trtllm_metrics_disabled = ( + self.config.frontend.type == "dynamo" + and self.config.backend_type == "trtllm" + and not ( + (not self.config.dynamo.sidecar and getattr(self.config.backend, "dynamo_metrics_flags", ())) + or getattr(self.config.backend, "publish_events_and_metrics", False) + ) + ) # trtllm-serve serves Prometheus at /prometheus/metrics on the worker # OpenAI port (GET /metrics there is JSON iteration stats, not # exposition text); every other frontend serves it at /metrics. @@ -645,7 +653,10 @@ def _get_aiperf_server_metrics_env( if logical_workers_only: if logical_endpoints is None: logical_endpoints = self._logical_worker_endpoints() - urls = [f"http://{host}:{port}{metrics_path}" for _, host, port in logical_endpoints] + # Sidecars use native worker commands, so publish_metrics does not + # control their existing logical-worker URL discovery. + if self.config.dynamo.sidecar or not dynamo_trtllm_metrics_disabled: + urls = [f"http://{host}:{port}{metrics_path}" for _, host, port in logical_endpoints] else: if self.config.frontend.type in {"vllm", "vllm-router"}: for process in self.backend_processes: @@ -676,15 +687,13 @@ def _get_aiperf_server_metrics_env( continue host = get_hostname_ip(process.node, self.runtime.network_interface) urls.append(f"http://{host}:{process.http_port}{metrics_path}") - # TRT-LLM workers only publish engine metrics when launched with - # --publish-events-and-metrics (pre-v1.3.0 Dynamo gates the whole - # worker /metrics surface on it; observability.enabled sets it at - # config load). Without the flag the sys-port endpoints serve - # nothing, so advertising them would only create the impression - # that worker metrics are being captured. - elif self.config.backend_type != "trtllm" or getattr( - self.config.backend, "publish_events_and_metrics", False - ): + # Dynamo TRT-LLM engine metrics require either the metrics-only + # flag (the default) or the legacy combined flag (also enabled by + # observability). Retain the existing sidecar gate because sidecars + # do not receive --publish-metrics. An explicit legacy False disables + # both flags. Runtime-only metrics may still exist with publication disabled, + # but must not be advertised as an engine-metrics capture. + elif not dynamo_trtllm_metrics_disabled: for process in self.backend_processes: if process.sys_port > 0: host = get_hostname_ip(process.node, self.runtime.network_interface) diff --git a/src/srtctl/cli/submit.py b/src/srtctl/cli/submit.py index 7ad7c8322..373321d1b 100755 --- a/src/srtctl/cli/submit.py +++ b/src/srtctl/cli/submit.py @@ -218,6 +218,28 @@ def show_config_details(config: SrtConfig) -> None: environment variables (global and backend per-mode) so users can verify their config is correct before submitting. """ + if config.frontend.type == "dynamo" and not config.dynamo.sidecar: + from srtctl.backends.trtllm import TRTLLMProtocol + + if isinstance(config.backend, TRTLLMProtocol): + descriptions = { + "--publish-metrics": "metrics only", + "--publish-events-and-metrics": "metrics and KV events", + } + publication = [f"{flag} ({descriptions[flag]})" for flag in config.backend.dynamo_metrics_flags] + disabled_by = ( + "publish_events_and_metrics" + if config.backend.publish_events_and_metrics is False + else "publish_metrics" + ) + console.print( + Panel( + "\n".join(publication) or f"No publication flag (backend.{disabled_by}: false)", + title="Dynamo TRT-LLM Metrics", + border_style="cyan", + ) + ) + if config.frontend.type == "vllm": from srtctl.backends.vllm import VLLMProtocol, find_vllm_orchestration_recipe_flags diff --git a/src/srtctl/core/config.py b/src/srtctl/core/config.py index 71c64c148..94350dc16 100755 --- a/src/srtctl/core/config.py +++ b/src/srtctl/core/config.py @@ -624,9 +624,10 @@ def expand_observability(cfg: dict) -> dict: One knob, six effects -- see :class:`~srtctl.core.schema.ObservabilityConfig` for the rationale and the full list. Mutates ``cfg`` in place and returns it. - Every write is a ``setdefault``: an explicit value in the recipe always - wins. That makes it safe to flip ``observability.enabled`` on globally - without silently overriding a recipe that deliberately disabled something. + Defaults preserve explicit recipe values. The tri-state combined publishing + setting treats null as unset, while explicit False remains a master opt-out. + Enabling observability never overrides a recipe that deliberately disables + publication. No-op unless ``observability.enabled`` is truthy. """ @@ -662,23 +663,20 @@ def expand_observability(cfg: dict) -> dict: # keyed by x_request_id so all three legs join on one id. _setdefault_nested(frontend, "env", ANALYTICS_REQUEST_TRACE_ENV) - # --- metrics leg: the /metrics surface and what appears on it ------------ - # publish_events_and_metrics is what creates the endpoint; without it the - # engine-config keys below have nowhere to publish to. + # --- metrics leg: engine metrics on the worker /metrics surface ---------- + # Metrics-only publication defaults on independently of observability. + # Keep observability as the existing superset that also enables KV events. if backend.get("type", "sglang") == "trtllm": - # An explicit False here defeats the whole metrics leg -- no /metrics - # surface means no KV-cache gauges for anyone, including the in-job - # scraper. setdefault still lets the recipe win (that contract matters), - # but say so loudly: a recipe written before this knob existed will - # otherwise silently produce a run with half the data missing. - if backend.get("publish_events_and_metrics") is False: + # None preserves an omitted setting through schema dumps; treat it as + # unset here too. An explicit False must remain the master opt-out. + if backend.get("publish_events_and_metrics") is None: + backend["publish_events_and_metrics"] = True + if frontend.get("type", "dynamo") == "dynamo" and backend["publish_events_and_metrics"] is False: logger.warning( - "observability.enabled but backend.publish_events_and_metrics is " - "explicitly false — workers will NOT expose /metrics, so KV-cache " - "gauges and worker scrapes will be missing. Remove that line or " - "set it true to get the full analytics capture." + "observability.enabled but backend.publish_events_and_metrics is explicitly false " + "— srt-slurm will enable neither metrics nor KV-event publication. " + "This opt-out takes precedence over backend.publish_metrics." ) - backend.setdefault("publish_events_and_metrics", True) trtllm_config = backend.get("trtllm_config") if not isinstance(trtllm_config, dict): diff --git a/src/srtctl/core/schema.py b/src/srtctl/core/schema.py index cc5eff4be..12ae2e9d2 100755 --- a/src/srtctl/core/schema.py +++ b/src/srtctl/core/schema.py @@ -1177,8 +1177,10 @@ class ObservabilityConfig: having to remember six independent flags. It expands (at config-load time, via :func:`srtctl.core.config.expand_observability`) into: - * ``backend.publish_events_and_metrics: true`` -- the worker/frontend - Prometheus ``/metrics`` surface exists at all. + * ``backend.publish_events_and_metrics: true`` -- enable KV-cache events + and TRT-LLM engine metrics. Metrics-only publication already defaults on + independently via ``backend.publish_metrics``. An explicit + ``publish_events_and_metrics: false`` disables both publication flags. * ``enable_iter_perf_stats`` + ``return_perf_metrics`` on every engine config -- the ``trtllm_kv_cache_*`` occupancy gauges and per-request histograms appear on that surface. @@ -1191,16 +1193,17 @@ class ObservabilityConfig: client does not already poll (see ``TelemetryStageMixin.start_tachometer`` and ``tachometer`` below). - Every expansion uses setdefault semantics: an explicit value in the recipe - always wins, so ``observability.enabled`` is safe to switch on globally. + Expansion preserves explicit recipe values; the tri-state combined + publishing setting treats null as unset. Explicit False is never replaced. Scope is deliberately server-side. The knob configures what the workers and frontend *emit*, and captures that surface by scraping the endpoints directly. It never asks the benchmark client to re-export what the servers already publish. (One indirect exception: on TRT-LLM the client's ``AIPERF_SERVER_METRICS_URLS`` worker list exists only when - ``publish_events_and_metrics`` gives those endpoints content, and this knob - is one way that flag gets set — see ``BenchmarkStageMixin``.) + the effective publication flags give those endpoints engine metrics, + respecting the explicit combined-setting opt-out — see + ``BenchmarkStageMixin``.) It does **not** decide whether the component perf dashboard is built. That happens on every run (see :mod:`srtctl.analysis.perf_dashboard`); ``enabled`` diff --git a/tests/test_benchmarks.py b/tests/test_benchmarks.py index 4f24e88a8..92a3cb0b7 100644 --- a/tests/test_benchmarks.py +++ b/tests/test_benchmarks.py @@ -234,7 +234,8 @@ def _benchmark_stage( *, benchmark_type="custom", backend_type="sglang", - publish_events_and_metrics=False, + publish_metrics=True, + publish_events_and_metrics=None, prefill_environment=None, aggregated_environment=None, environment=None, @@ -243,6 +244,7 @@ def _benchmark_stage( ): from types import SimpleNamespace + from srtctl.backends import TRTLLMProtocol, TRTLLMServerConfig from srtctl.cli.mixins.benchmark_stage import BenchmarkStageMixin class Stage(BenchmarkStageMixin): @@ -257,16 +259,28 @@ def backend_processes(self): def get_config_for_mode(mode): return dict(engine_sections.get("aggregated" if mode == "agg" else mode, {})) - stage = Stage() - stage.config = SimpleNamespace( - benchmark=SimpleNamespace(type=benchmark_type, aiperf_package=None), - backend=SimpleNamespace( + if backend_type == "trtllm": + backend = TRTLLMProtocol( + publish_metrics=publish_metrics, + publish_events_and_metrics=publish_events_and_metrics, + prefill_environment=prefill_environment or {}, + aggregated_environment=aggregated_environment or {}, + trtllm_config=TRTLLMServerConfig(**engine_sections), + ) + else: + backend = SimpleNamespace( type=backend_type, + publish_metrics=publish_metrics, publish_events_and_metrics=publish_events_and_metrics, prefill_environment=prefill_environment or {}, aggregated_environment=aggregated_environment or {}, get_config_for_mode=get_config_for_mode, - ), + ) + + stage = Stage() + stage.config = SimpleNamespace( + benchmark=SimpleNamespace(type=benchmark_type, aiperf_package=None), + backend=backend, backend_type=backend_type, dynamo=SimpleNamespace(sidecar=dynamo_sidecar), frontend=SimpleNamespace(type=frontend_type), @@ -427,7 +441,9 @@ def test_trtllm_serve_custom_endpoints_use_prometheus_path(self): "http://ip-node-a:6100/prometheus/metrics,http://ip-node-c:6100/prometheus/metrics" ) - def test_trtllm_serve_physical_endpoints_use_worker_http_ports(self): + @pytest.mark.parametrize("publish_metrics", [False, True]) + @pytest.mark.parametrize("publish_events_and_metrics", [None, False, True]) + def test_trtllm_serve_physical_endpoints_use_worker_http_ports(self, publish_metrics, publish_events_and_metrics): """Built-in AIPerf path: trtllm-serve never binds the DYN_SYSTEM_PORT sys-ports, so the physical-process URLs use leader http_ports at /prometheus/metrics. The route exists only when the worker's engine @@ -444,7 +460,13 @@ def test_trtllm_serve_physical_endpoints_use_worker_http_ports(self): Process("node-c", frozenset(range(4)), 7502, 6100, "decode", 0, node_rank=0), ] engine = {"prefill": {"return_perf_metrics": True}, "decode": {"return_perf_metrics": True}} - stage = self._benchmark_stage("trtllm_serve", processes, backend_type="trtllm", trtllm_config=engine) + publishing = { + "publish_metrics": publish_metrics, + "publish_events_and_metrics": publish_events_and_metrics, + } + stage = self._benchmark_stage( + "trtllm_serve", processes, backend_type="trtllm", trtllm_config=engine, **publishing + ) with patch( "srtctl.cli.mixins.benchmark_stage.get_hostname_ip", side_effect=lambda node, interface: f"ip-{node}", @@ -458,7 +480,7 @@ def test_trtllm_serve_physical_endpoints_use_worker_http_ports(self): # only the other mode's leader is advertised -- no dead URLs. engine_partial = {"prefill": {"return_perf_metrics": True}, "decode": {"return_perf_metrics": False}} stage_partial = self._benchmark_stage( - "trtllm_serve", processes, backend_type="trtllm", trtllm_config=engine_partial + "trtllm_serve", processes, backend_type="trtllm", trtllm_config=engine_partial, **publishing ) with patch( "srtctl.cli.mixins.benchmark_stage.get_hostname_ip", @@ -468,10 +490,8 @@ def test_trtllm_serve_physical_endpoints_use_worker_http_ports(self): assert env["AIPERF_SERVER_METRICS_URLS"] == "http://ip-node-a:6100/prometheus/metrics" # No engine config at all (nothing set the default): nothing to poll, and - # publish_events_and_metrics must not be mistaken for the trtllm-serve gate. - stage_off = self._benchmark_stage( - "trtllm_serve", processes, backend_type="trtllm", publish_events_and_metrics=True - ) + # Neither Dynamo publishing option may replace the trtllm-serve gate. + stage_off = self._benchmark_stage("trtllm_serve", processes, backend_type="trtllm", **publishing) with patch( "srtctl.cli.mixins.benchmark_stage.get_hostname_ip", side_effect=lambda node, interface: f"ip-{node}", @@ -573,11 +593,7 @@ def test_builtin_aiperf_retains_physical_process_metrics(self): ) def test_builtin_aiperf_omits_dead_trtllm_worker_urls(self): - """A TRT-LLM worker without --publish-events-and-metrics serves nothing. - - Advertising its sys-port endpoints to AIPerf only creates the - impression that worker metrics are captured, so the URLs are omitted. - """ + """An explicit opt-out of both publishing options omits engine URLs.""" from unittest.mock import patch from srtctl.benchmarks.trace_replay import TraceReplayRunner @@ -592,6 +608,7 @@ def test_builtin_aiperf_omits_dead_trtllm_worker_urls(self): processes, benchmark_type="trace-replay", backend_type="trtllm", + publish_metrics=False, publish_events_and_metrics=False, ) @@ -603,41 +620,67 @@ def test_builtin_aiperf_omits_dead_trtllm_worker_urls(self): assert "AIPERF_SERVER_METRICS_URLS" not in env - def test_builtin_aiperf_keeps_trtllm_worker_urls_when_publishing(self): - """With the publish flag on (e.g. via observability.enabled) the worker - endpoints have content, so the physical-process contract is retained.""" + @pytest.mark.parametrize("benchmark_type", ["trace-replay", "custom"]) + @pytest.mark.parametrize("mode", ["prefill", "decode", "agg"]) + @pytest.mark.parametrize( + ("publishing", "expected_enabled"), + [ + ({}, True), + ({"publish_metrics": False, "publish_events_and_metrics": None}, False), + ({"publish_metrics": True, "publish_events_and_metrics": None}, True), + ({"publish_metrics": False, "publish_events_and_metrics": False}, False), + ({"publish_metrics": True, "publish_events_and_metrics": False}, False), + ({"publish_metrics": False, "publish_events_and_metrics": True}, True), + ({"publish_metrics": True, "publish_events_and_metrics": True}, True), + ], + ) + def test_dynamo_trtllm_metric_urls_follow_publishing_policy( + self, benchmark_type, mode, publishing, expected_enabled + ): + """Both URL paths honor metrics-only, opt-out, and the legacy combined flag.""" from unittest.mock import patch + from srtctl.benchmarks.custom import CustomBenchmarkRunner from srtctl.benchmarks.trace_replay import TraceReplayRunner from srtctl.core.topology import Process processes = [ - Process("node-a", frozenset(range(4)), 7500, 6100, "prefill", 0, node_rank=0), - Process("node-b", frozenset(range(4)), 7501, 0, "decode", 0, node_rank=0), + Process("node-a", frozenset(range(4)), 7500, 6100, mode, 0, node_rank=0), + Process("node-b", frozenset(range(4)), 7501, 0, mode, 0, node_rank=1), ] stage = self._benchmark_stage( "dynamo", processes, - benchmark_type="trace-replay", + benchmark_type=benchmark_type, backend_type="trtllm", - publish_events_and_metrics=True, + **publishing, ) + runner = CustomBenchmarkRunner() if benchmark_type == "custom" else TraceReplayRunner() with patch( "srtctl.cli.mixins.benchmark_stage.get_hostname_ip", side_effect=lambda node, interface: f"ip-{node}", ): - env = stage._get_benchmark_env(TraceReplayRunner()) - - assert env["AIPERF_SERVER_METRICS_URLS"] == ( - "http://ip-node-a:7500/metrics,http://ip-node-b:7501/metrics" - ) - - def test_dead_trtllm_worker_urls_still_advertise_kvbm(self): + env = stage._get_benchmark_env(runner) + + if expected_enabled: + expected = "http://ip-node-a:7500/metrics" + if benchmark_type != "custom": + expected += ",http://ip-node-b:7501/metrics" + assert env["AIPERF_SERVER_METRICS_URLS"] == expected + else: + assert "AIPERF_SERVER_METRICS_URLS" not in env + if benchmark_type == "custom": + # Disabling metric URLs must not remove routable worker topology. + assert env[f"SRT_{mode.upper()}_ENDPOINTS"] == "ip-node-a:7500" + + @pytest.mark.parametrize("benchmark_type", ["trace-replay", "custom"]) + def test_dead_trtllm_worker_urls_still_advertise_kvbm(self, benchmark_type): """KVBM serves its own /metrics independently of the publish flag, so its endpoints survive the dead-worker-URL omission.""" from unittest.mock import patch + from srtctl.benchmarks.custom import CustomBenchmarkRunner from srtctl.benchmarks.trace_replay import TraceReplayRunner from srtctl.core.topology import Process @@ -648,8 +691,9 @@ def test_dead_trtllm_worker_urls_still_advertise_kvbm(self): stage = self._benchmark_stage( "dynamo", processes, - benchmark_type="trace-replay", + benchmark_type=benchmark_type, backend_type="trtllm", + publish_metrics=False, publish_events_and_metrics=False, prefill_environment={"DYN_KVBM_METRICS_PORT": "9345"}, ) @@ -658,15 +702,56 @@ def test_dead_trtllm_worker_urls_still_advertise_kvbm(self): "srtctl.cli.mixins.benchmark_stage.get_hostname_ip", side_effect=lambda node, interface: f"ip-{node}", ): - env = stage._get_benchmark_env(TraceReplayRunner()) + runner = CustomBenchmarkRunner() if benchmark_type == "custom" else TraceReplayRunner() + env = stage._get_benchmark_env(runner) assert env["AIPERF_SERVER_METRICS_URLS"] == "http://ip-node-a:9345/metrics" - def test_explicit_server_metrics_urls_env_wins(self): + @pytest.mark.parametrize("benchmark_type", ["trace-replay", "custom"]) + @pytest.mark.parametrize("publish_metrics", [False, True]) + @pytest.mark.parametrize("publish_events_and_metrics", [None, False, True]) + def test_sidecar_metric_urls_ignore_standalone_publishing_option( + self, benchmark_type, publish_metrics, publish_events_and_metrics + ): + """The standalone dynamo.trtllm flag does not change existing sidecar URLs.""" + from unittest.mock import patch + + from srtctl.benchmarks.custom import CustomBenchmarkRunner + from srtctl.benchmarks.trace_replay import TraceReplayRunner + from srtctl.core.topology import Process + + processes = [Process("node-a", frozenset(range(4)), 7500, 6100, "agg", 0, node_rank=0)] + stage = self._benchmark_stage( + "dynamo", + processes, + benchmark_type=benchmark_type, + backend_type="trtllm", + dynamo_sidecar=True, + publish_metrics=publish_metrics, + publish_events_and_metrics=publish_events_and_metrics, + ) + runner = CustomBenchmarkRunner() if benchmark_type == "custom" else TraceReplayRunner() + + with patch( + "srtctl.cli.mixins.benchmark_stage.get_hostname_ip", + side_effect=lambda node, interface: f"ip-{node}", + ): + env = stage._get_benchmark_env(runner) + + if benchmark_type == "custom": + assert env["AIPERF_SERVER_METRICS_URLS"] == "http://ip-node-a:6100/metrics" + elif publish_events_and_metrics: + assert env["AIPERF_SERVER_METRICS_URLS"] == "http://ip-node-a:7500/metrics" + else: + assert "AIPERF_SERVER_METRICS_URLS" not in env + + @pytest.mark.parametrize("benchmark_type", ["trace-replay", "custom"]) + def test_explicit_server_metrics_urls_env_wins(self, benchmark_type): """An operator-supplied AIPERF_SERVER_METRICS_URLS in the recipe environment is respected verbatim, never clobbered by injection.""" from unittest.mock import patch + from srtctl.benchmarks.custom import CustomBenchmarkRunner from srtctl.benchmarks.trace_replay import TraceReplayRunner from srtctl.core.topology import Process @@ -676,7 +761,10 @@ def test_explicit_server_metrics_urls_env_wins(self): stage = self._benchmark_stage( "dynamo", processes, - benchmark_type="trace-replay", + benchmark_type=benchmark_type, + backend_type="trtllm", + publish_metrics=False, + publish_events_and_metrics=False, environment={"AIPERF_SERVER_METRICS_URLS": "http://curated:9999/metrics"}, ) @@ -684,7 +772,8 @@ def test_explicit_server_metrics_urls_env_wins(self): "srtctl.cli.mixins.benchmark_stage.get_hostname_ip", side_effect=lambda node, interface: f"ip-{node}", ): - env = stage._get_benchmark_env(TraceReplayRunner()) + runner = CustomBenchmarkRunner() if benchmark_type == "custom" else TraceReplayRunner() + env = stage._get_benchmark_env(runner) assert env["AIPERF_SERVER_METRICS_URLS"] == "http://curated:9999/metrics" diff --git a/tests/test_dry_run.py b/tests/test_dry_run.py index 8b1746074..bf4dcbb15 100644 --- a/tests/test_dry_run.py +++ b/tests/test_dry_run.py @@ -8,6 +8,7 @@ from pathlib import Path from unittest.mock import patch +import pytest import yaml from srtctl.cli.submit import show_config_details @@ -48,6 +49,87 @@ def _make_config(overrides: dict | None = None) -> SrtConfig: return SrtConfig.from_yaml(tmp_path) +class TestDryRunDynamoMetrics: + @pytest.mark.parametrize( + ("settings", "expected", "excluded"), + [ + ({}, "--publish-metrics", "--publish-events-and-metrics"), + ({"publish_events_and_metrics": None}, "--publish-metrics", "--publish-events-and-metrics"), + ({"publish_events_and_metrics": False}, "No publication flag", "--publish-"), + ( + {"publish_metrics": True, "publish_events_and_metrics": False}, + "No publication flag", + "--publish-", + ), + ({"publish_metrics": False}, "No publication flag", "--publish-metrics"), + ( + {"publish_metrics": False, "publish_events_and_metrics": True}, + "--publish-events-and-metrics", + "--publish-metrics", + ), + ], + ) + def test_selected_flag_is_visible(self, capsys, settings, expected, excluded): + config = _make_config({"backend": {"type": "trtllm", **settings}, "frontend": {"type": "dynamo"}}) + show_config_details(config) + output = capsys.readouterr().out + assert "Dynamo TRT-LLM Metrics" in output + assert expected in output + assert excluded not in output + + @pytest.mark.parametrize("frontend", ["trtllm_serve", "dynamo"]) + def test_unrelated_workers_have_no_dynamo_trtllm_publication_panel(self, capsys, frontend): + backend = "trtllm" if frontend == "trtllm_serve" else "sglang" + config = _make_config( + {"backend": {"type": backend}, "frontend": {"type": frontend, "enable_multiple_frontends": False}} + ) + show_config_details(config) + assert "Dynamo TRT-LLM Metrics" not in capsys.readouterr().out + + def test_both_flags_are_visible(self, capsys): + config = _make_config( + {"backend": {"type": "trtllm", "publish_events_and_metrics": True}, "frontend": {"type": "dynamo"}} + ) + show_config_details(config) + output = capsys.readouterr().out + assert "--publish-metrics" in output + assert "--publish-events-and-metrics" in output + + @pytest.mark.parametrize("enabled", [False, True]) + def test_explicit_combined_false_wins_over_observability(self, capsys, enabled): + config = _make_config( + { + "backend": {"type": "trtllm", "publish_events_and_metrics": False}, + "frontend": {"type": "dynamo"}, + "observability": {"enabled": enabled}, + } + ) + show_config_details(config) + output = capsys.readouterr().out + assert "No publication flag" in output + assert "backend.publish_events_and_metrics: false" in output + assert "--publish-" not in output + + def test_sidecar_has_no_dynamo_trtllm_publication_panel(self, capsys): + config = _make_config( + { + "backend": {"type": "trtllm", "trtllm_config": {"aggregated": {"max_seq_len": 8192}}}, + "frontend": {"type": "dynamo"}, + "dynamo": {"sidecar": True}, + "resources": { + "prefill_nodes": 0, + "decode_nodes": 0, + "prefill_workers": 0, + "decode_workers": 0, + "agg_nodes": 1, + "agg_workers": 1, + }, + } + ) + show_config_details(config) + assert "Dynamo TRT-LLM Metrics" not in capsys.readouterr().out + + class TestDryRunMounts: """Test that container mounts from all sources appear in dry-run output.""" diff --git a/tests/test_model_staging.py b/tests/test_model_staging.py index 4684ba603..1b7313e3d 100644 --- a/tests/test_model_staging.py +++ b/tests/test_model_staging.py @@ -8,11 +8,12 @@ from pathlib import Path from unittest.mock import MagicMock +import pytest import yaml from srtctl.backends import TRTLLMProtocol, TRTLLMServerConfig from srtctl.core.runtime import Nodes, RuntimeContext -from srtctl.core.schema import SrtConfig +from srtctl.core.schema import DynamoConfig, SrtConfig def _runtime(*, staged=None, hf=False, model="/lustre/DeepSeek-V4-Pro"): @@ -46,6 +47,53 @@ def test_hf_uses_model_id(self): class TestSchema: + @pytest.mark.parametrize( + ("publishing", "expected_metrics", "expected_events", "expected_flags"), + [ + ({}, True, None, ("--publish-metrics",)), + ({"publish_metrics": False}, False, None, ()), + ({"publish_metrics": True, "publish_events_and_metrics": None}, True, None, ("--publish-metrics",)), + ({"publish_metrics": False, "publish_events_and_metrics": None}, False, None, ()), + ({"publish_metrics": True, "publish_events_and_metrics": False}, True, False, ()), + ({"publish_metrics": False, "publish_events_and_metrics": False}, False, False, ()), + ( + {"publish_metrics": False, "publish_events_and_metrics": True}, + False, + True, + ("--publish-events-and-metrics",), + ), + ( + {"publish_metrics": True, "publish_events_and_metrics": True}, + True, + True, + ("--publish-metrics", "--publish-events-and-metrics"), + ), + ], + ) + def test_trtllm_publishing_defaults_and_schema_roundtrip( + self, publishing, expected_metrics, expected_events, expected_flags + ): + data = { + "name": "publishing-test", + "model": {"path": "/lustre/m", "container": "trtllm", "precision": "fp4"}, + "resources": {"gpu_type": "gb300", "gpus_per_node": 4, "agg_nodes": 1, "agg_workers": 1}, + "backend": {"type": "trtllm", **publishing}, + } + schema = SrtConfig.Schema() + config = schema.load(data) + dumped = schema.dump(config) + reloaded = schema.load(dumped) + + assert config.backend.publish_metrics is expected_metrics + assert config.backend.publish_events_and_metrics is expected_events + assert dumped["backend"]["publish_metrics"] is expected_metrics + assert dumped["backend"]["publish_events_and_metrics"] is expected_events + assert reloaded.backend.publish_metrics is expected_metrics + assert reloaded.backend.publish_events_and_metrics is expected_events + assert TRTLLMProtocol(**publishing).dynamo_metrics_flags == expected_flags + assert config.backend.dynamo_metrics_flags == expected_flags + assert reloaded.backend.dynamo_metrics_flags == expected_flags + def test_stage_dir_loads(self): data = { "name": "stage-test", @@ -138,25 +186,72 @@ def test_dynamo_worker_uses_staged_path(self, tmp_path): # dynamo path passes it as --model-path assert "/raid/scratch/models/DeepSeek-V4-Pro" in cmd - def test_dynamo_worker_does_not_publish_events_by_default(self, tmp_path): - backend = TRTLLMProtocol(trtllm_config=TRTLLMServerConfig(decode={"tensor_parallel_size": 4})) + @pytest.mark.parametrize("mode", ["prefill", "decode", "agg"]) + @pytest.mark.parametrize( + ("publishing", "expected_flags"), + [ + ({}, ["--publish-metrics"]), + ({"publish_metrics": False}, []), + ({"publish_metrics": True, "publish_events_and_metrics": None}, ["--publish-metrics"]), + ({"publish_metrics": False, "publish_events_and_metrics": None}, []), + ({"publish_metrics": True, "publish_events_and_metrics": False}, []), + ({"publish_metrics": False, "publish_events_and_metrics": False}, []), + ({"publish_metrics": False, "publish_events_and_metrics": True}, ["--publish-events-and-metrics"]), + ( + {"publish_metrics": True, "publish_events_and_metrics": True}, + ["--publish-metrics", "--publish-events-and-metrics"], + ), + ], + ) + def test_dynamo_worker_publishing_policy(self, tmp_path, mode, publishing, expected_flags): + backend = TRTLLMProtocol(**publishing) + assert backend.publish_metrics is publishing.get("publish_metrics", True) + assert backend.publish_events_and_metrics is publishing.get("publish_events_and_metrics") + assert backend.dynamo_metrics_flags == tuple(expected_flags) + process = replace(self._proc(), endpoint_mode=mode) cmd = backend.build_worker_command( - self._proc(), - [self._proc()], + process, + [process], self._runtime_mock(tmp_path, "/raid/scratch/models/DeepSeek-V4-Pro"), frontend_type="dynamo", ) - assert "--publish-events-and-metrics" not in cmd - - def test_dynamo_worker_publish_events_enabled(self, tmp_path): - backend = TRTLLMProtocol( - trtllm_config=TRTLLMServerConfig(decode={"tensor_parallel_size": 4}), - publish_events_and_metrics=True, + assert sorted(arg for arg in cmd if isinstance(arg, str) and arg.startswith("--publish-")) == sorted( + expected_flags ) - cmd = backend.build_worker_command( - self._proc(), - [self._proc()], - self._runtime_mock(tmp_path, "/raid/scratch/models/DeepSeek-V4-Pro"), - frontend_type="dynamo", - ) - assert "--publish-events-and-metrics" in cmd + + @pytest.mark.parametrize("mode", ["prefill", "decode", "agg"]) + @pytest.mark.parametrize("publish_metrics", [False, True]) + @pytest.mark.parametrize("publish_events_and_metrics", [None, False, True]) + def test_native_worker_ignores_dynamo_publishing_options( + self, tmp_path, mode, publish_metrics, publish_events_and_metrics + ): + process = replace(self._proc(), endpoint_mode=mode) + runtime = self._runtime_mock(tmp_path, "/model") + runtime.frontend_port = 8000 + baseline = TRTLLMProtocol(publish_metrics=False, publish_events_and_metrics=False) + backend = TRTLLMProtocol(publish_metrics=publish_metrics, publish_events_and_metrics=publish_events_and_metrics) + + expected = baseline.build_worker_command(process, [process], runtime, frontend_type="trtllm_serve") + actual = backend.build_worker_command(process, [process], runtime, frontend_type="trtllm_serve") + + assert actual == expected + assert "trtllm-serve" in actual + assert not any(arg.startswith("--publish-") for arg in actual) + + @pytest.mark.parametrize("publish_metrics", [False, True]) + @pytest.mark.parametrize("publish_events_and_metrics", [None, False, True]) + def test_sidecar_worker_ignores_dynamo_publishing_options( + self, tmp_path, publish_metrics, publish_events_and_metrics + ): + process = replace(self._proc(), endpoint_mode="agg") + runtime = self._runtime_mock(tmp_path, "/model") + runtime.dynamo = DynamoConfig(sidecar=True) + baseline = TRTLLMProtocol(publish_metrics=False, publish_events_and_metrics=False) + backend = TRTLLMProtocol(publish_metrics=publish_metrics, publish_events_and_metrics=publish_events_and_metrics) + + expected = baseline.build_worker_command(process, [process], runtime, frontend_type="dynamo") + actual = backend.build_worker_command(process, [process], runtime, frontend_type="dynamo") + + assert actual == expected + assert "dynamo.trtllm.sidecar" in " ".join(actual) + assert "--publish-" not in " ".join(actual) diff --git a/tests/test_observability.py b/tests/test_observability.py index 38afe3654..7a13cc414 100644 --- a/tests/test_observability.py +++ b/tests/test_observability.py @@ -7,7 +7,7 @@ import yaml from marshmallow import ValidationError -from srtctl.core.config import expand_observability, expand_trtllm_serve_defaults +from srtctl.core.config import expand_observability, expand_trtllm_serve_defaults, load_config from srtctl.core.schema import SrtConfig BASE_CONFIG = { @@ -40,6 +40,87 @@ def _trtllm_config(**observability): # --------------------------------------------------------------- expansion --- class TestExpandObservability: + @pytest.mark.parametrize("enabled", [None, False, True]) + def test_metrics_default_does_not_depend_on_observability(self, enabled): + cfg = _trtllm_config() if enabled is None else _trtllm_config(enabled=enabled) + loaded = SrtConfig.Schema().load(expand_observability(cfg)) + + assert loaded.backend.publish_metrics is True + # The master observability switch remains a superset that adds events. + assert loaded.backend.publish_events_and_metrics is (True if enabled is True else None) + expected = ("--publish-metrics",) + if enabled is True: + expected += ("--publish-events-and-metrics",) + assert loaded.backend.dynamo_metrics_flags == expected + + @pytest.mark.parametrize("loader_name", ["from_yaml", "load_config"]) + @pytest.mark.parametrize("enabled", [False, True]) + @pytest.mark.parametrize("publish_metrics", [False, True]) + @pytest.mark.parametrize("publish_events_and_metrics", [None, False, True]) + def test_publishing_yaml_loaders_and_roundtrip( + self, tmp_path, monkeypatch, loader_name, enabled, publish_metrics, publish_events_and_metrics + ): + monkeypatch.setattr("srtctl.core.config.load_cluster_config", lambda: None) + cfg = _trtllm_config(enabled=enabled) + cfg["benchmark"] = {"type": "sa-bench", "concurrencies": [4]} + cfg["backend"]["publish_metrics"] = publish_metrics + cfg["backend"]["publish_events_and_metrics"] = publish_events_and_metrics + loader = SrtConfig.from_yaml if loader_name == "from_yaml" else load_config + expected_events = True if enabled and publish_events_and_metrics is None else publish_events_and_metrics + if expected_events is False: + expected_flags = () + else: + expected_flags = ("--publish-metrics",) if publish_metrics else () + if expected_events is True: + expected_flags += ("--publish-events-and-metrics",) + + # Check the raw recipe and a schema-dumped recipe through the same real + # loader: configured values and effective flags must both survive. + for filename in ("recipe.yaml", "roundtrip.yaml"): + path = tmp_path / filename + path.write_text(yaml.safe_dump(cfg)) + loaded = loader(path) + assert loaded.backend.publish_metrics is publish_metrics + assert loaded.backend.publish_events_and_metrics is expected_events + assert loaded.backend.dynamo_metrics_flags == expected_flags + cfg = SrtConfig.Schema().dump(loaded) + assert cfg["backend"]["publish_metrics"] is publish_metrics + assert cfg["backend"]["publish_events_and_metrics"] is expected_events + + @pytest.mark.parametrize("loader_name", ["from_yaml", "load_config"]) + @pytest.mark.parametrize("publish_metrics", [False, True]) + @pytest.mark.parametrize("explicit_optout", [False, True]) + def test_schema_dump_preserves_omission_before_observability_is_enabled( + self, tmp_path, monkeypatch, loader_name, publish_metrics, explicit_optout + ): + """The sweep's load/dump/reload path must not turn omission into False.""" + monkeypatch.setattr("srtctl.core.config.load_cluster_config", lambda: None) + cfg = _trtllm_config() + cfg["benchmark"] = {"type": "sa-bench", "concurrencies": [4]} + cfg["backend"]["publish_metrics"] = publish_metrics + if explicit_optout: + cfg["backend"]["publish_events_and_metrics"] = False + else: + assert "publish_events_and_metrics" not in cfg["backend"] + schema = SrtConfig.Schema() + dumped = schema.dump(schema.load(cfg)) + assert "publish_events_and_metrics" in dumped["backend"] + assert dumped["backend"]["publish_events_and_metrics"] is (False if explicit_optout else None) + + dumped["observability"]["enabled"] = True + path = tmp_path / "enable-observability.yaml" + path.write_text(yaml.safe_dump(dumped)) + loader = SrtConfig.from_yaml if loader_name == "from_yaml" else load_config + loaded = loader(path) + + assert loaded.backend.publish_metrics is publish_metrics + assert loaded.backend.publish_events_and_metrics is (not explicit_optout) + if explicit_optout: + assert loaded.backend.dynamo_metrics_flags == () + else: + expected = ("--publish-metrics",) if publish_metrics else () + assert loaded.backend.dynamo_metrics_flags == (*expected, "--publish-events-and-metrics") + def test_disabled_is_a_noop(self): cfg = expand_observability(_trtllm_config(enabled=False)) assert "publish_events_and_metrics" not in cfg["backend"] @@ -82,6 +163,7 @@ def test_enabled_turns_on_metrics_surface_and_iteration_stats(self): cfg = expand_observability(_trtllm_config(enabled=True)) assert "telemetry" not in cfg assert cfg["backend"]["publish_events_and_metrics"] is True + assert SrtConfig.Schema().load(cfg).backend.publish_metrics is True for mode in ("prefill", "decode"): section = cfg["backend"]["trtllm_config"][mode] # enable_iter_perf_stats is what produces trtllm_kv_cache_*_blocks. @@ -99,6 +181,37 @@ def test_explicit_recipe_values_win(self): assert out["backend"]["trtllm_config"]["decode"]["return_perf_metrics"] is False assert out["backend"]["publish_events_and_metrics"] is False + @pytest.mark.parametrize("frontend_type", ["dynamo", "trtllm_serve"]) + @pytest.mark.parametrize("publish_metrics", [False, True]) + @pytest.mark.parametrize("publish_events_and_metrics", [False, True]) + def test_publishing_optouts_are_preserved_and_only_disabled_dynamo_metrics_warn( + self, caplog, frontend_type, publish_metrics, publish_events_and_metrics + ): + cfg = _trtllm_config(enabled=True) + cfg["frontend"]["type"] = frontend_type + cfg["backend"]["publish_metrics"] = publish_metrics + cfg["backend"]["publish_events_and_metrics"] = publish_events_and_metrics + + with caplog.at_level("WARNING"): + out = expand_observability(cfg) + + assert out["backend"]["publish_metrics"] is publish_metrics + assert out["backend"]["publish_events_and_metrics"] is publish_events_and_metrics + publishing_warnings = [record for record in caplog.records if "publish_events_and_metrics" in record.message] + should_warn = frontend_type == "dynamo" and publish_events_and_metrics is False + assert bool(publishing_warnings) is should_warn + + def test_observability_can_publish_legacy_metrics_when_standalone_flag_is_disabled(self, caplog): + cfg = _trtllm_config(enabled=True) + cfg["backend"]["publish_metrics"] = False + + with caplog.at_level("WARNING"): + out = expand_observability(cfg) + + assert out["backend"]["publish_metrics"] is False + assert out["backend"]["publish_events_and_metrics"] is True + assert not [record for record in caplog.records if "publish_metrics" in record.message] + def test_preexisting_env_is_preserved(self): cfg = expand_observability(_trtllm_config(enabled=True)) assert cfg["backend"]["prefill_environment"]["TLLM_LOG_LEVEL"] == "INFO"