[WIP] miles dashboard#1654
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive training-dynamics and efficiency dashboard for miles runs, featuring a live telemetry collector, GPU utilization samplers, sglang engine metric scrapers, and a standalone FastAPI server with a static single-page application. The feedback is highly constructive, highlighting a critical concurrency race condition in the collector's router setup that could leak scraper threads. Additionally, the reviewer correctly points out multiple instances where assert statements are used for argument and CLI validation, recommending they be replaced with ValueError or parser.error() to prevent them from being optimized away under Python's -O flag.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| with self._lock: | ||
| previous = self._scraper | ||
| if previous is not None and previous.router_addr == router_addr and previous.mode == mode: | ||
| return | ||
| self._scraper = None | ||
| if previous is not None: | ||
| previous.stop() | ||
| kwargs = dict( | ||
| mode=mode, | ||
| router_addr=router_addr, | ||
| engine_addrs=self._current_engine_addrs, | ||
| interval=self.config.scrape_interval_seconds, | ||
| whitelist=self.config.metric_whitelist, | ||
| ) | ||
| if self._scraper_http_get is not None: | ||
| kwargs["http_get"] = self._scraper_http_get | ||
| scraper = SglangScraper(self._append, **kwargs) | ||
| with self._lock: | ||
| self._scraper = scraper | ||
| scraper.start() |
There was a problem hiding this comment.
There is a concurrency race condition in set_router. If multiple threads call set_router concurrently, a previously created scraper can be overwritten in self._scraper without being stopped, leading to leaked running scraper threads. Creating the new scraper inside the lock and assigning it to self._scraper before releasing the lock ensures that concurrent calls correctly identify existing scrapers and avoid duplicate/leaked threads.
| with self._lock: | |
| previous = self._scraper | |
| if previous is not None and previous.router_addr == router_addr and previous.mode == mode: | |
| return | |
| self._scraper = None | |
| if previous is not None: | |
| previous.stop() | |
| kwargs = dict( | |
| mode=mode, | |
| router_addr=router_addr, | |
| engine_addrs=self._current_engine_addrs, | |
| interval=self.config.scrape_interval_seconds, | |
| whitelist=self.config.metric_whitelist, | |
| ) | |
| if self._scraper_http_get is not None: | |
| kwargs["http_get"] = self._scraper_http_get | |
| scraper = SglangScraper(self._append, **kwargs) | |
| with self._lock: | |
| self._scraper = scraper | |
| scraper.start() | |
| with self._lock: | |
| previous = self._scraper | |
| if previous is not None and previous.router_addr == router_addr and previous.mode == mode: | |
| return | |
| kwargs = dict( | |
| mode=mode, | |
| router_addr=router_addr, | |
| engine_addrs=self._current_engine_addrs, | |
| interval=self.config.scrape_interval_seconds, | |
| whitelist=self.config.metric_whitelist, | |
| ) | |
| if self._scraper_http_get is not None: | |
| kwargs["http_get"] = self._scraper_http_get | |
| new_scraper = SglangScraper(self._append, **kwargs) | |
| self._scraper = new_scraper | |
| if previous is not None: | |
| previous.stop() | |
| new_scraper.start() |
| assert args.dump_details is not None, ( | ||
| "--use-miles-dashboard writes telemetry under {dump-details}/dashboard/ and the " | ||
| "trajectory views read the rollout/train dumps, so --dump-details is required" | ||
| ) |
There was a problem hiding this comment.
Using assert for validating command-line arguments or function arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
| assert args.dump_details is not None, ( | |
| "--use-miles-dashboard writes telemetry under {dump-details}/dashboard/ and the " | |
| "trajectory views read the rollout/train dumps, so --dump-details is required" | |
| ) | |
| if args.dump_details is None: | |
| raise ValueError( | |
| "--use-miles-dashboard writes telemetry under {dump-details}/dashboard/ and the " | |
| "trajectory views read the rollout/train dumps, so --dump-details is required" | |
| ) |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
| def collector_config_from_args(args, *, start_ts: float) -> CollectorConfig: | ||
| if args.dashboard_sglang_metrics is not None: | ||
| whitelist = tuple(m for m in args.dashboard_sglang_metrics.split(",") if m) | ||
| assert whitelist, f"empty --dashboard-sglang-metrics: {args.dashboard_sglang_metrics!r}" |
There was a problem hiding this comment.
Using assert for validating function arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
| assert whitelist, f"empty --dashboard-sglang-metrics: {args.dashboard_sglang_metrics!r}" | |
| if not whitelist: | |
| raise ValueError(f"empty --dashboard-sglang-metrics: {args.dashboard_sglang_metrics!r}") |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
| interval: float = 1.0, | ||
| nvml=None, | ||
| ): | ||
| assert interval > 0, f"{interval=}" |
There was a problem hiding this comment.
Using assert for validating constructor arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
| assert interval > 0, f"{interval=}" | |
| if interval <= 0: | |
| raise ValueError(f"interval must be positive, got {interval}") |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
| if handle is None: | ||
| return | ||
| # a None ip here is a wiring-order bug, not runtime flakiness: fail loud | ||
| assert args.sglang_router_ip is not None, "register_router must run after start_rollout_servers" |
There was a problem hiding this comment.
Using assert for validating function arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
if args.sglang_router_ip is None:
raise ValueError("register_router must run after start_rollout_servers")References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
| if args.demo: | ||
| import tempfile | ||
|
|
||
| dump_dir = make_demo_dir(Path(tempfile.mkdtemp(prefix="miles_dashboard_demo_"))) | ||
| else: | ||
| assert args.dump_details is not None, "--dump-details is required (or use --demo)" | ||
| dump_dir = Path(args.dump_details) | ||
| assert dump_dir.is_dir(), f"--dump-details directory not found: {dump_dir}" |
There was a problem hiding this comment.
Using assert for validating CLI arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Using parser.error() is standard and safer for CLI argument validation.
| if args.demo: | |
| import tempfile | |
| dump_dir = make_demo_dir(Path(tempfile.mkdtemp(prefix="miles_dashboard_demo_"))) | |
| else: | |
| assert args.dump_details is not None, "--dump-details is required (or use --demo)" | |
| dump_dir = Path(args.dump_details) | |
| assert dump_dir.is_dir(), f"--dump-details directory not found: {dump_dir}" | |
| if args.demo: | |
| import tempfile | |
| dump_dir = make_demo_dir(Path(tempfile.mkdtemp(prefix="miles_dashboard_demo_"))) | |
| else: | |
| if args.dump_details is None: | |
| parser.error("--dump-details is required (or use --demo)") | |
| dump_dir = Path(args.dump_details) | |
| if not dump_dir.is_dir(): | |
| parser.error(f"--dump-details directory not found: {dump_dir}") |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
| if mode == ScrapeMode.ROUTER: | ||
| assert router_addr is not None, "router mode needs router_addr" | ||
| else: | ||
| assert engine_addrs is not None, "direct mode needs an engine_addrs provider" |
There was a problem hiding this comment.
Using assert for validating constructor arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
| if mode == ScrapeMode.ROUTER: | |
| assert router_addr is not None, "router mode needs router_addr" | |
| else: | |
| assert engine_addrs is not None, "direct mode needs an engine_addrs provider" | |
| if mode == ScrapeMode.ROUTER and router_addr is None: | |
| raise ValueError("router mode needs router_addr") | |
| if mode != ScrapeMode.ROUTER and engine_addrs is None: | |
| raise ValueError("direct mode needs an engine_addrs provider") |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
| else: | ||
| raise ValueError(f"unknown lane selector kind {kind!r} in {token!r}") | ||
| return selected | ||
|
|
There was a problem hiding this comment.
Using assert for validating function arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
| if x_buckets < 2: | |
| raise ValueError(f"x_buckets must be at least 2, got {x_buckets}") |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
|
|
||
| def stride_downsample(xs, ys, max_points: int) -> tuple[np.ndarray, np.ndarray]: | ||
| xs, ys = np.asarray(xs), np.asarray(ys) | ||
| assert max_points >= 2, f"{max_points=}" |
There was a problem hiding this comment.
Using assert for validating function arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
| assert max_points >= 2, f"{max_points=}" | |
| if max_points < 2: | |
| raise ValueError(f"max_points must be at least 2, got {max_points}") |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
| """Bucketed downsampling keeping each bucket's min and max, so spikes and | ||
| dips survive. Output length is at most ``max_points``.""" | ||
| xs, ys = np.asarray(xs), np.asarray(ys) | ||
| assert max_points >= 2, f"{max_points=}" |
There was a problem hiding this comment.
Using assert for validating function arguments is discouraged because assertions can be optimized away when Python is run with the -O flag. Raising a ValueError is preferred for argument validation.
| assert max_points >= 2, f"{max_points=}" | |
| if max_points < 2: | |
| raise ValueError(f"max_points must be at least 2, got {max_points}") |
References
- Use ValueError instead of assert for validating function or constructor arguments (such as checking for positive or non-negative values).
split_train_data_by_dp_local, process_rollout_data_shard, and save_debug_train_data_for_rank carry the pure logic; the original functions keep their signatures and wrap ray/torch.distributed transport around them. No behavior change.
…eline tests/fast/dashboard/dummy_dump.py pushes fake samples through the actual save_debug_rollout_data / convert_samples_to_train_data / split_train_data_by_dp_local / process_rollout_data_shard / save_debug_train_data_for_rank chain, so dumping-logic changes break the reader tests instead of drifting silently. Building it this way surfaced a real reader bug, fixed here: raw_reward is stored batch-global (unlike every other per-shard column) and must be indexed by rollout position, not shard row.
…caches summary() computes the per-sample L1 table (masked per-token stats, spec accept rate, prefix-cache hit rate, batch-global raw_reward) and caches it as parquet keyed on source mtimes + SUMMARY_VERSION. groups() adds GRPO-group aggregates with zero_std degeneracy flags; step_aggregates() is the dump-derived L0 fallback. tokens() serves the per-token L2 payload (imp_ratio, lp_diff, entropy, advantages) over a clamped token window, decoding text via the dumped tokenizer; joined() bounds tensor memory with a per-rollout LRU.
49cecf5 to
f755958
Compare
|
Pushed a batch of fixes from live dogfooding on real runs (colocate 8×H200 3-step, 4+4 fully-async, and a 744B disaggregated agentic run):
Same 23-commit stack shape; all commits amended in place. 149 dashboard tests green. |
make_app(store, reader) exposes /api/meta, /api/metrics (store keys plus dump/<column> aggregates so L0 works without metrics.jsonl), rollout summary/groups tables, and the per-token payload. Error mapping: DumpStillWriting -> 503, missing rollout/sample -> 404, bad input -> 400; NaN/inf sanitize to null. serve.py runs it standalone over any --dump-details directory, with --follow tailing telemetry streams.
f755958 to
d34d87b
Compare
|
Batch: metrics L0 redesign (wandb/grafana parity) — force-pushed
Verification: local tip 151 passed; devbox |
d34d87b to
d32fd8c
Compare
|
sglang page polish (follow-up, force-pushed |
d32fd8c to
5f11f27
Compare
|
Metrics interaction follow-up (force-pushed |
5f11f27 to
874548a
Compare
|
Nav + step-jump follow-up (force-pushed |
874548a to
49aa283
Compare
|
Drag-zoom fix (force-pushed |
49aa283 to
8839f1b
Compare
|
sglang legend (force-pushed |
8839f1b to
c7e322a
Compare
|
Zoom rework + slow-load notice (force-pushed
Verified with real mouse input on the live run: box drag on gen_throughput zooms x to +6:40–+23:20 and y to the selected band; narrow drags on 3-point charts now zoom reliably. |
c7e322a to
ae2b9ee
Compare
|
Weight-version hue ring (force-pushed |
ae2b9ee to
222a80f
Compare
|
Weight-version coloring, corrected semantics (force-pushed |
222a80f to
7547be7
Compare
|
Follow-up (force-pushed |
7547be7 to
53b2f65
Compare
|
avg staleness stat (force-pushed |
0c4d838 to
edf549d
Compare
Zero-dependency ES-module frontend served from /static: hand-rolled canvas charts (no vendored libs, no build step). L0 metric explorer with store + dump/ catalogs and click-through to steps; L1 rollout view with headline stats, reward-vs-length scatter, sortable sample table, GRPO groups tab, eval toggle; L2 token strip colored by imp_ratio / entropy / advantages with loss-mask dimming, prompt/response split, hover stats, and a per-token chart.
dummy_telemetry.py fabricates collector output through the real MetricStore write path (colocate cadence, long-tail drain, mid-rollout engine restart, util spike) and is the output contract the live collector must satisfy. Store gains lanes/topology-window/phase-lane queries — rollout_manager events expand onto rollout-engine lanes with clipping at topology boundaries — plus downsampled gpu/engine series and the per-step bubble strip. Server exposes them and flips the has_timeline/has_engine_series capabilities.
Canvas lane renderer per the design mockup: phase band (train_wait hatched as explicit idle), GPU util area, engine-metric overlay mapped onto each engine's lanes via topology windows, per-step bubble strip with click-to-zoom, wheel zoom / drag pan, hover inspection of phase, util, overlay and owning engine. serve --demo generates dump + telemetry demo data via the dummy generators for cluster-free development.
Router mode does one GET /engine_metrics per tick (sgl router fans out and labels samples with worker_addr); direct mode fetches each engine's /metrics concurrently for --use-miles-router runs, skipping failed engines. Names are normalized (sglang:x == sglang_x), whitelisted, and label-reduced; failures leave gaps with warnings rate-limited to one per 5 minutes. The sink is a plain callable, so no collector or Ray dependency.
Plain-Python NVML sampler (the collector will spawn it as a per-node Ray actor): daemon thread samples every physical device at 1 Hz, buffers locally, and pushes one batch per 5 s flush through the injected push callable. NVML being unavailable disables it with a single warning; a device failing mid-run (GPU reset) is skipped per tick with rate-limited warnings while the rest keep reporting.
…heus switch Ray-free by design: backend glue will wrap it in the named driver-node actor and spawn per-node samplers, so every behavior here is plain unit-testable. The contract test replays dummy_telemetry records through the RPC surface and asserts timeline queries match the fixture's direct writes exactly. Topology snapshots dedup on change; set_router resolves auto mode (miles router -> direct) and re-points the scraper on router restarts without holding the ingest lock during stop (deadlock). Disk failures stay loud on every flush while buffers drop oldest past a cap; the optional prometheus forwarding (default off) pushes sanitized latest-value gauges from caches that survive flushes.
Timer.end() now reports (name, start_ts, end_ts) to registered event_sinks before accumulating, giving observers wall-clock phase intervals instead of durations only. No sinks registered (the default) leaves behavior byte-identical; miles.dashboard registers one when enabled.
backend.init_dashboard implements the design's call matrix: the driver creates the named DashboardCollector actor (driver-node pinned) and spawns one GpuSampler actor per GPU node; secondaries resolve the named actor with a timeout and warn-once-then-noop; the rollout manager additionally registers the router (starting the scraper) and a rollout-role phase sink. hooks.py carries the Timer phase sink (batched, identity stamped lazily until torch.distributed knows the rank, exceptions swallowed with rate-limited warnings) plus register_train_actor / register_engines — the engine fingerprint costs one tuple compare per rollout step and re-registers topology only when actor handles change. args.py adds the --dashboard-* CLI group, validation, and the CollectorConfig builder.
…pology The complete core-file budget from the design doc, ~90 lines: MilesDashboardBackend adapter + registry entry in tracking_utils; --dashboard-* argument group and validation hookup; rollout_manager registers engine topology and wraps rollout/eval in timers (interval events only — perf metrics are unchanged); the train actor attaches the per-rank phase sink outside the main-rank gate; SGLangEngine gains the additive get_topology_info() (node-physical GPU ids matching NVML order, best-effort UUIDs). The e2e test drives the real tracking registry on local Ray: init -> log fan-out -> Timer sink -> finish, then loads and serves the produced directory.
edf549d to
d8d7fc9
Compare
|
Per-token metrics chart (force-pushed |
9153bf2 to
fde52b9
Compare
|
PR-24 + PR-25 end-to-end verified on a real multi-turn run (tip Run recipe (simplest trajectory trigger, zero new code): the qwen30b async recipe with Field notes from getting there, each worth knowing:
Also in this push (PR-18): |
fde52b9 to
44d7f39
Compare
|
L2 split into tabs (force-pushed, PR-25): the sample page now opens on a conversation | tokens tab bar when a trajectory sidecar exists — conversation (sidecar read, instant) is the default; the token machinery (probe → full dump load → detokenize, the several-minutes first-open cost) fires only when the tokens tab is first clicked (verified: zero /tokens requests before the click). Runs without a sidecar keep today's behavior: no tab bar, tokens load directly. |
44d7f39 to
d918901
Compare
|
Raw-assistant display parsing (force-pushed, PR-25): the engine multi_turn path records the model's verbatim emission, so the conversation renderer now does a display-level split when the structured fields are absent — |
Observability dashboard for miles RL post-training: an efficiency view (per-GPU wall-clock timeline: phase bands, GPU util, sglang engine overlay, rank carpet), a training-dynamics view (step metrics → per-sample table → token-level entropy/logprob drill-down), and a trajectory lifecycle view (per-sample generate/tool/queue swimlanes with segment-level weight-version staleness — the fully-async batch anatomy).
Design principles
{dump-details}/dashboard/as append-only JSONL (high-rate streams hourly-partitioned). The server is a stateless reader — a finished (or crashed) run reopens identically from files alone.miles/dashboard/+ tests the whole feature is ~200 lines across 15 files (registry entry, CLI args, Timer event sinks, engine topology accessor, transport-free dump-helper extractions, and the trajectory lifecycle probes ingenerate_and_rm/multi_turn/ openai record assembly — gated behind a stdlib-only seam inmiles/utils/lifecycle.py, same layer asTimer.event_sinks).rank:/node:/gpu:/engine:/role:grammar, URL-shareable), 32-lane render budget, binary heatmap endpoint.Usage
Validation
tests/fast/rollout/green (530 passed on an H200 devbox against real dump data).Review guide
23 stacked single-purpose commits, each self-contained with its tests — commit-by-commit is the intended review path. Bottom-up: store → dump reader → server/SPA → timeline → scraper/sampler/collector → core wiring → carpet/selection → scaling (columnar/partitions/viewport) → trajectory lifecycle (probes → sink → columns → views).
WIP
SessionRecord.request_timestamp(+3 lines) only if sglang chatmeta_infolackse2e_latency— to verify on a live engine.01a6d7bb7; will rebase onto current main (the in-flight session-server refactors were designed for — lifecycle metadata rides the Sample through assembly, pinned bytest_lifecycle_metadata.py).🤖 Generated with Claude Code