[2/3] feat(observability): tachometer scrapes the complement of client-polled endpoints - #350
Merged
Merged
Conversation
A TRT-LLM worker launched without --publish-events-and-metrics (the default; observability.enabled sets it at config load) serves nothing on its sys-port /metrics, yet AIPERF_SERVER_METRICS_URLS still advertised every worker endpoint -- creating the impression that worker metrics were captured when the export contains frontend families only. - Omit TRT-LLM worker sys-port URLs when publish_events_and_metrics is off. KVBM endpoints are unaffected: KVBM serves its own /metrics independently of the flag. - Respect an explicit AIPERF_SERVER_METRICS_URLS in the recipe environment instead of silently clobbering it. - Custom-benchmark logical-leader injection is unchanged, so consumers that use the URL list for drain detection keep receiving it. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
Rows carried only time_since_start (monotonic-relative), so they could not be joined with AIPerf records or Dynamo spans, and every value was downcast to f32, corrupting cumulative counters above 2^24. The writer now captures SystemTime at start and emits a non-nullable Int64 timestamp_ns per row (start_epoch_ns + monotonic elapsed, so rows stay ordered under wall-clock steps); metric_value and the histogram columns are Float64 end to end. Adds a regression test: timestamp_ns present/recent/non-decreasing and 16777217.0 round-trips exactly (fails under the old f32 schema). Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
… client complement - tachometer.enabled becomes tri-state: null follows observability.enabled (no tachometer: block needed), explicit false opts out, explicit true without the master knob still raises. - The scraper targets the complement of AIPERF_SERVER_METRICS_URLS via _client_polled_metric_urls(), derived from the same injection logic so the complement moves when the injected set changes; worker endpoints are never double-polled (the incident the old warning documents). Frontend, DCGM and node-exporter endpoints always belong to tachometer; backend targets are leaders-only. - Storage path becomes <subdir>/raw/scrape and srtctl pre-creates only the parent: tachometer-scraper aborts on a pre-existing storage dir, which killed every sweep-mode run to date (job 452644). - Best-effort contract: the scraper ManagedProcess is critical=False, and the default binary name resolves against the checkout's bin/ before PATH. - The legacy RAW scraper no longer follows enabled (scrape_metrics: true is a pure opt-in); the host sampler re-gates onto observability.enabled. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
… processors Both new sources converge on the frozen schema-2 server_metrics_export.jsonl so rows are source-interchangeable at the panel layer: metrics_tachometer reconstructs exposition-style _bucket/_sum/_count series from the folded parquet rows and injects worker_id/dynamo_component like the prometheus path; metrics_aiperf_jsonl re-attaches aiperf's stripped name suffixes. Auto-selection prefers tachometer parquet, then raw_prometheus.jsonl, then the per-scrape jsonl, then the aggregate json (historical corpus). mooncake-router now requests --server-metrics-formats json jsonl like trace-replay. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
…lection Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
nv-yna
requested review from
alec-flowers,
csahithi,
hjjq,
ishandhanani,
kedarpotdar-nv,
kyleliang-nv,
nlevin-ui and
qiching
as code owners
August 27, 2026 00:10
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #350 +/- ##
=======================================
Coverage ? 72.17%
=======================================
Files ? 96
Lines ? 13148
Branches ? 0
=======================================
Hits ? 9489
Misses ? 3659
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Restore all-rank backend coverage (the pre-complement behavior): follower metadata columns keep rows distinguishable, and rank-level coverage is the point of the capture — leaders-only silently halved multi-node worker visibility. The vLLM agg-follower skip stays. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
… non-critical Matches the power-telemetry launch contract. Caught by the first real run: node-exporter's FROM-scratch image has no bash, so the wrapped launch died with execve() ENOENT — and as a critical process it tore down a 7-node job at startup. Telemetry sidecars must never kill the benchmark. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
This was referenced Aug 27, 2026
Collaborator
Author
|
Correction to the "known pre-existing gap" note in this PR's description: node-exporter meminfo families ARE collected. Re-inspection of run 2806252's Parquet shows 58 |
ishandhanani
added a commit
that referenced
this pull request
Sep 8, 2026
…r parquet Two findings from the first cluster runs of the graceful-shutdown path (sa-b200 jobs 12807 and 12808): Native sglang.launch_server workers drain in about five seconds after SIGTERM and then treat their own exit as a crash: py-spy dumps (which need root) and a 60s wait for CUDA coredumps that are never produced unless SGLANG_CUDA_COREDUMP=1. Cleanup killed them 30s in. Workers under engine sglang now default SGLANG_CUDA_COREDUMP_BEFORE_CRASH=0 and SGLANG_PYSPY_DUMP_BEFORE_CRASH=0; a recipe that opts into coredumps (SGLANG_CUDA_COREDUMP=1 in a role's env or the global environment) keeps the wait, and any explicit value wins. The tachometer-scraper binaries attached to the GitHub releases (what make setup installs) were built before the writer gained the timestamp_ns column (#350), so the perf dashboard ingest crashed on every fresh install with "Field timestamp_ns does not exist in schema" (job 12807). The ingest now derives timestamps from time_since_start anchored at the scraper start logged in tachometer.out, or at the parquet mtime when no log is nearby, and says so in the log.
nv-yna
added a commit
to nv-yna/srt-slurm
that referenced
this pull request
Sep 10, 2026
Every published tachometer-scraper release asset up to v1.0.96 predates the vendored writer's timestamp_ns column (NVIDIA#350, 2026-08-26): the release workflow's build-tachometer-scraper jobs have failed at actions/checkout since the NVIDIA#350 merge (runs 33043846919, 34112274235), and every release since re-uses the previous release's binaries. Such scrapers write only time_since_start, and the ingest died on every capture with KeyError: 'Field "timestamp_ns" does not exist in schema' (seen on hecate 487539 and again on 565810), so the metrics leg of the perf dashboard bundle was silently empty for runs that had the data on disk. When the column is absent, derive it: epoch of time_since_start == 0 from an explicit --start-ns, else the first timestamped line of the run's tachometer.out (the scraper logs within the second it starts counting from), else the newest parquet's mtime minus its largest time_since_start (row-group statistics). The source is logged at WARNING; precision equals the one-second grid every timestamp is snapped to. Parquet with the real column is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
nv-yna
added a commit
that referenced
this pull request
Sep 11, 2026
Every published tachometer-scraper release asset up to v1.0.96 predates the vendored writer's timestamp_ns column (#350, 2026-08-26): the release workflow's build-tachometer-scraper jobs have failed at actions/checkout since the #350 merge (runs 33043846919, 34112274235), and every release since re-uses the previous release's binaries. Such scrapers write only time_since_start, and the ingest died on every capture with KeyError: 'Field "timestamp_ns" does not exist in schema' (seen on hecate 487539 and again on 565810), so the metrics leg of the perf dashboard bundle was silently empty for runs that had the data on disk. When the column is absent, derive it: epoch of time_since_start == 0 from an explicit --start-ns, else the first timestamped line of the run's tachometer.out (the scraper logs within the second it starts counting from), else the newest parquet's mtime minus its largest time_since_start (row-group statistics). The source is logged at WARNING; precision equals the one-second grid every timestamp is snapped to. Parquet with the real column is unchanged. Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com> Co-authored-by: Yuewei Na <nv-yna@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
ishandhanani
added a commit
that referenced
this pull request
Sep 13, 2026
…ervability, MCP, and the migrator (#386-#406 and after) (#407) * Remove the --bash direct-host execution path srt-slurm 2.0 is the Slurm piece only. The direct-host lifecycle that rendered a Docker launcher from a recipe (srtctl apply --bash) is removed so every later 2.0 change has one implementation to write instead of two. Deleted: - src/srtctl/render/ (direct plan, host runner, container runner, and the direct stage mixins) - src/srtctl/templates/direct_container.sh.j2 - docs/direct-host.md - tests/test_direct_host_runner.py, tests/test_direct_plan.py, tests/test_direct_runner.py Edited: - cli/submit.py: drop the --bash flag, render_bash_script, and the bash-mode argument checks; the Slurm submit path is unchanged - tests/test_submit_cli.py: replace the direct-container test with a check that --bash is now rejected by argparse - docs (cli.md, README.md, docs/README.md, ruter.md, SUMMARY.md) and two comments in telemetry_stage.py, core/telemetry.py, ruter/normalize.py Nothing under core/power/ or the power hooks is touched. The path can return later as a separate execution target; nothing in 2.0 core depends on it. Part of the 2.0 plan: #385 * chore: replace recipes with curated examples Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com> * Replace the recipes archive with a curated examples matrix The 563-file recipes/ tree was a benchmark-results archive, not a set of starting points. It is removed. examples/ now holds one small, runnable configuration per frontend and topology, all serving Qwen3-0.6B on one node so the files differ only in the frontend and prefill/decode layout: sglang/ dynamo-agg dynamo-disagg sglang-router-agg sglang-router-disagg vllm/ dynamo-agg dynamo-disagg vllm-router-agg vllm-router-disagg vllm-direct-agg trtllm/ dynamo-agg dynamo-disagg trtllm-serve-agg trtllm-serve-disagg mocker/ dynamo-agg features/ sweep override profiling examples/README.md documents the matrix, the srtslurm.yaml aliases the files rely on, and how to validate them. Wiring (carried from #338): CI's validate step, the Makefile target, the interactive selector, CODEOWNERS, docs, and tests point at examples/. validate_config_file now expands top-level `sweep:` files the same way it expands override files, so CI validates every file under examples/ including features/sweep.yaml. tests/test_configs.py checks that every topology example loads as a plain config, that every example validates, and that the documented matrix is present on disk. Part of the 2.0 plan: #385 * Point the e2e example tests at the one-node matrix test_e2e.py encoded the old 6P+2D two-node Qwen3-32B layout. The example-dependent tests now parametrize over every topology example, and the shared-node disaggregation checks assert the 1P+1D TP1 layout (decode_nodes: 0) that all four disagg examples use. * Drop --disable-log-requests from the vLLM examples Current vLLM rejects the flag (argparse exit 2), which killed the direct vLLM example on the first cluster run. * Register the mocker under the name the benchmark requests dynamo.mocker derives its model name from --model-path, which for the /model container mount is "model", while sa-bench asks for the model path basename (SrtConfig.served_model_name). With a local model path the benchmark 404s with "Model not found"; it only worked for hf: paths. Pass --model-name explicitly. Surfaced by examples/mocker/dynamo-agg.yaml on sa-b200 (job 11846). * Generate docs/schema-reference.md from the config dataclasses docs/config-reference.md is hand-written prose and drifts from the schema (the power docs merged last week already reference a field that #317 removed). This adds a generated, CI-checked field-level reference so the truth lives in the code. - src/srtctl/core/schema_docs.py walks the dataclass tree rooted at SrtConfig plus the four backend types and ClusterConfig, and renders one Markdown table per dataclass: YAML key (honoring marshmallow data_key), type (nested dataclasses linked, Literals expanded), default, and a description taken from the class docstring `Attributes:` block or the comment on the field. - `srtctl schema-docs` writes docs/schema-reference.md; `--check` exits 1 when the checked-in file is stale. `make schema-docs-check` and a CI lint step run the check; tests/test_schema_docs.py enforces it in the test suite too and covers the renderer and the CLI. - docs/config-reference.md points at the generated file as authoritative for keys, types, and defaults; SUMMARY.md links it; CLAUDE.md tells contributors to regenerate after any schema change. Part of the 2.0 plan: #385 * docs: regenerate schema reference after the observability changes on main * Add the recipe schema version key and srtctl migrate Recipes gain a top-level `schema:` key (attribute `schema_version`, YAML key `schema` via marshmallow data_key). Absent means 1, the pre-2.0 layout; `schema: 2` is the 2.0 layout. Both load; unknown versions are rejected at load time. This is the dispatch point the structural 2.0 changes hang off, and it gives downstream recipes a way to say which layout they target instead of failing on unknown fields. - SrtConfig.schema_version with CURRENT_SCHEMA_VERSION / SUPPORTED_SCHEMA_VERSIONS - generate_override_configs and resolve_override_yaml carry a file-level `schema` (declared beside `base`) into every expanded variant - src/srtctl/core/migrate.py: ruamel round-trip migration that inserts or bumps `schema: 2` and preserves comments, key order, and quoting. Structural v1 -> v2 rewrites plug into _migrate_1_to_2 in later steps. - `srtctl migrate -f recipe.yaml [--in-place | --output PATH]` - all 17 examples migrated to `schema: 2` with the new command - docs: config-reference `## schema`, regenerated schema-reference Part of the 2.0 plan: #385 * Add --set KEY=VALUE and --unset KEY recipe overrides Downstream runners edit recipes with sed against v1 indentation and field names, which breaks silently under any re-nesting. This gives them a supported, path-based way to tweak a recipe from a script before the 2.0 structural changes land. - src/srtctl/core/overrides.py: dotted paths with [N] indexes and quoted segments; values parse as YAML scalars or lists, mappings stay literal strings (engine flags take JSON); apply to a plain, sweep, or override document. On override files a --set is written into base and every override_*/zip_override_* variant (one-element list broadcast for zip groups) so no variant can shadow it; --unset removes from all of them and is a no-op on missing paths. - cli: --set/--unset on apply, dry-run, preflight, and resolve-override. Applied in materialize_config_path to the raw document (ruamel round trip, comments preserved) before cluster defaults, observability expansion, sweep expansion, and schema validation, so an explicit --set wins and {placeholder} values still expand. The overridden document is what gets copied as config.yaml into the job directory; the source file is never modified. Every --json record lists applied_overrides. - docs/cli.md: flags, semantics, examples. Part of the 2.0 plan: #385 * Resolve every container alias with one recipe walker Container-alias resolution was seven hand-written blocks in resolve_config_with_defaults, one per image key, and the Mooncake master container had no block at all. Replace them with a single walk over the recipe: any string under a `container`, `container_image`, `image`, or `nginx_container` key that names a `containers:` alias is resolved; literal paths and registry URIs pass through. Free-form maps (environment, *_environment, env, args, engine config blocks, container_mounts) and the identity block are skipped, so a key that happens to be called `image` in user data is never rewritten. A new recipe block that names an image (e.g. a future services: list) resolves with no resolver code. - src/srtctl/core/config.py: resolve_container_aliases() + CONTAINER_ALIAS_KEYS - tests/test_container_aliases.py: every image key in one pass, skip list, literal/URI/unknown pass-through, new-block coverage, mutate-in-place notes - docs/config-reference.md: document the containers resolver Part of the 2.0 plan: #385 * Default resources.gpu_type and gpus_per_node from srtslurm.yaml gpu_type and gpus_per_node describe the cluster, not the deployment, yet every recipe repeated them. Make both inheritable so one recipe can move between clusters unchanged: - resources.gpu_type is now optional (str | None); a recipe that omits it inherits srtslurm.yaml default_gpu_type. - resources.gpus_per_node inherits the cluster gpus_per_node when omitted (else the existing default of 4). - ClusterConfig gains default_gpu_type. Applied in resolve_config_with_defaults; an explicit recipe value always wins. The fields stay valid in recipes so a run is self-describing for result rollups. No consumer required gpu_type to be non-None. Docs: config-reference resources + cluster tables; regenerated schema-reference. Part of the 2.0 plan: #385 * Reject unknown benchmark.type at load and delete the dead BenchmarkType enum An unknown benchmark.type (a typo like `gsm8k-bench`, or a removed type) loaded fine and only failed deep in the benchmark stage after a full allocation. SrtConfig.__post_init__ now checks benchmark.type against the runner registry plus the special `manual` type, so a bad type fails at load / dry-run instead. The registry import is lazy and guarded, so an import hiccup never blocks a load. The BenchmarkType enum listed 10 of the 13 registered types and had zero references anywhere; it is removed. The registry (register_benchmark / list_benchmarks) is the single source of truth for valid types. This is the safe half of the benchmark step: it validates the type. The per-type field split (rejecting stray fields like use_chat_template on a custom benchmark) is deferred to the discriminated-union follow-up. Part of the 2.0 plan: #385 * Add the roles: authoring surface for recipe topology A recipe can now describe a worker role in one block: roles: prefill: {nodes: 2, workers: 6, gpus: 2, env: {...}, args: {...}} decode: {nodes: 0, workers: 2, gpus: 2, env: {...}, args: {...}} instead of spreading it across resources.prefill_workers / gpus_per_prefill, backend.prefill_environment, and backend.<engine>_config.prefill. src/srtctl/core/roles.py normalizes a roles: block into those existing internal fields before schema load (expand_roles), wired into resolve_config_with_defaults and SrtConfig.from_yaml so every load path is covered and no downstream consumer changes. Role names are prefill/decode/agg; agg maps to the aggregated env/config keys; args route to the engine's config by backend.type; extra_args to <mode>_extra_args. roles_from_legacy is the inverse (used to prove equivalence and for the future migrator). Mixing roles: with the fields it expands into is rejected. The legacy layout still loads unchanged, so v1 recipes keep working, and `srtctl migrate` still only stamps schema: 2 (the comment-preserving legacy->roles rewrite over the downstream corpus is deferred to the golden-CI migration release). The examples are converted to roles:, except features/override.yaml which stays legacy to show v1 still loads. Part of the 2.0 plan: #385 * docs(roles): show env and args as block-style YAML mappings The reference and module docstring used flow-style mappings, which read like JSON. roles.<role>.env and roles.<role>.args are plain mappings and are written exactly like the legacy backend fields they normalize into. * Add the placement: vocabulary for frontend, benchmark, and infra One placement vocabulary replaces the per-block knobs: frontend: {placement: {node: head | first_decode | dedicated}} benchmark: {placement: {node: head | last_decode | dedicated}} infra: {placement: {node: head | dedicated}} node: dedicated reserves a node for that component (and implies the head location, which the legacy validation already required); any other value is a location string. src/srtctl/core/placement.py normalizes these blocks into the existing fields (frontend.orchestrator_placement / dedicated_node, benchmark.client_placement / client_dedicated_node, infra.etcd_nats_dedicated_node) before schema load, wired into resolve_config_with_defaults and SrtConfig.from_yaml alongside expand_roles. No consumer changes; legacy fields still load; mixing placement: with the fields it fills is rejected. Part of the 2.0 plan: #385 * docs(placement): show placement blocks as block-style YAML * Add the services: block for sidecars and standalone Mooncake stores One top-level list for every long-running process launched next to the job. Each entry has a type that selects a registered ServiceKind (@register_service, like @register_benchmark): generic launches exactly the argv written; mooncake-store runs a standalone Mooncake Store wired to the managed master. The kind supplies defaults (command, start phase, criticality) and the env it injects; ServiceStageMixin launches every kind the same way: placement.node -> physical nodes (head, infra, prefill, decode, agg, workers), optional clone/build of an immutable git source, one srun per node, optional TCP readiness gate, ManagedProcess into the shared registry. start: before_workers runs after the Mooncake master; after_frontend (default) runs once the frontend is healthy. Folds the two open sidecar PRs onto the 2.0 shape: the generic sidecar with source/build_command/inherit_discovery_env/critical from #374 (its --bash direct-runner half is gone with that path), and the standalone Mooncake Store services from #265 as a typed service instead of a backend.mooncake_kv_store.standalone sub-block with a per-role placements map. Per-role segment sizes are now two entries placed on prefill and decode; two services on one node with the same port are rejected before launch. Adds examples/features/services.yaml (an HTTP log browser gated on its port), docs/services.md, a config-reference section, dry-run output, the regenerated schema reference, and tests for the schema, kinds, and stage. * fix(sglang): kv_events_config: true also covers aggregated workers The global-bool shortcut only matched prefill and decode, so an aggregated topology never got --kv-events-config and the sidecar's cache-overlap score stayed at zero. Carried over from #374 with its regression test. * services: register every srun at launch, bound clone/build, fail fast on death Close the windows where a service process could run untracked: - start_services takes the ProcessRegistry and registers each service process the moment its srun exists, not after the whole stage returns. A SIGTERM during a readiness wait now finds every launched process in the registry; the stage also terminates what it started on any BaseException before re-raising. - The one-shot clone and build sruns are ManagedProcesses in the registry too (non-critical), and wait under a wall-clock bound: the clone by the sum of its per-command git timeouts, the build by a new build_timeout_seconds (default 1800). A step that overruns is killed and the job fails pointing at its log instead of holding the allocation. - The readiness gate polls in short slices and checks the process between them, so a service that dies before opening its port fails at once with its exit code rather than after the full timeout. Documented under a Cleanup section in docs/services.md. * Add dynamo.source and pin source revs to commits at submit One shape for "this code, from git", shared by services[].source and the new dynamo.source: dynamo: source: git: https://github.com/ai-dynamo/dynamo # default when only rev is set rev: refs/pull/14000/head # commit, tag, or PR head dynamo.source takes exactly one of git+rev (with optional patches, the legacy cargo_patches), pypi (the legacy version), or wheel. It maps onto the legacy fields in DynamoConfig.__post_init__, so the install code and every downstream consumer keep reading hash / version / wheel unchanged, and v1 recipes still load. Combining source with a legacy field is rejected. Testing an unmerged Dynamo PR was impossible before: hash needed a SHA, and a plain clone does not carry PR refs. The cached install now clones the configured repo (forks work) and fetches a non-commit ref by name. A ref like refs/pull/N/head moves, so srtctl apply resolves every unpinned source.rev with git ls-remote (peeled tags preferred) and records the commit as source.sha in the submitted config.yaml, comments preserved and the recipe on disk untouched. The job builds exactly the commit the lockfile names and the /configs/dynamo-wheels cache is keyed by it; two runs of one recipe cannot silently build different code because the PR was pushed to while the job queued. dry-run and preflight never touch the network. A resolution failure warns and leaves the ref unpinned rather than blocking the submit; --json lists what was pinned as pinned_sources. Adds examples/features/dynamo-source.yaml (Dynamo built from the v1.4.2 tag), docs, dry-run output, tests for the shape, resolution, pinning, the Dynamo mapping, and the submit path, and the regenerated schema reference. * Add the readiness probe vocabulary for services: tcp, http, log services[].readiness names exactly one probe: tcp (a port accepts a connection; `port:` alone is shorthand), http (GET a path on a port and expect a status), or log (a regular expression matched against the service's log file), plus timeout_seconds and interval_seconds. The generic wait loop in core/readiness.py re-runs the probe until it passes, the deadline expires, or the process dies, so a crashed service fails at once instead of after the full timeout. The global health_check block and the worker and frontend health checks are untouched, per the schema design: this fixes the silent-dead-sidecar class without touching the 377 downstream recipes that set health_check. The services example now gates on an http probe against the log browser's directory listing. * Add post_eval: passthrough_env and a command override for the eval dispatch The RUN_EVAL / EVAL_ONLY evaluation forwards a built-in list of workflow variables into the eval process. Downstream runners extended that list by patching srtctl's source text (patch_srt_eval_dispatch.py, anchored on exact lines in do_sweep.py). post_eval.passthrough_env makes the extension config, and post_eval.command replaces the built-in lm-eval runner argv with placeholders for {endpoint} and {infmax_workspace}, so a runner that rewrites bench.sh can point at its own script instead. MODEL_NAME and EVAL_CONC are still set by srtctl. Shown in dry-run when set. * Split benchmark fields per type: schema 2 rejects fields the type does not use Every runner now declares the BenchmarkConfig fields it reads as config_fields, next to a shared set every type may set (client placement, sweep, aiperf plumbing, post-processing). SrtConfig rejects a schema: 2 recipe that sets a field outside shared + its type's fields, naming the stray fields and the accepted ones; a schema 1 recipe gets a warning and keeps loading. Before this, a field the runner never read (isl on gsm8k, num_shots on sa-bench) was a silent no-op. Checked against the full recipe corpus: 555 historical in-repo recipes plus 481 downstream InferenceMAX recipes, 1179 variants after override expansion. The rule rejects exactly one (a manual-type recipe carrying isl/osl/concurrencies); every other field set per type is accepted. * Migrate v1 recipes to the v2 layout and prove equality with srtctl migrate --verify srtctl migrate now rewrites the legacy layout, not just the schema key, on a ruamel round-trip document so comments, key order, and quoting survive and moved keys keep their comments: - resources.<role>_nodes/_workers, gpus_per_<role>, backend.<mode>_environment, backend.<engine>_config.<mode>, backend.<mode>_extra_args -> roles.<role> - frontend.orchestrator_placement/dedicated_node, benchmark.client_placement/ client_dedicated_node, infra.etcd_nats_dedicated_node -> placement.node - dynamo.hash/cargo_patches/wheel/version -> dynamo.source (top_of_tree is left with a note; it has no immutable rev) - benchmark fields the type never reads are removed (schema 2 rejects them) Every variant of an override file (base, override_*, zip_override_*) is rewritten, since a half-migrated file would collide roles with the legacy keys on merge. A backend block emptied by the folds keeps an explicit type instead of disappearing, because a zip variant may null its way back to the default and that only works while the base has the key. -f accepts directories and repeats; --in-place rewrites many files at once. srtctl migrate --verify migrates in memory, expands plain, override, and sweep files, resolves both documents through the loader, and compares the dumps (masking dynamo.source, which only records spelling, and benchmark fields the type never reads). CI runs it over the examples and the 555 historical recipes extracted from the last commit that carried recipes/ (make golden-check does the same locally). Results: examples 19/19 identical. Historical corpus 553 identical, 0 mismatched, 2 skipped because v1 itself rejects benchmark.type 'gsm8k-bench'. Downstream InferenceMAX corpus (481, run locally; the repo is private): 370 identical, 0 mismatched, 105 skipped because v1 itself rejects telemetry.provider or benchmark.tokenizer_mode, 6 unreadable because the recipe has a duplicate YAML key (reported by name). * ci: fix schema-docs drift on Python 3.10 and fetch the golden corpus by full SHA The schema reference generator recognised Annotated by comparing str(get_origin(...)) to "typing.Annotated", which only holds on newer Pythons; on the 3.10 CI interpreter the marshmallow field metadata leaked into the type column and the drift check failed. Compare identity with typing.Annotated instead; the generated file is now identical on 3.10 and 3.13. The golden-equality job fetched the historical recipes commit by its short SHA, which git fetch cannot resolve as a remote ref. Use the full 40-char SHA in the workflow and the Makefile target. * Scrape the SGLang Model Gateway and native workers; deliver SIGTERM to steps Tachometer coverage for frontend.type: sglang. The Model Gateway only starts its Prometheus listener when --prometheus-port is passed (PrometheusConfig is None otherwise), and sglang.launch_server only serves /metrics on its HTTP port with --enable-metrics, so every worker and frontend scrape failed for the whole run (job 12440: 287+288 worker errors, 52 frontend errors). Now: - SGLangFrontend.get_managed_frontend_args adds --prometheus-port 29000 --prometheus-host 0.0.0.0 unless the recipe set them; - native sglang.launch_server workers get --enable-metrics under the sglang frontend unless the role's args already set it; - generate_tachometer_config targets worker leaders on http_port (followers serve nothing) and the gateway on its Prometheus port, not the routing port, via a new frontend_metrics_port argument the stage derives from frontend.args. Graceful shutdown. tachometer compacts its arrow buffer to parquet on SIGTERM, but registry cleanup only SIGTERMed the srun, which srun turns into a step abort that SIGKILLs the task ("srun: forcing job termination ... task 0: Killed"; probed on sa-b200 with and without exec, the task never saw SIGTERM). So the parquet was never written and the perf dashboard showed scrapes=0. Now start_srun_process(step_name=...) names the step (srun --job-name) and ManagedProcess.terminate() for a named step delivers the signal with `scancel --signal=TERM --full <job>.<step>` (probed: "CANCELLED DUE to SIGNAL Terminated", task handled it, srun exited 0), waits terminate_timeout (90 s for tachometer, 10 s default), and only then falls back to signalling srun. The bash wrapper also execs the final command so the task is the process itself rather than a bash parent. * Fold the engine into the recipe: top-level engine:, per-role kv_events and sidecar The v2 recipe no longer needs a backend: block. `engine: sglang` (or a mapping with `type` plus engine-wide knobs such as vLLM's connector or TRT-LLM's served_model_name) names the engine that builds every role's command; `roles.<r>.kv_events` replaces the job-wide kv_events_config and `roles.<r>.sidecar` the job-wide dynamo.sidecar (every role must agree, the sidecar knobs stay under dynamo). All three are pre-schema normalizers onto the existing internal fields, so no consumer changes, backend: still loads as the v1 spelling, and mixing the two forms is rejected. srtctl migrate folds backend into engine (a bare string when only the type remains, placed where backend was), kv_events_config into the roles, and dynamo.sidecar into the roles. The golden check now compares kv_events_config by effect (per active mode) rather than by spelling, since `true` and an explicit per-mode map are the same worker flags. The YAML writer indents block sequences under their key so migrated lists keep the ` - item` style. Every example is rewritten by the migrator itself (features/override.yaml stays v1 on purpose). Golden equality: examples 19/19, historical 553/0/2, downstream 370/0/105/6, unchanged. * Services own the discovery plane, the Mooncake master, and the exporters Everything that is not a worker or the frontend is now a service launched by ServiceStageMixin: etcd and NATS (implied by frontend.type dynamo, phase infra), the Mooncake master (implied by backend.mooncake_kv_store, phase before_workers), and the DCGM and node exporters tachometer scrapes (implied while tachometer is on, phase after_frontend, on every worker node). The bespoke start_head_infrastructure readiness loop, the Mooncake-master stage, and the exporter launches inside the tachometer stage are gone; every one of these is a ManagedProcess from the same stage with the same registry, cleanup, and dry-run output. New service kinds: etcd, nats, mooncake-master, dcgm-exporter, node-exporter (src/srtctl/services/{infra,mooncake_master,exporters}.py). Kinds may build their own command, declare default readiness ports and placement, skip the bash wrapper (distroless images), and accept validated `options`. services/implicit.py derives the implied list and merges it with the declared one: a declared entry of the same name takes over, `enabled: false` drops it, `external:` points at an instance that already runs (nothing launches, the address is injected). `placement.node: dedicated` on etcd/nats reserves the infra node; services/normalize.py maps declared etcd/nats/mooncake-master entries back onto `infra` and `backend.mooncake_kv_store` before schema load so the runtime reads one set of fields. `infra:` and `backend.mooncake_kv_store` are v1 spellings; `infra.placement` leaves the placement vocabulary. Every long-running service is a named Slurm step (service_<name>) so cleanup delivers SIGTERM through scancel --signal and the process gets 30s to flush. Shell-less kinds receive their environment through srun --export. Dry-run lists every effective service, marks the implied ones with what implies them, prints the kind's built command, container fallback, default readiness ports, and options. Workers, the Dynamo frontend, and generic services get ETCD_ENDPOINTS / NATS_SERVER from one place (discovery_env), which honours `external`. srtctl migrate folds `infra:` into etcd/nats services (dedicated placement, NATS payload option; override variants spell out `placement.node: infra` to undo a base `true`) and `backend.mooncake_kv_store` into a mooncake-master service plus Mooncake env on every role. Under a static frontend the infra block is not folded: the payload knob is dropped as inert and the dedicated flag stays, since it still reserves a node. Golden verification compares services by effect (the effective list, kind defaults spelled out) and the inert NATS knob blanked. A MOONCAKE_LOCAL_HOSTNAME in a role's env now pins the NIC the way mooncake_kv_store.env did. Golden equality: examples 20/20, historical 553 identical / 0 mismatched / 2 skipped, downstream 370 / 0 / 107 skipped (pre-existing unknown fields) / 6 unreadable (duplicate YAML keys). Suite green on Python 3.10 and 3.13. Also fixes the golden-check Makefile target, whose mrm had been mangled. * Graceful shutdown for workers and frontends; tiered registry cleanup Every worker srun (per-process and MPI endpoint), every frontend (Dynamo, SGLang router, vLLM router, trtllm-serve orchestrator, nginx) is now a named Slurm step, and its ManagedProcess carries the step name, so cleanup delivers SIGTERM with scancel --signal=TERM --full to the task instead of to the srun client (which only aborts the step and SIGKILLs the task). Engines get 30s to deregister and free their GPUs, routers 20s to drain; services already had this from the previous commit, tachometer from the scrape fixes. ProcessRegistry.cleanup() is two-phase per shutdown tier: SIGTERM to every process in the tier at once (reverse registration order), wait for each up to its own terminate_timeout, escalate to SIGKILL, then the next tier. Tier 0 is workers, frontends, and after_frontend sidecars; tier 1 the Mooncake master and stores; tier 2 etcd and NATS. A tier is fully stopped before the next is signalled, so nothing deregisters from a plane that has already gone, and a job with dozens of workers finishes cleanup in about one timeout rather than one per process. Steps are listed with one squeue call per cleanup. Without Slurm tools on PATH (tests, the mock) the step path declines quietly and the srun client is signalled as before. * SGLang workers exit right after draining; ingest tolerates old scraper parquet Two findings from the first cluster runs of the graceful-shutdown path (sa-b200 jobs 12807 and 12808): Native sglang.launch_server workers drain in about five seconds after SIGTERM and then treat their own exit as a crash: py-spy dumps (which need root) and a 60s wait for CUDA coredumps that are never produced unless SGLANG_CUDA_COREDUMP=1. Cleanup killed them 30s in. Workers under engine sglang now default SGLANG_CUDA_COREDUMP_BEFORE_CRASH=0 and SGLANG_PYSPY_DUMP_BEFORE_CRASH=0; a recipe that opts into coredumps (SGLANG_CUDA_COREDUMP=1 in a role's env or the global environment) keeps the wait, and any explicit value wins. The tachometer-scraper binaries attached to the GitHub releases (what make setup installs) were built before the writer gained the timestamp_ns column (#350), so the perf dashboard ingest crashed on every fresh install with "Field timestamp_ns does not exist in schema" (job 12807). The ingest now derives timestamps from time_since_start anchored at the scraper start logged in tachometer.out, or at the parquet mtime when no log is nearby, and says so in the log. * Observability: tachometer is the only scrape path; delete the legacy Python analysis Roadmap #385, observability track: all metrics scraping goes through tachometer, and anything that turned run data into graphs by other means goes. Removed: - src/ingest/metrics_prometheus.py, the reader for raw_prometheus.jsonl. Its producer (the in-job Python scraper) was deleted in #351; the ingest's metrics source is now tachometer parquet first, then AIPerf's own exports, with no `--metrics prometheus` / `--raw-prometheus`. The shared dedup fold moves to src/ingest/dedup.py. - The in-flight batch-metrics snapshotter and its log parser and matplotlib renderer (srtctl.analysis.live_metrics, batch_log_parser, batch_plot_matrix, benchmarks/scripts/plot_batch_metrics.py) and the reporting.live_metrics cluster knob that switched it on. - The Streamlit dashboard and the srtlog log-parsing stack under analysis/ (5300 lines), run_dashboard.sh, docs/analyzing.md, and with them benchmark.export_node_metrics and the post-process CSV export. No historical or downstream recipe set that field. - srtlog parsing inside the S3 upload container. The container now only installs awscli and syncs the log directory; _run_postprocess_container returns the S3 URL. Kept: srtctl.analysis.host_sampler (reads /proc for what no /metrics endpoint publishes; not a scraper) and the per-run HTML perf dashboard (src/ingest + src/visualization), which is fed by the tachometer parquet. Power telemetry is untouched. Tests for the dashboard pipeline now build their fixture as a tachometer parquet in the writer's layout instead of a raw_prometheus.jsonl. * MCP job lifecycle tools and an in-package agent skill Roadmap #385, MCP track. The srtctl MCP server gains the job half: submit_job (srtctl apply -y --json with --set/--unset/--tags), dry_run, job_status (sacct row, job metadata, the orchestrator's current stage, [ERROR] lines, benchmark rollup, sweep-log tail), job_logs (list the log directory or tail one file, confined to the job's logs), list_jobs (squeue), and cancel_job (scancel). They are thin layers over the same commands an operator types and only work where Slurm is; the schema tools keep working anywhere and still never read host-side srtslurm.yaml. The server now imports under the mcp SDK the lock file pins (2.x renamed FastMCP to MCPServer and moved host/port to run()); the 1.x import still works. A test lists the registered tools through the server. `srtctl skill --target claude|codex|cursor [--root DIR] [--print]` installs src/srtctl/skills/SKILL.md, one document teaching an agent the 2.0 recipe shape, dry-run before apply, --set overrides, migrate --verify, where a run's logs and artifacts live, how cleanup behaves, and the MCP tools, into the layout each agent reads project skills from. * migrate --in-place: report an unreadable recipe and continue with the rest of the directory A recipe with duplicate YAML keys used to abort the whole directory run with a ruamel traceback after rewriting the files before it. Each failure is now reported by name with the duplicate key, the rest of the directory is migrated, and the command exits 1 with a migrated / not-migrated count. * [2.0] roles.decode.nodes: colocate replaces the decode_nodes: 0 sentinel Under roles:, a decode role that shares the prefill nodes is now written `nodes: colocate`. It normalizes to the v1 `resources.decode_nodes: 0`, so nothing downstream changes and v1 recipes keep loading; the bare `nodes: 0` is rejected under roles: (with a pointer to `colocate`), and `colocate` is rejected on prefill/agg. A colocated recipe must state `gpus:` on both prefill and decode. The v1 derivation (prefill = nodes * gpus_per_node // workers, decode inherits prefill) cannot express a split, so leaving either out is an error under roles:. `srtctl migrate` writes `colocate` for `decode_nodes: 0` and materializes the numbers v1 derived, right after `workers:`. SrtConfig also validates a colocated layout at load time: it checks the GPU sum against prefill_nodes * gpus_per_node and then runs the backend's real packer against a placeholder node list, so an oversubscribed or unpackable layout fails in dry-run instead of as "Not enough nodes" inside the job. Skipped when the backend packs across extra nodes itself (vLLM colocation). The migration verifier compares worker GPU sizes by effect, so a derived and an explicit split of the same size are identical. Examples and docs updated. Full suite 1964 passed; golden equality over the historical recipe corpus: 574 identical, 0 mismatched. * benchmark.concurrencies is a shared field: power telemetry reads it for every type The per-type benchmark field split treated concurrencies as belonging to the runners that consume it (sa-bench, sglang-bench, trace-replay, agentperf). Power telemetry also reads it, for every type: the expected measurement windows come from benchmark.get_concurrency_list(), and a telemetry.enabled recipe without it fails to load. A custom client with DCGM power (the InferenceMAX power lane shape: `type: custom` running agentic_srt.sh with CONC in env, plus `telemetry:`) therefore lost its concurrencies to the migrator's strip and no longer loaded as schema 2. Found migrating a live gb300 power recipe; verify caught it as a mismatch. concurrencies joins SHARED_BENCHMARK_FIELDS; the strip leaves it alone and schema 2 accepts it on every type. use_chat_template stays sa-bench only. * Move aiperf out of the dev dependency group aiperf is only needed by the trace-replay integration tests, which the default pytest run excludes (`-m 'not integration'`) and no CI job runs. It pulls pyzmq, whose wheels stop at CPython 3.13, so `uv sync` on a Python 3.14 host (the Vera Rubin login node) fell back to building libzmq from source and failed. aiperf now lives in its own `integration` dependency group: `uv sync --dev --group integration && uv run pytest -m integration`. A plain `uv sync --dev` no longer installs pyzmq or aiperf; the full default suite (2361 tests) is unchanged. uv.lock is gitignored, so there is no lockfile change to review. * Declare the sa-bench test imports in the dev group Moving aiperf out of `dev` dropped aiohttp, transformers, tqdm, datasets, pillow, and pandas from the dev environment, which tests/test_sa_bench_*.py need because they import the sa-bench scripts directly (19 failures on a fresh `uv sync --dev`). List them explicitly instead of relying on aiperf's dependency tree. Full suite: 2363 passed. * [2.0] Docs describe only the 2.0 layout; v1 lives in one generated file `srtctl schema-docs` now renders two documents from the code and `--check` covers both: - docs/schema-reference.md is 2.0-only. The top-level table gains `engine` and `roles`; an "Authoring surface" section is rendered from srtctl.core.roles and srtctl.core.placement (roles keys incl. `colocate`, placement values); every dataclass table omits the v1 fields; "Backend types" becomes "Engine types" keyed by `engine.type`. - docs/legacy-v1.md is the single place the v1 layout is documented: a v1 key -> 2.0 spelling table, the legacy fields of resources / frontend / benchmark / dynamo, the per-engine `backend` fields, the server-config and Mooncake classes, and `infra`. Which fields are legacy is declared once (LEGACY_FIELDS, LEGACY_TOP_LEVEL, LEGACY_CLASSES), derived from ROLE_NAMES, ENGINE_CONFIG_KEY and the placement field tuples; tests prove the two documents partition the keys and that `srtctl migrate` removes every listed key. Prose docs (config-reference and the rest of docs/, README, examples/README, the srtctl skill) are rewritten to the 2.0 layout with every complete recipe validated; the backend / infra / resources-topology / Backward Compatibility sections are replaced by pointers to legacy-v1.md. The sweep section now documents the flat `sweep:` map the code implements. The last v1 example (features/override.yaml) is migrated in place. Code comments and the Mooncake validation errors that named v1 fields are reworded so the generated tables and error messages speak 2.0. Full suite 2367 passed; golden equality 574 identical, 0 mismatched. --------- Signed-off-by: Ishan Dhanani <ishandhanani@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Second of the 3-PR observability-consolidation series. Stacked on #349 — review the last 4 commits; the first commit here is #349's.
observability.enabled: truenow collects server metrics with the native Tachometer scraper for the whole run, with notachometer:block required, scraping the complement of what the benchmark client polls. The legacy Python RAW scraper no longer followsenabled(strictly opt-in viascrape_metrics: true), removed entirely in PR 3/3.Why
AIPERF_SERVER_METRICS_URLS, so if the client's endpoint set ever changes, tachometer's complement moves with it.raw/scrapenested layout the --bash lifecycle already uses.timestamp_ns(Int64) and Float64 values.server_metrics_export.jsonl:metrics_tachometer(parquet; reconstructs_bucket/_sum/_count, injectsworker_id/dynamo_componentlike the prometheus path) andmetrics_aiperf_jsonl(aiperf's per-scrape export, preferred over the opinionated aggregate json). Auto-selection: tachometer → raw_prometheus.jsonl → aiperf-jsonl → aiperf-json.Behavior notes
critical=False): a dead scraper costs the capture, never the benchmark.validate-setupstill fails fast at submit whenbin/tachometer-scraperis missing; the default bare binary name now resolves against the checkout'sbin/before $PATH.tachometer.enabled: falseopts out; explicittruewithoutobservability.enabledstill raises.observability.enabled.Verification
make checkgreen (1477 passed),cargo test --workspace --lockedgreen (41 tests incl. new timestamp/precision regression guards), recipe validation green (the qwen3-32b ruter recipe simplifies to justenabled: true). New tests: complement exclusion, leaders-only, storage-leaf invariant, tri-state resolution, critical=False, binary resolution, dry-run display, both L2 processors (incl. f64 fidelity and source auto-selection).